diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 6d99a77b6..f1212ff3a 100644 --- a/.palantir/revapi.yml +++ b/.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)\ + \ @ 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)\ + \ @ com.palantir.tracing.api.OpenSpan.Builder" + justification: "Type is not meant for external creation" diff --git a/changelog/@unreleased/pr-778.v2.yml b/changelog/@unreleased/pr-778.v2.yml new file mode 100644 index 000000000..9fc244d42 --- /dev/null +++ b/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 diff --git a/tracing-api/src/main/java/com/palantir/tracing/api/OpenSpan.java b/tracing-api/src/main/java/com/palantir/tracing/api/OpenSpan.java index c32024529..4bbd2df7b 100644 --- a/tracing-api/src/main/java/com/palantir/tracing/api/OpenSpan.java +++ b/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 getOriginatingSpanId(); + @Deprecated + // Intentionally does not set @Value.Default because the value is always empty. + public Optional 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 parentSpanId, - Optional originatingSpanId) { - return ImmutableOpenSpan.of( - operation, getNowInMicroSeconds(), System.nanoTime(), parentSpanId, originatingSpanId, spanId, type); + Optional _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 parentSpanId) { - return of(operation, spanId, type, parentSpanId, Optional.empty()); + return ImmutableOpenSpan.of(operation, getNowInMicroSeconds(), System.nanoTime(), parentSpanId, spanId, type); } private static long getNowInMicroSeconds() { diff --git a/tracing-api/src/main/java/com/palantir/tracing/api/TraceHttpHeaders.java b/tracing-api/src/main/java/com/palantir/tracing/api/TraceHttpHeaders.java index 692e61059..80ed5ecf2 100644 --- a/tracing-api/src/main/java/com/palantir/tracing/api/TraceHttpHeaders.java +++ b/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. * - *

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"; } diff --git a/tracing-jersey/src/test/java/com/palantir/tracing/jersey/TraceEnrichingFilterTest.java b/tracing-jersey/src/test/java/com/palantir/tracing/jersey/TraceEnrichingFilterTest.java index 6570b49fe..65e3ad3cd 100644 --- a/tracing-jersey/src/test/java/com/palantir/tracing/jersey/TraceEnrichingFilterTest.java +++ b/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"); diff --git a/tracing-okhttp3/src/main/java/com/palantir/tracing/OkhttpTraceInterceptor2.java b/tracing-okhttp3/src/main/java/com/palantir/tracing/OkhttpTraceInterceptor2.java index b2ddfa94a..d51460559 100644 --- a/tracing-okhttp3/src/main/java/com/palantir/tracing/OkhttpTraceInterceptor2.java +++ b/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()); } } } diff --git a/tracing-okhttp3/src/main/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptor.java b/tracing-okhttp3/src/main/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptor.java index 052c75f2a..70fa091e9 100644 --- a/tracing-okhttp3/src/main/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptor.java +++ b/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 { diff --git a/tracing-okhttp3/src/test/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptorTest.java b/tracing-okhttp3/src/test/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptorTest.java index 08812f304..31ee58584 100644 --- a/tracing-okhttp3/src/test/java/com/palantir/tracing/okhttp3/OkhttpTraceInterceptorTest.java +++ b/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 diff --git a/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedOperationHandlerTest.java b/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedOperationHandlerTest.java index d589f8b1f..fcd3500ba 100644 --- a/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedOperationHandlerTest.java +++ b/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 capturedParentSpanId = new AtomicReference<>(); - doAnswer((Answer) _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"); diff --git a/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedStateHandlerTest.java b/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedStateHandlerTest.java index dfd1827f1..d614bf2b5 100644 --- a/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedStateHandlerTest.java +++ b/tracing-undertow/src/test/java/com/palantir/tracing/undertow/TracedStateHandlerTest.java @@ -18,43 +18,32 @@ 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; -import com.palantir.tracing.api.Span; import com.palantir.tracing.api.SpanObserver; import com.palantir.tracing.api.TraceHttpHeaders; import io.undertow.server.HttpHandler; import io.undertow.server.HttpServerExchange; import io.undertow.util.HeaderMap; import io.undertow.util.HttpString; -import java.util.List; import java.util.concurrent.atomic.AtomicReference; import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -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) public class TracedStateHandlerTest { - @Captor - private ArgumentCaptor spanCaptor; - @Mock private SpanObserver observer; @@ -94,7 +83,6 @@ public void whenNoTraceIsInHeader_generatesNewTrace() throws Exception { assertThat(Tracer.hasTraceId()).isFalse(); 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))) .isNotEmpty(); @@ -108,45 +96,6 @@ public void whenTraceIsInHeader_usesGivenTraceId() throws Exception { .isEqualTo(traceId); } - @Test - public void whenParentSpanIsGiven_usesParentSpan() throws Exception { - setRequestTraceId(traceId); - String parentSpanId = Tracers.randomId(); - setRequestSpanId(parentSpanId); - - handler.handleRequest(exchange); - // Since we're not running a full request, the completion handler cannot execute normally. - exchange.getAttachment(UndertowTracing.REQUEST_SPAN).complete(); - verify(observer, times(1)).consume(spanCaptor.capture()); - List spans = spanCaptor.getAllValues(); - assertThat(spans).hasSize(1); - Span span = spans.get(0); - assertThat(span.getParentSpanId()).hasValue(parentSpanId); - 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 capturedParentSpanId = new AtomicReference<>(); - doAnswer((Answer) _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"); @@ -208,8 +157,4 @@ public void populatesSlf4jMdc() throws Exception { private void setRequestTraceId(String theTraceId) { exchange.getRequestHeaders().put(HttpString.tryFromString(TraceHttpHeaders.TRACE_ID), theTraceId); } - - private void setRequestSpanId(String spanId) { - exchange.getRequestHeaders().put(HttpString.tryFromString(TraceHttpHeaders.SPAN_ID), spanId); - } } diff --git a/tracing/src/main/java/com/palantir/tracing/Trace.java b/tracing/src/main/java/com/palantir/tracing/Trace.java index f61c5fcf3..7ffbb3fad 100644 --- a/tracing/src/main/java/com/palantir/tracing/Trace.java +++ b/tracing/src/main/java/com/palantir/tracing/Trace.java @@ -63,12 +63,7 @@ private Trace(String traceId, Optional requestId) { final OpenSpan startSpan(String operation, String parentSpanId, SpanType type) { checkState(isEmpty(), "Cannot start a span with explicit parent if the current thread's trace is non-empty"); checkArgument(!Strings.isNullOrEmpty(parentSpanId), "parentSpanId must be non-empty"); - OpenSpan span = OpenSpan.of( - operation, - Tracers.randomId(), - type, - Optional.of(parentSpanId), - orElse(getOriginatingSpanId(), Optional.of(parentSpanId))); + OpenSpan span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.of(parentSpanId)); push(span); return span; } @@ -87,23 +82,15 @@ final OpenSpan startSpan(String operation, SpanType type) { operation, Tracers.randomId(), type, - Optional.of(prevState.get().getSpanId()), - orElse(getOriginatingSpanId(), prevState.get().getParentSpanId())); + Optional.of(prevState.get().getSpanId())); } else { - span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.empty(), getOriginatingSpanId()); + span = OpenSpan.of(operation, Tracers.randomId(), type, Optional.empty()); } push(span); return span; } - private static Optional orElse(Optional left, Optional right) { - if (left.isPresent()) { - return left; - } - return right; - } - /** Like {@link #startSpan(String, String, SpanType)}, but does not return an {@link OpenSpan}. */ abstract void fastStartSpan(String operation, String parentSpanId, SpanType type); @@ -140,8 +127,6 @@ final Optional getRequestId() { return requestId; } - abstract Optional getOriginatingSpanId(); - /** Returns a copy of this Trace which can be independently mutated. */ abstract Trace deepCopy(); @@ -199,14 +184,6 @@ boolean isObservable() { return true; } - @Override - Optional getOriginatingSpanId() { - if (stack.isEmpty()) { - return Optional.empty(); - } - return stack.peekLast().getParentSpanId(); - } - @Override Trace deepCopy() { return new Sampled(new ArrayDeque<>(stack), getTraceId(), getRequestId()); @@ -225,8 +202,6 @@ private static final class Unsampled extends Trace { */ private int numberOfSpans; - private Optional originatingSpanId = Optional.empty(); - private Unsampled(int numberOfSpans, String traceId, Optional requestId) { super(traceId, requestId); this.numberOfSpans = numberOfSpans; @@ -238,8 +213,8 @@ private Unsampled(String traceId, Optional requestId) { } @Override - void fastStartSpan(String _operation, String parentSpanId, SpanType _type) { - startSpan(Optional.of(parentSpanId)); + void fastStartSpan(String _operation, String _parentSpanId, SpanType _type) { + numberOfSpans++; } @Override @@ -248,14 +223,7 @@ void fastStartSpan(String _operation, SpanType _type) { } @Override - protected void push(OpenSpan span) { - startSpan(span.getParentSpanId()); - } - - private void startSpan(Optional parentSpanId) { - if (numberOfSpans == 0) { - originatingSpanId = parentSpanId; - } + protected void push(OpenSpan _span) { numberOfSpans++; } @@ -270,9 +238,6 @@ Optional pop() { if (numberOfSpans > 0) { numberOfSpans--; } - if (numberOfSpans == 0) { - originatingSpanId = Optional.empty(); - } return Optional.empty(); } @@ -287,11 +252,6 @@ boolean isObservable() { return false; } - @Override - Optional getOriginatingSpanId() { - return originatingSpanId; - } - @Override Trace deepCopy() { return new Unsampled(numberOfSpans, getTraceId(), getRequestId()); diff --git a/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java b/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java index 8530b2ad8..7abd455aa 100644 --- a/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java +++ b/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java @@ -33,14 +33,22 @@ public interface TraceMetadata { */ Optional getRequestId(); - /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#SPAN_ID}. */ + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#SPAN_ID} on outgoing requests. */ String getSpanId(); - /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#PARENT_SPAN_ID}. */ + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#SPAN_ID} on incoming requests. */ Optional getParentSpanId(); - /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#ORIGINATING_SPAN_ID}. */ - Optional getOriginatingSpanId(); + /** + * Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#ORIGINATING_SPAN_ID} which is no longer used. + * + * @deprecated No longer used + */ + @Deprecated + // Intentionally does not set @Value.Default because the value is always empty. + default Optional getOriginatingSpanId() { + return Optional.empty(); + } static Builder builder() { return new Builder(); diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index ed37a9759..6b1722bfa 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -117,7 +117,6 @@ public static Optional maybeGetTraceMetadata() { return trace.top().map(openSpan -> TraceMetadata.builder() .spanId(openSpan.getSpanId()) .parentSpanId(openSpan.getParentSpanId()) - .originatingSpanId(trace.getOriginatingSpanId()) .traceId(trace.getTraceId()) .requestId(trace.getRequestId()) .build()); @@ -125,7 +124,6 @@ public static Optional maybeGetTraceMetadata() { return Optional.of(TraceMetadata.builder() .spanId(Tracers.randomId()) .parentSpanId(Optional.empty()) - .originatingSpanId(trace.getOriginatingSpanId()) .traceId(trace.getTraceId()) .requestId(trace.getRequestId()) .build()); @@ -390,12 +388,7 @@ private static final class SampledDetachedSpan implements DetachedSpan { Optional parentSpanId) { this.traceId = traceId; this.requestId = requestId; - this.openSpan = OpenSpan.builder() - .parentSpanId(parentSpanId) - .spanId(Tracers.randomId()) - .operation(operation) - .type(type) - .build(); + this.openSpan = OpenSpan.of(operation, Tracers.randomId(), type, parentSpanId); } @MustBeClosed diff --git a/tracing/src/test/java/com/palantir/tracing/TraceTest.java b/tracing/src/test/java/com/palantir/tracing/TraceTest.java index daa7fb787..5e67e5d29 100644 --- a/tracing/src/test/java/com/palantir/tracing/TraceTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TraceTest.java @@ -25,7 +25,6 @@ import org.junit.Test; public final class TraceTest { - private static final String ORIGINATING_SPAN_ID = "originating span id"; @Test public void constructTrace_emptyTraceId() { @@ -41,38 +40,4 @@ public void testToString() { .contains(span.getOperation()) .contains(span.getSpanId()); } - - @Test - public void testOriginatingTraceId_slow_sampled() { - Trace trace = Trace.of(true, "traceId", Optional.empty()); - OpenSpan originating = trace.startSpan("1", ORIGINATING_SPAN_ID, SpanType.LOCAL); - assertThat(originating.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - OpenSpan span = trace.startSpan("2", SpanType.LOCAL); - assertThat(span.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - } - - @Test - public void testOriginatingTraceId_fast_sampled() { - Trace trace = Trace.of(true, "traceId", Optional.empty()); - trace.fastStartSpan("1", ORIGINATING_SPAN_ID, SpanType.LOCAL); - OpenSpan span = trace.startSpan("2", SpanType.LOCAL); - assertThat(span.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - } - - @Test - public void testOriginatingTraceId_slow_unsampled() { - Trace trace = Trace.of(false, "traceId", Optional.empty()); - OpenSpan originating = trace.startSpan("1", ORIGINATING_SPAN_ID, SpanType.LOCAL); - assertThat(originating.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - OpenSpan span = trace.startSpan("2", SpanType.LOCAL); - assertThat(span.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - } - - @Test - public void testOriginatingTraceId_fast_unsampled() { - Trace trace = Trace.of(false, "traceId", Optional.empty()); - trace.fastStartSpan("1", ORIGINATING_SPAN_ID, SpanType.LOCAL); - OpenSpan span = trace.startSpan("2", SpanType.LOCAL); - assertThat(span.getOriginatingSpanId()).contains(ORIGINATING_SPAN_ID); - } }