-
Notifications
You must be signed in to change notification settings - Fork 893
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
Clarify b3 debug flag and parent span id handling #1045
Conversation
58a972d
to
10c99be
Compare
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@open-telemetry/specs-approvers Please review this PR, as it's gone stale given the lack of reviews. Observe that at this moment is basically mentions the current state of B3 Propagators in our implementations. |
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.
The PR looks reasonable but I don't know B3 and cannot assess the implications of this.
Do we have any reviewer that's more familiar with B3 that we can ping besides you, @carlosalberto?
Co-authored-by: Armin Ruech <[email protected]>
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.
This seems fine to me, if we decide to support propagating span ID in the future, it would be a backwards compatible spec change.
Would #881 help with handling the debug flag better? You could store it in the Context then and the sampler would receive it. |
@Oberon00 I think it could definitely help, also adding it is probably a post-GA task at this point. |
@Oberon00 Yes :) |
This looks good to me. As a hedge, at some point we could get the Zipkin community to review the B3 portion of our spec, to make sure we didn't miss something. |
Co-authored-by: Christian Neumüller <[email protected]>
My guess is that the Zipkin community will request that we propagate ParentSpanID to support client and server spans sharing the same span id. We should get their input to understand what the potential side effects are for not propagating it. |
Co-authored-by: Christian Neumüller <[email protected]>
Merging as there are enough approvals, this PR has been standing for a while, and it also clarifies what we already have now in most of the implementations. |
do we have instructions on how we're supposed to propagate the debug flag, exactly? Is this something that the propagator needs to put into the Context? |
See #1045 (comment) and #1045 (comment): I guess so, yes. |
Hmm. implementation recommendations in hidden comments probably aren't the best way to communicate them. ;) |
@mwear said
But I guess a non-normative note would indeed be helpful here. |
Should this be an implementation detail, @mwear ? Is there some other way in OTel for information to be propagated that I'm not aware of? Isn't the context the only way that propagators can possibly propagate this data? |
Fixes #1004
Changes
This PR clarifies how OpenTelemetry should handle the B3 debug trace flag and parent span id propagation.
As discussed in issue #1004, OpenTelemetry does not support client and server spans sharing an id when they are part of the same request. To enable this, B3 support propagating a parent span id, in addition to the current span id. As a result, OpenTelemetry safely ignore the parent span id when propagating B3.
When receiving a B3 debug trace flag, an OpenTelemetry implementation should preserve and propagate the debug trace flag, but internally treat it as a sampled trace flag.
See https://github.com/openzipkin/b3-propagation for more details.