Multiple baggage headers do not work for gRPC

[ncabanis]

Describe the bug
The specification allows to add multiple baggage headers to a request. The otel-agent regards only the last one.

Steps to reproduce
Doing a gRPC request with multiple keys in one header does work:

grpcurl 
    -plaintext -d '{ "message": "something" }' 
    -H "baggage: first=A, second=B"
    localhost:8080 echo.EchoService.ping

Calling headers.get(baggageKey) results here in "first=A; second=B".

Doing a gRPC request with multiple keys over multiple headers does not work:

grpcurl 
    -plaintext -d '{ "message": "something" }' 
    -H "baggage: first=A" 
    -H "baggage: second=B"
    localhost:8080 echo.EchoService.ping

Calling headers.get(baggageKey) results here in "second=B".
Correct would be to call headers.getAll(baggageKey) which returns ["first=A", "second=B"].

What did you expect to see?
All baggage keys should be available via Baggage.current().

What did you see instead?
Only the last headers value.

What version are you using?
v1.2.0, v1.3.0, v1.5.3

Environment
gRPC 1.40.1

[mateuszrzeszutek]

Link to a related discussion in the spec repo: open-telemetry/opentelemetry-specification#433

Also an interesting excerpt from the HTTP spec:

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.

I guess we could just concatenate all known header values and return them as one in the getter - maybe should we do that in all getter implementations.

[trask]

I guess we could just concatenate all known header values and return them as one in the getter - maybe should we do that in all getter implementations.

this seems reasonable to me

[xiangtianyu]

Is there a possible case that there are different concatenation character in different situations? Maybe add new api in TextMapGetter is a better way?

[mateuszrzeszutek]

Is there a possible case that there are different concatenation character in different situations? Maybe add new api in TextMapGetter is a better way?

In general I'd agree with you and suggest returning multiple values from the getter; but in this case, the text map propagation API is heavily based off of HTTP headers, and the HTTP headers spec is pretty well-guarded against any possible misuse:

   A sender MUST NOT generate multiple header fields with the same field
   name in a message unless either the entire field value for that
   header field is defined as a comma-separated list [i.e., #(values)]
   or the header field is a well-known exception (as noted below).

All the "standard" propagation protocols were designed with these criteria in mind, joining multiple values with a comma should not break anything.
If the spec ever redesigns this API we'll come back to it, but for now the concatenation seems to be a safe approach.

Also see open-telemetry/opentelemetry-specification#433 (comment)

[xiangtianyu]

Is there a possible case that there are different concatenation character in different situations? Maybe add new api in TextMapGetter is a better way?

In general I'd agree with you and suggest returning multiple values from the getter; but in this case, the text map propagation API is heavily based off of HTTP headers, and the HTTP headers spec is pretty well-guarded against any possible misuse:

   A sender MUST NOT generate multiple header fields with the same field
   name in a message unless either the entire field value for that
   header field is defined as a comma-separated list [i.e., #(values)]
   or the header field is a well-known exception (as noted below).

All the "standard" propagation protocols were designed with these criteria in mind, joining multiple values with a comma should not break anything. If the spec ever redesigns this API we'll come back to it, but for now the concatenation seems to be a safe approach.

Also see open-telemetry/opentelemetry-specification#433 (comment)

You are right. But i'm afraid if users use their custom propagators, the comma may cause opposite effects

[mateuszrzeszutek]

You are right. But i'm afraid if users use their custom propagators, the comma may cause opposite effects

It is possible. But without a change in the OpenTelemetry spec, we cannot introduce a new TextMapGetter API.

[xiangtianyu]

You are right. But i'm afraid if users use their custom propagators, the comma may cause opposite effects

It is possible. But without a change in the OpenTelemetry spec, we cannot introduce a new TextMapGetter API.

So is there any other efficient way to solve this problem without these defects?

[anuraaga]

You are right. But i'm afraid if users use their custom propagators, the comma may cause opposite effects

Baggage is a comma-separated list of pairs even with a single header. Is there any case we can imagine where there would be breakage by combining multiple baggage header values with comma?

[xiangtianyu]

You are right. But i'm afraid if users use their custom propagators, the comma may cause opposite effects

Baggage is a comma-separated list of pairs even with a single header. Is there any case we can imagine where there would be breakage by combining multiple baggage header values with comma?

If we just do this change on baggage, it's ok. But if we want to put it into getter/setter, we should consider the effects to other propagators especially the user custom propagators.

[howardjohn]

Since no one has linked it, thought I would add the actual baggage spec, which shows this: https://www.w3.org/TR/baggage/#examples-of-http-headers