-
Notifications
You must be signed in to change notification settings - Fork 825
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(jaeger-propagator): propagate/extract baggage #2137 #2158
feat(jaeger-propagator): propagate/extract baggage #2137 #2158
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2158 +/- ##
==========================================
+ Coverage 92.82% 92.83% +0.01%
==========================================
Files 139 139
Lines 5003 5024 +21
Branches 1030 1035 +5
==========================================
+ Hits 4644 4664 +20
- Misses 359 360 +1
|
packages/opentelemetry-propagator-jaeger/src/JaegerHttpTracePropagator.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-propagator-jaeger/src/JaegerHttpTracePropagator.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-propagator-jaeger/src/JaegerHttpTracePropagator.ts
Outdated
Show resolved
Hide resolved
09c9302
to
00eca4d
Compare
I've rebased and addressed comments |
00eca4d
to
bc560ac
Compare
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
packages/opentelemetry-propagator-jaeger/src/JaegerHttpTracePropagator.ts
Outdated
Show resolved
Hide resolved
@vmarchaud after conflicts and @obecny's nit you can merge |
@vmarchaud I think you can resolve conflicts and merge it |
i think he mentioned in SIG he will have time to work on otel this weekend. |
bc560ac
to
067266f
Compare
Spec: https://www.jaegertracing.io/docs/1.21/client-libraries/#propagation-format
I checked and the spec doesn't say if we should support both
HTTP_HEADERS
andTEXT_MAP
encoding but since this propagator is mostly used for http propagation, i'm not sure how we should do that.There is already a discussion in the java SIG for this (open-telemetry/opentelemetry-java#2032) and its apparently waiting for the spec to be solved. I think we should wait for spec to know if we need both or not
Fixes #2137