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

Case insensitive keys TextMapGetter #191

Open
lacarvalho91 opened this issue May 2, 2023 · 5 comments
Open

Case insensitive keys TextMapGetter #191

lacarvalho91 opened this issue May 2, 2023 · 5 comments
Milestone

Comments

@lacarvalho91
Copy link
Contributor

As mentioned on #147 (comment) 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
Copy link
Contributor Author

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.

@rossabaker
Copy link
Member

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 👍 on this, but I wonder what others think.

@AprilAtBanno
Copy link
Contributor

AprilAtBanno commented May 3, 2023

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

@rossabaker
Copy link
Member

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 rossabaker added this to the 0.3 milestone May 3, 2023
@lacarvalho91
Copy link
Contributor Author

lacarvalho91 commented Jun 27, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants