-
Notifications
You must be signed in to change notification settings - Fork 890
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 handling of multi-value headers/attributes #433
Comments
In the spec call, there was some consensus that we should handle multiple values within the getter. This implies that the propagator will always need implicitly handle the possibility of multiple values rather than explicitly dealing with a list. @bogdandrutu is this a good summary? Think the spec should be changed or should I just close this issue? |
Bringing this over from #424 : Jaeger context cannot be parsed using only the Getter interface. Here's an example of Jaeger context:
The trace context has a fixed key name Also, the W3C In the OpenTracing there was an iterator over all headers in order to handle this case, not just a Getter. Consider switching to an iterator-based approach. |
Does the Otel spec have to support Uber's baggage? Is that considered a OpenTracing bridge requirement? |
If we are going to adopt this, instead of an iterator approach, I suggest exposing two methods:
implementing the pure iterator interface in opentracing was inefficient and resulted in additional instrumentation complexity. |
Given that headers can be converted between their single line and multiline equivalents we could require that
The exception is |
In fact, Dynatrace uses cookies in some cases to link JavaScript-based user-actions with backend services. Not sure if we need Set-Cookie or just Cookie for that though. |
Should this issue be merged with #444 by the way? It seems to me there is the same discussion on both issues. |
Hey @Oberon00 could you dig/elaborate on the usage of cookies? I'm thinking this should be moved to After-GA - this is because A) What @tedsuo mentions and B) if we really need to add this functionality, @tylerbenson proposed to add a @open-telemetry/specs-approvers Please comment on this. |
Ping @Oberon00 |
I looked and we do use Set-Cookie for injection in some cases, but I think this is special enough that we would not use the propagator interface anyway there. |
Perfect, so during our triage meeting today will discuss making this After-GA. Thanks for the reply @Oberon00 ! |
from the spec issue triage mtg today, discussed this and moving to after ga |
Discussed in the 11/5/24 spec SIG. There was consensus that this is something we ought to fix, with different ideas on how. Two general ideas were discussed:
It seems like the second option is more correct, and better in the long term. Labeling as ready with sponsor, as I'm willing to sponsor this and work with @jamesmoessis to explore solutions. |
I've prototyped method (2) from @jack-berg's above comment. Referenced above are the two prototypes in Java and Golang. Tests with HTTP are given. No issues regarding backwards compatibility came up. I can start on a spec PR to resolve this once the prototypes are sanity checked. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Spec PR is up, #4295 |
Some formats (http) allow for a single key to return multiple values. This is used in w3c trace-context which we plan to support:
We don't currently specify this requirement in our spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md#getter-argument
As a consequence, we aren't handling that well in java, and likely other languages:
https://github.com/open-telemetry/opentelemetry-java/blob/v0.2.x/api/src/main/java/io/opentelemetry/context/propagation/HttpTextFormat.java#L123
How do we want to handle this? Should we push responsibility for joining them properly to the
Getter
, or should the API support returning multiple values?The text was updated successfully, but these errors were encountered: