Improve threadsafety and organization
MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java
dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java
@@ -16,6 +16,7 @@ import java.util.HashMap;
import java.util.Map;
import java.util.Queue;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
import lombok.extern.slf4j.Slf4j;
/**
@@ -40,7 +41,7 @@ public class DDSpan implements Span {
* The duration in nanoseconds computed using the startTimeMicro or startTimeNano. Span is
* considered finished when this is set.
*/
- private volatile long durationNano;
+ private final AtomicLong durationNano = new AtomicLong();
/**
* Spans should be constructed using the builder, not by calling the constructor directly.
@@ -69,12 +70,13 @@ public class DDSpan implements Span {
@Override
public final void finish() {
if (startTimeNano != 0) {
- if (durationNano != 0) {
- log.debug("Span already finished: {}", this);
- }
// no external clock was used, so we can rely on nanotime.
- this.durationNano = Math.max(1, Clock.currentNanoTicks() - startTimeNano);
+ if (this.durationNano.compareAndSet(
+ 0, Math.max(1, Clock.currentNanoTicks() - startTimeNano))) {
afterFinish();
+ } else {
+ log.debug("Span already finished: {}", this);
+ }
} else {
finish(Clock.currentMicroTime());
}
@@ -82,20 +84,20 @@ public class DDSpan implements Span {
@Override
public final void finish(final long stoptimeMicros) {
- if (durationNano != 0) {
- log.debug("Span already finished: {}", this);
- }
// Ensure that duration is at least 1. Less than 1 is possible due to our use of system clock instead of nano time.
- this.durationNano =
- Math.max(1, TimeUnit.MICROSECONDS.toNanos(stoptimeMicros - this.startTimeMicro));
+ if (this.durationNano.compareAndSet(
+ 0, Math.max(1, TimeUnit.MICROSECONDS.toNanos(stoptimeMicros - this.startTimeMicro)))) {
afterFinish();
+ } else {
+ log.debug("Span already finished: {}", this);
+ }
}
/**
* Close the span. If the current span is the parent, check if each child has also been closed If
* not, warned it
*/
- protected final void afterFinish() {
+ private void afterFinish() {
log.debug("{} - Closing the span.", this);
// warn if one of the parent's children is not finished
@@ -303,7 +305,7 @@ public class DDSpan implements Span {
@JsonGetter("duration")
public long getDurationNano() {
- return durationNano;
+ return durationNano.get();
}
@JsonGetter("service")

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java
dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java
package datadog.opentracing;
import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.google.common.collect.Maps;
import datadog.opentracing.decorators.AbstractDecorator;
import datadog.trace.api.DDTags;
import datadog.trace.common.sampling.PrioritySampling;
import io.opentracing.tag.Tags;
import java.util.Collections;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.TreeMap;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import lombok.extern.slf4j.Slf4j;
@@ -26,37 +25,42 @@ import lombok.extern.slf4j.Slf4j;
@Slf4j
public class DDSpanContext implements io.opentracing.SpanContext {
- // Opentracing attributes
+ // Shared with other span contexts
+ /** For technical reasons, the ref to the original tracer */
+ private final DDTracer tracer;
+
+ /** The collection of all span related to this one */
+ private final Queue<DDSpan> trace;
+
+ /** Baggage is associated with the whole trace and shared with other spans */
+ private final Map<String, String> baggageItems;
+
+ // Not Shared with other span contexts
private final long traceId;
private final long spanId;
private final long parentId;
- private final String threadName = Thread.currentThread().getName();
- private final long threadId = Thread.currentThread().getId();
- /** The collection of all span related to this one */
- private final Queue<DDSpan> trace;
- // DD attributes
- /** For technical reasons, the ref to the original tracer */
- private final DDTracer tracer;
+ /** Tags are associated to the current span, they will not propagate to the children span */
+ private final Map<String, Object> tags = new ConcurrentHashMap<>();
- private Map<String, String> baggageItems;
/** The service name is required, otherwise the span are dropped by the agent */
- private String serviceName;
+ private volatile String serviceName;
/** The resource associated to the service (server_web, database, etc.) */
- private String resourceName;
- /** True indicates that the span reports an error */
- private boolean errorFlag;
- /** The type of the span. If null, the Datadog Agent will report as a custom */
- private String spanType;
+ private volatile String resourceName;
/** Each span have an operation name describing the current span */
- private String operationName;
+ private volatile String operationName;
+ /** The type of the span. If null, the Datadog Agent will report as a custom */
+ private volatile String spanType;
+ /** True indicates that the span reports an error */
+ private volatile boolean errorFlag;
/** The sampling priority of the trace */
private volatile int samplingPriority = PrioritySampling.UNSET;
/** When true, the samplingPriority cannot be changed. */
private volatile boolean samplingPriorityLocked = false;
- // Others attributes
- /** Tags are associated to the current span, they will not propagate to the children span */
- private Map<String, Object> tags;
+
+ // Additional Metadata
+ private final String threadName = Thread.currentThread().getName();
+ private final long threadId = Thread.currentThread().getId();
public DDSpanContext(
final long traceId,
@@ -78,7 +82,7 @@ public class DDSpanContext implements io.opentracing.SpanContext {
this.parentId = parentId;
if (baggageItems == null) {
- this.baggageItems = Collections.emptyMap();
+ this.baggageItems = new ConcurrentHashMap<>(0);
} else {
this.baggageItems = baggageItems;
}
@@ -90,7 +94,7 @@ public class DDSpanContext implements io.opentracing.SpanContext {
this.errorFlag = errorFlag;
this.spanType = spanType;
- this.tags = tags;
+ this.tags.putAll(tags);
if (trace == null) {
// TODO: figure out better concurrency model.
@@ -132,6 +136,14 @@ public class DDSpanContext implements io.opentracing.SpanContext {
this.resourceName = resourceName;
}
+ public String getOperationName() {
+ return operationName;
+ }
+
+ public void setOperationName(final String operationName) {
+ this.operationName = operationName;
+ }
+
public boolean getErrorFlag() {
return errorFlag;
}
@@ -189,9 +201,6 @@ public class DDSpanContext implements io.opentracing.SpanContext {
}
public void setBaggageItem(final String key, final String value) {
- if (this.baggageItems.isEmpty()) {
- this.baggageItems = new HashMap<>();
- }
this.baggageItems.put(key, value);
}
@@ -244,9 +253,6 @@ public class DDSpanContext implements io.opentracing.SpanContext {
return;
}
- if (this.tags.isEmpty()) {
- this.tags = new HashMap<>();
- }
this.tags.put(tag, value);
// Call decorators
@@ -271,9 +277,6 @@ public class DDSpanContext implements io.opentracing.SpanContext {
}
public synchronized Map<String, Object> getTags() {
- if (tags.isEmpty()) {
- tags = Maps.newHashMapWithExpectedSize(3);
- }
tags.put(DDTags.THREAD_NAME, threadName);
tags.put(DDTags.THREAD_ID, threadId);
tags.put(DDTags.SPAN_TYPE, getSpanType());
@@ -307,12 +310,4 @@ public class DDSpanContext implements io.opentracing.SpanContext {
}
return s.toString();
}
-
- public String getOperationName() {
- return operationName;
- }
-
- public void setOperationName(final String operationName) {
- this.operationName = operationName;
- }
}

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/scopemanager/ContextualScopeManager.java
dd-trace-ot/src/main/java/datadog/opentracing/scopemanager/ContextualScopeManager.java
@@ -5,7 +5,6 @@ import io.opentracing.ScopeManager;
import io.opentracing.Span;
import java.util.Deque;
import java.util.concurrent.ConcurrentLinkedDeque;
-import java.util.concurrent.atomic.AtomicInteger;
public class ContextualScopeManager implements ScopeManager {
final ThreadLocal<Scope> tlsScope = new ThreadLocal<>();
@@ -18,7 +17,7 @@ public class ContextualScopeManager implements ScopeManager {
return context.activate(span, finishOnClose);
}
}
- return new RefCountingScope(this, new AtomicInteger(1), span, finishOnClose);
+ return new ContinuableScope(this, span, finishOnClose);
}
@Override

RENAME
dd-trace-ot/src/main/java/datadog/opentracing/scopemanager/RefCountingScope.java
dd-trace-ot/src/main/java/datadog/opentracing/scopemanager/ContinuableScope.java
@@ -4,18 +4,24 @@ import io.opentracing.Scope;
import io.opentracing.Span;
import java.util.concurrent.atomic.AtomicInteger;
-public class RefCountingScope implements Scope {
+public class ContinuableScope implements Scope {
final ContextualScopeManager scopeManager;
final AtomicInteger refCount;
private final Span wrapped;
private final boolean finishOnClose;
private final Scope toRestore;
- RefCountingScope(
+ ContinuableScope(
+ final ContextualScopeManager scopeManager, final Span wrapped, final boolean finishOnClose) {
+ this(scopeManager, new AtomicInteger(1), wrapped, finishOnClose);
+ }
+
+ private ContinuableScope(
final ContextualScopeManager scopeManager,
final AtomicInteger refCount,
final Span wrapped,
final boolean finishOnClose) {
+
this.scopeManager = scopeManager;
this.refCount = refCount;
this.wrapped = wrapped;
@@ -57,7 +63,7 @@ public class RefCountingScope implements Scope {
return context.activate(wrapped, finishOnClose);
}
}
- return new RefCountingScope(scopeManager, refCount, wrapped, finishOnClose);
+ return new ContinuableScope(scopeManager, refCount, wrapped, finishOnClose);
}
}
}

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy
dd-trace-ot/src/test/groovy/datadog/opentracing/scopemanager/ScopeManagerTest.groovy
@@ -42,7 +42,7 @@ class ScopeManagerTest extends Specification {
then:
!spanReported(scope.span())
scopeManager.active() == scope
- scope instanceof RefCountingScope
+ scope instanceof ContinuableScope
writer.empty
when:
@@ -92,13 +92,13 @@ class ScopeManagerTest extends Specification {
def "ref counting scope doesn't close if non-zero"() {
setup:
def builder = tracer.buildSpan("test")
- def scope = (RefCountingScope) builder.startActive(true)
+ def scope = (ContinuableScope) builder.startActive(true)
def continuation = scope.capture()
expect:
!spanReported(scope.span())
scopeManager.active() == scope
- scope instanceof RefCountingScope
+ scope instanceof ContinuableScope
writer.empty
when:
@@ -128,7 +128,7 @@ class ScopeManagerTest extends Specification {
setup:
def parentScope = tracer.buildSpan("parent").startActive(false) //false or trace is reported early
def parentSpan = parentScope.span()
- RefCountingScope childScope = (RefCountingScope) tracer.buildSpan("parent").startActive(true)
+ ContinuableScope childScope = (ContinuableScope) tracer.buildSpan("parent").startActive(true)
def childSpan = childScope.span()
def cont = childScope.capture()
@@ -234,7 +234,7 @@ class ScopeManagerTest extends Specification {
contexts.each {
scopeManager.addScopeContext(it)
}
- RefCountingScope scope = (RefCountingScope) tracer.buildSpan("parent").startActive(true)
+ ContinuableScope scope = (ContinuableScope) tracer.buildSpan("parent").startActive(true)
expect:
scopeManager.tlsScope.get() == scope
@@ -285,7 +285,7 @@ class ScopeManagerTest extends Specification {
scope = scopeManager.activate(span, true)
then:
- scope instanceof RefCountingScope
+ scope instanceof ContinuableScope
scopeManager.tlsScope.get() == scope
where:

Improve threadsafety and organization