Remove unused headers: X-B3-ParentSpanId and X-OrigSpanId (#778)

Remove unused headers: `X-B3-ParentSpanId` and `X-OrigSpanId`

.palantir/revapi.yml

@@ -38,3 +38,22 @@ acceptedBreaks:
     - code: "java.method.addedToInterface"
       new: "method com.palantir.tracing.CloseableSpan com.palantir.tracing.DetachedSpan::attach()"
       justification: "DetachedSpan is not meant for external implementations"
+  "6.3.0":
+    com.palantir.tracing:tracing:
+    - code: "java.method.removed"
+      old: "method com.palantir.tracing.TraceMetadata.Builder com.palantir.tracing.ImmutableTraceMetadata.Builder::originatingSpanId(java.lang.String)\
+        \ @ com.palantir.tracing.TraceMetadata.Builder"
+      justification: "Type is not meant for external creation"
+    - code: "java.method.removed"
+      old: "method com.palantir.tracing.TraceMetadata.Builder com.palantir.tracing.ImmutableTraceMetadata.Builder::originatingSpanId(java.util.Optional<java.lang.String>)\
+        \ @ com.palantir.tracing.TraceMetadata.Builder"
+      justification: "Type is not meant for external creation"
+    com.palantir.tracing:tracing-api:
+    - code: "java.method.removed"
+      old: "method com.palantir.tracing.api.OpenSpan.Builder com.palantir.tracing.api.ImmutableOpenSpan.Builder::originatingSpanId(java.lang.String)\
+        \ @ com.palantir.tracing.api.OpenSpan.Builder"
+      justification: "Type is not meant for external creation"
+    - code: "java.method.removed"
+      old: "method com.palantir.tracing.api.OpenSpan.Builder com.palantir.tracing.api.ImmutableOpenSpan.Builder::originatingSpanId(java.util.Optional<java.lang.String>)\
+        \ @ com.palantir.tracing.api.OpenSpan.Builder"
+      justification: "Type is not meant for external creation"

changelog/@unreleased/pr-778.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: 'Remove unused headers: `X-B3-ParentSpanId` and `X-OrigSpanId`'
+  links:
+  - https://github.com/palantir/tracing-java/pull/778

tracing-api/src/main/java/com/palantir/tracing/api/OpenSpan.java

@@ -59,10 +59,14 @@ public abstract class OpenSpan {
     /**
      * Returns the identifier of the 'originating' span if one exists.
      *
-     * @see TraceHttpHeaders
+     * @see TraceHttpHeaders#ORIGINATING_SPAN_ID
+     * @deprecated No longer used.
      */
-    @Value.Parameter
-    public abstract Optional<String> getOriginatingSpanId();
+    @Deprecated
+    // Intentionally does not set @Value.Default because the value is always empty.
+    public Optional<String> getOriginatingSpanId() {
+        return Optional.empty();
+    }
 
     /** Returns a globally unique identifier representing a single span within the call trace. */
     @Value.Parameter
@@ -83,26 +87,25 @@ public static Builder builder() {
         return new Builder().startTimeMicroSeconds(getNowInMicroSeconds()).startClockNanoSeconds(System.nanoTime());
     }
 
-    /** Use this factory method to avoid allocate {@link Builder} in hot path. */
+    /**
+     * Use this factory method to avoid {@link Builder} allocations in hot paths.
+     *
+     * @deprecated Use the variant without an originating span id
+     */
+    @SuppressWarnings("InlineMeSuggester")
+    @Deprecated
     public static OpenSpan of(
             String operation,
             String spanId,
             SpanType type,
             Optional<String> parentSpanId,
-            Optional<String> originatingSpanId) {
-        return ImmutableOpenSpan.of(
-                operation, getNowInMicroSeconds(), System.nanoTime(), parentSpanId, originatingSpanId, spanId, type);
+            Optional<String> _originatingSpanId) {
+        return ImmutableOpenSpan.of(operation, getNowInMicroSeconds(), System.nanoTime(), parentSpanId, spanId, type);
     }
 
-    /**
-     * Deprecated.
-     *
-     * @deprecated Use the variant that accepts an originating span id
-     */
-    @SuppressWarnings("InlineMeSuggester")
-    @Deprecated
+    /** Use this factory method to avoid {@link Builder} allocations in hot paths. */
     public static OpenSpan of(String operation, String spanId, SpanType type, Optional<String> parentSpanId) {
-        return of(operation, spanId, type, parentSpanId, Optional.empty());
+        return ImmutableOpenSpan.of(operation, getNowInMicroSeconds(), System.nanoTime(), parentSpanId, spanId, type);
     }
 
     private static long getNowInMicroSeconds() {

tracing-api/src/main/java/com/palantir/tracing/api/TraceHttpHeaders.java

@@ -19,19 +19,22 @@
 /** Zipkin-compatible HTTP header names. */
 public interface TraceHttpHeaders {
     String TRACE_ID = "X-B3-TraceId";
+    /**
+     * Field is no longer used by the tracing library.
+     *
+     * @deprecated No longer used
+     */
+    @Deprecated
     String PARENT_SPAN_ID = "X-B3-ParentSpanId";
+
     String SPAN_ID = "X-B3-SpanId";
     String IS_SAMPLED = "X-B3-Sampled"; // Boolean (either “1” or “0”, can be absent)
 
     /**
-     * Conceptually, a trace is a stack of spans. In implementation, this is actually many stacks, in many servers,
-     * where a server's stack will typically contain a single parent span from a different server at the bottom, and
-     * many spans of its own above it.
+     * Field is no longer used by the tracing library.
      *
-     * <p>By communicating this deepest span id with future network calls as an 'originating' span id, this enables
-     * network-level tracing to be enabled always in a low-fidelity form, with request logs containing enough
-     * information to reconstruct a request-level trace, even when the trace is not sampled. For server-internal
-     * tracing, the typical trace logs (with sampling) are still required.
+     * @deprecated No longer used
      */
+    @Deprecated
     String ORIGINATING_SPAN_ID = "X-OrigSpanId";
 }

tracing-jersey/src/test/java/com/palantir/tracing/jersey/TraceEnrichingFilterTest.java

@@ -112,11 +112,9 @@ public void testTraceState_withHeaderUsesTraceId() {
         Response response = target.path("/trace")
                 .request()
                 .header(TraceHttpHeaders.TRACE_ID, "traceId")
-                .header(TraceHttpHeaders.PARENT_SPAN_ID, "parentSpanId")
                 .header(TraceHttpHeaders.SPAN_ID, "spanId")
                 .get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isEqualTo("traceId");
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /trace");
@@ -127,11 +125,9 @@ public void testTraceState_respectsMethod() {
         Response response = target.path("/trace")
                 .request()
                 .header(TraceHttpHeaders.TRACE_ID, "traceId")
-                .header(TraceHttpHeaders.PARENT_SPAN_ID, "parentSpanId")
                 .header(TraceHttpHeaders.SPAN_ID, "spanId")
                 .post(Entity.json(""));
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isEqualTo("traceId");
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: POST /trace");
@@ -142,11 +138,9 @@ public void testTraceState_doesNotIncludePathParams() {
         Response response = target.path("/trace/no")
                 .request()
                 .header(TraceHttpHeaders.TRACE_ID, "traceId")
-                .header(TraceHttpHeaders.PARENT_SPAN_ID, "parentSpanId")
                 .header(TraceHttpHeaders.SPAN_ID, "spanId")
                 .get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isEqualTo("traceId");
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /trace/{param}");
@@ -156,7 +150,6 @@ public void testTraceState_doesNotIncludePathParams() {
     public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeaders() {
         Response response = target.path("/trace").request().get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         Span span = spanCaptor.getValue();
@@ -169,7 +162,6 @@ public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeade
     public void testTraceState_setsResponseStatus() {
         Response response = target.path("/trace").request().post(null);
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         Span span = spanCaptor.getValue();
@@ -185,7 +177,6 @@ public void testTraceState_setsResponseStatus() {
     public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailing() {
         Response response = target.path("/failing-trace").request().get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         Span span = spanCaptor.getValue();
@@ -198,7 +189,6 @@ public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeade
     public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenStreaming() {
         Response response = target.path("/streaming-trace").request().get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /streaming-trace");
@@ -208,7 +198,6 @@ public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeade
     public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailingToStream() {
         Response response = target.path("/failing-streaming-trace").request().get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /failing-streaming-trace");
@@ -233,7 +222,6 @@ public void testTraceState_withEmptyTraceIdGeneratesValidTraceResponseHeaders()
                 .header(TraceHttpHeaders.TRACE_ID, "")
                 .get();
         assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID)).isNotNull();
-        assertThat(response.getHeaderString(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(response.getHeaderString(TraceHttpHeaders.SPAN_ID)).isNull();
         verify(observer).consume(spanCaptor.capture());
         assertThat(spanCaptor.getValue().getOperation()).isEqualTo("Jersey: GET /trace");

tracing-okhttp3/src/main/java/com/palantir/tracing/OkhttpTraceInterceptor2.java

@@ -47,24 +47,11 @@ public Response intercept(Chain chain) throws IOException {
             TraceMetadata metadata = Tracer.maybeGetTraceMetadata()
                     .orElseThrow(() -> new SafeRuntimeException("Trace with no spans in progress"));
 
-            Request.Builder tracedRequest = request.newBuilder()
+            return chain.proceed(request.newBuilder()
                     .header(TraceHttpHeaders.TRACE_ID, Tracer.getTraceId())
                     .header(TraceHttpHeaders.SPAN_ID, metadata.getSpanId())
-                    .header(TraceHttpHeaders.IS_SAMPLED, Tracer.isTraceObservable() ? "1" : "0");
-
-            if (metadata.getParentSpanId().isPresent()) {
-                tracedRequest.header(
-                        TraceHttpHeaders.PARENT_SPAN_ID,
-                        metadata.getParentSpanId().get());
-            }
-
-            if (metadata.getOriginatingSpanId().isPresent()) {
-                tracedRequest.header(
-                        TraceHttpHeaders.ORIGINATING_SPAN_ID,
-                        metadata.getOriginatingSpanId().get());
-            }
-
-            return chain.proceed(tracedRequest.build());
+                    .header(TraceHttpHeaders.IS_SAMPLED, Tracer.isTraceObservable() ? "1" : "0")
+                    .build());
         }
     }
 }

tracing-okhttp3/src/main/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptor.java

@@ -53,15 +53,6 @@ public Response intercept(Chain chain) throws IOException {
                 .header(TraceHttpHeaders.TRACE_ID, Tracer.getTraceId())
                 .header(TraceHttpHeaders.SPAN_ID, span.getSpanId())
                 .header(TraceHttpHeaders.IS_SAMPLED, Tracer.isTraceObservable() ? "1" : "0");
-        if (span.getParentSpanId().isPresent()) {
-            tracedRequest.header(
-                    TraceHttpHeaders.PARENT_SPAN_ID, span.getParentSpanId().get());
-        }
-        if (span.getOriginatingSpanId().isPresent()) {
-            tracedRequest.header(
-                    TraceHttpHeaders.ORIGINATING_SPAN_ID,
-                    span.getOriginatingSpanId().get());
-        }
 
         Response response;
         try {

tracing-okhttp3/src/test/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptorTest.java

@@ -82,8 +82,6 @@ public void testPopulatesNewTrace_whenNoTraceIsPresentInGlobalState() throws IOE
         Request intercepted = requestCaptor.getValue();
         assertThat(intercepted.headers(TraceHttpHeaders.SPAN_ID)).hasSize(1);
         assertThat(intercepted.headers(TraceHttpHeaders.TRACE_ID)).hasSize(1);
-        assertThat(intercepted.headers(TraceHttpHeaders.ORIGINATING_SPAN_ID)).isEmpty();
-        assertThat(intercepted.headers(TraceHttpHeaders.PARENT_SPAN_ID)).isEmpty();
     }
 
     @Test
@@ -104,7 +102,6 @@ public void testPopulatesNewTrace_whenParentTraceIsPresent() throws IOException
         Request intercepted = requestCaptor.getValue();
         assertThat(intercepted.headers(TraceHttpHeaders.SPAN_ID)).isNotNull();
         assertThat(intercepted.headers(TraceHttpHeaders.TRACE_ID)).containsOnly(traceId);
-        assertThat(intercepted.headers(TraceHttpHeaders.ORIGINATING_SPAN_ID)).containsOnly(originatingSpanId);
     }
 
     @Test

tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedOperationHandlerTest.java

@@ -18,14 +18,12 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-import com.palantir.tracing.TraceMetadata;
 import com.palantir.tracing.TraceSampler;
 import com.palantir.tracing.Tracer;
 import com.palantir.tracing.Tracers;
@@ -46,7 +44,6 @@
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
-import org.mockito.stubbing.Answer;
 import org.slf4j.MDC;
 
 @RunWith(MockitoJUnitRunner.class)
@@ -97,7 +94,6 @@ public void whenNoTraceIsInHeader_generatesNewTrace() throws Exception {
         assertThat(Tracer.hasTraceId()).isFalse();
         assertThat(span.getOperation()).isEqualTo("Undertow: GET /foo");
         HeaderMap responseHeaders = exchange.getResponseHeaders();
-        assertThat(responseHeaders.get(TraceHttpHeaders.PARENT_SPAN_ID)).isNull();
         assertThat(responseHeaders.get(TraceHttpHeaders.SPAN_ID)).isNull();
         assertThat(responseHeaders.get(HttpString.tryFromString(TraceHttpHeaders.TRACE_ID)))
                 .containsExactly(span.getTraceId());
@@ -129,28 +125,6 @@ public void whenParentSpanIsGiven_usesParentSpan() throws Exception {
         assertThat(span.getSpanId()).isNotEqualTo(parentSpanId);
     }
 
-    @Test
-    public void whenParentSpanIsGiven_usesParentSpan_unsampled() throws Exception {
-        when(traceSampler.sample()).thenReturn(false);
-        setRequestTraceId(traceId);
-        String parentSpanId = Tracers.randomId();
-        setRequestSpanId(parentSpanId);
-
-        AtomicReference<String> capturedParentSpanId = new AtomicReference<>();
-        doAnswer((Answer<Void>) _invocation -> {
-                    Tracer.maybeGetTraceMetadata()
-                            .flatMap(TraceMetadata::getOriginatingSpanId)
-                            .ifPresent(capturedParentSpanId::set);
-                    return null;
-                })
-                .when(delegate)
-                .handleRequest(any());
-
-        handler.handleRequest(exchange);
-
-        assertThat(capturedParentSpanId).hasValue(parentSpanId);
-    }
-
     @Test
     public void whenTraceIsAlreadySampled_doesNotCallSampler() throws Exception {
         exchange.getRequestHeaders().put(HttpString.tryFromString(TraceHttpHeaders.IS_SAMPLED), "1");