search

lightstep / lightstep-tracer-go

The Lightstep distributed tracing library for Go
https://lightstep.com
MIT License
98 stars 54 forks source link

Propagate W3C traceparent and tracestate headers #184

Closed iredelmeier closed 3 years ago

iredelmeier commented 5 years ago

tl;dr: What the title says! If you want to skim, the tests are (hopefully) the key part.

Some key notes/open questions:

  1. The trace ID from tracestate is currently truncated for intra-LightStep purposes, but the full 32-byte string is retained for cross-vendor propagation.
    • Is this too confusing?
    • There's a risk that other tracers may truncate differently, causing trace fragmentation.
  2. If the carrier passed to #Inject already contains W3C headers, they'll currently be ignored and overwritten. There are a few pended tests that should demonstrate the problem.
    • This shouldn't be a problem with most workflows but may cause some weird bugs...
    • If the interface{} carrier passed to #Inject doesn't satisfy opentracing.TextMapReader, we don't really have a choice :/
  3. The sample bit from traceparent is currently ignored and always propagated as true/1. We should probably propagate the prior setting, but that seems like a slightly bigger issue across the library.
  4. How should we handle collisions with the existing headers (ot-tracer-traceid, etc.)? The current behaviour is undefined if/when they differ.
  5. I used tracestate to store baggage, which is encoded with JSON + unpadded URL-safe base64. This should be supportable by other languages, but they'll need to be consistent.
  6. Do we want lightstep as the tracestate vendor key?
  7. The tracestate behaviour is undefined if the encoded LS value is > 512 characters.
    • Should we truncate it? If so, how? (e.g., do we want to order deterministically by key?)
  8. The changes also affect the TextContent type, in keeping with existing implementation. We may want to differentiate between the two at some point.
  9. ...and of course: can some (all?) of this move into the OT core?
tedsuo commented 5 years ago

Thanks Isobel!

Will have more comments forthcoming soon. But for now, here’s a handy link to the spec for anyone following along: https://w3c.github.io/trace-context/

jmacd commented 5 years ago

Re: "If the carrier passed to #Inject already contains W3C headers, they'll currently be ignored and overwritten." This seems like the correct thing to do. Why would the incoming carrier not be empty?

codeboten commented 3 years ago

Closing the PR as the issue it was intended to fix #225 was closed