search

steffan-westcott / clj-otel

An idiomatic Clojure API for adding telemetry to your libraries and applications using OpenTelemetry.
https://cljdoc.org/d/com.github.steffan-westcott/clj-otel-api
Apache License 2.0
183 stars 12 forks source link

Convert TextMapGetter key to lower-case #16

Closed ghaskins closed 6 months ago

ghaskins commented 6 months ago

Certain propagators submit .get requests using camel-case keys.

For instance, the B3Propagator looks for headers such as 'X-B3-TraceId'.

Since HTTP tends to favor/enforce lower-case, there are cases where there is an impedance mismatch, and the get will fail. This mismatch materializes as a failure to connect to the propagated context. For example, the headers arriving over my HTTP/2 connection look like "x-b3-traceid" and are missed by the current TextMapGetter implementation.

I am unsure of the best fix here. The current proposal assumes that the headers are already in lower-case format. There are other places within the clj-otel codebase where this is likewise assumed, so I went in this direction for consistency.

For an example, see https://github.com/ghaskins/clj-otel/blob/28f9be09fe870243db6f7a270473975c772deac6/clj-otel-api/src/steffan_westcott/clj_otel/api/trace/http.clj#L44

This code assumes that headers like "host" and "x-forwarded-for" are already in lower-case form.

We should consider whether this is good enough or should be more proactive and either normalize the headers or use other case-insensitive approaches. The effort to clean this up can probably be undertaken as a follow-up patch.

steffan-westcott commented 6 months ago

@ghaskins Thank you for highlighting this issue. I think your change is valid as it stands and no further changes are needed. I will take a closer look with some tests before merging it in. My time is limited as it's holiday season, but I will try to get this done before the end of the month.

The Ring spec states that headers keys in the request map are in lower case. For example, we see in code used by the Jetty adapter that the request header keys are transformed to lower case. clj-otel relies on this header key transformation as it processes header values for manually created server spans, as you have noted.

The HTTP spec states that header names are case-insensitive, so any text map propagator needs to take this into account. Normalizing (lowercasing) the header keys for comparison purposes is one approach to satisfy the requirement. The Ring spec mandates the lowercasing of the request map keys; your change lowercases the queried key.

Thanks again!

steffan-westcott commented 6 months ago

I've confirmed this change to the text map getter fixes the B3 multi header propagator, for manually instrumented applications. This was verified by (temporarily) changing one of the microservices examples to us B3 multi headers for trace context propagation. Without the fix, the traces were incomplete.

This change will also potentially address similar issues when using manually instrumented applications with other propagators that get headers with names that are not all lower case. One such example is the AWS X-Ray Trace Header propagator.