-
Notifications
You must be signed in to change notification settings - Fork 227
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: ensure correct run-context for 'memcached' instrumentation #2475
Conversation
This ensures that a created 'memcached' span is not the currentSpan in user code after the call. Refs: #2430
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
run module tests for memcached |
run module tests for memcached |
TAV=memcached test run passed: https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-nodejs%2Fapm-agent-nodejs-mbp/detail/PR-2475/4/pipeline |
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.
👍 Approving. diff was a little gnarly, but it looks like it does the thing.
} | ||
} | ||
} | ||
return function wrappedCommand (queryCompiler, _server) { |
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.
nit: it doesn't look like the function ever uses _server
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.
Yah, a _
prefix on args is my sometimes way of saying "this is the expected position of this argument in the call signature, but I'm not using it". This _
prefix pattern is this eslint rule (typescript-eslint/typescript-eslint#1510) which I believe is a default in standard
s set of eslint style rules.
Happy to drop it if you prefer. For trailing args it is purely for self-documenting. For non-trailing args (e.g.
apm-agent-nodejs/lib/noop-transport.js
Line 28 in 3928f8d
sendError (_error, cb) { |
This ensures that a created 'memcached' span is not the currentSpan
in user code after the call.
Refs: #2430
Checklist