Remove synchronization from RateByServiceSampler
This change avoids lock contention in a hot code path.
I also added double check locking in ClassLoaderMatcher to reduce the need of locking there.
MODIFY
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java
@@ -150,9 +150,14 @@ public class ClassLoaderMatcher {
@Override
public boolean matches(final ClassLoader target) {
if (target != null) {
+ Boolean result = cache.get(target);
+ if (result != null) {
+ return result;
+ }
synchronized (target) {
- if (cache.containsKey(target)) {
- return cache.get(target);
+ result = cache.get(target);
+ if (result != null) {
+ return result;
}
for (final String name : names) {
if (target.getResource(Utils.getResourceName(name)) == null) {
@@ -184,9 +189,14 @@ public class ClassLoaderMatcher {
@Override
public boolean matches(final ClassLoader target) {
if (target != null) {
+ Boolean result = cache.get(target);
+ if (result != null) {
+ return result;
+ }
synchronized (target) {
- if (cache.containsKey(target)) {
- return cache.get(target);
+ result = cache.get(target);
+ if (result != null) {
+ return result;
}
try {
final Class<?> aClass = Class.forName(className, false, target);
@@ -225,9 +235,14 @@ public class ClassLoaderMatcher {
@Override
public boolean matches(final ClassLoader target) {
if (target != null) {
+ Boolean result = cache.get(target);
+ if (result != null) {
+ return result;
+ }
synchronized (target) {
- if (cache.containsKey(target)) {
- return cache.get(target);
+ result = cache.get(target);
+ if (result != null) {
+ return result;
}
try {
final Class<?> aClass = Class.forName(className, false, target);

MODIFY
dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java
dd-trace-ot/src/main/java/datadog/trace/common/sampling/RateByServiceSampler.java
@@ -4,6 +4,7 @@ import com.fasterxml.jackson.databind.JsonNode;
import datadog.opentracing.DDSpan;
import datadog.trace.api.sampling.PrioritySampling;
import datadog.trace.common.writer.DDApi.ResponseListener;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
@@ -19,19 +20,19 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
/** Key for setting the baseline rate */
private static final String BASE_KEY = "service:,env:";
/** Sampler to use if service+env is not in the map */
- private RateSampler baseSampler = new RateSampler(1.0);
+ private volatile RateSampler baseSampler = new RateSampler(1.0);
- private final Map<String, RateSampler> serviceRates = new HashMap<String, RateSampler>();
+ private volatile Map<String, RateSampler> serviceRates = Collections.emptyMap();
@Override
- public synchronized boolean sample(DDSpan span) {
+ public boolean sample(final DDSpan span) {
// Priority sampling sends all traces to the core agent, including traces marked dropped.
// This allows the core agent to collect stats on all traces.
return true;
}
/** If span is a root span, set the span context samplingPriority to keep or drop */
- public void initializeSamplingPriority(DDSpan span) {
+ public void initializeSamplingPriority(final DDSpan span) {
if (span.isRootSpan()) {
// Run the priority sampler on the new span
setSamplingPriorityOnSpanContext(span);
@@ -42,14 +43,13 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
}
}
- private synchronized void setSamplingPriorityOnSpanContext(DDSpan span) {
+ private void setSamplingPriorityOnSpanContext(final DDSpan span) {
final String serviceName = span.getServiceName();
final String env = getSpanEnv(span);
final String key = "service:" + serviceName + ",env:" + env;
- final RateSampler sampler;
- if (serviceRates.containsKey(key)) {
- sampler = serviceRates.get(key);
- } else {
+
+ RateSampler sampler = serviceRates.get(key);
+ if (sampler == null) {
sampler = baseSampler;
}
if (sampler.sample(span)) {
@@ -59,18 +59,17 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
}
}
- private static String getSpanEnv(DDSpan span) {
+ private static String getSpanEnv(final DDSpan span) {
return null == span.getTags().get("env") ? "" : String.valueOf(span.getTags().get("env"));
}
@Override
- public void onResponse(String endpoint, JsonNode responseJson) {
- JsonNode newServiceRates = responseJson.get("rate_by_service");
+ public void onResponse(final String endpoint, final JsonNode responseJson) {
+ final JsonNode newServiceRates = responseJson.get("rate_by_service");
if (null != newServiceRates) {
log.debug("Update service sampler rates: {} -> {}", endpoint, responseJson);
- synchronized (this) {
- serviceRates.clear();
- Iterator<String> itr = newServiceRates.fieldNames();
+ final Map<String, RateSampler> updatedServiceRates = new HashMap<>();
+ final Iterator<String> itr = newServiceRates.fieldNames();
while (itr.hasNext()) {
final String key = itr.next();
try {
@@ -78,13 +77,13 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
if (BASE_KEY.equals(key)) {
baseSampler = new RateSampler(val);
} else {
- serviceRates.put(key, new RateSampler(val));
+ updatedServiceRates.put(key, new RateSampler(val));
}
- } catch (NumberFormatException nfe) {
+ } catch (final NumberFormatException nfe) {
log.debug("Unable to parse new service rate {} -> {}", key, newServiceRates.get(key));
}
}
- }
+ serviceRates = Collections.unmodifiableMap(updatedServiceRates);
}
}
@@ -110,7 +109,7 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
*/
public RateSampler(double sampleRate) {
- if (sampleRate <= 0) {
+ if (sampleRate < 0) {
sampleRate = 1;
log.error("SampleRate is negative or null, disabling the sampler");
} else if (sampleRate > 1) {
@@ -123,13 +122,13 @@ public class RateByServiceSampler implements Sampler, ResponseListener {
@Override
public boolean doSample(final DDSpan span) {
- final boolean sample = Math.random() <= this.sampleRate;
+ final boolean sample = Math.random() <= sampleRate;
log.debug("{} - Span is sampled: {}", span, sample);
return sample;
}
public double getSampleRate() {
- return this.sampleRate;
+ return sampleRate;
}
@Override

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanTest.groovy
dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanTest.groovy
package datadog.opentracing
+
+import com.fasterxml.jackson.databind.ObjectMapper
import datadog.opentracing.propagation.ExtractedContext
import datadog.opentracing.propagation.TagContext
import datadog.trace.agent.test.utils.ConfigUtils
+import datadog.trace.api.DDTags
import datadog.trace.api.sampling.PrioritySampling
import datadog.trace.common.sampling.RateByServiceSampler
import datadog.trace.common.writer.ListWriter
@@ -20,11 +23,17 @@ class DDSpanTest extends Specification {
}
def writer = new ListWriter()
- def tracer = new DDTracer(DEFAULT_SERVICE_NAME, writer, new RateByServiceSampler(), [:])
+ def sampler = new RateByServiceSampler()
+ def tracer = new DDTracer(DEFAULT_SERVICE_NAME, writer, sampler, [:])
@Shared
def defaultSamplingPriority = PrioritySampling.SAMPLER_KEEP
+ def setup() {
+ sampler.onResponse("test", new ObjectMapper()
+ .readTree('{"rate_by_service":{"service:,env:":1.0,"service:spock,env:":0.0}}'))
+ }
+
def "getters and setters"() {
setup:
final DDSpanContext context =
@@ -254,8 +263,29 @@ class DDSpanTest extends Specification {
new ExtractedContext("123", "456", 1, "789", [:], [:]) | false
}
- def "setting forced tracing via tag"() {
+ def "sampling priority set on init"() {
+ setup:
+ def span = tracer.buildSpan("test").start()
+
+ expect:
+ span.getSamplingPriority() == PrioritySampling.SAMPLER_KEEP
+
+ when:
+ span.setTag(DDTags.SERVICE_NAME, "spock")
+
+ then:
+ // FIXME: priority currently only applies if service name set before span started.
+ span.getSamplingPriority() == PrioritySampling.SAMPLER_KEEP
+// span.getSamplingPriority() == PrioritySampling.SAMPLER_DROP
+
+ when:
+ span = tracer.buildSpan("test").withTag(DDTags.SERVICE_NAME, "spock").start()
+ then:
+ span.getSamplingPriority() == PrioritySampling.SAMPLER_DROP
+ }
+
+ def "setting forced tracing via tag"() {
setup:
def span = tracer.buildSpan("root").start()
if (tagName) {

Remove synchronization from RateByServiceSampler

This change avoids lock contention in a hot code path.

I also added double check locking in ClassLoaderMatcher to reduce the need of locking there.