Add support for Trace Context headers #82
Conversation
ocelotl
force-pushed
the
trace_context
branch
3 times, most recently
from
9d1a729 to
3b1a3c0
Compare
lightstep/propagation.py
Outdated
| # The TRACE_CONTEXT format represents SpanContexts in Trace Context format. | ||
| # https://www.w3.org/TR/trace-context/ | ||
| TRACE_CONTEXT = "trace_context" | ||
|
|
There was a problem hiding this comment.
Not seeing this being used anywhere
lightstep/trace_context.py
Outdated
| tracestate_dictionary[key] = value | ||
|
|
||
| else: | ||
| trace_context_free_carrier["tracestate"] = ",".join( |
There was a problem hiding this comment.
Minor change here: should use _TRACESTATE
ocelotl
force-pushed
the
trace_context
branch
from
3b1a3c0 to
9cf7425
Compare
| format(span_context.baggage.pop("trace-flags", 0), "02x") | ||
| ) | ||
|
|
||
| carrier.update(span_context.baggage) |
There was a problem hiding this comment.
We are dropping our own baggage here, right? I'm afraid that this will clash with some other fields/data already existing in the carrier. I don't remember this from the W3C, but will double check ;)
There was a problem hiding this comment.
Do you want this removed now, or should I wait for your double-check?
There was a problem hiding this comment.
Lets go ahead and keep it like this (open a ticket so we can double verify later ;) )
lightstep/trace_context.py
Outdated
| traceparent = value | ||
|
|
||
| else: | ||
| raise Exception( |
There was a problem hiding this comment.
What about SpanContextCorruptedException?
There was a problem hiding this comment.
And same for the errors below (I know you put a FIXME, but I think it should be straightforward to replace almost all of them with SpanContextCorruptedException ;) )
|
|
||
| trace_context_free_carrier = {} | ||
|
|
||
| for key, value in carrier.items(): |
There was a problem hiding this comment.
I think we need to protect here ourselves, in case carrier has no items(), so we report instead an InvalidCarrierException instead.
There was a problem hiding this comment.
But, if there are no items, then the for loop won't be accessed, traceparent will be None and we'll end up here. Isn't it the right flow? 🤔
There was a problem hiding this comment.
I mean, in case the carrier has no items() method ;) (not super important, so we can also keep it like this with a related Issue and fix later on).
| "Unable to parse version from traceparent" | ||
| ) | ||
| return SpanContext( | ||
| trace_id=getrandbits(128), span_id=getrandbits(64) |
There was a problem hiding this comment.
Are we cool here using 128-bits for trace id?
There was a problem hiding this comment.
The document specifies 16 bytes for trace-id. Am I understanding this wrong? 🤔
There was a problem hiding this comment.
Sure, we need to use 16 bytes for the format (and propagate 16 bytes ids as well). But I'm not sure we properly handle such a long id in the rest of the code base (SpanContext within lightstep module), nor the backend.
I.e. we should do what we did for B3, where we propagate the 'original' id but internally use only 8 bytes ;)
There was a problem hiding this comment.
@codeboten what do you think?
There was a problem hiding this comment.
@carlosalberto after discussing this with @codeboten I have submitted a commit that truncates the value of trace_id automatically when exiting a span context. I think this is the right approach, since we should keep the span context attributes as they are for inject and extract and only do truncation for LightStep specific code.
| "Received an invalid traceparent: {}".format(traceparent) | ||
| ) | ||
| return SpanContext( | ||
| trace_id=getrandbits(128), span_id=getrandbits(64) |
ocelotl
force-pushed
the
trace_context
branch
from
9cf7425 to
56c2f6b
Compare
ocelotl
force-pushed
the
trace_context
branch
from
56c2f6b to
c42e99d
Compare
|
Hey, just reviewed the updated code. Just for reference, in Java/C# we kept the original, long trace id in the The code here looks right though, and as long as you guys are comfortable with it, I'm fine as well. |
Closes #79