-
Notifications
You must be signed in to change notification settings - Fork 795
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
Simplify SDK registration #837
Simplify SDK registration #837
Conversation
Codecov Report
@@ Coverage Diff @@
## master #837 +/- ##
==========================================
- Coverage 92.63% 91.62% -1.02%
==========================================
Files 251 192 -59
Lines 11057 7310 -3747
Branches 1079 646 -433
==========================================
- Hits 10243 6698 -3545
+ Misses 814 612 -202
|
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
@@ -15,7 +15,12 @@ | |||
*/ | |||
|
|||
import { BasePlugin } from '@opentelemetry/core'; | |||
import { BasicTracerProvider, TracerConfig } from '@opentelemetry/tracing'; | |||
import { ZoneScopeManager } from '@opentelemetry/scope-zone'; |
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.
Switch to stack
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.
Is there any specific reason to use the stack scope manager by default ?
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.
Zone scope manager depends on zone.js which may not be available. If we have it as a dependency, then we may have a different zone.js than the end user. If we have it as a peer dependency, the scope manager would be broken for any user that doesn't have zone.js.
@mayurkale22 when this looks good to you I think it is ok to merge. Just waiting on last resolution confirmation from @obecny |
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, are you planning to update the examples in separate PR?
The old registration method still works so I think a separate pr makes sense here |
Which problem is this PR solving?
Short description of the changes
register
method to theTracerProvider
which registers all configured SDK components with the API.New
register
method onTracerProvider
Defaults
Propagator
Context Manager
Override Scope Manager or Propagator
Use
null
to skip a default