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

Create a combined B3 Propagator that handle both single and multi-header cases #1684

Closed
jkwatson opened this issue Sep 23, 2020 · 6 comments · Fixed by #1775
Closed

Create a combined B3 Propagator that handle both single and multi-header cases #1684

jkwatson opened this issue Sep 23, 2020 · 6 comments · Fixed by #1775
Assignees
Labels
help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release

Comments

@jkwatson
Copy link
Contributor

See the recently updated spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#b3-requirements

@jkwatson jkwatson added help wanted release:required-for-ga Required for 1.0 GA release priority:p2 Medium priority issues and bugs. labels Sep 23, 2020
@iNikem
Copy link
Contributor

iNikem commented Sep 23, 2020

Won't TraceMultiPropagator solve that?

@jkwatson
Copy link
Contributor Author

Won't TraceMultiPropagator solve that?

It might, but the spec says when you enable the B3 propagator, it should handle both cases, which we don't at the moment. Are you thinking that this should be implemented in the agent, rather than propagator itself?

@carlosalberto
Copy link
Contributor

Won't TraceMultiPropagator solve that?

I think we should still have this as a single B3Propagator.

@jarebudev
Copy link
Contributor

i can pick this one up

@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Oct 8, 2020

I don't know if this is the proper place but I think that https://github.com/open-telemetry/opentelemetry-java/blob/master/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/B3PropagatorInjectorSingleHeader.java is not really working exactly as it should. If there's a parent span id it should also get appended to the the single b3 header. You can check how they do it in Brave https://github.com/openzipkin/brave/blob/4a4d8bf49318ae049669c5ccc8e451102e826cca/brave/src/main/java/brave/propagation/B3SingleFormat.java#L114-L139

Example in Brave

traceId - spanId - sampled - parentSpanId
00000000000000010000000000000000-ac85aa59b0498d38-1-0000000000000002

Example in Brave without parentSpanId

traceId - spanId - sampled - parentSpanId
00000000000000010000000000000000-ac85aa59b0498d38-1

how it's done in OTel

traceId - spanId - sampled
00000000000000010000000000000000-0000000000000002-1

@carlosalberto
Copy link
Contributor

@marcingrzejszczak See open-telemetry/opentelemetry-specification#1045 ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants