Feat request: Support W3C Trace Context header

[Renz2018]

see jaegertracing/jaeger#855
W3C trace context now is "W3C Recommendation 06 February 2020"

[yurishkuro]

up for grabs

[Tecktron]

Looking at the W3C spec, it seems that the list of To-Dos is as follow:

• set constant TRACE_ID_HEADER to use traceparent as default
• set constant BAGGAGE_HEADER_PREFIX to use tracestate as default.
• update the the output format of the traceparent header
• add the ability to handle different version formats of the traceparent in the future, with 00 being the only version currently supported.
• adjust the baggage headers to generate/accept a single header in format of "key=value,key=value" instead of multiple headers.

Some question regarding this:

• How would the backwards compatibility work, would this be the new default with a flag for legacy or would the current be the default and the flag would be for switching to w3c?
• Is py2 support really still a requirement?

Some other important thoughts/considerations:
From what I'm reading, it seem that opentracing is merging with opencensus to make the opentelemetry package. If that truly is the case, does it make sense to deprecate this and the opentacing package in favor of switching to the opentelemery project? It would seem that they have provided a shim for compatibility with opentracing code but as my linked issue showcases, it doesn't seem to be fully compatible.

I'm left at a cross roads here. I need to become compatible with the W3C standards because other services in the stack I'm working with are now using this standard, rendering this package incompatible and therefore obsolete for my use. So is it worth the effort to update this client or is it better to just going all in and refactor with opentelemetry? Either way, it seems like there is currently no easy/working drop-in solution and that code is going to need to be refactored no matter which outcome.

Any thoughts or reasonings would be appreciated. Cheers.

[yurishkuro]

First, there's no need to change anything, the w3c should be a new codec, similar to Zipkin, because it uses a different encoding format from the default Jaeger encoding in TextCodec.

jaeger-client-python/jaeger_client/codecs.py

Line 213 in 7984b28

class ZipkinCodec(Codec):

The choice of the codec should be controlled by the JAEGER_PROPAGATION environment variable and the corresponding Configuration option. It seems right now only Java client supports this property (jaegertracing/jaeger-client-java#736, jaegertracing/jaeger-client-go#277). By default the Jaeger native codec should be used as today.

Regarding OpenTelemetry, as of today it has not released a 1.0/GA version. If you're just starting instrumentation in a large code base, I would definitely look at OpenTelemetry first, maybe take a bit of precaution against potential API changes (given how close it is to GA those changes are not very likely). Having said that, OpenTracing and this library in particular have a large production install base already, so there are no plans to deprecate it, not to mention that OpenTelemetry SDK does not yet have the equivalent functionality like remotely controlled sampling.

W3C trace context is already supported in Java and Go Jaeger SDKs, I think it's perfectly reasonable to support it here as well.

[Tecktron]

@yurishkuro thank you very much for the insights into this. This clears up a lot of the fog around the landscape here. Given this knowledge, I agree the most reasonable path based on where everything currently is will be to support the additional codec.

Already knowing this is going to be work for me no matter what the choice was, I think it only makes sense then to place my efforts here. In other words, I'll pick this up.

[Tecktron]

@yurishkuro I need to write a few more tests for further coverage and do a little code cleanup/commenting (which is why I have not made a PR yet) but before continuing any further, I wanted to make sure that I was on the correct path not only in terms of implementation but also in terms of handling the standard.

Please see the compare:
master...Tecktron:w3c-codec

Cheers.

[yurishkuro]

On a high level it looks ok (I couldn't leave comments in the code).

I am skeptical about needing the TraceState class. Jaeger span context mostly fits into traceparent field (1), so we only need to pass tracestate unchanged, which means we don't even need to parse it. It's probably more standard-compliant to parse it and reject when it's malformed than to send it out as malformed and get blamed - ymmv.

But mostly I prefer to avoid the overhead of instantiating a new TraceState object when it's not needed, so if it's scoped to only be used when W3C codec is used, then it's ok to have a class.

(1) there are a few bits in Jaeger flags that cannot be represented in traceparent.flags and if you want to preserve them then tracestate would need to be used, but I would start without that.

[Tecktron]

Thanks for the feedback. I've made it a draft PR instead of a compare so you can feel free to comment.
#288

I added the tracestate to keep track of the traces, pushing the current span id into it, not knowing if anything else along the line in the stack might actually be using it. I figure it was better for compliance, just in case, even if we're mostly just tearing it up and down as we pass it along.

To your point, yes, a TraceState instance does now get initiated in the span context by default, but it's does not get used by anything other than this codec so it basically just sits there. I wasn't sure if/how we could peek at the config at the level where the context was being generated and felt an unused class was an easier route than trying to push that though. I also was not sure about any possible side effects with simply tacking it on with a setattr/getattr during the extraction/injection process, though I'm open to exploring this possibility as well if you feel this adds too much overhead in the context.

To that point, I was also thinking that the maybe the base trace state class might be better off eventually living in the opentracing project, but to not add further complexity or requirements, maybe that's an update best left for another day.

[yurishkuro]

maybe that's an update best left for another day

Another day will probably never come, given the current state of OpenTelemetry :-)

[Tecktron]

Sorry for the delay in getting back to this. I added more tests and have opened the PR for review. Still awaiting the coverage report, seems like that might be stuck waiting for something? If there's anything more I need to do here, please let me know.
Cheers!

[PatrikSteuer]

What is the state of this? It still seem to be stuck ;)

[Tecktron]

I recently switched companies which has left me with little to no time to come back to this at the moment and while I will come back to this, it may not be soon. Feel free to pickup where I left off on the PR. However, with all the changes required you might be better off using it as a jumping point and starting from scratch.

[sergiomacedo]

Hello @Tecktron. Any hope you'll be able to come back to this?

Cheers.

[Tecktron]

Sorry for the long delay, yes, I should be able to get back to this in the next coming month.

[tungdam]

Hi @Tecktron any news on this ?
Thanks in advance

[zhichli]

W3C trace context is already supported in Java and Go Jaeger SDKs

@yurishkuro a bit off-topic question: could you please tell how to enable W3C tracing context in Jaeger Go client (which is used by our Grafana instance) because I could not find how ? All I found was to enable 128 bit traceId that is compatible with W3C, but it seems that the W3C traceparent header is not recognizable in Jaeger Go client from what I see in the source code and my test. I have some upstream service that emits W3C traceparent header with ApplicationInsights SDK, but the downstream Grafana with Jaeger Go client could not extract the tracing context.

Also this doc link shows the W3C feature is still coming. https://www.jaegertracing.io/docs/1.24/client-features/

[yurishkuro]

@zhichli it's not supported by Go client yet.