otel4s icon indicating copy to clipboard operation
otel4s copied to clipboard

Case insensitive keys TextMapGetter

Open lacarvalho91 opened this issue 2 years ago • 5 comments

As mentioned on https://github.com/typelevel/otel4s/issues/147#issuecomment-1531469127 I was seeing an issue where a gRPC server was not linking spans to a distributed trace. This turned out to be because I was using the provided TextMapGetter[Map[String, String]] and gRPC metadata keys are lower case. In my case I am using the B3 propagator so it would have been trying to find [X-B3-TraceId, X-B3-SpanId, X-B3-Sampled] in ["x-b3-traceid", "x-b3-spanid", "x-b3-sampled"].

To resolve this I changed my carrier type to a Map[CIString, String] and wrote a TextMapGetter implementation for that type.

Is this the correct way to deal with this? If so, shall we provide the Map[CIString, String] TextMapGetter instance in this lib?

lacarvalho91 avatar May 02 '23 15:05 lacarvalho91

I thought this might have a been an issue with the Java B3 propagator at first, but this does all seem to come down to the TextMapGetter.

lacarvalho91 avatar May 02 '23 15:05 lacarvalho91

I'm always reluctant with dependencies, but I think most practical uses will be case-insensitive. The library itself has been quite stable. There's a proposal for a 2.0 that has a lot of merit in a vacuum, but the ecosystem churn continues to make me queasy.

I'm mildly :+1: on this, but I wonder what others think.

rossabaker avatar May 02 '23 16:05 rossabaker

I'm not certain this works very well. while it's easy to implement TextMapGetter#get case-insensitively, TextMapGetter#keys has to return a collection of String, not CIString, so it might end up ignored anyway, and you just have to hope that whoever calls TextMapGetter#keys bothers to use String#equalsIgnoreCase during iteration.

Edit: incidentally, your hope will be in vain; the two Java implementations that use TextMapGetter#keys (OtTracePropagator and JaegerPropagator) both use String#startsWith, which is case sensitive

AprilAtBanno avatar May 03 '23 17:05 AprilAtBanno

The spec wants to come out and say things are case-insensitive, but because it abstracts over protocols, can't.

On setter:

The implementation SHOULD preserve casing (e.g. it should not transform Content-Type to content-type) if the used protocol is case insensitive, otherwise it MUST preserve casing.

On getter:

The Get function is responsible for handling case sensitivity. If the getter is intended to work with a HTTP request object, the getter MUST be case insensitive.

In practice, some text map getters normalize their keys, and some others don't. Oof.

rossabaker avatar May 03 '23 18:05 rossabaker

I looked into this again. It seems the problem only comes when a caller uses .keys as highlighted above by @AprilAtBanno. After a little look I could only see .keys used for baggage propagator implementations.

I've confirmed that this 100% fixes the problem of case sensitivity for my use case, which is using the b3 propagator for grpc requests. Every propagator does a .get so I think this adds value, the problem is just when baggage is involved as that seems to be when .keys is used.

lacarvalho91 avatar Jun 27 '23 12:06 lacarvalho91