From e83d76e32bd1a845f8ea1c6f177913ce258084dc Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 6 Jun 2019 17:16:07 -0400 Subject: [PATCH 1/4] Avoid trace ID allocations when not observing Lazily generate trace IDs only for observable traces to avoid the memory allocation, GC, and CPU overhead of random generation. --- .../tracing/jersey/TraceEnrichingFilter.java | 2 +- .../main/java/com/palantir/tracing/Trace.java | 21 +++++- .../java/com/palantir/tracing/Tracer.java | 19 ++++-- .../java/com/palantir/tracing/Tracers.java | 65 ++++++++++++++++++- .../java/com/palantir/tracing/TraceTest.java | 24 +++++++ .../java/com/palantir/tracing/TracerTest.java | 16 +++-- .../com/palantir/tracing/TracersTest.java | 16 +++++ 7 files changed, 146 insertions(+), 17 deletions(-) diff --git a/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java b/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java index fb0a6029f..4c57f1fde 100644 --- a/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java +++ b/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java @@ -67,7 +67,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { // Set up thread-local span that inherits state from HTTP headers if (Strings.isNullOrEmpty(traceId)) { // HTTP request did not indicate a trace; initialize trace state and create a span. - Tracer.initTrace(getObservabilityFromHeader(requestContext), Tracers.randomId()); + Tracer.initTrace(getObservabilityFromHeader(requestContext), Tracers.lazyRandomId()); Tracer.startSpan(operation, SpanType.SERVER_INCOMING); } else { Tracer.initTrace(getObservabilityFromHeader(requestContext), traceId); diff --git a/tracing/src/main/java/com/palantir/tracing/Trace.java b/tracing/src/main/java/com/palantir/tracing/Trace.java index dbc8db4e3..0a1e882cf 100644 --- a/tracing/src/main/java/com/palantir/tracing/Trace.java +++ b/tracing/src/main/java/com/palantir/tracing/Trace.java @@ -30,11 +30,13 @@ */ public final class Trace { + private static final Trace NOOP = new Trace(false, "noop"); + private final Deque stack; private final boolean isObservable; private final String traceId; - private Trace(ArrayDeque stack, boolean isObservable, String traceId) { + private Trace(Deque stack, boolean isObservable, String traceId) { checkArgument(!traceId.isEmpty(), "traceId must be non-empty"); this.stack = stack; @@ -42,12 +44,21 @@ private Trace(ArrayDeque stack, boolean isObservable, String traceId) this.traceId = traceId; } + static Trace create(boolean isObservable, CharSequence traceId) { + if (isObservable) { + return new Trace(isObservable, traceId.toString()); + } + return NOOP; + } + Trace(boolean isObservable, String traceId) { this(new ArrayDeque<>(), isObservable, traceId); } void push(OpenSpan span) { - stack.push(span); + if (isObservable) { + stack.push(span); + } } Optional top() { @@ -79,7 +90,11 @@ String getTraceId() { /** Returns a copy of this Trace which can be independently mutated. */ Trace deepCopy() { - return new Trace(new ArrayDeque<>(stack), isObservable, traceId); + if (isObservable) { + return new Trace(new ArrayDeque<>(stack), isObservable, traceId); + } else { + return NOOP; + } } @Override diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 9364a5e23..b105de8b2 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -65,10 +65,10 @@ private Tracer() {} /** * Creates a new trace, but does not set it as the current trace. */ - private static Trace createTrace(Observability observability, String traceId) { - checkArgument(!Strings.isNullOrEmpty(traceId), "traceId must be non-empty"); + private static Trace createTrace(Observability observability, CharSequence traceId) { + checkArgument(traceId != null && traceId.length() != 0, "traceId must be non-empty"); boolean observable = shouldObserve(observability); - return new Trace(observable, traceId); + return Trace.create(observable, traceId); } private static boolean shouldObserve(Observability observability) { @@ -94,14 +94,21 @@ public static void initTrace(Optional isObservable, String traceId) { Observability observability = isObservable .map(value -> Boolean.TRUE.equals(value) ? Observability.SAMPLE : Observability.DO_NOT_SAMPLE) .orElse(Observability.UNDECIDED); - - setTrace(createTrace(observability, traceId)); + createAndInitTrace(observability, traceId); } /** * Initializes the current thread's trace, erasing any previously accrued open spans. */ public static void initTrace(Observability observability, String traceId) { + createAndInitTrace(observability, traceId); + } + + public static void initTrace(Observability observability, CharSequence traceId) { + createAndInitTrace(observability, traceId); + } + + private static void createAndInitTrace(Observability observability, CharSequence traceId) { setTrace(createTrace(observability, traceId)); } @@ -371,7 +378,7 @@ private static void setTraceSampledMdcIfObservable(boolean observable) { private static Trace getOrCreateCurrentTrace() { Trace trace = currentTrace.get(); if (trace == null) { - trace = createTrace(Observability.UNDECIDED, Tracers.randomId()); + trace = createTrace(Observability.UNDECIDED, Tracers.lazyRandomId()); setTrace(trace); } return trace; diff --git a/tracing/src/main/java/com/palantir/tracing/Tracers.java b/tracing/src/main/java/com/palantir/tracing/Tracers.java index 47dee7d7e..462333b8b 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracers.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracers.java @@ -16,6 +16,7 @@ package com.palantir.tracing; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; @@ -41,6 +42,11 @@ public static String randomId() { return longToPaddedHex(ThreadLocalRandom.current().nextLong()); } + /** Returns a random ID suitable for span and trace IDs. */ + public static CharSequence lazyRandomId() { + return new LazyRandomId(); + } + /** * Convert a long to a big-endian hex string. Hand-coded implementation is more efficient than * Strings.pad(Long.toHexString) because that code has to deal with mixed length longs, and then mixed length @@ -234,7 +240,7 @@ public static Callable wrapWithNewTrace(String operation, Observability o Optional originalTrace = Tracer.getAndClearTraceIfPresent(); try { - Tracer.initTrace(observability, Tracers.randomId()); + Tracer.initTrace(observability, Tracers.lazyRandomId()); Tracer.startSpan(operation); return delegate.call(); } finally { @@ -270,7 +276,7 @@ public static Runnable wrapWithNewTrace(String operation, Observability observab Optional originalTrace = Tracer.getAndClearTraceIfPresent(); try { - Tracer.initTrace(observability, Tracers.randomId()); + Tracer.initTrace(observability, Tracers.lazyRandomId()); Tracer.startSpan(operation); delegate.run(); } finally { @@ -383,4 +389,59 @@ public void run() { public interface ThrowingCallable { T call() throws E; } + + private static final class LazyRandomId implements CharSequence { + private String randomId; + + private String id() { + String id = this.randomId; + if (id == null) { + synchronized (this) { + if (this.randomId == null) { + id = randomId(); + this.randomId = id; + } + } + } + return id; + } + + @Override + public int length() { + return id().length(); + } + + @Override + public char charAt(int index) { + return id().charAt(index); + } + + @Override + public CharSequence subSequence(int start, int end) { + return id().subSequence(start, end); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + + LazyRandomId that = (LazyRandomId) obj; + return Objects.equals(this.id(), that.id()); + } + + @Override + public int hashCode() { + return id().hashCode(); + } + + @Override + public String toString() { + return id(); + } + } } diff --git a/tracing/src/test/java/com/palantir/tracing/TraceTest.java b/tracing/src/test/java/com/palantir/tracing/TraceTest.java index e2fa8c2f5..063a5ef52 100644 --- a/tracing/src/test/java/com/palantir/tracing/TraceTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TraceTest.java @@ -44,4 +44,28 @@ public void testToString() { assertThat(trace.toString()).isEqualTo("Trace{stack=[OpenSpan{operation=operation, startTimeMicroSeconds=0, " + "startClockNanoSeconds=0, spanId=spanId, type=LOCAL}], isObservable=true, traceId='traceId'}"); } + + @Test + public void noop() { + Trace traceId = Trace.create(false, "traceId"); + assertThat(traceId.getTraceId()).isEqualTo("noop"); + assertThat(traceId.isObservable()).isFalse(); + assertThat(traceId.isEmpty()).isTrue(); + assertThat(traceId.pop()).isNotPresent(); + assertThat(traceId.top()).isNotPresent(); + assertThat(traceId.deepCopy()).isSameAs(traceId); + + traceId.push(OpenSpan.builder().spanId("spanId").operation("operation").type(SpanType.LOCAL).build()); + assertThat(traceId.getTraceId()).isEqualTo("noop"); + assertThat(traceId.isObservable()).isFalse(); + assertThat(traceId.isEmpty()).isTrue(); + assertThat(traceId.pop()).isNotPresent(); + assertThat(traceId.top()).isNotPresent(); + assertThat(traceId.deepCopy()).isSameAs(traceId); + + assertThat(traceId.toString()).isEqualTo("Trace{stack=[], isObservable=false, traceId='noop'}"); + + assertThat(traceId).isSameAs(Trace.create(false, "traceId2")); + } + } diff --git a/tracing/src/test/java/com/palantir/tracing/TracerTest.java b/tracing/src/test/java/com/palantir/tracing/TracerTest.java index d9d63e473..3e9f9d78e 100644 --- a/tracing/src/test/java/com/palantir/tracing/TracerTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TracerTest.java @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import javax.annotation.Nullable; import org.assertj.core.util.Sets; import org.junit.After; import org.junit.Before; @@ -138,17 +139,20 @@ public void testObserversAreInvokedOnObservableTracesOnly() throws Exception { verifyNoMoreInteractions(observer1); Tracer.initTrace(Observability.DO_NOT_SAMPLE, Tracers.randomId()); - startAndCompleteSpan(); // not sampled, see above + assertThat(Tracer.isTraceObservable()).isFalse(); + assertThat(startAndCompleteSpan()).isNull(); // not sampled, see above verifyNoMoreInteractions(observer1); } @Test public void testDerivesNewSpansWhenTraceIsNotObservable() throws Exception { Tracer.initTrace(Observability.DO_NOT_SAMPLE, Tracers.randomId()); + assertThat(Tracer.isTraceObservable()).isFalse(); Tracer.startSpan("foo"); Tracer.startSpan("bar"); - assertThat(Tracer.completeSpan().get().getOperation()).isEqualTo("bar"); - assertThat(Tracer.completeSpan().get().getOperation()).isEqualTo("foo"); + assertThat(Tracer.completeSpan()).isNotPresent(); + assertThat(Tracer.completeSpan()).isNotPresent(); + assertThat(Tracer.isTraceObservable()).isFalse(); } @Test @@ -163,7 +167,8 @@ public void testInitTraceCallsSampler() throws Exception { verifyNoMoreInteractions(observer1, sampler); Mockito.reset(observer1, sampler); - startAndCompleteSpan(); // not sampled, see above + assertThat(Tracer.isTraceObservable()).isFalse(); + assertThat(startAndCompleteSpan()).isNull(); // not sampled, see above verify(sampler).sample(); verifyNoMoreInteractions(observer1, sampler); } @@ -329,8 +334,9 @@ public void testHasTraceId() { assertThat(Tracer.hasTraceId()).isEqualTo(false); } + @Nullable private static Span startAndCompleteSpan() { Tracer.startSpan("operation"); - return Tracer.completeSpan().get(); + return Tracer.completeSpan().orElse(null); } } diff --git a/tracing/src/test/java/com/palantir/tracing/TracersTest.java b/tracing/src/test/java/com/palantir/tracing/TracersTest.java index b46a5aed3..ee26cceca 100644 --- a/tracing/src/test/java/com/palantir/tracing/TracersTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TracersTest.java @@ -566,6 +566,22 @@ public void testTraceIdGeneration() throws Exception { assertThat(Tracers.longToPaddedHex(123456789L)).isEqualTo("00000000075bcd15"); } + @Test + public void testLazyIdGeneration() { + CharSequence lazyRandomId = Tracers.lazyRandomId(); + assertThat(lazyRandomId) + .isNotInstanceOf(String.class) + .isEqualTo(lazyRandomId) + .hasSize(16); + assertThat(lazyRandomId.length()).isEqualTo(16); + assertThat(lazyRandomId.toString()).isSameAs(lazyRandomId.toString()); + assertThat(Tracers.lazyRandomId()).isNotEqualTo(Tracers.lazyRandomId()); + + for (int i = 0; i < 160; i++) { + assertThat(Tracers.lazyRandomId()).hasSize(16); // fails with p=1/16 if generated string is not padded + } + } + private static Callable newTraceExpectingCallable(String expectedOperation) { final Set seenTraceIds = new HashSet<>(); seenTraceIds.add(Tracer.getTraceId()); From 96651be163b25c8eaefdca281f3669314de7ab6e Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 6 Jun 2019 17:34:59 -0400 Subject: [PATCH 2/4] Avoid creating spans when trace is not observable --- .../main/java/com/palantir/tracing/Tracer.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 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 b105de8b2..6d23a701b 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -48,6 +48,13 @@ public final class Tracer { private static final Logger log = LoggerFactory.getLogger(Tracer.class); + private static final OpenSpan NOOP_SPAN = OpenSpan.builder() + .type(SpanType.LOCAL) + .operation("noop") + .parentSpanId(Optional.empty()) + .spanId("noop") + .build(); + private Tracer() {} // Thread-safe since thread-local @@ -118,6 +125,10 @@ private static void createAndInitTrace(Observability observability, CharSequence */ public static OpenSpan startSpan(String operation, String parentSpanId, SpanType type) { Trace current = getOrCreateCurrentTrace(); + if (!current.isObservable()) { + return NOOP_SPAN; + } + checkState(current.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"); @@ -146,12 +157,16 @@ public static OpenSpan startSpan(String operation) { } private static OpenSpan startSpanInternal(String operation, SpanType type) { + Trace trace = getOrCreateCurrentTrace(); + if (!trace.isObservable()) { + return NOOP_SPAN; + } + OpenSpan.Builder spanBuilder = OpenSpan.builder() .operation(operation) .spanId(Tracers.randomId()) .type(type); - Trace trace = getOrCreateCurrentTrace(); Optional prevState = trace.top(); // Avoid lambda allocation in hot paths if (prevState.isPresent()) { From d8c16ab6fb25e62b3ef500e32e7f9600f9e7374e Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Thu, 6 Jun 2019 17:48:47 -0400 Subject: [PATCH 3/4] update tests to handle optionals properly --- .../com/palantir/tracing/jaxrs/JaxRsTracersTest.java | 7 +++++-- .../src/main/java/com/palantir/tracing/Tracer.java | 6 ++++++ .../com/palantir/tracing/CloseableTracerTest.java | 11 +++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/tracing-jaxrs/src/test/java/com/palantir/tracing/jaxrs/JaxRsTracersTest.java b/tracing-jaxrs/src/test/java/com/palantir/tracing/jaxrs/JaxRsTracersTest.java index 081625f1a..4d29d58d9 100644 --- a/tracing-jaxrs/src/test/java/com/palantir/tracing/jaxrs/JaxRsTracersTest.java +++ b/tracing-jaxrs/src/test/java/com/palantir/tracing/jaxrs/JaxRsTracersTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import com.palantir.tracing.Tracer; +import com.palantir.tracing.api.Span; import java.io.ByteArrayOutputStream; import javax.ws.rs.core.StreamingOutput; import org.junit.Test; @@ -32,14 +33,16 @@ public void testWrappingStreamingOutput_streamingOutputTraceIsIsolated() throws Tracer.startSpan("inside"); // never completed }); streamingOutput.write(new ByteArrayOutputStream()); - assertThat(Tracer.completeSpan().get().getOperation()).isEqualTo("outside"); + Tracer.completeSpan().map(Span::getOperation) + .ifPresent(op -> assertThat(op).isEqualTo("outside")); } @Test public void testWrappingStreamingOutput_traceStateIsCapturedAtConstructionTime() throws Exception { Tracer.startSpan("before-construction"); StreamingOutput streamingOutput = JaxRsTracers.wrap(os -> { - assertThat(Tracer.completeSpan().get().getOperation()).isEqualTo("streaming-output"); + Tracer.completeSpan().map(Span::getOperation) + .ifPresent(op -> assertThat(op).isEqualTo("streaming-output")); }); Tracer.startSpan("after-construction"); streamingOutput.write(new ByteArrayOutputStream()); diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 6d23a701b..e87ccea4b 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -20,6 +20,7 @@ import static com.palantir.logsafe.Preconditions.checkNotNull; import static com.palantir.logsafe.Preconditions.checkState; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.errorprone.annotations.CheckReturnValue; import com.palantir.logsafe.SafeArg; @@ -326,6 +327,11 @@ public static void setSampler(TraceSampler sampler) { Tracer.sampler = sampler; } + @VisibleForTesting + static TraceSampler getSampler() { + return Tracer.sampler; + } + /** Returns true if there is an active trace on this thread. */ public static boolean hasTraceId() { return currentTrace.get() != null; diff --git a/tracing/src/test/java/com/palantir/tracing/CloseableTracerTest.java b/tracing/src/test/java/com/palantir/tracing/CloseableTracerTest.java index 6a31cf72c..a5b35e30f 100644 --- a/tracing/src/test/java/com/palantir/tracing/CloseableTracerTest.java +++ b/tracing/src/test/java/com/palantir/tracing/CloseableTracerTest.java @@ -20,6 +20,7 @@ import com.palantir.tracing.api.OpenSpan; import com.palantir.tracing.api.SpanType; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -27,11 +28,21 @@ @RunWith(MockitoJUnitRunner.class) public final class CloseableTracerTest { + + private TraceSampler originalSampler; + @Before public void before() { + originalSampler = Tracer.getSampler(); + Tracer.setSampler(AlwaysSampler.INSTANCE); Tracer.getAndClearTrace(); } + @After + public void after() { + Tracer.setSampler(originalSampler); + } + @Test public void startsAndClosesSpan() { try (CloseableTracer tracer = CloseableTracer.startSpan("foo")) { From de0800fc7d41ff4e5fa6694129766ce36194c6e4 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 7 Jun 2019 13:24:55 -0400 Subject: [PATCH 4/4] Remove lazy trace ID generation --- .../tracing/jersey/TraceEnrichingFilter.java | 2 +- .../com/palantir/tracing/DeferredTracer.java | 2 +- .../palantir/tracing/ImmutableEmptyDeque.java | 232 ++++++++++++++++++ .../main/java/com/palantir/tracing/Trace.java | 21 +- .../java/com/palantir/tracing/Tracer.java | 19 +- .../java/com/palantir/tracing/Tracers.java | 65 +---- .../java/com/palantir/tracing/TraceTest.java | 28 ++- .../java/com/palantir/tracing/TracerTest.java | 10 +- .../com/palantir/tracing/TracersTest.java | 16 -- 9 files changed, 271 insertions(+), 124 deletions(-) create mode 100644 tracing/src/main/java/com/palantir/tracing/ImmutableEmptyDeque.java diff --git a/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java b/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java index 4c57f1fde..fb0a6029f 100644 --- a/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java +++ b/tracing-jersey/src/main/java/com/palantir/tracing/jersey/TraceEnrichingFilter.java @@ -67,7 +67,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { // Set up thread-local span that inherits state from HTTP headers if (Strings.isNullOrEmpty(traceId)) { // HTTP request did not indicate a trace; initialize trace state and create a span. - Tracer.initTrace(getObservabilityFromHeader(requestContext), Tracers.lazyRandomId()); + Tracer.initTrace(getObservabilityFromHeader(requestContext), Tracers.randomId()); Tracer.startSpan(operation, SpanType.SERVER_INCOMING); } else { Tracer.initTrace(getObservabilityFromHeader(requestContext), traceId); diff --git a/tracing/src/main/java/com/palantir/tracing/DeferredTracer.java b/tracing/src/main/java/com/palantir/tracing/DeferredTracer.java index 5f81369a1..7a2a650bd 100644 --- a/tracing/src/main/java/com/palantir/tracing/DeferredTracer.java +++ b/tracing/src/main/java/com/palantir/tracing/DeferredTracer.java @@ -104,7 +104,7 @@ public T withTrace(Tracers.ThrowingCallable inner Optional originalTrace = Tracer.copyTrace(); - Tracer.setTrace(new Trace(isObservable, traceId)); + Tracer.setTrace(Trace.create(isObservable, traceId)); if (parentSpanId != null) { Tracer.startSpan(operation, parentSpanId, SpanType.LOCAL); } else { diff --git a/tracing/src/main/java/com/palantir/tracing/ImmutableEmptyDeque.java b/tracing/src/main/java/com/palantir/tracing/ImmutableEmptyDeque.java new file mode 100644 index 000000000..42373ab65 --- /dev/null +++ b/tracing/src/main/java/com/palantir/tracing/ImmutableEmptyDeque.java @@ -0,0 +1,232 @@ +/* + * (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.Collection; +import java.util.Collections; +import java.util.Deque; +import java.util.Iterator; +import java.util.NoSuchElementException; + +final class ImmutableEmptyDeque implements Deque { + + private static final Object[] EMPTY_ARRAY = new Object[0]; + private static final ImmutableEmptyDeque EMPTY_DEQUE = new ImmutableEmptyDeque<>(); + + ImmutableEmptyDeque() {} + + @SuppressWarnings("unchecked") + static Deque instance() { + return (Deque) EMPTY_DEQUE; + } + + private static UnsupportedOperationException cannotModify() { + return new UnsupportedOperationException("cannot modify immutable empty deque"); + } + + @Override + public boolean isEmpty() { + return true; + } + + @Override + public int size() { + return 0; + } + + @Override + public boolean equals(Object obj) { + return this == obj || this.getClass() == obj.getClass(); + } + + @Override + public int hashCode() { + return 0; + } + + @Override + public String toString() { + return "[]"; + } + + @Override + public Iterator iterator() { + return Collections.emptyIterator(); + } + + @Override + public Iterator descendingIterator() { + return Collections.emptyIterator(); + } + + @Override + public Object[] toArray() { + return EMPTY_ARRAY; + } + + @Override + @SuppressWarnings("unchecked") + public T[] toArray(T[] array) { + return (T[]) EMPTY_ARRAY; + } + + @Override + public boolean add(E element) { + throw cannotModify(); + } + + @Override + public boolean addAll(Collection collection) { + throw cannotModify(); + } + + @Override + public void addFirst(E element) { + throw cannotModify(); + } + + @Override + public void addLast(E element) { + throw cannotModify(); + } + + @Override + public void clear() { + throw cannotModify(); + } + + @Override + public boolean contains(Object obj) { + return false; + } + + @Override + public boolean containsAll(Collection collection) { + return false; + } + + @Override + public E element() { + throw new NoSuchElementException(); + } + + @Override + public E getFirst() { + throw new NoSuchElementException(); + } + + @Override + public E getLast() { + throw new NoSuchElementException(); + } + + @Override + public boolean offer(E element) { + throw cannotModify(); + } + + @Override + public boolean offerFirst(E element) { + throw cannotModify(); + } + + @Override + public boolean offerLast(E element) { + throw cannotModify(); + } + + @Override + public E peek() { + return null; + } + + @Override + public E peekFirst() { + return null; + } + + @Override + public E peekLast() { + return null; + } + + @Override + public E poll() { + return null; + } + + @Override + public E pollFirst() { + return null; + } + + @Override + public E pollLast() { + return null; + } + + @Override + public E pop() { + throw cannotModify(); + } + + @Override + public void push(E element) { + throw cannotModify(); + } + + @Override + public E remove() { + throw cannotModify(); + } + + @Override + public boolean remove(Object obj) { + throw cannotModify(); + } + + @Override + public boolean removeAll(Collection collection) { + throw cannotModify(); + } + + @Override + public E removeFirst() { + throw cannotModify(); + } + + @Override + public boolean removeFirstOccurrence(Object obj) { + throw cannotModify(); + } + + @Override + public E removeLast() { + throw cannotModify(); + } + + @Override + public boolean removeLastOccurrence(Object obj) { + throw cannotModify(); + } + + @Override + public boolean retainAll(Collection collection) { + throw cannotModify(); + } + +} diff --git a/tracing/src/main/java/com/palantir/tracing/Trace.java b/tracing/src/main/java/com/palantir/tracing/Trace.java index 0a1e882cf..a31c2b36d 100644 --- a/tracing/src/main/java/com/palantir/tracing/Trace.java +++ b/tracing/src/main/java/com/palantir/tracing/Trace.java @@ -30,8 +30,6 @@ */ public final class Trace { - private static final Trace NOOP = new Trace(false, "noop"); - private final Deque stack; private final boolean isObservable; private final String traceId; @@ -44,15 +42,9 @@ private Trace(Deque stack, boolean isObservable, String traceId) { this.traceId = traceId; } - static Trace create(boolean isObservable, CharSequence traceId) { - if (isObservable) { - return new Trace(isObservable, traceId.toString()); - } - return NOOP; - } - - Trace(boolean isObservable, String traceId) { - this(new ArrayDeque<>(), isObservable, traceId); + static Trace create(boolean isObservable, String traceId) { + Deque deque = isObservable ? new ArrayDeque<>() : ImmutableEmptyDeque.instance(); + return new Trace(deque, isObservable, traceId); } void push(OpenSpan span) { @@ -90,11 +82,8 @@ String getTraceId() { /** Returns a copy of this Trace which can be independently mutated. */ Trace deepCopy() { - if (isObservable) { - return new Trace(new ArrayDeque<>(stack), isObservable, traceId); - } else { - return NOOP; - } + Deque deque = isObservable ? new ArrayDeque<>(stack) : ImmutableEmptyDeque.instance(); + return new Trace(deque, isObservable, traceId); } @Override diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index e87ccea4b..0d92e732e 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -49,6 +49,9 @@ public final class Tracer { private static final Logger log = LoggerFactory.getLogger(Tracer.class); + /** + * No-op span used internally to avoid allocating when trace is unobservable. + */ private static final OpenSpan NOOP_SPAN = OpenSpan.builder() .type(SpanType.LOCAL) .operation("noop") @@ -73,8 +76,8 @@ private Tracer() {} /** * Creates a new trace, but does not set it as the current trace. */ - private static Trace createTrace(Observability observability, CharSequence traceId) { - checkArgument(traceId != null && traceId.length() != 0, "traceId must be non-empty"); + private static Trace createTrace(Observability observability, String traceId) { + checkArgument(!Strings.isNullOrEmpty(traceId), "traceId must be non-empty"); boolean observable = shouldObserve(observability); return Trace.create(observable, traceId); } @@ -102,21 +105,13 @@ public static void initTrace(Optional isObservable, String traceId) { Observability observability = isObservable .map(value -> Boolean.TRUE.equals(value) ? Observability.SAMPLE : Observability.DO_NOT_SAMPLE) .orElse(Observability.UNDECIDED); - createAndInitTrace(observability, traceId); + setTrace(createTrace(observability, traceId)); } /** * Initializes the current thread's trace, erasing any previously accrued open spans. */ public static void initTrace(Observability observability, String traceId) { - createAndInitTrace(observability, traceId); - } - - public static void initTrace(Observability observability, CharSequence traceId) { - createAndInitTrace(observability, traceId); - } - - private static void createAndInitTrace(Observability observability, CharSequence traceId) { setTrace(createTrace(observability, traceId)); } @@ -399,7 +394,7 @@ private static void setTraceSampledMdcIfObservable(boolean observable) { private static Trace getOrCreateCurrentTrace() { Trace trace = currentTrace.get(); if (trace == null) { - trace = createTrace(Observability.UNDECIDED, Tracers.lazyRandomId()); + trace = createTrace(Observability.UNDECIDED, Tracers.randomId()); setTrace(trace); } return trace; diff --git a/tracing/src/main/java/com/palantir/tracing/Tracers.java b/tracing/src/main/java/com/palantir/tracing/Tracers.java index 462333b8b..a66d272db 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracers.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracers.java @@ -16,7 +16,6 @@ package com.palantir.tracing; -import java.util.Objects; import java.util.Optional; import java.util.concurrent.Callable; import java.util.concurrent.ExecutorService; @@ -42,11 +41,6 @@ public static String randomId() { return longToPaddedHex(ThreadLocalRandom.current().nextLong()); } - /** Returns a random ID suitable for span and trace IDs. */ - public static CharSequence lazyRandomId() { - return new LazyRandomId(); - } - /** * Convert a long to a big-endian hex string. Hand-coded implementation is more efficient than * Strings.pad(Long.toHexString) because that code has to deal with mixed length longs, and then mixed length @@ -240,7 +234,7 @@ public static Callable wrapWithNewTrace(String operation, Observability o Optional originalTrace = Tracer.getAndClearTraceIfPresent(); try { - Tracer.initTrace(observability, Tracers.lazyRandomId()); + Tracer.initTrace(observability, randomId()); Tracer.startSpan(operation); return delegate.call(); } finally { @@ -276,7 +270,7 @@ public static Runnable wrapWithNewTrace(String operation, Observability observab Optional originalTrace = Tracer.getAndClearTraceIfPresent(); try { - Tracer.initTrace(observability, Tracers.lazyRandomId()); + Tracer.initTrace(observability, randomId()); Tracer.startSpan(operation); delegate.run(); } finally { @@ -389,59 +383,4 @@ public void run() { public interface ThrowingCallable { T call() throws E; } - - private static final class LazyRandomId implements CharSequence { - private String randomId; - - private String id() { - String id = this.randomId; - if (id == null) { - synchronized (this) { - if (this.randomId == null) { - id = randomId(); - this.randomId = id; - } - } - } - return id; - } - - @Override - public int length() { - return id().length(); - } - - @Override - public char charAt(int index) { - return id().charAt(index); - } - - @Override - public CharSequence subSequence(int start, int end) { - return id().subSequence(start, end); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null || getClass() != obj.getClass()) { - return false; - } - - LazyRandomId that = (LazyRandomId) obj; - return Objects.equals(this.id(), that.id()); - } - - @Override - public int hashCode() { - return id().hashCode(); - } - - @Override - public String toString() { - return id(); - } - } } diff --git a/tracing/src/test/java/com/palantir/tracing/TraceTest.java b/tracing/src/test/java/com/palantir/tracing/TraceTest.java index 063a5ef52..7c74c891e 100644 --- a/tracing/src/test/java/com/palantir/tracing/TraceTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TraceTest.java @@ -27,13 +27,13 @@ public final class TraceTest { @Test public void constructTrace_emptyTraceId() { - assertThatThrownBy(() -> new Trace(false, "")) + assertThatThrownBy(() -> Trace.create(false, "")) .isInstanceOf(IllegalArgumentException.class); } @Test public void testToString() { - Trace trace = new Trace(true, "traceId"); + Trace trace = Trace.create(true, "traceId"); trace.push(OpenSpan.builder() .type(SpanType.LOCAL) .spanId("spanId") @@ -46,26 +46,34 @@ public void testToString() { } @Test - public void noop() { - Trace traceId = Trace.create(false, "traceId"); - assertThat(traceId.getTraceId()).isEqualTo("noop"); + public void unobservedTraces() { + Trace traceId = Trace.create(false, "testTraceId"); + assertThat(traceId.getTraceId()).isEqualTo("testTraceId"); assertThat(traceId.isObservable()).isFalse(); assertThat(traceId.isEmpty()).isTrue(); assertThat(traceId.pop()).isNotPresent(); assertThat(traceId.top()).isNotPresent(); - assertThat(traceId.deepCopy()).isSameAs(traceId); + assertEquals(traceId, traceId.deepCopy()); traceId.push(OpenSpan.builder().spanId("spanId").operation("operation").type(SpanType.LOCAL).build()); - assertThat(traceId.getTraceId()).isEqualTo("noop"); + assertThat(traceId.getTraceId()).isEqualTo("testTraceId"); assertThat(traceId.isObservable()).isFalse(); assertThat(traceId.isEmpty()).isTrue(); assertThat(traceId.pop()).isNotPresent(); assertThat(traceId.top()).isNotPresent(); - assertThat(traceId.deepCopy()).isSameAs(traceId); + assertEquals(traceId, traceId.deepCopy()); - assertThat(traceId.toString()).isEqualTo("Trace{stack=[], isObservable=false, traceId='noop'}"); + assertThat(traceId.toString()).isEqualTo("Trace{stack=[], isObservable=false, traceId='testTraceId'}"); - assertThat(traceId).isSameAs(Trace.create(false, "traceId2")); + assertThat(traceId).isNotEqualTo(Trace.create(false, "traceId2")); + } + + private static void assertEquals(Trace one, Trace two) { + assertThat(one.getTraceId()).isEqualTo(two.getTraceId()); + assertThat(one.isObservable()).isEqualTo(two.isObservable()); + assertThat(one.isEmpty()).isEqualTo(two.isEmpty()); + assertThat(one.top()).isEqualTo(two.top()); + assertThat(one.toString()).isEqualTo(two.toString()); } } diff --git a/tracing/src/test/java/com/palantir/tracing/TracerTest.java b/tracing/src/test/java/com/palantir/tracing/TracerTest.java index 3e9f9d78e..cb6d6b717 100644 --- a/tracing/src/test/java/com/palantir/tracing/TracerTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TracerTest.java @@ -145,7 +145,7 @@ public void testObserversAreInvokedOnObservableTracesOnly() throws Exception { } @Test - public void testDerivesNewSpansWhenTraceIsNotObservable() throws Exception { + public void testIgnoresNewSpansWhenTraceIsNotObservable() throws Exception { Tracer.initTrace(Observability.DO_NOT_SAMPLE, Tracers.randomId()); assertThat(Tracer.isTraceObservable()).isFalse(); Tracer.startSpan("foo"); @@ -188,7 +188,7 @@ public void testTraceCopyIsIndependent() throws Exception { @Test public void testSetTraceSetsCurrentTraceAndMdcTraceIdKey() throws Exception { Tracer.startSpan("operation"); - Tracer.setTrace(new Trace(true, "newTraceId")); + Tracer.setTrace(Trace.create(true, "newTraceId")); assertThat(Tracer.getTraceId()).isEqualTo("newTraceId"); assertThat(MDC.get(Tracers.TRACE_ID_KEY)).isEqualTo("newTraceId"); assertThat(Tracer.completeSpan()).isEmpty(); @@ -197,7 +197,7 @@ public void testSetTraceSetsCurrentTraceAndMdcTraceIdKey() throws Exception { @Test public void testSetTraceSetsMdcTraceSampledKeyWhenObserved() { - Tracer.setTrace(new Trace(true, "observedTraceId")); + Tracer.setTrace(Trace.create(true, "observedTraceId")); assertThat(MDC.get(Tracers.TRACE_SAMPLED_KEY)).isEqualTo("1"); assertThat(Tracer.completeSpan()).isEmpty(); assertThat(MDC.get(Tracers.TRACE_SAMPLED_KEY)).isNull(); @@ -205,7 +205,7 @@ public void testSetTraceSetsMdcTraceSampledKeyWhenObserved() { @Test public void testSetTraceMissingMdcTraceSampledKeyWhenNotObserved() { - Tracer.setTrace(new Trace(false, "notObservedTraceId")); + Tracer.setTrace(Trace.create(false, "notObservedTraceId")); assertThat(MDC.get(Tracers.TRACE_SAMPLED_KEY)).isNull(); assertThat(Tracer.completeSpan()).isEmpty(); assertThat(MDC.get(Tracers.TRACE_SAMPLED_KEY)).isNull(); @@ -280,7 +280,7 @@ public void testObserversThrow() { @Test public void testGetAndClearTraceIfPresent() { - Trace trace = new Trace(true, "newTraceId"); + Trace trace = Trace.create(true, "newTraceId"); Tracer.setTrace(trace); Optional nonEmptyTrace = Tracer.getAndClearTraceIfPresent(); diff --git a/tracing/src/test/java/com/palantir/tracing/TracersTest.java b/tracing/src/test/java/com/palantir/tracing/TracersTest.java index ee26cceca..b46a5aed3 100644 --- a/tracing/src/test/java/com/palantir/tracing/TracersTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TracersTest.java @@ -566,22 +566,6 @@ public void testTraceIdGeneration() throws Exception { assertThat(Tracers.longToPaddedHex(123456789L)).isEqualTo("00000000075bcd15"); } - @Test - public void testLazyIdGeneration() { - CharSequence lazyRandomId = Tracers.lazyRandomId(); - assertThat(lazyRandomId) - .isNotInstanceOf(String.class) - .isEqualTo(lazyRandomId) - .hasSize(16); - assertThat(lazyRandomId.length()).isEqualTo(16); - assertThat(lazyRandomId.toString()).isSameAs(lazyRandomId.toString()); - assertThat(Tracers.lazyRandomId()).isNotEqualTo(Tracers.lazyRandomId()); - - for (int i = 0; i < 160; i++) { - assertThat(Tracers.lazyRandomId()).hasSize(16); // fails with p=1/16 if generated string is not padded - } - } - private static Callable newTraceExpectingCallable(String expectedOperation) { final Set seenTraceIds = new HashSet<>(); seenTraceIds.add(Tracer.getTraceId());