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 handling of multi-value headers/attributes #433

Open
tylerbenson opened this issue Jan 31, 2020 · 18 comments · May be fixed by #4295
Open

Propagation - Getter handling of multi-value headers/attributes #433

tylerbenson opened this issue Jan 31, 2020 · 18 comments · May be fixed by #4295
Assignees
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Milestone

Comments

@tylerbenson
Copy link
Member

Some formats (http) allow for a single key to return multiple values. This is used in w3c trace-context which we plan to support:

Tracestate MAY be sent or received as multiple header fields. ... The tracestate header SHOULD be sent as a single field when possible, but MAY be split into multiple header fields.

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?

@tylerbenson
Copy link
Member Author

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?

@carlosalberto
Copy link
Contributor

Bringing this over from #424 :

Jaeger context cannot be parsed using only the Getter interface. Here's an example of Jaeger context:

uber-trace-id: <some string, similar to W3C trace context>
uberctx-tenancy: test
uberctx-user: Yuri

The trace context has a fixed key name uber-trace-id, but baggage is encoded using variable header names uberctx-{baggage-key}.

Also, the W3C tracestate header may be split into multiple entries (repeated headers).

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.

@carlosalberto carlosalberto self-assigned this Apr 21, 2020
@carlosalberto carlosalberto added this to the v0.4 milestone Apr 21, 2020
@tsloughter
Copy link
Member

Does the Otel spec have to support Uber's baggage? Is that considered a OpenTracing bridge requirement?

@bogdandrutu bogdandrutu modified the milestones: v0.4, v0.5 Apr 28, 2020
@tylerbenson
Copy link
Member Author

If we are going to adopt this, instead of an iterator approach, I suggest exposing two methods:

  • getting a list or iterator of header names/keys
  • getting the value(s) of a single header

implementing the pure iterator interface in opentracing was inefficient and resulted in additional instrumentation complexity.

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@arminru arminru added the spec:context Related to the specification/context directory label Jun 9, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 2, 2020
@reyang reyang added the priority:p1 Highest priority level label Jul 24, 2020
@tedsuo
Copy link
Contributor

tedsuo commented Sep 10, 2020

Given that headers can be converted between their single line and multiline equivalents we could require that Set and Get must both use single line representation. Under the hood, the carrier can split and concatenate multiline headers as needed.

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 exception is Set-Cookie, but I don't believe we should ever be touching that.

@Oberon00
Copy link
Member

The exception is Set-Cookie, but I don't believe we should ever be touching that.

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.

@Oberon00
Copy link
Member

Should this issue be merged with #444 by the way? It seems to me there is the same discussion on both issues.

@carlosalberto
Copy link
Contributor

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 Getter.GetAll() method to return all values for a given key (if there are multiple values, that is), as an additive change.

@open-telemetry/specs-approvers Please comment on this.

@carlosalberto
Copy link
Contributor

Ping @Oberon00

@Oberon00
Copy link
Member

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.

@carlosalberto
Copy link
Contributor

Perfect, so during our triage meeting today will discuss making this After-GA. Thanks for the reply @Oberon00 !

@andrewhsu
Copy link
Member

from the spec issue triage mtg today, discussed this and moving to after ga

@andrewhsu andrewhsu removed the priority:p1 Highest priority level label Sep 18, 2020
@jack-berg
Copy link
Member

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:

  1. Extend TextMap Get operation to return a comma separated list when a carrier has multiple values. I.e. update Get function in propagators to combine duplicate keys #1884.
  • May break callers. But if most or all callers are within control of OpenTelemetry maintainers, maybe this is ok?
  • Requires that callers be updated to be aware of how multiple values are encoded, and parse accordingly.
  1. Extend TextMap with a new operation called GetList
  • Would need to be optional to implement, since not all carriers necessarily support multiple values.
  • Callers which support multiple values need to update to call GetList instead of Get.
  • Disruptive to languages where it is difficult to extend interfaces.

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.

@jamesmoessis
Copy link
Contributor

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.

@pellared

This comment was marked as outdated.

@jack-berg

This comment was marked as outdated.

@pellared

This comment was marked as outdated.

@jamesmoessis
Copy link
Contributor

Spec PR is up, #4295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:context Related to the specification/context directory triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
Status: No status