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

Clarify b3 debug flag and parent span id handling #1045

Merged
merged 4 commits into from
Oct 28, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Oct 1, 2020

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.

@mwear mwear requested review from a team October 1, 2020 00:40
@carlosalberto carlosalberto added area:api Cross language API specification issue release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory labels Oct 7, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 15, 2020
@carlosalberto
Copy link
Contributor

@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.

Copy link
Member

@arminru arminru left a 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?

specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Oct 16, 2020
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Copy link
Contributor

@anuraaga anuraaga left a 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.

@Oberon00
Copy link
Member

Would #881 help with handling the debug flag better? You could store it in the Context then and the sampler would receive it.

@carlosalberto
Copy link
Contributor

@Oberon00 I think it could definitely help, also adding it is probably a post-GA task at this point.

@Oberon00
Copy link
Member

To clarify: You mean both this PR and #881 should be OK for GA, but a more advanced debug flag handling for B3 based on #881 would be after-ga, right?

@carlosalberto
Copy link
Contributor

@Oberon00 Yes :)

@tedsuo tedsuo self-requested a review October 20, 2020 15:43
@tedsuo
Copy link
Contributor

tedsuo commented Oct 20, 2020

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.

specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Outdated Show resolved Hide resolved
specification/context/api-propagators.md Show resolved Hide resolved
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@mwear
Copy link
Member Author

mwear commented Oct 22, 2020

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.

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 <christian+github@neumueller.me>
@carlosalberto
Copy link
Contributor

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.

@jkwatson
Copy link
Contributor

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?

@Oberon00
Copy link
Member

See #1045 (comment) and #1045 (comment): I guess so, yes.

@jkwatson
Copy link
Contributor

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. ;)

@Oberon00
Copy link
Member

@mwear said

I purposely left this ambiguous because it's an implementation detail

But I guess a non-normative note would indeed be helpful here.

@jkwatson
Copy link
Contributor

@mwear said

I purposely left this ambiguous because it's an implementation detail

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

B3 consistent handling of debug flags and parent span id
7 participants