diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java index 1aac77f8c..ca6ecea8f 100644 --- a/tracing/src/main/java/com/palantir/tracing/Tracer.java +++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java @@ -17,7 +17,6 @@ package com.palantir.tracing; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableList; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.UnsafeArg; import com.palantir.tracing.api.OpenSpan; @@ -26,9 +25,9 @@ import com.palantir.tracing.api.SpanType; import java.util.Collections; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Consumer; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.slf4j.MDC; @@ -50,8 +49,9 @@ private Tracer() {} // Only access in a class-synchronized fashion private static final Map observers = new HashMap<>(); - // we want iterating through tracers to be very fast, and it's faster to iterate through a list than a Map.values() - private static volatile List observersList = ImmutableList.of(); + // we want iterating through tracers to be very fast, and it's faster to pre-define observer execution + // when our observers are modified. + private static volatile Consumer compositeObserver = span -> { }; // Thread-safe since stateless private static volatile TraceSampler sampler = AlwaysSampler.INSTANCE; @@ -190,9 +190,7 @@ public static Optional completeSpan(Map metadata) { } private static void notifyObservers(Span span) { - for (SpanObserver observer : observersList) { - observer.consume(span); - } + compositeObserver.accept(span); } private static Optional popCurrentSpan() { @@ -252,7 +250,23 @@ public static synchronized SpanObserver unsubscribe(String name) { } private static void computeObserversList() { - observersList = ImmutableList.copyOf(observers.values()); + Consumer newCompositeObserver = span -> { }; + for (Map.Entry entry : observers.entrySet()) { + String observerName = entry.getKey(); + SpanObserver spanObserver = entry.getValue(); + newCompositeObserver = newCompositeObserver.andThen(span -> { + try { + spanObserver.consume(span); + } catch (RuntimeException e) { + log.error("Failed to invoke observer {} registered as {}", + SafeArg.of("observer", spanObserver), + SafeArg.of("name", observerName), + e); + } + }); + } + // Single volatile write, updating observers should not disrupt tracing + compositeObserver = newCompositeObserver; } /** Sets the sampler (for all threads). */ diff --git a/tracing/src/test/java/com/palantir/tracing/TracerTest.java b/tracing/src/test/java/com/palantir/tracing/TracerTest.java index 4e28f9da2..c4fc2ac25 100644 --- a/tracing/src/test/java/com/palantir/tracing/TracerTest.java +++ b/tracing/src/test/java/com/palantir/tracing/TracerTest.java @@ -63,6 +63,7 @@ public void before() { public void after() { Tracer.initTrace(Optional.of(true), Tracers.randomId()); Tracer.setSampler(AlwaysSampler.INSTANCE); + Tracer.unsubscribe("0"); Tracer.unsubscribe("1"); Tracer.unsubscribe("2"); Tracer.getAndClearTrace(); @@ -251,6 +252,23 @@ public void testFastCompleteSpanWithMetadata() { assertThat(spanCaptor.getValue().getMetadata()).isEqualTo(metadata); } + @Test + public void testObserversThrow() { + Tracer.subscribe("0", span -> { + throw new IllegalStateException("0"); + }); + Tracer.subscribe("1", observer1); + Tracer.subscribe("2", span -> { + throw new IllegalStateException("2"); + }); + String operation = "operation"; + Tracer.startSpan(operation); + Tracer.fastCompleteSpan(); + verify(observer1).consume(spanCaptor.capture()); + assertThat(spanCaptor.getValue().getOperation()).isEqualTo(operation); + } + + @Test public void testGetAndClearTraceIfPresent() { Trace trace = new Trace(true, "newTraceId");