-
Notifications
You must be signed in to change notification settings - Fork 497
feat(contrib/gomongodb.org/mongo-driver): add optional query tag truncation #3679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
### What does this PR do? This PR adds support for a `WithMaxQuerySize` method to the MongoDB package that enables truncation on the `mongodb.query` span tag. I also ported this behavior to the new `mongo-driver.v2` package which supports the v2 release of the Mongo drivers. Opening this PR here first for feedback from our team and then will open on the upstream repo. The default is to not truncate query span tags, to avoid breaking changes. I'll ask in the upstream PR about whether we could truncate by default instead. ### Motivation This reduces the memory footprint of tracing Mongo applications that write large documents.
This PR updates the behavior of `WithMaxQuerySize` when `max=0` to avoid attaching the query tag entirely. This is more intuitive ("max query size of zero") and gives folks a way to disable serializing the command entirely.
|
Hey @colinking , Thanks for your contribution! The Datadog Agent has logic to manipulate db queries and I am concerned that truncation could break the agent logic. That said, 100MB (referenced in #3672) is quite large, and I imagine the agent is flat-out rejecting these payloads. So that we can find the best solution to your problem, can you tell me a little bit more about your user experience? What is the actual impact of these [very large] span tags? Thanks! |
| // If max is < 0, the query is never truncated. | ||
| // | ||
| // Defaults to -1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following with my previous comments, I'd rather to make 0 value something more meaningful, and default to a higher max length instead of not truncating. As long as it's configurable, it's ok to do this kind of changes.
The question is "is it possible to have an empty query?". If not, I'd say that 0 should mean the same as a negative value. If it's possible, then 0 should set "" as tag value. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 0 value is currently meaningful, as that's the only way to specify you want to drop the query tag entirely.
In other words:
- Negative value: do not truncate
- Zero: do not attach the
querytag - Positive value: attach the
querytag, but truncate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If y'all want to avoid truncation here (e.g. due to the obfuscation logic mentioned above), then another option is to drop the query tag if the length is beyond the configurable limit. Therefore, you always get a well-formed query.
|
@rarguelloF WDYT about this PR? Thanks! |
|
Sorry for the delayed responses here y'all. @mtoffl01: In our case, the process that was generating these spans was OOMing. You're probably right that the agent would reject such a large span, but in our case the span was never getting sent since the process OOMed first. For context, we had some code that would generate these large bulk MongoDB writes. This code could get retried multiple times, all of which was occurring within a parent span. Therefore, the total memory required to hold this trace in memory was on the order of GBs. We rolled out this change to production and haven't encountered any OOMs since and overall memory usage dropped significantly. |
|
Just gently bumping this PR. Thanks in advance y'all. @darccio @rarguelloF @mtoffl01 |
|
@colinking Thanks for pinging us. I reviewed it again and I'd suggest a comment to I'm approving this. Once the comment is added, I'll merge this. |
|
Thanks @darccio. I've updated the |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@rarguelloF The PR is ready for merging. Are we good from @DataDog/apm-idm-go? |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
There was an unexpected error while creating the working branch: cannot create empty commit This could indicate that something doesn't work properly with the build system or that this one has reached its maximum capacity. DetailsError: cannot create empty commit FullStacktrace: |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
437d647
into
DataDog:main
What does this PR do?
This PR adds support for a
WithMaxQuerySizeoption to the MongoDB module that configures truncation on themongodb.queryspan tag.Ideally, we default to enabling truncation. However, this PR currently maintains backwards-compatibility by defaulting to no truncation. I'll defer to the DataDog team on whether we can change this default.
I've also ported this behavior to the
mongo-driver.v2module.Fixes #3672
Motivation
See: #3672
Reviewer's Checklist
golangci-lint runlocally.Unsure? Have a question? Request a review!