Use ThreadLocalActiveSpanSource instead of DDActiveSpan
This resolves several bugs and allows us to delete unecessary code.
MODIFY
dd-trace/dd-trace.gradle
dd-trace/dd-trace.gradle
@@ -12,6 +12,7 @@ minimumBranchCoverage = 0.3
minimumInstructionCoverage = 0.5
whitelistedInstructionClasses += whitelistedBranchClasses += [
"com.datadoghq.trace.integration.*",
+ 'com.datadoghq.trace.writer.ListWriter',
'com.datadoghq.trace.DDTags'
]

MODIFY
dd-trace/docs/opentracing-api.md
dd-trace/docs/opentracing-api.md
@@ -79,7 +79,7 @@ If child spans are unfinished when the parent attempts to finish, the span will
Now, you are able to create, start and stop very simple spans.
-You can manipulate them at any time and add contextual information using tags.
+You can manipulate them at any time and add extra contextual information using the tags.
### Tags

DELETE
dd-trace/src/main/java/com/datadoghq/trace/DDActiveSpan.java
-package com.datadoghq.trace;
-
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import io.opentracing.ActiveSpan;
-
-/** Base implementation for opentracing {@link ActiveSpan} */
-public class DDActiveSpan extends DDBaseSpan<ActiveSpan> implements ActiveSpan {
-
- protected final DDActiveSpan parent;
- protected boolean deactivated = false;
-
- protected DDActiveSpan(DDActiveSpan parent, DDSpan span) {
- super(span.startTimeMicro, span.context());
- this.startTimeNano = span.startTimeNano;
- this.durationNano = span.durationNano;
- this.parent = parent;
- }
-
- protected DDActiveSpan(DDActiveSpan parent, long timestampMicro, DDSpanContext context) {
- super(timestampMicro, context);
- this.parent = parent;
- }
-
- /** @return the generating parent if not null */
- @JsonIgnore
- public DDActiveSpan getParent() {
- return parent;
- }
-
- /** @return true if the span has already been deactivated */
- @JsonIgnore
- public boolean isDeactivated() {
- return deactivated;
- }
-
- @Override
- public void deactivate() {
- DDTracer tracer = context().getTracer();
- if (tracer != null) {
- tracer.deactivate(this);
- }
- finish();
- deactivated = true;
- }
-
- @Override
- public Continuation capture() {
- return new DDContinuation();
- }
-
- @Override
- protected ActiveSpan thisInstance() {
- return this;
- }
-
- public class DDContinuation implements Continuation {
-
- @Override
- public ActiveSpan activate() {
- //Reactivate the current span
- context().getTracer().makeActive(DDActiveSpan.this);
-
- //And return the encapsulating ActiveSpan
- return DDActiveSpan.this;
- }
- }
-
- @Override
- public void close() {
- deactivate();
- }
-}

MODIFY
dd-trace/src/main/java/com/datadoghq/trace/DDTracer.java
dd-trace/src/main/java/com/datadoghq/trace/DDTracer.java
@@ -7,16 +7,18 @@ import com.datadoghq.trace.sampling.AllSampler;
import com.datadoghq.trace.sampling.Sampler;
import com.datadoghq.trace.writer.LoggingWriter;
import com.datadoghq.trace.writer.Writer;
+import io.opentracing.ActiveSpan;
+import io.opentracing.ActiveSpanSource;
import io.opentracing.BaseSpan;
-import io.opentracing.Span;
import io.opentracing.SpanContext;
import io.opentracing.propagation.Format;
+import io.opentracing.util.ThreadLocalActiveSpanSource;
import java.util.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/** DDTracer makes it easy to send traces and span to DD using the OpenTracing integration. */
-public class DDTracer implements io.opentracing.Tracer {
+public class DDTracer extends ThreadLocalActiveSpanSource implements io.opentracing.Tracer {
public static final String JAVA_VERSION = System.getProperty("java.version", "unknown");
public static final String CURRENT_VERSION;
@@ -93,7 +95,7 @@ public class DDTracer implements io.opentracing.Tracer {
}
public DDSpanBuilder buildSpan(String operationName) {
- return new DDSpanBuilder(operationName);
+ return new DDSpanBuilder(operationName, this);
}
public <T> void inject(SpanContext spanContext, Format<T> format, T carrier) {
@@ -136,53 +138,9 @@ public class DDTracer implements io.opentracing.Tracer {
writer.close();
}
- private final ThreadLocal<DDActiveSpan> currentActiveSpan = new ThreadLocal<DDActiveSpan>();
-
- @Override
- public DDActiveSpan activeSpan() {
- return currentActiveSpan.get();
- }
-
- /**
- * Set the newly created active span as the active one from the Tracer's perspective
- *
- * @param activeSpan
- */
- protected void makeActive(DDActiveSpan activeSpan) {
- //We cannot make active a preably deactivated span
- if (activeSpan != null && activeSpan.isDeactivated()) currentActiveSpan.set(null);
- else currentActiveSpan.set(activeSpan);
- }
-
- /**
- * Deactivate the current span (if active) and make the parent active (again)
- *
- * @param activeSpan
- */
- protected void deactivate(DDActiveSpan activeSpan) {
- DDActiveSpan current = activeSpan();
- if (current == activeSpan) {
- //The parent becomes the active span
- makeActive(activeSpan.getParent());
- }
- }
-
- @Override
- public DDActiveSpan makeActive(Span span) {
- if (!(span instanceof DDSpan))
- throw new IllegalArgumentException(
- "Cannot transform a non DDSpan into a DDActiveSpan. Provided class: " + span.getClass());
-
- //Wrap the provided manual span into an active one with the current parent
- DDActiveSpan activeSpan = new DDActiveSpan(activeSpan(), (DDSpan) span);
-
- makeActive(activeSpan);
-
- return activeSpan;
- }
-
/** Spans are built using this builder */
public class DDSpanBuilder implements SpanBuilder {
+ private final ActiveSpanSource spanSource;
/** Each span must have an operationName according to the opentracing specification */
private String operationName;
@@ -203,32 +161,20 @@ public class DDTracer implements io.opentracing.Tracer {
return this;
}
- @Override
- public DDActiveSpan startActive() {
- //Set the active span as parent if ignoreActiveSpan==true
- DDActiveSpan activeParent = null;
- if (!ignoreActiveSpan) {
- DDActiveSpan current = activeSpan();
- if (current != null) {
- activeParent = current;
-
- //Ensure parent inheritance
- asChildOf(activeParent);
- }
+ private DDSpan startSpan() {
+ return new DDSpan(this.timestamp, buildSpanContext());
}
- //Create the active span
- DDActiveSpan activeSpan = new DDActiveSpan(activeParent, this.timestamp, buildSpanContext());
+ @Override
+ public ActiveSpan startActive() {
+ ActiveSpan activeSpan = spanSource.makeActive(startSpan());
logger.debug("{} - Starting a new active span.", activeSpan);
-
- makeActive(activeSpan);
-
return activeSpan;
}
@Override
public DDSpan startManual() {
- DDSpan span = new DDSpan(this.timestamp, buildSpanContext());
+ DDSpan span = startSpan();
logger.debug("{} - Starting a new manuel span.", span);
return span;
}
@@ -240,12 +186,12 @@ public class DDTracer implements io.opentracing.Tracer {
}
@Override
- public DDTracer.DDSpanBuilder withTag(String tag, Number number) {
+ public DDSpanBuilder withTag(String tag, Number number) {
return withTag(tag, (Object) number);
}
@Override
- public DDTracer.DDSpanBuilder withTag(String tag, String string) {
+ public DDSpanBuilder withTag(String tag, String string) {
if (tag.equals(DDTags.SERVICE_NAME)) {
return withServiceName(string);
} else if (tag.equals(DDTags.RESOURCE_NAME)) {
@@ -258,36 +204,37 @@ public class DDTracer implements io.opentracing.Tracer {
}
@Override
- public DDTracer.DDSpanBuilder withTag(String tag, boolean bool) {
+ public DDSpanBuilder withTag(String tag, boolean bool) {
return withTag(tag, (Object) bool);
}
- public DDSpanBuilder(String operationName) {
+ public DDSpanBuilder(String operationName, ActiveSpanSource spanSource) {
this.operationName = operationName;
+ this.spanSource = spanSource;
}
@Override
- public DDTracer.DDSpanBuilder withStartTimestamp(long timestampMillis) {
+ public DDSpanBuilder withStartTimestamp(long timestampMillis) {
this.timestamp = timestampMillis;
return this;
}
- public DDTracer.DDSpanBuilder withServiceName(String serviceName) {
+ public DDSpanBuilder withServiceName(String serviceName) {
this.serviceName = serviceName;
return this;
}
- public DDTracer.DDSpanBuilder withResourceName(String resourceName) {
+ public DDSpanBuilder withResourceName(String resourceName) {
this.resourceName = resourceName;
return this;
}
- public DDTracer.DDSpanBuilder withErrorFlag() {
+ public DDSpanBuilder withErrorFlag() {
this.errorFlag = true;
return this;
}
- public DDTracer.DDSpanBuilder withSpanType(String spanType) {
+ public DDSpanBuilder withSpanType(String spanType) {
this.spanType = spanType;
return this;
}
@@ -300,24 +247,24 @@ public class DDTracer implements io.opentracing.Tracer {
}
@Override
- public DDTracer.DDSpanBuilder asChildOf(BaseSpan<?> span) {
+ public DDSpanBuilder asChildOf(BaseSpan<?> span) {
return asChildOf(span == null ? null : span.context());
}
@Override
- public DDTracer.DDSpanBuilder asChildOf(SpanContext spanContext) {
+ public DDSpanBuilder asChildOf(SpanContext spanContext) {
this.parent = spanContext;
return this;
}
@Override
- public DDTracer.DDSpanBuilder addReference(String referenceType, SpanContext spanContext) {
+ public DDSpanBuilder addReference(String referenceType, SpanContext spanContext) {
logger.debug("`addReference` method is not implemented. Doing nothing");
return this;
}
// Private methods
- private DDTracer.DDSpanBuilder withTag(String tag, Object value) {
+ private DDSpanBuilder withTag(String tag, Object value) {
if (this.tags.isEmpty()) {
this.tags = new HashMap<String, Object>();
}
@@ -336,23 +283,39 @@ public class DDTracer implements io.opentracing.Tracer {
* @return the context
*/
private DDSpanContext buildSpanContext() {
- long generatedId = generateNewId();
- DDSpanContext context;
- DDSpanContext p = this.parent != null ? (DDSpanContext) this.parent : null;
+ final long traceId;
+ final long spanId = generateNewId();
+ final long parentSpanId;
+ final Map<String, String> baggage;
+ final List<DDBaseSpan<?>> parentTrace;
- String spanType = this.spanType;
- if (spanType == null && this.parent != null) {
- spanType = p.getSpanType();
+ DDSpanContext context;
+ SpanContext parentContext = this.parent;
+ if (parentContext == null && !ignoreActiveSpan) {
+ // use the ActiveSpan as parent unless overridden or ignored.
+ ActiveSpan activeSpan = activeSpan();
+ if (activeSpan != null) parentContext = activeSpan.context();
+ }
+
+ if (parentContext instanceof DDSpanContext) {
+ DDSpanContext ddsc = (DDSpanContext) parentContext;
+ traceId = ddsc.getTraceId();
+ parentSpanId = ddsc.getSpanId();
+ baggage = ddsc.getBaggageItems();
+ parentTrace = ddsc.getTrace();
+
+ if (this.serviceName == null) this.serviceName = ddsc.getServiceName();
+ if (this.spanType == null) this.spanType = ddsc.getSpanType();
+ } else {
+ traceId = spanId;
+ parentSpanId = 0L;
+ baggage = null;
+ parentTrace = null;
}
- String serviceName = this.serviceName;
if (serviceName == null) {
- if (p != null && p.getServiceName() != null) {
- serviceName = p.getServiceName();
- } else {
serviceName = defaultServiceName;
}
- }
String operationName = this.operationName != null ? this.operationName : this.resourceName;
@@ -361,17 +324,17 @@ public class DDTracer implements io.opentracing.Tracer {
// some attributes are inherited from the parent
context =
new DDSpanContext(
- this.parent == null ? generatedId : p.getTraceId(),
- generatedId,
- this.parent == null ? 0L : p.getSpanId(),
+ traceId,
+ spanId,
+ parentSpanId,
serviceName,
operationName,
this.resourceName,
- this.parent == null ? null : p.getBaggageItems(),
+ baggage,
errorFlag,
spanType,
this.tags,
- this.parent == null ? null : p.getTrace(),
+ parentTrace,
DDTracer.this);
// Force the lang meta

MODIFY
dd-trace/src/main/java/com/datadoghq/trace/writer/ListWriter.java
dd-trace/src/main/java/com/datadoghq/trace/writer/ListWriter.java
@@ -5,30 +5,28 @@ import java.util.ArrayList;
import java.util.List;
/** List writer used by tests mostly */
-public class ListWriter implements Writer {
-
- protected List<List<DDBaseSpan<?>>> list = new ArrayList<List<DDBaseSpan<?>>>();
+public class ListWriter extends ArrayList<List<DDBaseSpan<?>>> implements Writer {
public List<List<DDBaseSpan<?>>> getList() {
- return list;
+ return this;
}
public List<DDBaseSpan<?>> firstTrace() {
- return list.get(0);
+ return get(0);
}
@Override
public void write(List<DDBaseSpan<?>> trace) {
- list.add(trace);
+ add(trace);
}
@Override
public void start() {
- list.clear();
+ clear();
}
@Override
public void close() {
- list.clear();
+ clear();
}
}

ADD
dd-trace/src/test/groovy/com/datadoghq/trace/ActiveSpanContinuationTest.groovy
+package com.datadoghq.trace
+
+import com.datadoghq.trace.writer.ListWriter
+import spock.lang.Ignore
+import spock.lang.Specification
+
+import java.util.concurrent.Phaser
+
+class ActiveSpanContinuationTest extends Specification {
+
+ def traceCollector = new ListWriter()
+ def tracer = new DDTracer(traceCollector)
+ def activeSpan = tracer.buildSpan("test").startActive()
+
+ def "calling activate from multiple continuations at once with no child spans tracks separately"() {
+ setup:
+ def phaser = new Phaser()
+ phaser.register()
+
+ for (int i = 0; i < count; i++) {
+ phaser.register()
+ def capture = activeSpan.capture()
+ new Thread({
+ phaser.arriveAndAwaitAdvance()
+ def activeSpan = capture.activate()
+ phaser.arriveAndAwaitAdvance()
+ activeSpan.deactivate()
+ phaser.arriveAndDeregister()
+ }).start()
+ }
+
+ activeSpan.deactivate() // allow the trace to be reported when all continuations deactivate
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to activate capture
+
+ then:
+ traceCollector == []
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to deactivate their span
+ phaser.arriveAndAwaitAdvance() // wait till all threads have deactivated
+
+ then:
+ traceCollector.size() == 1
+ traceCollector.firstTrace().size() == 1
+
+ where:
+ count = 5
+ }
+
+ def "concurrent threads with manual spans and continuations report correctly"() {
+ setup:
+ def phaser = new Phaser()
+ phaser.register()
+
+ for (int i = 0; i < count; i++) {
+ phaser.register()
+ def capture = activeSpan.capture()
+ new Thread({
+ phaser.arriveAndAwaitAdvance()
+ def activeSpan = capture.activate()
+ def childSpan = tracer.buildSpan("child " + i).startManual()
+ phaser.arriveAndAwaitAdvance()
+ childSpan.finish()
+ activeSpan.deactivate()
+ phaser.arriveAndDeregister()
+ }).start()
+ }
+
+ activeSpan.deactivate() // allow the trace to be reported when all continuations deactivate
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to activate capture
+
+ then:
+ traceCollector == []
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to deactivate their span
+ phaser.arriveAndAwaitAdvance() // wait till all threads have deactivated
+
+ traceCollector.size()
+
+ then:
+ traceCollector.size() == 1
+ def trace = traceCollector.remove(0)
+ def parent = trace.remove(0)
+
+ trace.size() == count
+ parent.context.parentId == 0
+
+ trace.every {
+ it.context.parentId == parent.context.spanId
+ }
+
+ where:
+ count = 3
+ }
+
+ def "concurrent threads with active spans and continuations report correctly"() {
+ setup:
+ def phaser = new Phaser()
+ phaser.register()
+
+ for (int i = 0; i < count; i++) {
+ String spanName = "child " + i
+ phaser.register()
+ def capture = activeSpan.capture()
+ new Thread({
+ phaser.arriveAndAwaitAdvance()
+ def activeSpan = capture.activate()
+ def childSpan = tracer.buildSpan(spanName).startActive()
+ phaser.arriveAndAwaitAdvance()
+ childSpan.deactivate()
+ activeSpan.deactivate()
+ phaser.arriveAndDeregister()
+ }).start()
+ }
+
+ activeSpan.deactivate() // allow the trace to be reported when all continuations deactivate
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to activate capture
+
+ then:
+ traceCollector == []
+
+ when:
+ phaser.arriveAndAwaitAdvance() //allow threads to deactivate their span
+ phaser.arriveAndAwaitAdvance() // wait till all threads have deactivated
+
+ traceCollector.size()
+
+ then:
+ traceCollector.size() == 1
+ def trace = traceCollector.remove(0)
+ def parent = trace.remove(0)
+
+ trace.size() == count
+ parent.context.parentId == 0
+
+ trace.every {
+ it.context.parentId == parent.context.spanId
+ }
+
+ where:
+ count = 3
+ }
+
+ @Ignore("Not yet implemented in ThreadLocalActiveSpan.Continuation")
+ def "calling activate more than once results in an error"() {
+ setup:
+ def capture = activeSpan.capture()
+
+ when:
+ activeSpan.deactivate()
+
+ then:
+ traceCollector == []
+
+ when:
+ capture.activate().deactivate()
+ // parent span should be finished at this point.
+ then:
+ traceCollector == []
+
+ when:
+ capture.activate().deactivate()
+
+ then:
+ thrown(RuntimeException)
+ }
+}

DELETE
dd-trace/src/test/java/com/datadoghq/trace/DDActiveSpanTest.java
-package com.datadoghq.trace;
-
-import static org.assertj.core.api.Assertions.assertThat;
-
-import com.datadoghq.trace.writer.ListWriter;
-import io.opentracing.ActiveSpan.Continuation;
-import org.junit.Before;
-import org.junit.Test;
-
-public class DDActiveSpanTest {
-
- private ListWriter listWriter = new ListWriter();
- private DDTracer ddTracer = new DDTracer(listWriter);
-
- @Before
- public void setUp() {
- listWriter.start();
- }
-
- @Test
- public void testThreadContextPropagation() {
-
- DDActiveSpan span1 = ddTracer.buildSpan("op1").startActive();
-
- assertThat(span1.getOperationName()).isEqualTo("op1");
- assertThat(ddTracer.activeSpan()).isEqualTo(span1);
-
- DDActiveSpan span2 = ddTracer.buildSpan("op2").startActive();
- assertThat(span2.getOperationName()).isEqualTo("op2");
- assertThat(span2.getParent()).isEqualTo(span1);
- assertThat(span2.context().getParentId()).isEqualTo(span1.getSpanId());
- assertThat(span2.context().getTraceId()).isEqualTo(span1.getTraceId());
- assertThat(ddTracer.activeSpan()).isEqualTo(span2);
-
- span2.deactivate();
- assertThat(ddTracer.activeSpan()).isEqualTo(span1);
-
- assertThat(listWriter.getList().size()).isEqualTo(0);
- span1.deactivate();
- assertThat(ddTracer.activeSpan()).isNull();
-
- assertThat(listWriter.getList().size()).isEqualTo(1);
- assertThat(listWriter.getList().get(0).size()).isEqualTo(2);
- assertThat(listWriter.getList().get(0).get(0)).isEqualTo(span1);
- assertThat(listWriter.getList().get(0).get(1)).isEqualTo(span2);
- }
-
- @Test
- public void testParentDeactivationFirst() {
-
- DDActiveSpan span1 = ddTracer.buildSpan("op1").startActive();
-
- assertThat(span1.getOperationName()).isEqualTo("op1");
-
- DDActiveSpan span2 = ddTracer.buildSpan("op2").startActive();
-
- assertThat(ddTracer.activeSpan()).isEqualTo(span2);
-
- span1.deactivate();
-
- //If parent span deactivated first => Span 2 remains the active span
- assertThat(ddTracer.activeSpan()).isEqualTo(span2);
-
- span2.deactivate();
-
- //If span2 comes to be deactivated
- assertThat(ddTracer.activeSpan()).isNull();
-
- assertThat(listWriter.getList().size()).isEqualTo(1);
- assertThat(listWriter.getList().get(0).size()).isEqualTo(2);
- assertThat(listWriter.getList().get(0).get(0)).isEqualTo(span1);
- assertThat(listWriter.getList().get(0).get(1)).isEqualTo(span2);
- }
-
- @Test
- public void testContinuation() throws Exception {
- final DDActiveSpan span1 = ddTracer.buildSpan("op1").startActive();
-
- final Continuation continuation = span1.capture();
-
- Thread t =
- new Thread(
- new Runnable() {
- @Override
- public void run() {
- assertThat(ddTracer.activeSpan()).isNull();
- continuation.activate();
- assertThat(ddTracer.activeSpan()).isEqualTo(span1);
- DDActiveSpan span2 = ddTracer.buildSpan("op2").startActive();
- assertThat(ddTracer.activeSpan()).isEqualTo(span2);
- span2.deactivate();
- assertThat(ddTracer.activeSpan()).isEqualTo(span1);
- }
- });
-
- t.start();
- t.join();
-
- span1.deactivate();
- assertThat(ddTracer.activeSpan()).isNull();
-
- assertThat(listWriter.getList().size()).isEqualTo(1);
- assertThat(listWriter.getList().get(0).size()).isEqualTo(2);
- assertThat(listWriter.getList().get(0).get(0)).isEqualTo(span1);
- }
-
- @Test
- public void testMakeSpanActive() {
- final DDSpan manualSpan = ddTracer.buildSpan("op1").startManual();
-
- DDActiveSpan span1 = ddTracer.makeActive(manualSpan);
-
- assertThat(span1.getOperationName()).isEqualTo("op1");
- assertThat(ddTracer.activeSpan()).isEqualTo(span1);
- assertThat(manualSpan.context()).isEqualTo(span1.context());
- assertThat(manualSpan.startTimeNano).isEqualTo(span1.startTimeNano);
- assertThat(manualSpan.startTimeMicro).isEqualTo(span1.startTimeMicro);
- }
-
- @Test
- public void testCrossContextPropagation() {
- final DDSpan manualSpan = ddTracer.buildSpan("op1").startManual();
-
- DDActiveSpan activeSpan =
- ddTracer.buildSpan("op2").asChildOf(manualSpan.context()).startActive();
-
- assertThat(activeSpan.getParentId()).isEqualTo(manualSpan.getSpanId());
- assertThat(activeSpan.getTraceId()).isEqualTo(manualSpan.getTraceId());
- }
-}

MODIFY
dd-trace/src/test/java/com/datadoghq/trace/DDSpanSerializationTest.java
dd-trace/src/test/java/com/datadoghq/trace/DDSpanSerializationTest.java
@@ -13,7 +13,6 @@ public class DDSpanSerializationTest {
ObjectMapper serializer;
DDSpan span;
- DDActiveSpan activeSpan;
Map<String, Object> expected = Maps.newHashMap();
@Before
@@ -56,9 +55,6 @@ public class DDSpanSerializationTest {
span = new DDSpan(100L, context);
span.finish(133L);
-
- activeSpan = new DDActiveSpan(null, 100L, context);
- activeSpan.deactivate();
serializer = new ObjectMapper();
}

Use ThreadLocalActiveSpanSource instead of DDActiveSpan

This resolves several bugs and allows us to delete unecessary code.