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

ExtractHTTP should stop after extracting first valid trace context #501

Closed
rghetia opened this issue Feb 28, 2020 · 3 comments · Fixed by #923
Closed

ExtractHTTP should stop after extracting first valid trace context #501

rghetia opened this issue Feb 28, 2020 · 3 comments · Fixed by #923
Assignees
Labels
area:propagators Part of OpenTelemetry context propagation

Comments

@rghetia
Copy link
Contributor

rghetia commented Feb 28, 2020

There is an issue opened on spec to clarify the behavior. There are two problems highlighted in the current implementation.

  1. The Last trace context is invalid and the first trace context is valid - Even though the first context is valid it will not use it.
  2. Both of them are valid - The last context will be used but that may not be counter intuitive to the user. Also it involves unnecessary processing. Extracting is not cheap.
@rghetia rghetia added the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Feb 28, 2020
@rghetia rghetia added this to the Alpha v0.4 milestone Feb 28, 2020
@MrAlias MrAlias modified the milestones: Beta v0.6, Beta v0.7 May 15, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Jul 2, 2020

open-telemetry/opentelemetry-specification#671 merged with the added clarification:

If a value can not be parsed from the carrier for a cross-cutting concern, the implementation MUST NOT throw an exception and MUST NOT store a new value in the Context, in order to preserve any previously existing valid value.

This clarifies 1 directly.

As for 2, it seems implied that extracting both, though not optimal, is what the specification assumes to be done.

@MrAlias MrAlias added area:propagators Part of OpenTelemetry context propagation and removed blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Jul 2, 2020
@Aneurysm9
Copy link
Member

It seems that the correlation context propagator may be the only implementation that needs to change. Both the B3 and W3C trace context propagators return the input context unmodified if unable to extract a valid SpanContext. Are there other Propagator implementations I'm missing?

@MrAlias
Copy link
Contributor

MrAlias commented Jul 8, 2020

Are there other Propagator implementations I'm missing?

No, this should be all.

Did you want to take this issue @Aneurysm9?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:propagators Part of OpenTelemetry context propagation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants