-
Notifications
You must be signed in to change notification settings - Fork 838
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)!: drop ability to auto-instantiate propagators beyond defaults #5355
feat(sdk-trace-base)!: drop ability to auto-instantiate propagators beyond defaults #5355
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5355 +/- ##
==========================================
+ Coverage 94.63% 94.64% +0.01%
==========================================
Files 318 318
Lines 8032 8036 +4
Branches 1685 1689 +4
==========================================
+ Hits 7601 7606 +5
+ Misses 431 430 -1
|
@@ -117,16 +111,19 @@ export class BasicTracerProvider implements TracerProvider { | |||
*/ | |||
register(config: SDKRegistrationConfig = {}): void { |
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.
Removing/replacing this will likely take 3 more PRs to make the change somewhat reviewable
- introducing an alternative function called
registerTraceSdkGlobals()
or similar - migrating to that new alternative function
- dropping
register()
But this PR is still an improvement for now as we can shed the imports of non-default propagators/context managers.
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. Just a few nits.
Co-authored-by: Trent Mick <[email protected]>
Which problem is this PR solving?
It is currently confusing which packages implement auto-configure and to which extent. We currently auto-instantiated propagators based on environment variables in sdk-trace-base.
This PR makes it so that only the default propagators get registered when
*TracerProvider#register()
is called.The auto-configuration code moves to
@opentelemetry/sdk-node
. Dependencies to other propagators are removed from@opentelemetry/sdk-trace-node
. #4672I will follow up later on removing
BasicTracerProvider#register()
(tracked in #5290)Fixes #4672
Refs #5290
Type of change
How Has This Been Tested?