W3C Baggage Propagator cannot extract multiple baggage headers

[jamesmoessis]

Describe the bug

The W3CBaggagePropagator.extract() is generally not able to follow the baggage specification, which states:

Multiple baggage headers are allowed. Values can be combined in a single header according to RFC 7230.

Given that the TextMapGetter.get() method returns a String, as opposed to Enumeration<String>, all instrumentations that I saw return the value for the first header and so some state is lost.

Possible solutions

There are two possible solutions in my mind:

The first is to update the TextMapGetter with a method that is able to return multiple values for a single header, and propagators such as baggage would call this. To keep it stable it could return have a default method that returns the value of get() as some kind of single-valued collection. Then, the baggage propagator could be updated to use this method, and server instrumentations could follow suit to implement this method on their carriers.

The other is to go through all instrumentations and perform the "combined in a single header [RFC 7230]" when extracting the "single" baggage header. This seems likely more hacky and specific to baggage extraction, so it's less preferable in my mind.

[trask]

hi @jamesmoessis! I think option 1 would need to be addressed at the spec level, check out this (really) old issue: open-telemetry/opentelemetry-specification#433

[jamesmoessis]

Interesting, thanks @trask - I did fear it may be an ancient spec issue. Although reading those threads it seems like some other languages may opt for the other option, combining the headers into one. I suppose this would require a re implementation of all the TextMapGetter implementations in the instrumentation. Do you have an opinion on whether that would be a reasonable solution? Or would you prefer a spec decision on the matter.

[jack-berg]

Yikes seems like a miss that we don't have this. I don't think its that's difficult to solve at a technical level, but may be hard to drum up interest in a 4+ year old issue.

I suppose this would require a re implementation of all the TextMapGetter implementations in the instrumentation.

We could extend TextMapGetter with a new default method:

  default List<String> getList(@Nullable C carrier, String key) {
    String first = get(carrier, key);
    return first == null ? Collections.emptyList() : Collections.singletonList(first);
  }

And to your point, we'd need to override this default implementation in all the instrumentations.

Hard part seems to be landing this in the spec. If you want to push on that I would be happy to sponsor you working on the issue.

[jamesmoessis]

Thanks @jack-berg for the response. Given your sponsorship on the spec issue I actually had a read of the TextMap extract spec. What I found interesting is this part:

Getter allows a TextMapPropagator to read propagated fields from a carrier.
One of the ways to implement it is Getter class with Get and Keys methods as described below. Languages may decide on alternative implementations and expose corresponding methods as delegates or other ways.

So, by my reading, the spec actually permits the addition of your getList() or similar, it just doesn't explicitly outline it.

It might be worth getting more explicit guidance from the spec, but seemingly we may be able to shortcut by not having to go through the slow process of changing the spec. If you disagree I'd be happy to raise a PR on the spec to get it changed, but it would slow down fixing this issue which has actually caused some problems for us in real scenarios. Any thoughts?

[jack-berg]

Going up a bit further in the spec we see:

A Getter invoked for each propagation key to get. This is an additional argument that languages are free to define to help extract data from the carrier.

So in total we have:

• A TextMap extract operation which accepts an optional Getter
• Getter is "an additional argument [...] to help extract data from the carrier"
• Getter may have Get and Keys methods, and "Languages may decide on alternative implementations and expose corresponding methods as delegates or other ways"

The last bit is the only thing that gives me pause about adding getList() - it seems like the intent is that the functionality of Get and Keys is exposed, but the spec is not opinionated at exactly how. But the definition of Get clearly says "The Get function MUST return the first value of the given propagation key or return null if the key doesn't exist".

So if we're obligated to provide Get with those semantics, adding a getList() which returns multiple values seems like it wouldn't conform.

Another reason to try to fix this in the spec, and not just in opentelemetry-java, is better interop between languages. I presume that because of the missing getList() option, there aren't any SDKs which are injecting baggage context as a list of headers, so everything should work well today as long the client and server are OpenTelemetry-based. But it would be good if any server, regardless of language, using OpenTelemetry to extract context was able to fully extract baggage headers.