diff --git a/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/W3CPropagation.java b/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/W3CPropagation.java index 9d272e1c..20c91c39 100755 --- a/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/W3CPropagation.java +++ b/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/main/java/io/micrometer/tracing/brave/bridge/W3CPropagation.java @@ -15,6 +15,8 @@ */ package io.micrometer.tracing.brave.bridge; +import brave.Span; +import brave.Tracer; import brave.internal.baggage.BaggageFields; import brave.propagation.Propagation; import brave.propagation.TraceContext; @@ -110,6 +112,27 @@ private void addTraceState(Setter setter, TraceContext context, R } } + /** + *

This does not set the shared flag when extracting headers

+ * + *

+ * {@link brave.propagation.TraceContext#shared()} is not set here because it is not a + * remote propagation field. {@code shared} is a field in the Zipkin JSON v2 format + * only set after header extraction, for {@link Span.Kind#SERVER} spans + * implicitly via {@link brave.Tracer#joinSpan(TraceContext)}. + * + *

+ * Blindly setting {@code shared} regardless of this is harmful when + * {@link Tracer#currentSpan()} or similar are used, as any data tagged with these + * could also set the shared flag when reporting. Particularly, this can cause + * problems for multi- {@linkplain Span.Kind#CONSUMER} spans. Regardless, setting + * invalid flags add overhead. + * + *

+ * In summary, while {@code shared} is propagated in-process, it has never been + * propagated out of process, and so should never be set when extracting headers. + * Hence, this code will not set {@link brave.propagation.TraceContext#shared()}. + */ @Override public TraceContext.Extractor extractor(Getter getter) { Objects.requireNonNull(getter, "getter"); @@ -122,22 +145,6 @@ public TraceContext.Extractor extractor(Getter getter) { if (contextFromParentHeader == null) { return withBaggage(TraceContextOrSamplingFlags.EMPTY, carrier, getter); } - else { - // TODO: the previous implementation always set the shared flag to true. - // This is incorrect because it could be a messaging span which is never - // shared. Setting it to shared would cause message consumers to all join - // the same producer span and make a mess in UI and analysis. Also, some - // RPC spans are intentionally not shared. Finally, b3's "don't sample" - // "b3=0" is conflated with "not yet sampled" unless you propagate the b3 - // bits. - // - // W3C traceparent has no field to indicate shared status, which is why - // tracestate exists, to carry the extra data not possible to encode in - // traceparent, such as intentionally unsampled, debug and shared flags. - // https://github.com/openzipkin-contrib/brave-propagation-w3c has a - // correct impl. - contextFromParentHeader = contextFromParentHeader.toBuilder().shared(true).build(); - } String traceStateHeader = getter.get(carrier, TRACESTATE); TraceContextOrSamplingFlags context = context(contextFromParentHeader, traceStateHeader); if (this.baggagePropagator == null || this.braveBaggageManager == null) { @@ -166,7 +173,6 @@ TraceContextOrSamplingFlags context(TraceContext contextFromParentHeader, String .traceIdHigh(contextFromParentHeader.traceIdHigh()) .spanId(contextFromParentHeader.spanId()) .sampled(contextFromParentHeader.sampled()) - .shared(true) .build()) .build(); } diff --git a/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/W3CPropagationTest.java b/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/W3CPropagationTest.java index f64f6355..10425050 100644 --- a/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/W3CPropagationTest.java +++ b/micrometer-tracing-bridges/micrometer-tracing-bridge-brave/src/test/java/io/micrometer/tracing/brave/bridge/W3CPropagationTest.java @@ -139,7 +139,7 @@ void extract_SampledContext(W3CPropagationType propagationType) { Map carrier = new LinkedHashMap<>(); carrier.put(TRACEPARENT, TRACEPARENT_HEADER_SAMPLED); assertThat(propagationType.get().extractor(getter).extract(carrier).context()) - .isEqualTo(sharedTraceContext().build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -148,7 +148,7 @@ void extract_NullCarrier(W3CPropagationType propagationType) { Map carrier = new LinkedHashMap<>(); carrier.put(TRACEPARENT, TRACEPARENT_HEADER_SAMPLED); assertThat(propagationType.get().extractor((request, key) -> carrier.get(key)).extract(null).context()) - .isEqualTo(sharedTraceContext().build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -157,7 +157,7 @@ void extract_NotSampledContext(W3CPropagationType propagationType) { Map carrier = new LinkedHashMap<>(); carrier.put(TRACEPARENT, TRACEPARENT_HEADER_NOT_SAMPLED); assertThat(propagationType.get().extractor(getter).extract(carrier).context()) - .isEqualTo(notSampledTraceContext().shared(true).build()); + .isEqualTo(notSampledTraceContext().build()); } @ParameterizedTest @@ -166,7 +166,7 @@ void extract_NotSampledContext_NextVersion(W3CPropagationType propagationType) { Map carrier = new LinkedHashMap<>(); carrier.put(TRACEPARENT, "01-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-00-02"); assertThat(propagationType.get().extractor(getter).extract(carrier).context()) - .isEqualTo(sharedTraceContext().build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -176,7 +176,7 @@ void extract_NotSampledContext_EmptyTraceState(W3CPropagationType propagationTyp carrier.put(TRACEPARENT, TRACEPARENT_HEADER_NOT_SAMPLED); carrier.put(TRACESTATE, ""); assertThat(propagationType.get().extractor(getter).extract(carrier).context()) - .isEqualTo(notSampledTraceContext().shared(true).build()); + .isEqualTo(notSampledTraceContext().build()); } private TraceContext.Builder notSampledTraceContext() { @@ -190,7 +190,7 @@ void extract_NotSampledContext_TraceStateWithSpaces(W3CPropagationType propagati carrier.put(TRACEPARENT, TRACEPARENT_HEADER_NOT_SAMPLED); carrier.put(TRACESTATE, TRACESTATE_NOT_DEFAULT_ENCODING_WITH_SPACES); assertThat(propagationType.get().extractor(getter).extract(carrier).context()) - .isEqualTo(sharedTraceContext().sampled(false).build()); + .isEqualTo(sampledTraceContext().sampled(false).build()); } @Test @@ -286,11 +286,7 @@ void extract_InvalidTracestate_EntriesDelimiter(W3CPropagationType propagationTy invalidHeaders.put(TRACEPARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01"); invalidHeaders.put(TRACESTATE, "foo=bar;test=test"); assertThat(propagationType.get().extractor(getter).extract(invalidHeaders).context()) - .isEqualTo(sharedTraceContext().build()); - } - - private TraceContext.Builder sharedTraceContext() { - return sampledTraceContext().shared(true); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -299,8 +295,10 @@ void extract_InvalidTracestate_KeyValueDelimiter(W3CPropagationType propagationT Map invalidHeaders = new HashMap<>(); invalidHeaders.put(TRACEPARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01"); invalidHeaders.put(TRACESTATE, "foo=bar,test-test"); + TraceContext actual = propagationType.get().extractor(getter).extract(invalidHeaders).context(); + TraceContext expected = sampledTraceContext().build(); assertThat(propagationType.get().extractor(getter).extract(invalidHeaders).context()) - .isEqualTo(sharedTraceContext().build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -310,7 +308,7 @@ void extract_InvalidTracestate_OneString(W3CPropagationType propagationType) { invalidHeaders.put(TRACEPARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01"); invalidHeaders.put(TRACESTATE, "test-test"); assertThat(propagationType.get().extractor(getter).extract(invalidHeaders).context()) - .isEqualTo(sampledTraceContext().shared(true).build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest @@ -337,7 +335,7 @@ void extract_ValidTraceparent_nextVersion_extraTrailing(W3CPropagationType propa Map invalidHeaders = new HashMap<>(); invalidHeaders.put(TRACEPARENT, "01-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-00-01"); assertThat(propagationType.get().extractor(getter).extract(invalidHeaders).context()) - .isEqualTo(sharedTraceContext().build()); + .isEqualTo(sampledTraceContext().build()); } @ParameterizedTest