Skip to content

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Sep 12, 2025

Which problem is this PR solving?

This was discussed in a SIG meeting a long time ago. Makes a ContextManager and a default TextMapPropagator 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:

  • people that want logs, but don't want traces, but still want a traceId attached to their logs that was created by a different service - this needs a propagator and context manager
  • people that want to only propagate context, but don't want telemetry from that service - so they needed both a propagator and context manager.
  • people that run into http.route (or it's modern replacement) not being on metrics when no ContextManager is registered, since we need it to communicate from higher-level instrumentations to the lower-level HttpInstrumentation. Missing http_route attribute on the http_server_duration metric when traces export is disabled #3862

Once 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 the http 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

  • Bug fix (depending on how you look at it)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit tests

@maryliag maryliag changed the title feat(sdk-node): always set up propagtion and context manager feat(sdk-node): always set up propagation and context manager Sep 12, 2025
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.24%. Comparing base (f85cee1) to head (6da97b9).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ental/packages/opentelemetry-sdk-node/src/utils.ts 95.65% 1 Missing ⚠️
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     
Files with missing lines Coverage Δ
...imental/packages/opentelemetry-sdk-node/src/sdk.ts 96.03% <100.00%> (ø)
...ental/packages/opentelemetry-sdk-node/src/utils.ts 91.91% <95.65%> (+0.68%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


describe('setup exporter from env', () => {
let stubLoggerError: Sinon.SinonStub;
describe('setup exporter from env', () => {
Copy link
Member Author

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.

@pichlermarc pichlermarc marked this pull request as ready for review September 12, 2025 17:54
@pichlermarc pichlermarc requested a review from a team as a code owner September 12, 2025 17:54
Copy link
Contributor

@david-luna david-luna left a 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.

@pichlermarc
Copy link
Member Author

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

@pichlermarc pichlermarc enabled auto-merge October 9, 2025 07:26
@pichlermarc pichlermarc added this pull request to the merge queue Oct 9, 2025
Merged via the queue into open-telemetry:main with commit 0b720f6 Oct 9, 2025
25 checks passed
@pichlermarc pichlermarc deleted the feat/always-setup-context-and-propagator branch October 9, 2025 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants