-
Notifications
You must be signed in to change notification settings - Fork 930
feat(sdk-node): always set up propagation and context manager #5930
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(sdk-node): always set up propagation and context manager #5930
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5930 +/- ##
=======================================
Coverage 95.24% 95.24%
=======================================
Files 315 315
Lines 8604 8626 +22
Branches 1798 1801 +3
=======================================
+ Hits 8195 8216 +21
- Misses 409 410 +1
🚀 New features to boost your workflow:
|
|
||
describe('setup exporter from env', () => { | ||
let stubLoggerError: Sinon.SinonStub; | ||
describe('setup exporter from env', () => { |
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.
Note: these are not new, I moved these into the above describe block, these tests seemed to be out of place unintentionally. Sorry for the excessive diff.
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. I have a comment about registering context manager at the beginning of the start
method.
nice! I hadn't thought of that 🙂 Addressed in 8d4b3c5 |
Which problem is this PR solving?
This was discussed in a SIG meeting a long time ago. Makes a
ContextManager
and a defaultTextMapPropagator
be registered always, not just when using the Trace SDK.Context management is the cornerstone of many OTel features, so it makes sense to also have it on by default in the OTel JS Node.js SDK. When it is missing it is a common source for confusion, I've seen:
traceId
attached to their logs that was created by a different service - this needs a propagator and context managerhttp.route
(or it's modern replacement) not being on metrics when noContextManager
is registered, since we need it to communicate from higher-level instrumentations to the lower-levelHttpInstrumentation
. Missing http_route attribute on the http_server_duration metric when traces export is disabled #3862Once we have #5147, then there will also an expectation for a
ContextManager
to be registered for all metrics features to work, even when not using thehttp
instrumentation.Fixes #3862
Fixes #5640
(also this PR a way for me to find how many ways there are to misspell propagator)
Type of change
How Has This Been Tested?