From a8f6c767fa04284188f83e9f20d119341616c6af Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Wed, 4 Sep 2019 14:27:51 +0100 Subject: [PATCH 1/5] Revert "CloseableSpan exposes ids necessary to set headers" This reverts commit 1f188ac3c976250628cbc32fd5319db5e1c2cf8a. --- .../com/palantir/tracing/CloseableSpan.java | 7 --- .../java/com/palantir/tracing/Tracer.java | 45 ++++++------------- 2 files changed, 14 insertions(+), 38 deletions(-) diff --git a/tracing/src/main/java/com/palantir/tracing/CloseableSpan.java b/tracing/src/main/java/com/palantir/tracing/CloseableSpan.java index 76fe86123..85f89757e 100644 --- a/tracing/src/main/java/com/palantir/tracing/CloseableSpan.java +++ b/tracing/src/main/java/com/palantir/tracing/CloseableSpan.java @@ -17,7 +17,6 @@ package com.palantir.tracing; import java.io.Closeable; -import java.util.Optional; /** * Closeable marker around a tracing span operation. This object should be used in a try/with block. @@ -31,10 +30,4 @@ public interface CloseableSpan extends Closeable { */ @Override void close(); - - String getSpanId(); - - Optional getParentSpanId(); - - Optional getOriginatingSpanId(); } diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 3231db87e..65a9789f3 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -207,8 +207,8 @@ public CloseableSpan childSpan(String operationName, SpanType type) { warnIfCompleted("startSpanOnCurrentThread"); Trace maybeCurrentTrace = currentTrace.get(); setTrace(Trace.of(true, traceId)); - OpenSpan newSpan = Tracer.startSpan(operationName, openSpan.getSpanId(), type); - return TraceRestoringCloseableSpan.of(maybeCurrentTrace, newSpan); + Tracer.fastStartSpan(operationName, openSpan.getSpanId(), type); + return TraceRestoringCloseableSpan.of(maybeCurrentTrace); } @Override @@ -251,8 +251,8 @@ private static final class UnsampledDetachedSpan implements DetachedSpan { public CloseableSpan childSpan(String operationName, SpanType type) { Trace maybeCurrentTrace = currentTrace.get(); setTrace(Trace.of(false, traceId)); - OpenSpan newSpan = Tracer.startSpan(operationName, type); - return TraceRestoringCloseableSpan.of(maybeCurrentTrace, newSpan); + Tracer.fastStartSpan(operationName, type); + return TraceRestoringCloseableSpan.of(maybeCurrentTrace); } @Override @@ -273,40 +273,23 @@ public String toString() { private static final class TraceRestoringCloseableSpan implements CloseableSpan { - @Nullable - private final Trace traceToRestore; - private final OpenSpan newSpan; + // Complete the current span. + private static final CloseableSpan DEFAULT_TOKEN = Tracer::fastCompleteSpan; - TraceRestoringCloseableSpan(@Nullable Trace traceToRestore, OpenSpan newSpan) { - this.traceToRestore = traceToRestore; - this.newSpan = Preconditions.checkNotNull(newSpan, "OpenSpan"); - } + private final Trace original; - public static CloseableSpan of(@Nullable Trace traceToRestore, OpenSpan newSpan) { - return new TraceRestoringCloseableSpan(traceToRestore, newSpan); + static CloseableSpan of(@Nullable Trace original) { + return original == null ? DEFAULT_TOKEN : new TraceRestoringCloseableSpan(original); } - @Override - public void close() { - Tracer.fastCompleteSpan(); - if (traceToRestore != null) { - Tracer.setTrace(traceToRestore); - } + TraceRestoringCloseableSpan(Trace original) { + this.original = Preconditions.checkNotNull(original, "Expected an original trace instance"); } @Override - public String getSpanId() { - return newSpan.getSpanId(); - } - - @Override - public Optional getParentSpanId() { - return newSpan.getParentSpanId(); - } - - @Override - public Optional getOriginatingSpanId() { - return newSpan.getOriginatingSpanId(); + public void close() { + DEFAULT_TOKEN.close(); + Tracer.setTrace(original); } } From 1d030665bac9ef9e39d88c319773a22615f36e12 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Wed, 4 Sep 2019 15:11:31 +0100 Subject: [PATCH 2/5] Tracer#getTraceMetadata --- .../com/palantir/tracing/TraceMetadata.java | 44 +++++++++++++++++++ .../java/com/palantir/tracing/Tracer.java | 27 ++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 tracing/src/main/java/com/palantir/tracing/TraceMetadata.java diff --git a/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java b/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java new file mode 100644 index 000000000..2f2e4961e --- /dev/null +++ b/tracing/src/main/java/com/palantir/tracing/TraceMetadata.java @@ -0,0 +1,44 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.tracing; + +import java.util.Optional; +import org.immutables.value.Value; + +/** Ids necessary to write headers onto network requests. */ +@Value.Immutable +@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE) +interface TraceMetadata { + + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#TRACE_ID}. */ + String traceId(); + + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#SPAN_ID}. */ + String spanId(); + + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#PARENT_SPAN_ID}. */ + Optional parentSpanId(); + + /** Corresponds to {@link com.palantir.tracing.api.TraceHttpHeaders#ORIGINATING_SPAN_ID}. */ + Optional originatingSpanId(); + + static Builder builder() { + return new Builder(); + } + + class Builder extends ImmutableTraceMetadata.Builder {} +} diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 65a9789f3..f1c612169 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -28,6 +28,7 @@ import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.UnsafeArg; import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; +import com.palantir.logsafe.exceptions.SafeRuntimeException; import com.palantir.tracing.api.OpenSpan; import com.palantir.tracing.api.Span; import com.palantir.tracing.api.SpanObserver; @@ -89,6 +90,32 @@ private static boolean shouldObserve(Observability observability) { throw new SafeIllegalArgumentException("Unknown observability", SafeArg.of("observability", observability)); } + static TraceMetadata getTraceMetadata() { + Trace trace = checkNotNull(currentTrace.get(), "Unable to getTraceMetadata when there is trace in progress"); + + if (isTraceObservable()) { + OpenSpan openSpan = trace.top() + .orElseThrow(() -> new SafeRuntimeException("Trace with no spans in progress")); + return TraceMetadata.builder() + .spanId(openSpan.getSpanId()) + .parentSpanId(openSpan.getParentSpanId()) + .originatingSpanId(trace.getOriginatingSpanId()) + .traceId(trace.getTraceId()) + .build(); + } else { + // In the unsampled case, the Trace.Unsampled class doesn't actually store a spanId/parentSpanId + // stack, so we just make one up (just in time). This matches the behaviour of Tracer#startSpan. + + // 🌶🌶🌶 this is a bit funky because calling getTraceMetadata multiple times will return different spanIds + return TraceMetadata.builder() + .spanId(Tracers.randomId()) + .parentSpanId(Optional.of(Tracers.randomId())) + .originatingSpanId(trace.getOriginatingSpanId()) + .traceId(trace.getTraceId()) + .build(); + } + } + /** * Deprecated. * From cc77f829d4ebecd21a2c3ea789a047678ea018fb Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Wed, 4 Sep 2019 15:23:41 +0100 Subject: [PATCH 3/5] parentSpanId is empty --- tracing/src/main/java/com/palantir/tracing/Tracer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index f1c612169..4739bf006 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -109,7 +109,7 @@ static TraceMetadata getTraceMetadata() { // 🌶🌶🌶 this is a bit funky because calling getTraceMetadata multiple times will return different spanIds return TraceMetadata.builder() .spanId(Tracers.randomId()) - .parentSpanId(Optional.of(Tracers.randomId())) + .parentSpanId(Optional.empty()) .originatingSpanId(trace.getOriginatingSpanId()) .traceId(trace.getTraceId()) .build(); From b700633b0c2e32840c33a95aadcd72241a2740d1 Mon Sep 17 00:00:00 2001 From: Dan Fox Date: Wed, 4 Sep 2019 15:42:47 +0100 Subject: [PATCH 4/5] nit --- tracing/src/main/java/com/palantir/tracing/Tracer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 4739bf006..419b1108f 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -93,7 +93,7 @@ private static boolean shouldObserve(Observability observability) { static TraceMetadata getTraceMetadata() { Trace trace = checkNotNull(currentTrace.get(), "Unable to getTraceMetadata when there is trace in progress"); - if (isTraceObservable()) { + if (trace.isObservable()) { OpenSpan openSpan = trace.top() .orElseThrow(() -> new SafeRuntimeException("Trace with no spans in progress")); return TraceMetadata.builder() From b54a116632c45708c62406cd64959c16f0e3520f Mon Sep 17 00:00:00 2001 From: iamdanfox Date: Wed, 4 Sep 2019 15:53:47 +0100 Subject: [PATCH 5/5] Update tracing/src/main/java/com/palantir/tracing/Tracer.java Co-Authored-By: Carter Kozak --- tracing/src/main/java/com/palantir/tracing/Tracer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 419b1108f..d850cb2f5 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -106,7 +106,7 @@ static TraceMetadata getTraceMetadata() { // In the unsampled case, the Trace.Unsampled class doesn't actually store a spanId/parentSpanId // stack, so we just make one up (just in time). This matches the behaviour of Tracer#startSpan. - // 🌶🌶🌶 this is a bit funky because calling getTraceMetadata multiple times will return different spanIds + // n.b. this is a bit funky because calling getTraceMetadata multiple times will return different spanIds return TraceMetadata.builder() .spanId(Tracers.randomId()) .parentSpanId(Optional.empty())