Propagation: Support extraction of multiple values for a single key

[jamesmoessis]

In order to fully support W3C baggage specifications (and some others like tracestate), micrometer must be able to receive and extract multiple headers of the same key. For example, two baggage headers.

This is now fully supported by OpenTelemetry (spec PR) and also recently stabilised in opentelemetry-java (PR).

The micrometer-otel-bridge will need updating in order to support this, and to align with the W3C spec, and bridge to OpenTelemtry's getAll(), which will be released as stable in opentelemetry-java 1.50 release within the coming days.

The Getter interface in Micrometer will need to be updated - a method to allow List<String> getAll() on top of the existing String get(). This can be a default method that just return List.of(get()) if not implemented, for backwards compatibility.

It's possible I may have missed other parts that need to be handled - if so feel free to add / correct me!

[aelse-atlassian]

To help with triage/prioritisation, I see this is labeled as an enhancement but lack of this support should more correctly be noted as a bug.

The W3C baggage spec notes that multiple baggage headers are allowed. Specific examples given of header format which include this possibility.

[shakuzen]

will be released as stable in opentelemetry-java 1.50

To help with triage/prioritisation, I see this is labeled as an enhancement but lack of this support should more correctly be noted as a bug.

OpenTelemetry Java having no long-term support minor releases and releasing a new minor every month makes it at odds with version upgrade policies many (most?) open source Java projects take, which is that we do not generally upgrade to new minor (or major) versions of dependencies that will be transitive to our users unless it is in a new minor version of our project. That said, since new public API was added for this in OTel, it probably wouldn't have been appropriate for it to be added in a patch release anyway. So if I'm understanding correctly that to support this in the OTel bridge, we need to depend on opentelemetry-context 1.50.0 or later, that's probably going to have to go in our next feature release (minor version), not in a patch release where it would be against policy to upgrade to a new minor of a dependency (and especially to use new API added in that new minor).

I respect the argument that this didn't support the W3C spec, and that's fair to say, but if we can't solve it without new API, it's safest to fix it in a new minor.

I also wonder if this is more than a theoretical problem, given it apparently hasn't been supported until now in OTel? Is something propagating multiple baggage headers, and is there a reason why it's doing that rather than using a single baggage header?

I took a look at what we need to do to support this. In addition to the Getter interface in Tracing, we would need to update the Getter interface in the micrometer-observation module in the Micrometer repo, which is what most instrumentations actually implement. Of course, then we need to update all those implementations or this doesn't achieve the goal of being able to extract values from repeated headers.

[shakuzen]

I've begun a PR in the micrometer repo to add the necessary API to the Observation Propagator Getter: micrometer-metrics/micrometer#6312. Once that is merged, a PR can be made for this repo.

[jamesmoessis]

Thankyou @shakuzen! Apologies for getting back so late. Fixing in a minor release sounds good to me. Thanks for opening the PR in the observation API. Is there anything stopping it from moving out of draft? Just the version thing?

[shakuzen]

I was waiting to see if I got any response on my feedback about the API in OTel to know if that was going to change in some way before making changes depending on it.

[aelse-atlassian]

I'm a bit late responding to these (sorry).

I also wonder if this is more than a theoretical problem, ...

This has caused us production issues. These have been addressed when discovered, but the possibility of anything in the request chain not correctly propagating the header means ongoing risk of reoccurrence.

...given it apparently hasn't been supported until now in OTel?

I suspect this has been an oversight (but i'm not involved with this effort). The docs at https://www.w3.org/TR/baggage/ indicate an intent to audit the OTel implementations for compliance with the standard.

Is something propagating multiple baggage headers, and is there a reason why it's doing that rather than using a single baggage header?

Bear in mind that the intent of this header is to be propagated across multiple requests and systems. It is valid to add an item to Baggage without fully processing or normalizing the existing headers. That could happen with the request handling context of an individual system, but it's also likely to happen with distributed requests.

I think we're all acknowledging that the current behaviour doesn't comply with the spec (for whatever reason).

My thanks to everyone involved in the fix. :)

[shakuzen]

It is valid to add an item to Baggage without fully processing or normalizing the existing headers.

Right. But there's no need to "fully process or normalize" the existing headers. You just need to concatenate the baggage you want to add onto the end of the existing header. At least for HTTP headers, it must treat a repeated header that way, so it isn't different semantically. The producer of that extra header is probably doing what it thinks is easier for it, but the consequence is now every consumer needs to check ALL headers instead of just getting the one. In practice, that can have significant performance penalties. The spec I think would have done the ecosystem a favor to be more strict here. And producers could do everyone a favor by always using a single header and make this a non-problem in practice. I guess it won't matter anymore since everyone is going to pay the penalty to check all the headers to be spec compliant now.

[shakuzen]

With micrometer-metrics/micrometer#6312 merged, I've opened #1131 here.