Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Support for basic propagation of trace information using W3 TraceContext #649

Closed
wants to merge 6 commits into from

Conversation

backjo
Copy link
Contributor

@backjo backjo commented Aug 22, 2019

Closes #648

Short description of the changes

  • Basic codec for traceparent header. Only supports trace/span propagation (no baggage support at the moment).

@backjo
Copy link
Contributor Author

backjo commented Aug 22, 2019

This change is Reviewable

Signed-off-by: Jonah Back <jonah@jonahback.com>
Signed-off-by: Jonah Back <jonah@jonahback.com>
@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #649 into master will increase coverage by <.01%.
The diff coverage is 89.79%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #649      +/-   ##
============================================
+ Coverage     89.38%   89.39%   +<.01%     
- Complexity      563      573      +10     
============================================
  Files            69       70       +1     
  Lines          2073     2122      +49     
  Branches        263      269       +6     
============================================
+ Hits           1853     1897      +44     
- Misses          138      140       +2     
- Partials         82       85       +3
Impacted Files Coverage Δ Complexity Δ
...o/jaegertracing/internal/propagation/HexCodec.java 92.45% <100%> (+0.96%) 19 <1> (+1) ⬆️
...racing/internal/propagation/TraceContextCodec.java 88.37% <88.37%> (ø) 9 <9> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1a7ca9...633bc6c. Read the comment docs.

Signed-off-by: Jonah Back <jonah@jonahback.com>
Signed-off-by: Jonah Back <jonah@jonahback.com>
@backjo backjo closed this Aug 22, 2019
@backjo backjo reopened this Aug 22, 2019
long traceIdHigh = spanContext.getTraceIdHigh();

//From the specification:
//When a system operates with a trace-id that is shorter than 16 bytes,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a terrible idea. sometimes IDs are truncated due to old instrumentation and this will ensure you can't match them later. is this really in the spec?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean imagine if upstream is 128-bit a middle hop accidentally truncates making several outbound client calls. now you have effectively a new trace for all those client calls as there's no way to detect the truncation anymore, unless you process the entire tree and make some assumptions. All the last years heuristics to match no-longer work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also if you choose against changing the trace ID you were given, you have the second benefit of a consistent subtree. as this ends up in logs it is helpful. If it turns out the intent was not a 64-bit, rather a truncation, you would see that also in the logs.. and see the downstream affected. if everyone changes the trace id upon seeing a short one, not only can you not easily tell, but also nothing matches. I hope this makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't disagree that adding this additional information in makes it harder to use in older systems, but the spec does seem pretty rigid about this.

Because tracing systems may make sampling decisions based on the value of trace-id, for increased interoperability vendors MUST keep the random part of trace-id ID on the left side.

When a system operates with a trace-id that is shorter than 16 bytes, it SHOULD fill-in the extra bytes with random values rather than zeroes. Let's say the system works with an 8-byte trace-id like 3ce929d0e0e4736. Instead of setting trace-id value to 0000000000000003ce929d0e0e4736 it SHOULD generate a value like 4bf92f3577b34da6a3ce929d0e0e4736 where 4bf92f3577b34da6a is a random value or a function of time and host value.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec does not say that you need to correct this. The spec says that if you generate it you should follow that rule. As I mentioned in a different comment the left part can be 0 if it is randomly generated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @bogdandrutu's interpretation.

Having said that, that's what happens when specs are written without reference implementation. What if we all join forces and create a single reusable library for Zipkin, Jaeger, OpenCensus, and OpenTelemetry, for parsing W3C trace context? All 4 projects have identical format of trace IDs. The tracestate might need some customization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to remove this - thanks for the help clarifying :)

Curious how we should handle the case where users have configured the tracer to not be 128 bit but register the trace context codec

Signed-off-by: Jonah Back <jonah@jonahback.com>
…had 64 bits of data

Signed-off-by: Jonah Back <jonah@jonahback.com>
@pavolloffay
Copy link
Member

Done in #694

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support W3C Trace Context header
5 participants