-
Notifications
You must be signed in to change notification settings - Fork 491
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
feat(instrumentation-mongodb): support aggregation commands and support nested statements #1728
feat(instrumentation-mongodb): support aggregation commands and support nested statements #1728
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1728 +/- ##
==========================================
+ Coverage 90.97% 91.43% +0.45%
==========================================
Files 146 139 -7
Lines 7492 7179 -313
Branches 1502 1449 -53
==========================================
- Hits 6816 6564 -252
+ Misses 676 615 -61
|
@pichlermarc I'm not sure whether the failing tests are related to my changes at all, looks more like a flaky test? |
…ands and support nested statements Signed-off-by: Oliver Sand <[email protected]>
Signed-off-by: Oliver Sand <[email protected]>
e1a8bed
to
ea52e6a
Compare
@david-luna Thanks for notifying me. I updated the PR, can you retrigger the pipeline? |
Sorry I can't trigger the CI cc: @open-telemetry/javascript-maintainers |
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.
Thanks again @Fox32 for your contribution :)
Semantic conventions define the db.statement
as a string value (link). Also the specific section for mongodb
does not mention anything about statements (link)
However Java instrumentation and even this one also send an object in some cases (haven't checked other agents). I guess this is something that should be updated in semantic-conventions and we should check if there is an issue for that (or crate one if not).
Saying that this PR looks good :)
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v3.test.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v4.test.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-mongodb/test/mongodb-v5-v6.test.ts
Outdated
Show resolved
Hide resolved
update semantic conventions imports
Looks like the merge from main made the change for |
@JamieDanielson thanks for taking care! |
Which problem is this PR solving?
At Serrala, we have a lot of micro services build with different languages that we observe with OpenTelemetry. We noticed that the mongo instrumentation of Node lacks some details compared to the Java instrumentation:
mongo.command
).db.statement
is sanitized to a degree that it doesn't contain enough details to pin down what the query is doing.Short description of the changes
This PR adds support for aggregation command. As aggregation pipelines typically have a nested structure to describe the statement, the current way of sanitizing only leaves a low amount of hints what the command is about:
For example:
Is sanitized to:
The Java instrumentation is handling statement sanitizing differently. Instead of replacing all values of the top level object, it traverses into nested objects and array, keeping the existing structure. This PR implements the same behavior for the Node instrumentation. Therefore
db.statement
is in this case:While it is possible to configure a customer sanitizer for commands, I think it would better to add support for this upstream for better compatibility to the Java instrumentation.
I fixed some small typos I stumbled upon while reading through the contribution guidelines - I hope it is fine to include them here.