Skip to content

fix #66: SpanObserver doesn't prevent execution on failure #67

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 7, 2019
Merged

fix #66: SpanObserver doesn't prevent execution on failure #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions tracing/src/main/java/com/palantir/tracing/Tracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -50,8 +49,9 @@ private Tracer() {}

// Only access in a class-synchronized fashion
private static final Map<String, SpanObserver> 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<SpanObserver> 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<Span> compositeObserver = span -> { };

// Thread-safe since stateless
private static volatile TraceSampler sampler = AlwaysSampler.INSTANCE;
Expand Down Expand Up @@ -190,9 +190,7 @@ public static Optional<Span> completeSpan(Map<String, String> metadata) {
}

private static void notifyObservers(Span span) {
for (SpanObserver observer : observersList) {
observer.consume(span);
}
compositeObserver.accept(span);
}

private static Optional<OpenSpan> popCurrentSpan() {
Expand Down Expand Up @@ -252,7 +250,23 @@ public static synchronized SpanObserver unsubscribe(String name) {
}

private static void computeObserversList() {
observersList = ImmutableList.copyOf(observers.values());
Consumer<Span> newCompositeObserver = span -> { };
Copy link
Contributor

@iamdanfox iamdanfox Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're micro-optimising, why do we tolerate calling the empty observer first and calling andThen on it?

Copy link
Contributor Author

@carterkozak carterkozak Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just a method invocation which is free compared to anything that does allocation (list iterator, for example).

Copy link
Contributor Author

@carterkozak carterkozak Feb 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can get rid of it, but I don't think it's worth the complexity since we won't be able to measure a difference.

for (Map.Entry<String, SpanObserver> 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). */
Expand Down
18 changes: 18 additions & 0 deletions tracing/src/test/java/com/palantir/tracing/TracerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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");
Expand Down