-
Notifications
You must be signed in to change notification settings - Fork 540
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
fix: fix context loss when cursor are accesed concurrently #1721
fix: fix context loss when cursor are accesed concurrently #1721
Conversation
plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts
Show resolved
Hide resolved
@osherv could you please add your feedback and approval if applies? |
Codecov Report
@@ Coverage Diff @@
## main #1721 +/- ##
==========================================
- Coverage 91.62% 91.49% -0.14%
==========================================
Files 139 139
Lines 7151 7159 +8
Branches 1444 1438 -6
==========================================
- Hits 6552 6550 -2
- Misses 599 609 +10
|
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.
Looks great; thanks for fixing this. 🙂
Edit: looks like this still needs some lint fixes but then this should be good to merge 🙂
plugins/node/opentelemetry-instrumentation-mongodb/src/internal-types.ts
Outdated
Show resolved
Hide resolved
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.
LGTM, modulo the linting and typo that Marc mentioned.
FWIW, npm run test-all-versions
worked locally for me.
…id-luna/opentelemetry-js-contrib into dluna-1688-mongodb-async-context-fix
Thanks you both for the review :) @pichlermarc there are some workflows awaiting approval. Are you able to approve them? |
Which problem is this PR solving?
This PR is solving the problem of
mongodb
instrumentation loosing the context when cursors are accessed concurrently.Short description of the changes
Add a new instrumentation on
ConnectionPool
class to patch the method that produces the context loss. This class is present from v4 and up so the fix works also for v5Closes #1688