Skip to content

Commit

Permalink
[fix] Open a new span when wrapping with a new trace (#49)
Browse files Browse the repository at this point in the history
## Before this PR
Creating a new trace does not create a new root span. Thus all top-level calls in this trace will have no parent span. This breaks assumptions of trace log consumers who assume that there is only one root span for a given trace.

This also seems counter to the `parentId` specification in the [Zipkin API](https://github.com/openzipkin/zipkin-api/blob/5fd719b114ff2ba44f1d2757888fa123cdfb0da6/zipkin-api.yaml#L353):
> The parent span ID or absent if this the root span in a trace.

## After this PR
Creating a new trace will create a new root span.

The issue is part of the `Major API revision` milestone, but this fix does not break the API. Is a major API revision required for this change?
  • Loading branch information
pkoenig10 authored and bulldozer-bot[bot] committed Jan 7, 2019
1 parent 5c74cba commit 37719b4
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 0 deletions.
7 changes: 7 additions & 0 deletions tracing/src/main/java/com/palantir/tracing/Tracers.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
public final class Tracers {
/** The key under which trace ids are inserted into SLF4J {@link org.slf4j.MDC MDCs}. */
public static final String TRACE_ID_KEY = "traceId";
private static final String ROOT_SPAN_OPERATION = "root";
private static final char[] HEX_DIGITS =
{'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'};

Expand Down Expand Up @@ -148,8 +149,10 @@ public static <V> Callable<V> wrapWithNewTrace(Callable<V> delegate) {

try {
Tracer.initTrace(Optional.empty(), Tracers.randomId());
Tracer.startSpan(ROOT_SPAN_OPERATION);
return delegate.call();
} finally {
Tracer.fastCompleteSpan();
// restore the trace
Tracer.setTrace(originalTrace);
}
Expand All @@ -166,8 +169,10 @@ public static Runnable wrapWithNewTrace(Runnable delegate) {

try {
Tracer.initTrace(Optional.empty(), Tracers.randomId());
Tracer.startSpan(ROOT_SPAN_OPERATION);
delegate.run();
} finally {
Tracer.fastCompleteSpan();
// restore the trace
Tracer.setTrace(originalTrace);
}
Expand All @@ -187,8 +192,10 @@ public static Runnable wrapWithAlternateTraceId(String traceId, Runnable delegat

try {
Tracer.initTrace(Optional.empty(), traceId);
Tracer.startSpan(ROOT_SPAN_OPERATION);
delegate.run();
} finally {
Tracer.fastCompleteSpan();
// restore the trace
Tracer.setTrace(originalTrace);
}
Expand Down
55 changes: 55 additions & 0 deletions tracing/src/test/java/com/palantir/tracing/TracersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,22 @@ public void testWrapCallableWithNewTrace_traceStateInsideCallableIsIsolated() th
.isEqualTo(traceIdAfterCalls);
}

@Test
public void testWrapCallableWithNewTrace_traceStateInsideCallableHasSpan() throws Exception {
Callable<List<OpenSpan>> wrappedCallable = Tracers.wrapWithNewTrace(() -> {
return getCurrentFullTrace();
});

List<OpenSpan> spans = wrappedCallable.call();

assertThat(spans).hasSize(1);

OpenSpan span = spans.get(0);

assertThat(span.getOperation()).isEqualTo("root");
assertThat(span.getParentSpanId()).isEmpty();
}

@Test
public void testWrapCallableWithNewTrace_traceStateRestoredWhenThrows() throws Exception {
String traceIdBeforeConstruction = Tracer.getTraceId();
Expand Down Expand Up @@ -258,6 +274,24 @@ public void testWrapRunnableWithNewTrace_traceStateInsideRunnableIsIsolated() th
.isEqualTo(traceIdAfterCalls);
}

@Test
public void testWrapRunnableWithNewTrace_traceStateInsideRunnableHasSpan() throws Exception {
List<List<OpenSpan>> spans = Lists.newArrayList();

Runnable wrappedRunnable = Tracers.wrapWithNewTrace(() -> {
spans.add(getCurrentFullTrace());
});

wrappedRunnable.run();

assertThat(spans.get(0)).hasSize(1);

OpenSpan span = spans.get(0).get(0);

assertThat(span.getOperation()).isEqualTo("root");
assertThat(span.getParentSpanId()).isEmpty();
}

@Test
public void testWrapRunnableWithNewTrace_traceStateRestoredWhenThrows() throws Exception {
String traceIdBeforeConstruction = Tracer.getTraceId();
Expand Down Expand Up @@ -293,6 +327,25 @@ public void testWrapRunnableWithAlternateTraceId_traceStateInsideRunnableUsesGiv
assertThat(traceIdBeforeConstruction).isEqualTo(traceIdAfterCall);
}

@Test
public void testWrapRunnableWithAlternateTraceId_traceStateInsideRunnableHasSpan() throws Exception {
List<List<OpenSpan>> spans = Lists.newArrayList();

String traceIdToUse = "someTraceId";
Runnable wrappedRunnable = Tracers.wrapWithAlternateTraceId(traceIdToUse, () -> {
spans.add(getCurrentFullTrace());
});

wrappedRunnable.run();

assertThat(spans.get(0)).hasSize(1);

OpenSpan span = spans.get(0).get(0);

assertThat(span.getOperation()).isEqualTo("root");
assertThat(span.getParentSpanId()).isEmpty();
}

@Test
public void testWrapRunnableWithAlternateTraceId_traceStateRestoredWhenThrows() {
String traceIdBeforeConstruction = Tracer.getTraceId();
Expand Down Expand Up @@ -325,6 +378,7 @@ public Void call() throws Exception {

assertThat(MDC.get(Tracers.TRACE_ID_KEY)).isEqualTo(newTraceId);
assertThat(seenTraceIds).doesNotContain(newTraceId);
assertThat(getCurrentFullTrace()).hasSize(1);
seenTraceIds.add(newTraceId);
return null;
}
Expand All @@ -342,6 +396,7 @@ public void run() {

assertThat(MDC.get(Tracers.TRACE_ID_KEY)).isEqualTo(newTraceId);
assertThat(seenTraceIds).doesNotContain(newTraceId);
assertThat(getCurrentFullTrace()).hasSize(1);
seenTraceIds.add(newTraceId);
}
};
Expand Down

0 comments on commit 37719b4

Please sign in to comment.