feat: add tracecontext propagation #97
Conversation
Based on w3c TraceContext spec: https://w3c.github.io/distributed-tracing/ The `traceparent` header is validated and propagated according to spec. The only caveat is that the option flags are lost downstream and reset to "0x1" by the tracer. This will require a refactor of how the tracer treats the options as certain assumptions are being made that the least-significant bit will always represent "sampled". The `tracestate` is propagated downstream, but is currently not mutable by the tracer. It is simply stored as a string, but types have been generated to allow this to be refactored into a more structured type in the future, allowing the tracer to update the tracestate as necessary. This commit does not attempt to implement the `Correlation-Context` header. issue census-instrumentation#95
Defaults were being flagged as "Requested", which is incorrect. Also added documentation in the root-level readme about added Trace Context support.
| // Parse TraceParent into version, traceId, spanId, and option flags. All | ||
| // parts of the header should be present or it is considered invalid. | ||
| const parts = traceParent ? traceParent.split('-') : []; | ||
| if (parts.length === 4) { |
There was a problem hiding this comment.
Would it make more sense to return null if this condition (or subsequent conditions) isn't true?
There was a problem hiding this comment.
Good question. The spec does not describe how to handle the tracestate header when the traceparent is invalid. I decided to try to pass the tracestate if at all possible. The tracestate was added to the SpanContext model and you can see on line 66 that we attempted to set the traceState if possible, which means that even if the trace parent information can't be propagated, we can still propagate the trace state information. Returning null drops both.
That being said, looking at the tracer.ts code again, it looks like if the sampling flag isn't turned on the spanContext is set back to null anyways, so that information is dropped, so my statement above is basically moot at this point I think..
So two options from here:
- Refactor the tracer to make sure that the tracestate information is NOT dropped if the trace is not propagated.
- Return null from the propagation to make it more obvious what is happening
Thoughts between those two? Two is definitely the easier path forward.
There was a problem hiding this comment.
I think (1) is a better choice, even if it involves further modifying the Tracer. From the spec:
Notably, the tracestate field is unreliant on data in the traceparent.
So while it might not happen typically, a tracestate isn't necessarily invalid if sent in solation.
I see that 0x1 means "requested" here. My interpretation is that this is the same as being sampled -- would you agree with that? |
I'm not sure.. I had a hard time deciphering what the Here's the pull request that split the "sampled" flag into two different flags, which provides a little more context / discussion around the flags. I just noticed a recent comment about some confusion / disagreement around the proposed flags, so we might need to wait for that to get resolved / fleshed out. Let me see if I can get some clarification from that thread to at least clear things up in my mind. |
From the thread above, sounds like |
|
Sorry for the delay -- I will merge this PR now. We can refactor the Tracer to not drop isolated tracestate in a follow-up. |
|
FYI: This is still discussed in the w3c group. May change. |
Based on w3c TraceContext spec: https://w3c.github.io/distributed-tracing/
The
traceparentheader is validated and propagated according to spec.The only caveat is that the option flags are lost downstream and reset to
"0x1" by the tracer. This will require a refactor of how the tracer treats
the options as certain assumptions are being made that the least-significant
bit will always represent "sampled".
The
tracestateis propagated downstream, but is currently not mutable bythe tracer. It is simply stored as a string, but types have been generated
to allow this to be refactored into a more structured type in the future,
allowing the tracer to update the tracestate as necessary.
This commit does not attempt to implement the
Correlation-Contextheader.issue #95