Skip to content
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-trace-base)!: remove BasicTracerProvider#register() to improve tree-shaking #5503

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Feb 24, 2025

Which problem is this PR solving?

Removes BasicTracerProvider#register(), which in the past has auto-registered a W3CTraceContextPropagator and W3CBaggagePropagator, requiring it being included in bundles even when unused.

NodeTracerProvider#register() and WebTraceProvider#register() still provide the same behavior so impact to end-users should be minimal. Everyone that does not want to use the defaults can now use BasicTracerProvider with trace.setGlobalTracerProvider(), propagation.setGlobalPropagator() and context.setGlobalContextManager() to register, while not including the unused context managers/propagators in their bundle.

Refs #5290

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit tests

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.95%. Comparing base (7d6e11e) to head (a427fa8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5503   +/-   ##
=======================================
  Coverage   94.95%   94.95%           
=======================================
  Files         309      309           
  Lines        7924     7931    +7     
  Branches     1606     1604    -2     
=======================================
+ Hits         7524     7531    +7     
  Misses        400      400           
Files with missing lines Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 94.64% <100.00%> (-0.88%) ⬇️
...telemetry-sdk-trace-node/src/NodeTracerProvider.ts 100.00% <100.00%> (ø)

@pichlermarc pichlermarc marked this pull request as ready for review February 24, 2025 10:13
@pichlermarc pichlermarc requested a review from a team as a code owner February 24, 2025 10:13
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Feb 24, 2025
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Feb 24, 2025
@david-luna
Copy link
Contributor

There are some references to BasicTraceProvider#register() in some README files. Some examples:

@pichlermarc
Copy link
Member Author

There are some references to BasicTraceProvider#register() in some README files. Some examples:

* https://github.com/open-telemetry/opentelemetry-js/blob/7d6e11e02ffc655909b2de4dd6c4fef418c0e979/experimental/packages/exporter-trace-otlp-grpc/README.md?plain=1#L41

* https://github.com/open-telemetry/opentelemetry-js/blob/7d6e11e02ffc655909b2de4dd6c4fef418c0e979/experimental/packages/exporter-trace-otlp-http/README.md?plain=1#L85

* https://github.com/open-telemetry/opentelemetry-js/blob/7d6e11e02ffc655909b2de4dd6c4fef418c0e979/experimental/packages/exporter-trace-otlp-proto/README.md?plain=1#L40

Oh, good catch - I forgot to search for it in the other README.mds. Thanks 🙇‍♂️
Fixed in a427fa8

@david-luna
Copy link
Contributor

I guess BasicTracerProvider wasn't meant to be used by SDK users right? Either they use WebTracerProvider or NodeTracerProvider depending on which runtime they want to do instrumentation. right?

Also I see occurrences in several tests related to exporters and processors. do you think it's okay to use BasicTracerProvider there?

@pichlermarc
Copy link
Member Author

pichlermarc commented Feb 24, 2025

I guess BasicTracerProvider wasn't meant to be used by SDK users right? Either they use WebTracerProvider or NodeTracerProvider depending on which runtime they want to do instrumentation. right?

IIUC it was meant to be used by end-users as an unopinionated minimal implementation of the spec and that WebTracerProvider and NodeTracerProvider would provide some defaults for the specific runtime. So there might be cases where you may want to use BasicTracerProvider if the others give you something you don't want and you'd like more control over how it is used.

For Node.js, NodeSDK has largely surpassed NodeTracerProvider and we keep it mostly for backward compatibility until we have improved NodeSDK's interface to be production-ready.

For the Browser, WebTracerProvider is still a good starting point for now as we don't have a NodeSDK equivalent. Long-term I think we will need to transition these users to BasicTracerProvider as an opinionated SDK will always be at odds with the eventually important bundle-size requirements which make a more complex setup process worth its time. I expect that we will largely tackle this with documentation.

Ideally I'd like to fully replace NodeTracerProvider use by end-users with NodeSDK or its successor equivalent, and WebTracerProvider by BasicTracerProvider or its successor equivalent (ideally just called TracerProvider like the providers in logs and metrics to avoid confusion).

Also I see occurrences in several tests related to exporters and processors. do you think it's okay to use BasicTracerProvider there?

I think that's fine (should be encouraged even). The tests often use the more minimal implementation to maximize compatibility and reduce the chance of internal behavior changes breaking tests.

@pichlermarc
Copy link
Member Author

Ideally I'd like to fully replace NodeTracerProvider use by end-users with NodeSDK or its successor equivalent, and WebTracerProvider by BasicTracerProvider or its successor equivalent (ideally just called TracerProvider like the providers in logs and metrics to avoid confusion).

But that's something for 3.0 and beyond. 🙂

@pichlermarc pichlermarc added this pull request to the merge queue Feb 24, 2025
Merged via the queue into open-telemetry:main with commit 544c409 Feb 24, 2025
19 checks passed
@pichlermarc pichlermarc deleted the feat/remove-register branch February 24, 2025 16:51
trentm pushed a commit to trentm/opentelemetry-js that referenced this pull request Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:sdk-trace-base target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants