-
Notifications
You must be signed in to change notification settings - Fork 809
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
Add propagation API #797
Add propagation API #797
Conversation
Codecov Report
@@ Coverage Diff @@
## master #797 +/- ##
========================================
- Coverage 94.2% 93.9% -0.3%
========================================
Files 245 185 -60
Lines 10660 6676 -3984
Branches 1042 630 -412
========================================
- Hits 10042 6269 -3773
+ Misses 618 407 -211
|
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
b927da8
to
c81682a
Compare
c81682a
to
ff9bcb2
Compare
ff9bcb2
to
49b78c8
Compare
@open-telemetry/javascript-approvers we need more reviews on this one! |
/** | ||
* Set the current propagator. Returns the initialized propagator | ||
*/ | ||
public initGlobalPropagator(propagator: HttpTextFormat): HttpTextFormat { |
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.
nit: consistency with using public
keyword
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.
to be clear, we want to always use or never use?
Maybe I'm not understanding this fully but I miss the Composite Propagator. Or is this added by a separate PR? |
Will be a separate PR as the composite propagator is an SDK detail. From the API perspective it is the same as any other propagator. |
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
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
Really? API exposes |
In your opinion is the composite propagator an API concern or a blocker for this PR? Keep in mind, with this PR we are only adding the propagation API. No code paths are actually using it yet. |
Nope, it's not blocking. |
…entelemetry-js into propagation-api
…-telemetry#797) Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
This is the first step toward #777. After this, all call-sites will still need to be updated to use the new API.