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

HttpTraceContext requires version 00 #979

Closed
kintel opened this issue Apr 22, 2020 · 3 comments · Fixed by #1269
Closed

HttpTraceContext requires version 00 #979

kintel opened this issue Apr 22, 2020 · 3 comments · Fixed by #1269
Labels
bug Something isn't working up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@kintel
Copy link

kintel commented Apr 22, 2020

From reading code, it looks like we require the traceparent version to be 00:
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts#L30

The W3C spec says that we should still attempt to parse the known fields, as future extensions will be additive:
https://www.w3.org/TR/trace-context/#versioning-of-traceparent

This should probably be fixed by allowing any 2-digit lower-case hex value except ff.

While at it, we should also verify that we accept any additional data in the header, given that the first extra character is -.

Perhaps glance at the Go implementation, which has better compliance: https://github.com/open-telemetry/opentelemetry-go/blob/669d4b3a6c8d01821a65026f49df1b85d2972c14/api/trace/trace_context_propagator.go#L39

@kintel kintel added the bug Something isn't working label Apr 22, 2020
@mayurkale22
Copy link
Member

Thanks for reporting this! Yes, you're right. We should replace 00 => [\da-f]{2} or something like this.

@mayurkale22
Copy link
Member

If you are feeling motivated, maybe you can attempt to write solution and open a PR.

@mayurkale22 mayurkale22 added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Apr 22, 2020
@kintel
Copy link
Author

kintel commented Apr 22, 2020

I'll have a look once I get my C++ implementation solid. ..and I may have to open this issue against every language in Otel : /

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants