Add W3C TraceContext codec/propagation #694

pavolloffay · 2020-03-05T16:15:46Z

Supersedes #649
Related to jaegertracing/jaeger#855

I have made slight changes to #649 (the spanId was incorrect set, improved inject perfomance) and aligned the implementation with https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/propagation/HttpTraceContext.java#L40

codecov · 2020-03-05T16:21:22Z

Codecov Report

Merging #694 into master will decrease coverage by 0.1%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master     #694      +/-   ##
============================================
- Coverage     90.08%   89.98%   -0.11%     
- Complexity      571      590      +19     
============================================
  Files            69       70       +1     
  Lines          2088     2157      +69     
  Branches        266      281      +15     
============================================
+ Hits           1881     1941      +60     
- Misses          126      130       +4     
- Partials         81       86       +5

Impacted Files	Coverage Δ	Complexity Δ
...a/io/jaegertracing/internal/JaegerSpanContext.java	`98.11% <100%> (+0.19%)`	`32 <2> (+2)`	⬆️
.../src/main/java/io/jaegertracing/Configuration.java	`93.23% <100%> (+0.1%)`	`44 <0> (ø)`	⬇️
...racing/internal/propagation/TraceContextCodec.java	`83.33% <83.33%> (ø)`	`16 <16> (?)`
...ing/internal/samplers/RemoteControlledSampler.java	`92.5% <0%> (+1.25%)`	`17% <0%> (+1%)`	⬆️

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 f726853...525480b. Read the comment docs.

pavolloffay · 2020-03-05T16:29:05Z

The question also is whether we want to support baggage - tracestate

yurishkuro · 2020-03-05T16:37:04Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

+
+package io.jaegertracing.internal.propagation;
+
+import static io.jaegertracing.internal.propagation.B3TextMapCodec.SAMPLED_FLAG;


you sure you want this here?

This certainly no ;)

objectiser · 2020-03-05T16:57:40Z

jaeger-core/src/test/java/io/jaegertracing/internal/propagation/TraceContextCodecTest.java

+    assertNotNull(traceContextHeader);
+    assertTrue(traceContextHeader.contains("0000000000000001"));
+    //For 64 bit traces, we need to pad the left side with a random number to conform with the specification.
+    //It should not contain all zeros.


This comment doesn't seem to match the assertion below?

Based on the code commet seems wrong. I haven't found this in the spec. @yurishkuro do you know something about this?

objectiser · 2020-03-05T16:59:37Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+    /**
+     * The W3C TraceContext propagation format.
+     */
+    TRACE_CONTEXT


As mentioned in the doc PR, just wondering whether this should be W3C rather than trace_context which is very generic. But not sure if there is a convention being used across jaeger (or wider).

I don't have any preference. I don't think there will be any other variant of trace context that will collide with this name.

In which case, I would prefer W3C as it clearly identifies the standards body - in the same way as B3 above identifies the defacto-standard.

@yurishkuro Any preference?

W3C sgtm, or W3C_TRACE_CONTEXT (but if we also support it in the Config class as a string, then I would just go with "w3x" string)

In which case, I would prefer W3C as it clearly identifies the standards body - in the same way as B3 above identifies the defacto-standard.

I don't have much preference but. B3 identifies the protocol, the same as trace_context. Whereas W3C is the standard body like or organization which for B3 is Zipkin. So if we follow the same naming conventions the B3 should be rather named zipkin.

Agree the analogy was wrong. However the problem I have with trace_context is that it is a generic term so could cause confusion.

pavolloffay · 2020-03-05T17:00:06Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

+
+    boolean sampled = false;
+    long traceContextFlags = HexCodec.hexToUnsignedLong(traceparent, TRACE_OPTION_OFFSET, TRACE_OPTION_OFFSET + 2);
+    if ((traceContextFlags & SAMPLED_FLAG) == SAMPLED_FLAG) {


https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#trace-flags

yurishkuro

is there any way we can use OTel implementation instead of creating our own?
only doing traceparent is not compliant with W3C spec, is it?

yurishkuro · 2020-03-05T21:22:25Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+    /**
+     * The W3C TraceContext propagation format.
+     */
+    TRACE_CONTEXT


W3C sgtm, or W3C_TRACE_CONTEXT (but if we also support it in the Config class as a string, then I would just go with "w3x" string)

yurishkuro · 2020-03-05T21:22:42Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

@@ -0,0 +1,142 @@
+/*
+ * Copyright 2019, OpenTelemetry Authors


we should consider removing dates altogether

This is what the license plugin gave me

I will have a look at this in a separate PR.

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

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

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-03-06T13:32:55Z

is there any way we can use OTel implementation instead of creating our own?

It does not seem right to put on the classpath OTEL API only just for this class.

only doing traceparent is not compliant with W3C spec, is it?

Based on the OTEL impl it seems to be compliant. The header is not added if there is no baggage and not parsed if it is missing.

pavolloffay · 2020-03-06T13:40:42Z

@yurishkuro I have added support for tracestate.

There are two limitations associated with it: https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md

The tracestate field value is a list of list-members separated by commas (,). A list-member is a key/value pair separated by an equals sign (=). Spaces and horizontal tabs surrounding list-members are ignored. There can be a maximum of 32 list-members in a list.

The OTEL and this impl does not extract baggage if there are more than 32 items.

Vendors SHOULD propagate at least 512 characters of a combined header. This length includes commas required to separate list items and optional white space (OWS) characters.

There are systems where propagating of 512 characters of tracestate may be expensive. In this case, the maximum size of the propagated tracestate header SHOULD be documented and explained. The cost of propagating tracestate SHOULD be weighted against the value of monitoring scenarios enabled for the end users.

In a situation where tracestate needs to be truncated due to size limitations, the vendor MUST truncate whole entries. Entries larger than 128 characters long SHOULD be removed first. Then entries SHOULD be removed starting from the end of tracestate. Note that other truncation strategies like safe list entries, blocked list entries, or size-based truncation MAY be used, but are highly discouraged. Those strategies decrease the interoperability of various tracing vendors.

The OTEL and this impl allows inject/extract of tracestate longer than 512 chars

yurishkuro · 2020-03-06T17:24:03Z

@pavolloffay I don't believe tracestate is meant for baggage. Since Jaeger only needs traceparent, my understanding was that tracestate simply needs to be propagated intact (we don't even need to parse it).

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-03-09T15:43:06Z

My bad I mixed it with correlation context. I have removed the baggage and just add propagation of the tracestate

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro · 2020-03-09T20:08:41Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerObjectFactory.java

+    return new JaegerSpanContext(traceIdHigh, traceIdLow, spanId, parentId, flags, traceState, baggage, debugId, this);
+  }
+
+  public JaegerSpanContext createSpanContext(


Please keep the factory interface minimal.

Can we instead have a setTracestate() method on the context and not change the factory? I realize that we can't make the field final in that case, but it's better for backwards compatibility than breaking this interface.

JaegerObjectFactory is not an interface hence this is not a breaking change.

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

yurishkuro · 2020-03-09T20:23:07Z

jaeger-core/src/test/java/io/jaegertracing/internal/propagation/TraceContextCodecTest.java

@@ -0,0 +1,136 @@
+/*
+ * Copyright (c) 2016, Uber Technologies, Inc


Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2020-03-10T09:30:02Z

@yurishkuro PR updated

yurishkuro

thanks for restoring SpanContextFactory. It should've been an interface, and can't really work with overloaded methods because they would silently bypass the subclasses.

fischerman · 2020-03-17T10:27:56Z

@pavolloffay Can this be used together with the Uber format, i.e. extract the context from either format and inject both?

pavolloffay · 2020-03-17T14:57:32Z

I am not sure, there is a composite Codec but I we are probably not testing this.

You can try to play with tracer builder/configuration and see whether it is possible. Please open an issue with your findings.

yurishkuro · 2020-03-25T21:49:22Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TraceContextCodec.java

+    HexCodec.writeHexLong(chars, SPAN_ID_OFFSET, spanContext.getSpanId());
+    chars[TRACE_OPTION_OFFSET - 1] = TRACEPARENT_DELIMITER;
+    chars[TRACE_OPTION_OFFSET] = '0';
+    chars[TRACE_OPTION_OFFSET + 1] = spanContext.isSampled() ? '1' : '0';


we should be carrying all Jaeger flags, not just sampled bit: #702

pavolloffay requested a review from objectiser March 5, 2020 16:15

pavolloffay mentioned this pull request Mar 5, 2020

Support w3c tracecontext jaegertracing/documentation#380

Merged

pavolloffay changed the title ~~Add W3C TraceContext~~ Add W3C TraceContext, only traceparent Mar 5, 2020

yurishkuro reviewed Mar 5, 2020

View reviewed changes

objectiser reviewed Mar 5, 2020

View reviewed changes

pavolloffay commented Mar 5, 2020

View reviewed changes

yurishkuro reviewed Mar 5, 2020

View reviewed changes

backjo and others added 10 commits March 6, 2020 10:15

Add support for tracecontext

1a997da

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

Address formatting issues

17fa41d

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

Add additional tests, remove unused constructor

6f97731

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

Add test for version builder

7a7465b

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

Fix incorrect header name

34da107

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

Remove bit where we generate left hand side of trace id when we only …

e6f47a8

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

Add W3C TraceContext codec

a26b742

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Some cleanup

4fb8d77

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

smaller nits

d43b40f

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix padding with zeros in test

748b9bd

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the w3c-tracecontext branch from b6a145f to 748b9bd Compare March 6, 2020 09:15

Add tracestate header

f34e525

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Just propagate tracestate

130366e

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Cleanup

ee6f0e0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro reviewed Mar 9, 2020

View reviewed changes

pavolloffay added 2 commits March 10, 2020 10:28

Add tracestate factory method to span builder

d81885b

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Remove header

525480b

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro approved these changes Mar 11, 2020

View reviewed changes

pavolloffay merged commit 568ab68 into jaegertracing:master Mar 11, 2020

pavolloffay changed the title ~~Add W3C TraceContext, only traceparent~~ Add W3C TraceContext codec/propagation Mar 11, 2020

This was referenced Mar 11, 2020

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

Closed

Support W3C Trace Context header #648

Closed

codefromthecrypt mentioned this pull request Mar 11, 2020

WIP TraceContext propagation handler openzipkin/brave#693

Closed

ckho mentioned this pull request Mar 18, 2020

[docs] Document experimental Jaeger support elastic/apm-server#3123

Closed

fischerman mentioned this pull request Mar 18, 2020

Using Uber and W3C format together #696

Closed

yurishkuro reviewed Mar 25, 2020

View reviewed changes

yurishkuro mentioned this pull request Jun 15, 2020

Support W3C Trace Context headers jaegertracing/jaeger#855

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add W3C TraceContext codec/propagation #694

Add W3C TraceContext codec/propagation #694

pavolloffay commented Mar 5, 2020

codecov bot commented Mar 5, 2020 •

edited

Loading

pavolloffay commented Mar 5, 2020

yurishkuro Mar 5, 2020

pavolloffay Mar 5, 2020

objectiser Mar 5, 2020

pavolloffay Mar 5, 2020

objectiser Mar 5, 2020

pavolloffay Mar 5, 2020

objectiser Mar 5, 2020

yurishkuro Mar 5, 2020

pavolloffay Mar 6, 2020

objectiser Mar 6, 2020

pavolloffay Mar 5, 2020

yurishkuro left a comment

yurishkuro Mar 5, 2020

yurishkuro Mar 5, 2020

yurishkuro Mar 5, 2020

pavolloffay Mar 6, 2020

pavolloffay Mar 6, 2020

pavolloffay commented Mar 6, 2020

pavolloffay commented Mar 6, 2020 •

edited

Loading

yurishkuro commented Mar 6, 2020

pavolloffay commented Mar 9, 2020

yurishkuro Mar 9, 2020

pavolloffay Mar 10, 2020

yurishkuro Mar 9, 2020

pavolloffay commented Mar 10, 2020

yurishkuro left a comment

fischerman commented Mar 17, 2020

pavolloffay commented Mar 17, 2020

yurishkuro Mar 25, 2020


		package io.jaegertracing.internal.propagation;

		import static io.jaegertracing.internal.propagation.B3TextMapCodec.SAMPLED_FLAG;

		@@ -0,0 +1,136 @@
		/*
		* Copyright (c) 2016, Uber Technologies, Inc

Add W3C TraceContext codec/propagation #694

Add W3C TraceContext codec/propagation #694

Conversation

pavolloffay commented Mar 5, 2020

codecov bot commented Mar 5, 2020 • edited Loading

Codecov Report

pavolloffay commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Mar 6, 2020

pavolloffay commented Mar 6, 2020 • edited Loading

yurishkuro commented Mar 6, 2020

pavolloffay commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Mar 10, 2020

yurishkuro left a comment

Choose a reason for hiding this comment

fischerman commented Mar 17, 2020

pavolloffay commented Mar 17, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2020 •

edited

Loading

pavolloffay commented Mar 6, 2020 •

edited

Loading