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

Propagation Getter spec can't support W3C spec #1870

Closed
tsloughter opened this issue Aug 17, 2021 · 3 comments
Closed

Propagation Getter spec can't support W3C spec #1870

tsloughter opened this issue Aug 17, 2021 · 3 comments
Assignees
Labels
area:api Cross language API specification issue spec:context Related to the specification/context directory

Comments

@tsloughter
Copy link
Member

The specification for Otel's TextMap propagator Get function and the W3C spec conflict in how to handle multiple of the same key in a list of pairs. The Otel spec has an optional Get that must return value, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#get, while in W3C in the case of traceparent a new trace should be started: https://github.com/w3c/trace-context/blob/main/test/test.py#L136-L137

For tracestate W3C says it must combine the values of the tracestate key.

I think Getter has to change to return every pair with the key being looked up. But this is not backwards compatible? Maybe it will have to be that the get function can return a single value if only one is found but a list if there are multiple? That way existing Getter implementations continue to work.

@tsloughter tsloughter added the spec:context Related to the specification/context directory label Aug 17, 2021
@carlosalberto carlosalberto added the area:api Cross language API specification issue label Aug 17, 2021
@mwear
Copy link
Member

mwear commented Aug 17, 2021

It would be possible to combine the multiple headers into one by concatenating the values separated by a ,. This is briefly mentioned in the W3C Trace Context spec. It says:

When receiving multiple tracestate header fields, they MUST be combined into a single header according to RFC7230.

The relevant part of RFC7230 says:

A recipient MAY combine multiple header fields with the same field name into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field value to the combined field value in order, separated by a comma. The order in which header fields with the same field name are received is therefore significant to the interpretation of the combined field value; a proxy MUST NOT change the order of these field values when forwarding a message.

@tsloughter
Copy link
Member Author

Yes, this is what we do in the Erlang W3C propagator implementation. But this became not possible in the propagator itself when adding support for a Get function that can be set by the user and only returns the first element.

An option instead of adding a GetAll or having Get return a list would be to define Get to have to combine duplicates using the method of RFC7230 if it is for a data structure that can have duplicates -- meaning if your Get is defined for a dictionary it wouldn't have to do anything special, but one that takes a list or array would have to search for all matches and combine them.

@tsloughter tsloughter changed the title Propagation Setter spec can't support W3C spec Propagation Getter spec can't support W3C spec Aug 23, 2021
@tsloughter
Copy link
Member Author

Closing because I found #433 already exists from a year and a half ago and I've opened #1884

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 spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

4 participants