APM: Only do stats obfuscation when we know its safe to do so #3155
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 5227 Passed, 74 Skipped, 2m 11.46s Total Time |
ajgajg1134
changed the title
|
Overall, LGTM! Just some questions for clarification: Is a value of Doc you linked in Slack is very helpful! How much of it can we add to the PR notes/comments? At first, I was confused as to what constitutes a obfuscation "version", so I feel that adding more documentation for that might be helpful 👀 |
That's just a dummy / test value for the mock transport object to make it clear that no value has been set. This is useful since the default value of
Good idea! I've updated the PR description here with some excerpts which hopefully make it a bit clearer? I'm happy to add some more code comments too if there's any particular areas that aren't clear! |
| defaultURL = "http://" + defaultAddress | ||
| defaultHTTPTimeout = 10 * time.Second // defines the current timeout before giving up with the send process | ||
| traceCountHeader = "X-Datadog-Trace-Count" // header containing the number of traces in the payload | ||
| obfuscationVersionHeader = "Datadog-Obfuscation-Version" // header containing the version of obfuscation used, if any |
There was a problem hiding this comment.
out of curiosity, why do some Datadog- headers have the X- prefix, but others don't? Should it be added here?
There was a problem hiding this comment.
I had to look this up, apparently the old MIME headers RFC said this should be done for custom headers. However since at least 2011 that's apparently been deprecated and you can just name headers whatever you want :P https://datatracker.ietf.org/doc/html/draft-saintandre-xdash-00 I noticed some of our more recent headers dropped the X- so it seemed to make sense to do that here as well
Oh looks like that draft is now this RFC itself from 2012
What does this PR do?
This adds a fix where the tracer attempts to load the obfuscation version of the trace-agent before doing obfuscation on stats payloads. If the agent supports client stats being obfuscated and our version of obfuscation is at-least as recent then we can safely obfuscate. Full agent support is here: DataDog/datadog-agent#33765 and until that is released, systems talking to old agents and using client stats will not obfuscate so that the trace-agent can perform the obfuscation a single time.
Motivation
A few points here:
GET SETbut instead after double obfuscation becomesGETHere's a more thorough explanation from a design doc:
The trace-agent today accepts client (aka tracer) calculated stats payloads via the endpoint /v0.6/stats. This endpoint performs obfuscation on the resource name of the incoming payloads, today this is for SQL and Redis spans which may hold sensitive data in their resource name. The current implementation of the Go tracer’s client stats also performs this obfuscation using the same imported logic from the agent. This results in obfuscation logic occurring twice on the resource name, leading to unexpected resource names that are less useful. For example a redis span with resource name GET k1\nDEL k2\nMS… after initial obfuscation becomes GET DEL … However after being ingested by the trace-agent a second round of obfuscation occurs and the resource name is changed again, this time to the far less descriptive GET.
We can resolve this problem in the Go tracer by introducing a new header Datadog-Obfuscation-Version, this header is a simple version integer that we increment over time as new obfuscation features are added
Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.Unsure? Have a question? Request a review!