XCloudTraceContextPropagator assumes lowercase header

[mackenziestarr]

XCloudTraceContextPropagator assumes the carrier parameter contains a lowercase header x-cloud-trace-context and as a result other valid capitalization styles of this header e.g. X-Cloud-Trace-Context are not extracted. This constraint makes the propagator class less flexible and I think there is benefit to allowing other common capitalizations of the header.

opentelemetry-operations-java/propagators/gcp/src/main/java/com/google/cloud/opentelemetry/propagators/XCloudTraceContextPropagator.java

Lines 93 to 94 in 72be92b

	// TODO - Do we need to iterate over keys and lowercase them, or is java doing that for us?
	String value = getter.get(carrier, FIELD);

Proposed fix: support X-Cloud-Trace-Context in addition to the currently supported x-cloud-trace-context

• when injecting the context, write both keys to the carrier
• when extracting the context, lookup both keys in the carrier

this fix seems ideal from a performance perspective (vs. the suggested iterating over the keys and lowercasing them) but might have other implications

[dashpole]

Triage: IIRC, we do this for other languages, so we probably should here as well.

[dashpole]

I would have expected the carrier (e.g. http header implementation) to handle this. We should check that first.

@mackenziestarr what were you trying to do when you encountered this? Was it with OTel's HTTP library, or something else?

[mackenziestarr]

@dashpole in this case I'm integrating OTel into a thrift server that utilized a TCP transport so the "header" is passed in via a map<string, string> context field in the thrift request message - I can also change the clients to send a lowercase version and close this issue but i thought wider compatibility for different header capitalization styles might be desirable still because of the TODO in the source.

[psx95]

Hi @mackenziestarr,

I looked into this and I think the TODO comment should have been removed/updated. It is from a very old commit and I think spec might have changed since then - I searched around and found that the current spec around Get method mentions that implementation of Get function is responsible for handling case sensitivity.

Since we don't provide an implementation for TextMapGetter, I do not think this lowercasing should be addressed in these propagators. The users using this propagators should provide for an appropriate implementation for the getter method. For instance, we use the propagators in our e2e-tests and we provide a TextMapGetter implementation that deals with case-sensitivity.

Let me know if my interpretation of the spec makes sense to you.

[dashpole]

@psx95 should we open an issue upstream?

[psx95]

@dashpole an issue to update the propagators API spec or to somehow provide commonly used getters ?

[dashpole]

Sorry, I had forgotten context from #365 (comment). @mackenziestarr when you implement your TextMapGetter, that is where the lowercasing should happen.

@psx95 you can close the issue once you've removed the comment.

[mackenziestarr]

@psx95 thanks the explanations and links to the spec, it wasn't very intuitive to me that one would need to iterate over entries in the map to test for the header key provided by XCloudTraceContextPropagator.

i was hoping for an API that provided a default implementation of a TextMapGetter for Map so one could just call it like

Map<String, ?> input;
var context = new XCloudTraceContextPropagator(true).extract(Context.current(), input)

but understand if that is too specific to my own implementation

thanks for your time here @psx95 and @dashpole, we can call this closed