From 73fb7aa2b68a5d8aced24eca7dce97ed43429216 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 13 Feb 2020 14:17:52 -0800 Subject: [PATCH 01/37] Add TraceProcessor and move some existing functionality to rules Specifically, the Status5XXRule, URLAsResourceNameRule, and Status404Rule. --- .../URLAsResourceNameBenchmark.java | 34 ------ .../datadog/opentracing/DDSpanContext.java | 6 +- .../decorators/DBTypeDecorator.java | 1 + .../decorators/DDDecoratorsFactory.java | 5 +- .../decorators/Status404Decorator.java | 23 ---- .../decorators/Status5XXDecorator.java | 21 ---- .../common/processor/TraceProcessor.java | 66 +++++++++++ .../common/processor/rule/Status404Rule.java | 23 ++++ .../common/processor/rule/Status5XXRule.java | 24 ++++ .../rule/URLAsResourceNameRule.java} | 41 +++---- .../trace/common/writer/ListWriter.java | 5 +- .../trace/common/writer/LoggingWriter.java | 5 +- .../ddagent/TraceProcessingDisruptor.java | 3 + .../decorators/SpanDecoratorTest.groovy | 58 ++-------- .../groovy/datadog/trace/DDTracerTest.groovy | 2 +- .../processor/TraceProcessorTest.groovy | 109 ++++++++++++++++++ .../URLAsResourceNameRuleTest.groovy} | 50 ++++---- 17 files changed, 291 insertions(+), 185 deletions(-) delete mode 100644 dd-trace-ot/src/jmh/java/datadog/opentracing/decorators/URLAsResourceNameBenchmark.java delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status404Decorator.java delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java rename dd-trace-ot/src/main/java/datadog/{opentracing/decorators/URLAsResourceName.java => trace/common/processor/rule/URLAsResourceNameRule.java} (61%) create mode 100644 dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy rename dd-trace-ot/src/test/groovy/datadog/{opentracing/decorators/URLAsResourceNameTest.groovy => trace/common/processor/URLAsResourceNameRuleTest.groovy} (80%) diff --git a/dd-trace-ot/src/jmh/java/datadog/opentracing/decorators/URLAsResourceNameBenchmark.java b/dd-trace-ot/src/jmh/java/datadog/opentracing/decorators/URLAsResourceNameBenchmark.java deleted file mode 100644 index 0102e26be9..0000000000 --- a/dd-trace-ot/src/jmh/java/datadog/opentracing/decorators/URLAsResourceNameBenchmark.java +++ /dev/null @@ -1,34 +0,0 @@ -package datadog.opentracing.decorators; - -import ch.qos.logback.classic.Level; -import ch.qos.logback.classic.Logger; -import datadog.opentracing.DDSpanContext; -import datadog.opentracing.SpanFactory; -import org.openjdk.jmh.annotations.Benchmark; -import org.openjdk.jmh.annotations.Scope; -import org.openjdk.jmh.annotations.State; -import org.slf4j.LoggerFactory; - -public class URLAsResourceNameBenchmark { - static { - ((Logger) LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME)).setLevel(Level.WARN); - } - - @State(Scope.Benchmark) - public static class BenchmarkState { - private final AbstractDecorator base = new URLAsResourceName(); - - private final DDSpanContext ctx = SpanFactory.newSpanOf(0).context(); - } - - @Benchmark - public Object testPathOnly(final BenchmarkState state) { - return state.base.shouldSetTag(state.ctx, null, "/somepath/123/"); - } - - @Benchmark - public Object testFullUrl(final BenchmarkState state) { - return state.base.shouldSetTag( - state.ctx, null, "http://localhost:8080/somepath/123/?query=123#fragment"); - } -} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java index 88e0974135..240c4358d7 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java @@ -167,7 +167,11 @@ public class DDSpanContext implements io.opentracing.SpanContext { } public String getResourceName() { - return resourceName == null || resourceName.isEmpty() ? operationName : resourceName; + return isResourceNameSet() ? resourceName : operationName; + } + + public boolean isResourceNameSet() { + return !(resourceName == null || resourceName.isEmpty()); } public void setResourceName(final String resourceName) { diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBTypeDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBTypeDecorator.java index da4c21d671..2fd3705003 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBTypeDecorator.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBTypeDecorator.java @@ -9,6 +9,7 @@ import io.opentracing.tag.Tags; * This span decorator leverages DB tags. It allows the dev to define a custom service name and * retrieves some DB meta such as the statement */ +@Deprecated // This should be covered by instrumentation decorators now. public class DBTypeDecorator extends AbstractDecorator { public DBTypeDecorator() { diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index e4880cffa5..4dbc49c41b 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -27,10 +27,7 @@ public class DDDecoratorsFactory { new ServiceNameDecorator(), new ServiceNameDecorator("service", false), new ServletContextDecorator(), - new SpanTypeDecorator(), - new Status404Decorator(), - new Status5XXDecorator(), - new URLAsResourceName())) { + new SpanTypeDecorator())) { if (Config.get().isDecoratorEnabled(decorator.getClass().getSimpleName())) { decorators.add(decorator); diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status404Decorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status404Decorator.java deleted file mode 100644 index 35a5e54e32..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status404Decorator.java +++ /dev/null @@ -1,23 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; -import io.opentracing.tag.Tags; - -/** This span decorator protect against spam on the resource name */ -public class Status404Decorator extends AbstractDecorator { - - public Status404Decorator() { - super(); - this.setMatchingTag(Tags.HTTP_STATUS.getKey()); - this.setMatchingValue(404); - this.setReplacementTag(DDTags.RESOURCE_NAME); - this.setReplacementValue("404"); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - super.shouldSetTag(context, tag, value); - return true; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java deleted file mode 100644 index 368d0dbd5b..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/Status5XXDecorator.java +++ /dev/null @@ -1,21 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import io.opentracing.tag.Tags; - -/** Mark all 5xx status codes as an error */ -public class Status5XXDecorator extends AbstractDecorator { - public Status5XXDecorator() { - super(); - this.setMatchingTag(Tags.HTTP_STATUS.getKey()); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - final int responseCode = Integer.parseInt(value.toString()); - if (500 <= responseCode && responseCode < 600) { - context.setTag(Tags.ERROR.getKey(), true); - } - return true; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java new file mode 100644 index 0000000000..8e1585fb93 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -0,0 +1,66 @@ +package datadog.trace.common.processor; + +import datadog.opentracing.DDSpan; +import datadog.trace.api.Config; +import datadog.trace.common.processor.rule.Status404Rule; +import datadog.trace.common.processor.rule.Status5XXRule; +import datadog.trace.common.processor.rule.URLAsResourceNameRule; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public class TraceProcessor { + final Rule[] DEFAULT_RULES = + new Rule[] { + // Rules are applied in order. + new Status5XXRule(), new URLAsResourceNameRule(), new Status404Rule(), + }; + + private final List rules; + + public TraceProcessor() { + + rules = new ArrayList<>(DEFAULT_RULES.length); + for (final Rule rule : DEFAULT_RULES) { + if (isEnabled(rule)) { + rules.add(rule); + } + } + } + + private static boolean isEnabled(final Rule rule) { + boolean enabled = Config.get().isDecoratorEnabled(rule.getClass().getSimpleName()); + for (final String alias : rule.aliases()) { + enabled &= Config.get().isDecoratorEnabled(alias); + } + if (!enabled) { + log.debug("{} disabled", rule.getClass().getSimpleName()); + } + return enabled; + } + + public interface Rule { + String[] aliases(); + + void processSpan(DDSpan span, Map meta, Collection trace); + } + + public List onTraceComplete(final List trace) { + for (final DDSpan span : trace) { + applyRules(trace, span); + } + + // TODO: apply DDTracer's TraceInterceptors + return trace; + } + + private void applyRules(final Collection trace, final DDSpan span) { + final Map meta = span.getMeta(); + for (final Rule rule : rules) { + rule.processSpan(span, meta, trace); + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java new file mode 100644 index 0000000000..8cdb9758b9 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java @@ -0,0 +1,23 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; +import io.opentracing.tag.Tags; +import java.util.Collection; +import java.util.Map; + +/** This span decorator protect against spam on the resource name */ +public class Status404Rule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"Status404Decorator"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map meta, final Collection trace) { + if (!span.context().isResourceNameSet() && "404".equals(meta.get(Tags.HTTP_STATUS.getKey()))) { + span.setResourceName("404"); + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java new file mode 100644 index 0000000000..6b6238551f --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java @@ -0,0 +1,24 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; +import io.opentracing.tag.Tags; +import java.util.Collection; +import java.util.Map; + +/** Mark all 5xx status codes as an error */ +public class Status5XXRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"Status5XXDecorator"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map meta, final Collection trace) { + if (!span.context().getErrorFlag() && meta.containsKey(Tags.HTTP_STATUS.getKey())) { + final int responseCode = Integer.parseInt(meta.get(Tags.HTTP_STATUS.getKey())); + span.setError(500 <= responseCode && responseCode < 600); + } + } +} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java similarity index 61% rename from dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java rename to dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java index 3200efd59f..32280c351e 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/URLAsResourceName.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java @@ -1,38 +1,39 @@ -package datadog.opentracing.decorators; +package datadog.trace.common.processor.rule; +import datadog.opentracing.DDSpan; import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; +import datadog.trace.common.processor.TraceProcessor; import io.opentracing.tag.Tags; +import java.util.Collection; +import java.util.Map; import java.util.regex.Pattern; -public class URLAsResourceName extends AbstractDecorator { +public class URLAsResourceNameRule implements TraceProcessor.Rule { // Matches any path segments with numbers in them. (exception for versioning: "/v1/") public static final Pattern PATH_MIXED_ALPHANUMERICS = Pattern.compile("(?<=/)(?![vV]\\d{1,2}/)(?:[^\\/\\d\\?]*[\\d]+[^\\/\\?]*)"); - public URLAsResourceName() { - super(); - setMatchingTag(Tags.HTTP_URL.getKey()); - setReplacementTag(DDTags.RESOURCE_NAME); + @Override + public String[] aliases() { + return new String[] {"URLAsResourceName"}; } @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - final String statusCode = String.valueOf(context.getTags().get(Tags.HTTP_STATUS.getKey())); - // do nothing if the status code is already set and equals to 404. - // TODO: it assumes that Status404Decorator is active. If it's not, it will lead to unexpected - // behaviors - if (value == null || statusCode != null && statusCode.equals("404")) { - return true; + public void processSpan( + final DDSpan span, final Map meta, final Collection trace) { + final DDSpanContext context = span.context(); + if (context.isResourceNameSet() + || meta.get(Tags.HTTP_URL.getKey()) == null + || "404".equals(meta.get(Tags.HTTP_STATUS.getKey()))) { + return; } - final String rawPath = rawPathFromUrlString(String.valueOf(value).trim()); + final String rawPath = rawPathFromUrlString(meta.get(Tags.HTTP_URL.getKey()).trim()); final String normalizedPath = normalizePath(rawPath); - final String resourceName = addMethodIfAvailable(context, normalizedPath); + final String resourceName = addMethodIfAvailable(meta, normalizedPath); context.setResourceName(resourceName); - return true; } private String rawPathFromUrlString(final String url) { @@ -86,11 +87,11 @@ public class URLAsResourceName extends AbstractDecorator { return PATH_MIXED_ALPHANUMERICS.matcher(path).replaceAll("?"); } - private String addMethodIfAvailable(final DDSpanContext context, String path) { + private String addMethodIfAvailable(final Map meta, String path) { // if the verb (GET, POST ...) is present, add it - final String verb = (String) context.getTags().get(Tags.HTTP_METHOD.getKey()); + final String verb = meta.get(Tags.HTTP_METHOD.getKey()); if (verb != null && !verb.isEmpty()) { - path = verb + " " + path; + path = verb.toUpperCase() + " " + path; } return path; } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java index a92343350d..da152cfa13 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ListWriter.java @@ -1,6 +1,7 @@ package datadog.trace.common.writer; import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; import java.util.ArrayList; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; @@ -10,6 +11,7 @@ import java.util.concurrent.TimeoutException; /** List writer used by tests mostly */ public class ListWriter extends CopyOnWriteArrayList> implements Writer { + private final TraceProcessor processor = new TraceProcessor(); private final List latches = new ArrayList<>(); public List firstTrace() { @@ -17,8 +19,9 @@ public class ListWriter extends CopyOnWriteArrayList> implements Wr } @Override - public void write(final List trace) { + public void write(List trace) { synchronized (latches) { + trace = processor.onTraceComplete(trace); add(trace); for (final CountDownLatch latch : latches) { if (size() >= latch.getCount()) { diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java index c4a793f3b2..00547bdbc8 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/LoggingWriter.java @@ -3,14 +3,17 @@ package datadog.trace.common.writer; import static datadog.trace.common.serialization.JsonFormatWriter.TRACE_ADAPTER; import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; import java.util.List; import lombok.extern.slf4j.Slf4j; @Slf4j public class LoggingWriter implements Writer { + private final TraceProcessor processor = new TraceProcessor(); @Override - public void write(final List trace) { + public void write(List trace) { + trace = processor.onTraceComplete(trace); if (log.isInfoEnabled()) { try { log.info("write(trace): {}", toString(trace)); diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java index 67c8b91adf..16a5889c8f 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -4,6 +4,7 @@ import com.lmax.disruptor.EventHandler; import datadog.common.exec.DaemonThreadFactory; import datadog.opentracing.DDSpan; import datadog.opentracing.DDSpanContext; +import datadog.trace.common.processor.TraceProcessor; import datadog.trace.common.writer.DDAgentWriter; import java.util.List; import lombok.extern.slf4j.Slf4j; @@ -41,6 +42,7 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { // This class is threadsafe if we want to enable more processors. public static class TraceSerializingHandler implements EventHandler>> { + private final TraceProcessor processor = new TraceProcessor(); private final DDAgentApi api; private final BatchWritingDisruptor batchWritingDisruptor; private final Monitor monitor; @@ -69,6 +71,7 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { .setMetric(DDSpanContext.SAMPLE_RATE_KEY, 1d / event.representativeCount); } try { + event.data = processor.onTraceComplete(event.data); final byte[] serializedTrace = api.serializeTrace(event.data); batchWritingDisruptor.publish(serializedTrace, event.representativeCount); monitor.onSerialize(writer, event.data, serializedTrace); diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index 094b5d77ce..7a5f7b1453 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -351,32 +351,6 @@ class SpanDecoratorTest extends DDSpecification { something = "fake-query" } - def "set 404 as a resource on a 404 issue"() { - when: - Tags.HTTP_STATUS.set(span, 404) - - then: - span.getResourceName() == "404" - } - - def "set 5XX status code as an error"() { - when: - Tags.HTTP_STATUS.set(span, status) - - then: - span.isError() == error - - where: - status | error - 400 | false - 404 | false - 499 | false - 500 | true - 550 | true - 599 | true - 600 | false - } - def "set error flag when error tag reported"() { when: Tags.ERROR.set(span, error) @@ -417,30 +391,12 @@ class SpanDecoratorTest extends DDSpecification { then: span.serviceName == "my-servlet" - when: - span = tracer.buildSpan("decorator.test").withTag(Tags.HTTP_STATUS.key, 404).start() - - then: - span.resourceName == "404" - when: span = tracer.buildSpan("decorator.test").withTag("error", "true").start() then: span.error - when: - span = tracer.buildSpan("decorator.test").withTag(Tags.HTTP_STATUS.key, 500).start() - - then: - span.error - - when: - span = tracer.buildSpan("decorator.test").withTag(Tags.HTTP_URL.key, "http://example.com/path/number123/?param=true").start() - - then: - span.resourceName == "/path/?/" - when: span = tracer.buildSpan("decorator.test").withTag(Tags.DB_STATEMENT.key, "some-statement").start() @@ -451,7 +407,7 @@ class SpanDecoratorTest extends DDSpecification { def "disable decorator via config"() { setup: ConfigUtils.updateConfig { - System.setProperty("dd.trace.${decorator}.enabled", "false") + System.setProperty("dd.trace.${decorator}.enabled", "$enabled") } tracer = DDTracer.builder() @@ -461,11 +417,11 @@ class SpanDecoratorTest extends DDSpecification { .build() when: - def span = tracer.buildSpan("some span").withTag(Tags.PEER_SERVICE.key, "peer-service").start() + def span = tracer.buildSpan("some span").withTag(DDTags.SERVICE_NAME, "other-service").start() span.finish() then: - span.getServiceName() == "some-service" + span.getServiceName() == enabled ? "other-service" : "some-service" cleanup: ConfigUtils.updateConfig { @@ -473,9 +429,11 @@ class SpanDecoratorTest extends DDSpecification { } where: - decorator | _ - PeerServiceDecorator.getSimpleName().toLowerCase() | _ - PeerServiceDecorator.getSimpleName() | _ + decorator | enabled + ServiceNameDecorator.getSimpleName().toLowerCase() | true + ServiceNameDecorator.getSimpleName() | true + ServiceNameDecorator.getSimpleName().toLowerCase() | false + ServiceNameDecorator.getSimpleName() | false } def "disabling service decorator does not disable split by tags"() { diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy index adfb4e6dca..91d7e0d414 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/DDTracerTest.groovy @@ -50,7 +50,7 @@ class DDTracerTest extends DDSpecification { tracer.writer instanceof DDAgentWriter tracer.writer.monitor instanceof Monitor.Noop - tracer.spanContextDecorators.size() == 15 + !tracer.spanContextDecorators.isEmpty() tracer.injector instanceof HttpCodec.CompoundInjector tracer.extractor instanceof HttpCodec.CompoundExtractor diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy new file mode 100644 index 0000000000..59cff6577e --- /dev/null +++ b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy @@ -0,0 +1,109 @@ +package datadog.trace.common.processor + +import datadog.opentracing.SpanFactory +import datadog.trace.agent.test.utils.ConfigUtils +import datadog.trace.common.processor.rule.Status404Rule +import datadog.trace.common.processor.rule.URLAsResourceNameRule +import datadog.trace.util.test.DDSpecification +import io.opentracing.tag.Tags +import spock.lang.Subject + +class TraceProcessorTest extends DDSpecification { + + @Subject + def processor = new TraceProcessor() + + def span = SpanFactory.newSpanOf(0) + def trace = [span] + + def setup() { + span.context.resourceName = null + } + + def "test disable"() { + setup: + ConfigUtils.updateConfig { + System.setProperty("dd.trace.${name}.enabled", "false") + } + def processor = new TraceProcessor() + + expect: + !processor.rules.any { + it.class.name == rule.name + } + + cleanup: + ConfigUtils.updateConfig { + System.clearProperty("dd.trace.${name}.enabled") + } + + where: + rule | alias + URLAsResourceNameRule | null + URLAsResourceNameRule | URLAsResourceNameRule.simpleName.toLowerCase() + URLAsResourceNameRule | "URLAsResourceName" + Status404Rule | null + Status404Rule | Status404Rule.simpleName.toLowerCase() + Status404Rule | "Status404Decorator" + + name = alias == null ? rule.simpleName : alias + } + + def "set 404 as a resource on a 404 issue"() { + setup: + Tags.HTTP_STATUS.set(span, 404) + + when: + processor.onTraceComplete(trace) + + then: + span.getResourceName() == "404" + } + + def "status #status code error=#error"() { + setup: + Tags.HTTP_STATUS.set(span, status) + + when: + processor.onTraceComplete(trace) + + then: + span.isError() == error + + where: + status | error + 400 | false + 404 | false + 499 | false + 500 | true + 550 | true + 599 | true + 600 | false + } + + def "resource name set with url path #url to #resourceName"() { + setup: + if (method) { + Tags.HTTP_METHOD.set(span, method) + } + Tags.HTTP_URL.set(span, url) + Tags.HTTP_STATUS.set(span, status) + + when: + processor.onTraceComplete(trace) + + then: + span.resourceName == resourceName + + where: + method | url | status | resourceName + "GET" | "" | 200 | "fakeOperation" + null | "/" | 200 | "/" + null | "/path" | 200 | "/path" + "put" | "/" | 200 | "PUT /" + "Head" | "/path" | 200 | "HEAD /path" + "post" | "/post" | 400 | "POST /post" + "GET" | "/asdf" | 404 | "404" + null | "/error" | 500 | "/error" + } +} diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/URLAsResourceNameRuleTest.groovy similarity index 80% rename from dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy rename to dd-trace-ot/src/test/groovy/datadog/trace/common/processor/URLAsResourceNameRuleTest.groovy index 85945089f9..081df88c0e 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/URLAsResourceNameTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/URLAsResourceNameRuleTest.groovy @@ -1,21 +1,21 @@ -package datadog.opentracing.decorators +package datadog.trace.common.processor + -import datadog.opentracing.DDSpanContext import datadog.opentracing.DDTracer -import datadog.opentracing.PendingTrace -import datadog.trace.api.sampling.PrioritySampling +import datadog.opentracing.SpanFactory +import datadog.trace.common.processor.rule.URLAsResourceNameRule import datadog.trace.common.writer.ListWriter import datadog.trace.util.test.DDSpecification import io.opentracing.tag.Tags import spock.lang.Subject -class URLAsResourceNameTest extends DDSpecification { +class URLAsResourceNameRuleTest extends DDSpecification { def writer = new ListWriter() def tracer = DDTracer.builder().writer(writer).build() @Subject - def decorator = new URLAsResourceName() + def decorator = new URLAsResourceNameRule() def "pulls path from url #input"() { when: @@ -104,37 +104,29 @@ class URLAsResourceNameTest extends DDSpecification { } def "sets the resource name"() { + setup: + def span = SpanFactory.newSpanOf(0) + span.context.resourceName = null + meta.each { + span.setTag(it.key, (String) it.value) + } + when: - final DDSpanContext context = - new DDSpanContext( - 1G, - 1G, - 0G, - "fakeService", - "fakeOperation", - "fakeResource", - PrioritySampling.UNSET, - null, - [:], - false, - "fakeType", - tags, - new PendingTrace(tracer, 1G), - tracer, - [:]) + decorator.processSpan(span, meta, [span]) then: - decorator.shouldSetTag(context, Tags.HTTP_URL.getKey(), value) - context.resourceName == resourceName + span.resourceName == resourceName where: - value | resourceName | tags - null | "fakeResource" | [:] + value | resourceName | meta + null | "fakeOperation" | [:] " " | "/" | [:] "\t" | "/" | [:] "/path" | "/path" | [:] "/ABC/a-1/b_2/c.3/d4d/5f/6" | "/ABC/?/?/?/?/?/?" | [:] - "/not-found" | "fakeResource" | [(Tags.HTTP_STATUS.key): 404] - "/with-method" | "Post /with-method" | [(Tags.HTTP_METHOD.key): "Post"] + "/not-found" | "fakeOperation" | [(Tags.HTTP_STATUS.key): "404"] + "/with-method" | "POST /with-method" | [(Tags.HTTP_METHOD.key): "Post"] + + ignore = meta.put(Tags.HTTP_URL.key, value) } } From 97efa307d3e1a49393285f27df99922435e54a3f Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 18 Feb 2020 10:11:56 -0800 Subject: [PATCH 02/37] Stop setting error tag and update tests. --- .../src/lagomTest/groovy/LagomTest.groovy | 1 - .../groovy/AkkaHttpClientInstrumentationTest.groovy | 1 - .../groovy/AkkaHttpServerInstrumentationTest.groovy | 1 - .../dropwizard/src/test/groovy/DropwizardTest.groovy | 1 - .../src/test/groovy/FinatraServerTest.groovy | 9 ++++++--- .../finatra-2.9/src/test/scala/FinatraController.scala | 8 -------- .../src/test/groovy/GlassFishServerTest.groovy | 1 - .../test/groovy/AbstractGoogleHttpClientTest.groovy | 1 - .../grpc-1.5/src/test/groovy/GrpcTest.groovy | 4 ---- .../jaxrs/v1/JaxRsClientV1Instrumentation.java | 5 +---- .../instrumentation/jaxrs/ClientTracingFilter.java | 5 +---- .../jetty-8/src/test/groovy/JettyHandlerTest.groovy | 7 ++++++- .../test/groovy/JSPInstrumentationBasicTests.groovy | 2 -- .../src/test/groovy/server/PlayServerTest.groovy | 4 +--- .../src/test/groovy/server/PlayServerTest.groovy | 5 +---- .../test/groovy/server/RatpackHttpServerTest.groovy | 10 +++++++--- .../request-2/src/test/groovy/JettyServlet2Test.groovy | 1 - .../src/test/groovy/AbstractServlet3Test.groovy | 1 - .../request-3/src/test/groovy/JettyServlet3Test.groovy | 1 - .../src/test/groovy/TomcatServlet3Test.groovy | 1 - .../src/test/groovy/SpringWebfluxTest.groovy | 3 +-- .../src/test/groovy/test/SpringBootBasedTest.groovy | 1 - .../src/test/groovy/test/TwilioClientTest.groovy | 2 -- .../datadog/trace/agent/test/asserts/TagsAssert.groovy | 1 - .../trace/agent/test/base/HttpServerTest.groovy | 3 --- .../java/datadog/opentracing/decorators/ErrorFlag.java | 5 ++--- 26 files changed, 26 insertions(+), 58 deletions(-) diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy index 1690a95836..c88a876149 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy @@ -113,7 +113,6 @@ class LagomTest extends AgentTestRunner { "$Tags.HTTP_URL" "ws://localhost:${server.port()}/error" "$Tags.HTTP_METHOD" "GET" "$Tags.HTTP_STATUS" 500 - "$Tags.ERROR" true defaultTags() } } diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy index 0e04cbe190..133379747e 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy @@ -83,7 +83,6 @@ class AkkaHttpClientInstrumentationTest extends HttpClientTest { tags { "$Tags.COMPONENT" "akka-http-client" "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT - "$Tags.ERROR" true errorTags(NullPointerException) defaultTags() } diff --git a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy index b0375b1a8d..74e1ea7e1a 100644 --- a/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy @@ -57,7 +57,6 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest "$Tags.HTTP_METHOD" method "$Tags.HTTP_STATUS" endpoint.status if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy index 231e65fd44..c6a4919ca5 100644 --- a/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy +++ b/dd-java-agent/instrumentation/dropwizard/src/test/groovy/DropwizardTest.groovy @@ -118,7 +118,6 @@ class DropwizardTest extends HttpServerTest { "$Tags.HTTP_STATUS" endpoint.status "span.origin.type" ServletHandler.CachedChain.name if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy index 4a968880ae..871890483f 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy @@ -51,6 +51,12 @@ class FinatraServerTest extends HttpServerTest { return true } + @Override + boolean testNotFound() { + // Resource name is set to "GET /notFound" + false + } + @Override void stopServer(HttpServer httpServer) { Await.ready(httpServer.close(), TIMEOUT) @@ -81,9 +87,6 @@ class FinatraServerTest extends HttpServerTest { // Finatra doesn't propagate the stack trace or exception to the instrumentation // so the normal errorTags() method can't be used - if (errorEndpoint) { - "$Tags.ERROR" true - } defaultTags() } } diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala index 6a5503c8a4..b71e82aabe 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala @@ -22,14 +22,6 @@ class FinatraController extends Controller { }) } - any(NOT_FOUND.getPath) { request: Request => - controller(NOT_FOUND, new Closure[Response](null) { - override def call(): Response = { - response.notFound(NOT_FOUND.getBody) - } - }) - } - any(QUERY_PARAM.getPath) { request: Request => controller(QUERY_PARAM, new Closure[Response](null) { override def call(): Response = { diff --git a/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy index 116703b77c..a3c8ff43ca 100644 --- a/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy +++ b/dd-java-agent/instrumentation/glassfish/src/test/groovy/GlassFishServerTest.groovy @@ -112,7 +112,6 @@ class GlassFishServerTest extends HttpServerTest { "servlet.path" endpoint.path "span.origin.type" { it.startsWith("TestServlets\$") || it == DefaultServlet.name } if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy index 7faf438639..bfb87ff1e8 100644 --- a/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy +++ b/dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy @@ -71,7 +71,6 @@ abstract class AbstractGoogleHttpClientTest extends HttpClientTest { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - "$Tags.ERROR" true "$DDTags.ERROR_MSG" "Server Error" defaultTags() } diff --git a/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy b/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy index 2ac0245f34..f8a2cbe279 100644 --- a/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy +++ b/dd-java-agent/instrumentation/grpc-1.5/src/test/groovy/GrpcTest.groovy @@ -162,8 +162,6 @@ class GrpcTest extends AgentTestRunner { "status.description" description if (status.cause != null) { errorTags status.cause.class, status.cause.message - } else { - tag "error", true } defaultTags(true) } @@ -196,7 +194,6 @@ class GrpcTest extends AgentTestRunner { "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT "status.code" "${status.code.name()}" "status.description" description - tag "error", true defaultTags() } } @@ -281,7 +278,6 @@ class GrpcTest extends AgentTestRunner { "$Tags.COMPONENT" "grpc-client" "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT "status.code" "UNKNOWN" - tag "error", true defaultTags() } } diff --git a/dd-java-agent/instrumentation/jax-rs-client-1.1/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java b/dd-java-agent/instrumentation/jax-rs-client-1.1/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java index bbc6eb8966..086eced487 100644 --- a/dd-java-agent/instrumentation/jax-rs-client-1.1/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client-1.1/src/main/java/datadog/trace/instrumentation/jaxrs/v1/JaxRsClientV1Instrumentation.java @@ -19,7 +19,6 @@ import com.sun.jersey.api.client.ClientHandler; import com.sun.jersey.api.client.ClientRequest; import com.sun.jersey.api.client.ClientResponse; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.util.Map; @@ -72,9 +71,7 @@ public final class JaxRsClientV1Instrumentation extends Instrumenter.Default { // WARNING: this might be a chain...so we only have to trace the first in the chain. final boolean isRootClientHandler = null == request.getProperties().get(DD_SPAN_ATTRIBUTE); if (isRootClientHandler) { - final AgentSpan span = - startSpan("jax-rs.client.call") - .setTag(DDTags.RESOURCE_NAME, request.getMethod() + " jax-rs.client.call"); + final AgentSpan span = startSpan("jax-rs.client.call"); DECORATE.afterStart(span); DECORATE.onRequest(span, request); request.getProperties().put(DD_SPAN_ATTRIBUTE, span); diff --git a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java index 439a1ed746..4f36762dde 100644 --- a/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java +++ b/dd-java-agent/instrumentation/jax-rs-client-2.0/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java @@ -6,7 +6,6 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; import static datadog.trace.instrumentation.jaxrs.InjectAdapter.SETTER; import static datadog.trace.instrumentation.jaxrs.JaxRsClientDecorator.DECORATE; -import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import javax.annotation.Priority; @@ -24,9 +23,7 @@ public class ClientTracingFilter implements ClientRequestFilter, ClientResponseF @Override public void filter(final ClientRequestContext requestContext) { - final AgentSpan span = - startSpan("jax-rs.client.call") - .setTag(DDTags.RESOURCE_NAME, requestContext.getMethod() + " jax-rs.client.call"); + final AgentSpan span = startSpan("jax-rs.client.call"); try (final AgentScope scope = activateSpan(span, false)) { DECORATE.afterStart(span); DECORATE.onRequest(span, requestContext); diff --git a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy index e0f71f5be0..0c3386e989 100644 --- a/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy +++ b/dd-java-agent/instrumentation/jetty-8/src/test/groovy/JettyHandlerTest.groovy @@ -66,6 +66,12 @@ class JettyHandlerTest extends HttpServerTest { return "jetty.request" } + @Override + boolean testNotFound() { + // resource name is set to "GET JettyHandlerTest$TestHandler" + false + } + @Override boolean testExceptionBody() { false @@ -139,7 +145,6 @@ class JettyHandlerTest extends HttpServerTest { "$Tags.HTTP_STATUS" endpoint.status "span.origin.type" handlerName if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy index 82d3ee5cd3..3f1f0978d2 100644 --- a/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy +++ b/dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy @@ -320,7 +320,6 @@ class JSPInstrumentationBasicTests extends AgentTestRunner { "span.origin.type" "org.apache.catalina.core.ApplicationFilterChain" "servlet.context" "/$jspWebappContext" "servlet.path" "/$jspFileName" - "error" true "error.type" { String tagExceptionType -> return tagExceptionType == exceptionClass.getName() || tagExceptionType.contains(exceptionClass.getSimpleName()) } @@ -342,7 +341,6 @@ class JSPInstrumentationBasicTests extends AgentTestRunner { "span.origin.type" jspClassName "servlet.context" "/$jspWebappContext" "jsp.requestURL" reqUrl - "error" true "error.type" { String tagExceptionType -> return tagExceptionType == exceptionClass.getName() || tagExceptionType.contains(exceptionClass.getSimpleName()) } diff --git a/dd-java-agent/instrumentation/play-2.4/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.4/src/test/groovy/server/PlayServerTest.groovy index 105032f0d9..a2568cabc6 100644 --- a/dd-java-agent/instrumentation/play-2.4/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.4/src/test/groovy/server/PlayServerTest.groovy @@ -100,9 +100,7 @@ class PlayServerTest extends HttpServerTest { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - if (endpoint == ERROR) { - "$Tags.ERROR" true - } else if (endpoint == EXCEPTION) { + if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } if (endpoint.query) { diff --git a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy index de7a5be00e..24fbcde452 100644 --- a/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.6/src/test/groovy/server/PlayServerTest.groovy @@ -102,9 +102,7 @@ class PlayServerTest extends HttpServerTest { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - if (endpoint == ERROR) { - "$Tags.ERROR" true - } else if (endpoint == EXCEPTION) { + if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } if (endpoint.query) { @@ -135,7 +133,6 @@ class PlayServerTest extends HttpServerTest { "$Tags.HTTP_URL" "${endpoint.resolve(address)}" "$Tags.HTTP_METHOD" method if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy index 7878d7ef7d..10c95e56ee 100644 --- a/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy +++ b/dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/server/RatpackHttpServerTest.groovy @@ -106,6 +106,12 @@ class RatpackHttpServerTest extends HttpServerTest { true } + @Override + boolean testNotFound() { + // resource name is set by instrumentation, so not changed to 404 + false + } + @Override void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { trace.span(index) { @@ -122,9 +128,7 @@ class RatpackHttpServerTest extends HttpServerTest { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - if (endpoint == ERROR) { - "$Tags.ERROR" true - } else if (endpoint == EXCEPTION) { + if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } if (endpoint.query) { diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/JettyServlet2Test.groovy b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/JettyServlet2Test.groovy index 98176efb93..68c1b0ee0c 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/JettyServlet2Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/JettyServlet2Test.groovy @@ -107,7 +107,6 @@ class JettyServlet2Test extends HttpServerTest { "servlet.path" endpoint.path "span.origin.type" TestServlet2.Sync.name if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy index fb32262b16..e37b41d98c 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/AbstractServlet3Test.groovy @@ -96,7 +96,6 @@ abstract class AbstractServlet3Test extends HttpServerTest "span.origin.type" ApplicationFilterChain.name "servlet.path" endpoint.path if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored "error.msg" { it == null || it == EXCEPTION.body } "error.type" { it == null || it == Exception.name } "error.stack" { it == null || it instanceof String } diff --git a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy index c31801fa24..e2249fe955 100644 --- a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy +++ b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy @@ -435,7 +435,6 @@ class TwilioClientTest extends AgentTestRunner { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - "$Tags.ERROR" true defaultTags() } } @@ -589,7 +588,6 @@ class TwilioClientTest extends AgentTestRunner { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - "$Tags.ERROR" true defaultTags() } } diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/asserts/TagsAssert.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/asserts/TagsAssert.groovy index c5cac37b1e..06faaac57b 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/asserts/TagsAssert.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/asserts/TagsAssert.groovy @@ -63,7 +63,6 @@ class TagsAssert { } def errorTags(Class errorType, message) { - tag("error", true) tag("error.type", errorType.name) tag("error.stack", String) diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 16990954b1..67d3fcd5b1 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -538,9 +538,6 @@ abstract class HttpServerTest extends AgentTestRunner { "$Tags.HTTP_URL" "${endpoint.resolve(address)}" "$Tags.HTTP_METHOD" method "$Tags.HTTP_STATUS" endpoint.status - if (endpoint.errored) { - "$Tags.ERROR" endpoint.errored - } if (endpoint.query) { "$DDTags.HTTP_QUERY" endpoint.query } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java index 18ea0febbf..3b0c8c56be 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java @@ -6,7 +6,7 @@ import io.opentracing.tag.Tags; public class ErrorFlag extends AbstractDecorator { public ErrorFlag() { super(); - this.setMatchingTag(Tags.ERROR.getKey()); + setMatchingTag(Tags.ERROR.getKey()); } @Override @@ -17,7 +17,6 @@ public class ErrorFlag extends AbstractDecorator { } catch (final Throwable t) { // DO NOTHING } - // TODO: Do we really want an error tag if the error flag is already set? - return true; + return false; } } From 2028b100098777cdabc883d14e21e05af655c9f8 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 17:23:49 -0700 Subject: [PATCH 03/37] Move ErrorFlag to TraceProcessor --- .../test/groovy/TraceAnnotationsTest.groovy | 7 ------ .../decorators/DDDecoratorsFactory.java | 1 - .../opentracing/decorators/ErrorFlag.java | 22 ----------------- .../common/processor/TraceProcessor.java | 3 ++- .../common/processor/rule/ErrorRule.java | 24 +++++++++++++++++++ 5 files changed, 26 insertions(+), 31 deletions(-) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java diff --git a/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy b/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy index a2ffb96c05..82f44651f3 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy +++ b/dd-java-agent/instrumentation/trace-annotation/src/test/groovy/TraceAnnotationsTest.groovy @@ -1,4 +1,3 @@ -import datadog.opentracing.decorators.ErrorFlag import datadog.trace.agent.test.AgentTestRunner import datadog.trace.agent.test.utils.ConfigUtils import datadog.trace.api.Trace @@ -243,9 +242,6 @@ class TraceAnnotationsTest extends AgentTestRunner { def "test exception exit"() { setup: - - TEST_TRACER.addDecorator(new ErrorFlag()) - Throwable error = null try { SayTracedHello.sayERROR() @@ -272,9 +268,6 @@ class TraceAnnotationsTest extends AgentTestRunner { def "test exception exit with resource name"() { setup: - - TEST_TRACER.addDecorator(new ErrorFlag()) - Throwable error = null try { SayTracedHello.sayERRORWithResource() diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index 4dbc49c41b..909d533de5 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -18,7 +18,6 @@ public class DDDecoratorsFactory { new AnalyticsSampleRateDecorator(), new DBStatementAsResourceName(), new DBTypeDecorator(), - new ErrorFlag(), new ForceManualDropDecorator(), new ForceManualKeepDecorator(), new OperationDecorator(), diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java deleted file mode 100644 index 3b0c8c56be..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ErrorFlag.java +++ /dev/null @@ -1,22 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import io.opentracing.tag.Tags; - -public class ErrorFlag extends AbstractDecorator { - public ErrorFlag() { - super(); - setMatchingTag(Tags.ERROR.getKey()); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - // Assign resource name - try { - context.setErrorFlag(Boolean.parseBoolean(String.valueOf(value))); - } catch (final Throwable t) { - // DO NOTHING - } - return false; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 8e1585fb93..4d47b7eeb2 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -2,6 +2,7 @@ package datadog.trace.common.processor; import datadog.opentracing.DDSpan; import datadog.trace.api.Config; +import datadog.trace.common.processor.rule.ErrorRule; import datadog.trace.common.processor.rule.Status404Rule; import datadog.trace.common.processor.rule.Status5XXRule; import datadog.trace.common.processor.rule.URLAsResourceNameRule; @@ -16,7 +17,7 @@ public class TraceProcessor { final Rule[] DEFAULT_RULES = new Rule[] { // Rules are applied in order. - new Status5XXRule(), new URLAsResourceNameRule(), new Status404Rule(), + new Status5XXRule(), new ErrorRule(), new URLAsResourceNameRule(), new Status404Rule(), }; private final List rules; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java new file mode 100644 index 0000000000..94f6ed5bb8 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java @@ -0,0 +1,24 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; +import io.opentracing.tag.Tags; +import java.util.Collection; +import java.util.Map; + +/** Converts error tag to error field */ +public class ErrorRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"ErrorFlag"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map meta, final Collection trace) { + if (meta.containsKey(Tags.ERROR.getKey())) { + span.setError(Boolean.parseBoolean(meta.get(Tags.ERROR.getKey()))); + span.setTag(Tags.ERROR, null); // Remove the tag + } + } +} From 329e79b8012b0c80e13fdf90d510d27cfcff1ad4 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 17:29:58 -0700 Subject: [PATCH 04/37] Move SpanTypeDecorator to SpanTypeRule --- .../decorators/DDDecoratorsFactory.java | 3 +-- .../decorators/SpanTypeDecorator.java | 18 -------------- .../common/processor/TraceProcessor.java | 7 +++++- .../common/processor/rule/ErrorRule.java | 2 +- .../common/processor/rule/SpanTypeRule.java | 24 +++++++++++++++++++ 5 files changed, 32 insertions(+), 22 deletions(-) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/SpanTypeDecorator.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index 909d533de5..3769d71395 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -25,8 +25,7 @@ public class DDDecoratorsFactory { new ResourceNameDecorator(), new ServiceNameDecorator(), new ServiceNameDecorator("service", false), - new ServletContextDecorator(), - new SpanTypeDecorator())) { + new ServletContextDecorator())) { if (Config.get().isDecoratorEnabled(decorator.getClass().getSimpleName())) { decorators.add(decorator); diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/SpanTypeDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/SpanTypeDecorator.java deleted file mode 100644 index dc3f5e1c04..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/SpanTypeDecorator.java +++ /dev/null @@ -1,18 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; - -public class SpanTypeDecorator extends AbstractDecorator { - - public SpanTypeDecorator() { - super(); - setMatchingTag(DDTags.SPAN_TYPE); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - context.setSpanType(String.valueOf(value)); - return false; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 4d47b7eeb2..08eb7a2c56 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -3,6 +3,7 @@ package datadog.trace.common.processor; import datadog.opentracing.DDSpan; import datadog.trace.api.Config; import datadog.trace.common.processor.rule.ErrorRule; +import datadog.trace.common.processor.rule.SpanTypeRule; import datadog.trace.common.processor.rule.Status404Rule; import datadog.trace.common.processor.rule.Status5XXRule; import datadog.trace.common.processor.rule.URLAsResourceNameRule; @@ -17,7 +18,11 @@ public class TraceProcessor { final Rule[] DEFAULT_RULES = new Rule[] { // Rules are applied in order. - new Status5XXRule(), new ErrorRule(), new URLAsResourceNameRule(), new Status404Rule(), + new SpanTypeRule(), + new Status5XXRule(), + new ErrorRule(), + new URLAsResourceNameRule(), + new Status404Rule(), }; private final List rules; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java index 94f6ed5bb8..8f153a52e5 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java @@ -6,7 +6,7 @@ import io.opentracing.tag.Tags; import java.util.Collection; import java.util.Map; -/** Converts error tag to error field */ +/** Converts error tag to field */ public class ErrorRule implements TraceProcessor.Rule { @Override public String[] aliases() { diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java new file mode 100644 index 0000000000..43dab1333e --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java @@ -0,0 +1,24 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.api.DDTags; +import datadog.trace.common.processor.TraceProcessor; +import java.util.Collection; +import java.util.Map; + +/** Converts span type tag to field */ +public class SpanTypeRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"SpanTypeDecorator"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map meta, final Collection trace) { + if (meta.containsKey(DDTags.SPAN_TYPE)) { + span.setSpanType(meta.get(DDTags.SPAN_TYPE)); + span.setTag(DDTags.SPAN_TYPE, (String) null); // Remove the tag + } + } +} From f0eb73ef12b8e9dcb3d4b524e07779c0a98d33ee Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 17:48:45 -0700 Subject: [PATCH 05/37] Use tags instead of meta which includes baggage. --- .../trace/common/processor/TraceProcessor.java | 6 +++--- .../trace/common/processor/rule/ErrorRule.java | 11 ++++++++--- .../common/processor/rule/SpanTypeRule.java | 6 +++--- .../common/processor/rule/Status404Rule.java | 4 ++-- .../common/processor/rule/Status5XXRule.java | 8 +++++--- .../processor/rule/URLAsResourceNameRule.java | 18 +++++++++--------- 6 files changed, 30 insertions(+), 23 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 08eb7a2c56..11a6fe08ad 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -51,7 +51,7 @@ public class TraceProcessor { public interface Rule { String[] aliases(); - void processSpan(DDSpan span, Map meta, Collection trace); + void processSpan(DDSpan span, Map tags, Collection trace); } public List onTraceComplete(final List trace) { @@ -64,9 +64,9 @@ public class TraceProcessor { } private void applyRules(final Collection trace, final DDSpan span) { - final Map meta = span.getMeta(); + final Map tags = span.getTags(); for (final Rule rule : rules) { - rule.processSpan(span, meta, trace); + rule.processSpan(span, tags, trace); } } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java index 8f153a52e5..49c0b3e112 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java @@ -15,9 +15,14 @@ public class ErrorRule implements TraceProcessor.Rule { @Override public void processSpan( - final DDSpan span, final Map meta, final Collection trace) { - if (meta.containsKey(Tags.ERROR.getKey())) { - span.setError(Boolean.parseBoolean(meta.get(Tags.ERROR.getKey()))); + final DDSpan span, final Map tags, final Collection trace) { + if (tags.containsKey(Tags.ERROR.getKey())) { + final Object value = tags.get(Tags.ERROR.getKey()); + if (value instanceof Boolean) { + span.setError((Boolean) value); + } else { + span.setError(Boolean.parseBoolean(value.toString())); + } span.setTag(Tags.ERROR, null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java index 43dab1333e..5b1f409f84 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java @@ -15,9 +15,9 @@ public class SpanTypeRule implements TraceProcessor.Rule { @Override public void processSpan( - final DDSpan span, final Map meta, final Collection trace) { - if (meta.containsKey(DDTags.SPAN_TYPE)) { - span.setSpanType(meta.get(DDTags.SPAN_TYPE)); + final DDSpan span, final Map tags, final Collection trace) { + if (tags.containsKey(DDTags.SPAN_TYPE)) { + span.setSpanType(tags.get(DDTags.SPAN_TYPE).toString()); span.setTag(DDTags.SPAN_TYPE, (String) null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java index 8cdb9758b9..5cb8102d49 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java @@ -15,8 +15,8 @@ public class Status404Rule implements TraceProcessor.Rule { @Override public void processSpan( - final DDSpan span, final Map meta, final Collection trace) { - if (!span.context().isResourceNameSet() && "404".equals(meta.get(Tags.HTTP_STATUS.getKey()))) { + final DDSpan span, final Map tags, final Collection trace) { + if (!span.context().isResourceNameSet() && "404".equals(tags.get(Tags.HTTP_STATUS.getKey()))) { span.setResourceName("404"); } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java index 6b6238551f..107a58b2c9 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java @@ -15,9 +15,11 @@ public class Status5XXRule implements TraceProcessor.Rule { @Override public void processSpan( - final DDSpan span, final Map meta, final Collection trace) { - if (!span.context().getErrorFlag() && meta.containsKey(Tags.HTTP_STATUS.getKey())) { - final int responseCode = Integer.parseInt(meta.get(Tags.HTTP_STATUS.getKey())); + final DDSpan span, final Map tags, final Collection trace) { + if (!span.context().getErrorFlag() && tags.containsKey(Tags.HTTP_STATUS.getKey())) { + final Object value = tags.get(Tags.HTTP_STATUS.getKey()); + final int responseCode = + value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); span.setError(500 <= responseCode && responseCode < 600); } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java index 32280c351e..6e2fb4f734 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java @@ -21,17 +21,17 @@ public class URLAsResourceNameRule implements TraceProcessor.Rule { @Override public void processSpan( - final DDSpan span, final Map meta, final Collection trace) { + final DDSpan span, final Map tags, final Collection trace) { final DDSpanContext context = span.context(); if (context.isResourceNameSet() - || meta.get(Tags.HTTP_URL.getKey()) == null - || "404".equals(meta.get(Tags.HTTP_STATUS.getKey()))) { + || tags.get(Tags.HTTP_URL.getKey()) == null + || "404".equals(tags.get(Tags.HTTP_STATUS.getKey()))) { return; } - final String rawPath = rawPathFromUrlString(meta.get(Tags.HTTP_URL.getKey()).trim()); + final String rawPath = rawPathFromUrlString(tags.get(Tags.HTTP_URL.getKey()).toString().trim()); final String normalizedPath = normalizePath(rawPath); - final String resourceName = addMethodIfAvailable(meta, normalizedPath); + final String resourceName = addMethodIfAvailable(tags, normalizedPath); context.setResourceName(resourceName); } @@ -87,11 +87,11 @@ public class URLAsResourceNameRule implements TraceProcessor.Rule { return PATH_MIXED_ALPHANUMERICS.matcher(path).replaceAll("?"); } - private String addMethodIfAvailable(final Map meta, String path) { + private String addMethodIfAvailable(final Map meta, String path) { // if the verb (GET, POST ...) is present, add it - final String verb = meta.get(Tags.HTTP_METHOD.getKey()); - if (verb != null && !verb.isEmpty()) { - path = verb.toUpperCase() + " " + path; + final Object verb = meta.get(Tags.HTTP_METHOD.getKey()); + if (verb != null && !verb.toString().isEmpty()) { + path = verb.toString().toUpperCase() + " " + path; } return path; } From c288f69e645712d14267575367c94ec21f7e2026 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 17:49:17 -0700 Subject: [PATCH 06/37] Move ResourceNameDecorator to ResourceNameRule --- .../decorators/DDDecoratorsFactory.java | 1 - .../decorators/ResourceNameDecorator.java | 18 -------------- .../common/processor/TraceProcessor.java | 2 ++ .../processor/rule/ResourceNameRule.java | 24 +++++++++++++++++++ 4 files changed, 26 insertions(+), 19 deletions(-) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/ResourceNameDecorator.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index 3769d71395..89016ac0e6 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -22,7 +22,6 @@ public class DDDecoratorsFactory { new ForceManualKeepDecorator(), new OperationDecorator(), new PeerServiceDecorator(), - new ResourceNameDecorator(), new ServiceNameDecorator(), new ServiceNameDecorator("service", false), new ServletContextDecorator())) { diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ResourceNameDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ResourceNameDecorator.java deleted file mode 100644 index e42fd1bf6b..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/ResourceNameDecorator.java +++ /dev/null @@ -1,18 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; - -public class ResourceNameDecorator extends AbstractDecorator { - - public ResourceNameDecorator() { - super(); - this.setMatchingTag(DDTags.RESOURCE_NAME); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - context.setResourceName(String.valueOf(value)); - return false; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 11a6fe08ad..dea32a9406 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -3,6 +3,7 @@ package datadog.trace.common.processor; import datadog.opentracing.DDSpan; import datadog.trace.api.Config; import datadog.trace.common.processor.rule.ErrorRule; +import datadog.trace.common.processor.rule.ResourceNameRule; import datadog.trace.common.processor.rule.SpanTypeRule; import datadog.trace.common.processor.rule.Status404Rule; import datadog.trace.common.processor.rule.Status5XXRule; @@ -18,6 +19,7 @@ public class TraceProcessor { final Rule[] DEFAULT_RULES = new Rule[] { // Rules are applied in order. + new ResourceNameRule(), new SpanTypeRule(), new Status5XXRule(), new ErrorRule(), diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java new file mode 100644 index 0000000000..0a28ce8ac6 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java @@ -0,0 +1,24 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.api.DDTags; +import datadog.trace.common.processor.TraceProcessor; +import java.util.Collection; +import java.util.Map; + +/** Converts resource name tag to field */ +public class ResourceNameRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"ResourceNameDecorator"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map tags, final Collection trace) { + if (tags.containsKey(DDTags.RESOURCE_NAME)) { + span.setResourceName(tags.get(DDTags.RESOURCE_NAME).toString()); + span.setTag(DDTags.RESOURCE_NAME, (String) null); // Remove the tag + } + } +} From 7650efe5ed8c4d332df621f0b1bae8017d47acc1 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 17:55:24 -0700 Subject: [PATCH 07/37] Move AnalyticsSampleRateDecorator to AnalyticsSampleRateRule --- .../AnalyticsSampleRateDecorator.java | 25 ----------- .../decorators/DDDecoratorsFactory.java | 1 - .../common/processor/TraceProcessor.java | 1 + .../rule/AnalyticsSampleRateRule.java | 34 +++++++++++++++ .../decorators/SpanDecoratorTest.groovy | 41 +++++++++++-------- 5 files changed, 59 insertions(+), 43 deletions(-) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/AnalyticsSampleRateDecorator.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/AnalyticsSampleRateDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/AnalyticsSampleRateDecorator.java deleted file mode 100644 index b5a1dd4763..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/AnalyticsSampleRateDecorator.java +++ /dev/null @@ -1,25 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; - -public class AnalyticsSampleRateDecorator extends AbstractDecorator { - public AnalyticsSampleRateDecorator() { - super(); - setMatchingTag(DDTags.ANALYTICS_SAMPLE_RATE); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - if (value instanceof Number) { - context.setMetric(DDTags.ANALYTICS_SAMPLE_RATE, (Number) value); - } else if (value instanceof String) { - try { - context.setMetric(DDTags.ANALYTICS_SAMPLE_RATE, Double.parseDouble((String) value)); - } catch (final NumberFormatException ex) { - // ignore - } - } - return false; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index 89016ac0e6..f49356d306 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -15,7 +15,6 @@ public class DDDecoratorsFactory { for (final AbstractDecorator decorator : Arrays.asList( - new AnalyticsSampleRateDecorator(), new DBStatementAsResourceName(), new DBTypeDecorator(), new ForceManualDropDecorator(), diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index dea32a9406..56452db0e1 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -25,6 +25,7 @@ public class TraceProcessor { new ErrorRule(), new URLAsResourceNameRule(), new Status404Rule(), + new AnalyticsSampleRateRule(), }; private final List rules; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java new file mode 100644 index 0000000000..ede6778a48 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java @@ -0,0 +1,34 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.api.DDTags; +import datadog.trace.common.processor.TraceProcessor; +import java.util.Collection; +import java.util.Map; + +/** Converts analytics sample rate tag to metric */ +public class AnalyticsSampleRateRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"AnalyticsSampleRateDecorator"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map tags, final Collection trace) { + if (tags.containsKey(DDTags.ANALYTICS_SAMPLE_RATE)) { + final Object value = tags.get(DDTags.ANALYTICS_SAMPLE_RATE); + if (value instanceof Number) { + span.context().setMetric(DDTags.ANALYTICS_SAMPLE_RATE, (Number) value); + } else if (value instanceof String) { + try { + span.context() + .setMetric(DDTags.ANALYTICS_SAMPLE_RATE, Double.parseDouble((String) value)); + } catch (final NumberFormatException ex) { + // ignore + } + } + span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, (String) null); // Remove the tag + } + } +} diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index 7a5f7b1453..56be4a6b8e 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -238,6 +238,7 @@ class SpanDecoratorTest extends DDSpecification { def "set resource name"() { when: span.setTag(DDTags.RESOURCE_NAME, name) + span.finish() then: span.getResourceName() == name @@ -249,6 +250,7 @@ class SpanDecoratorTest extends DDSpecification { def "set span type"() { when: span.setTag(DDTags.SPAN_TYPE, type) + span.finish() then: span.getSpanType() == type @@ -285,26 +287,26 @@ class SpanDecoratorTest extends DDSpecification { span.setTag(ANALYTICS_SAMPLE_RATE, rate) then: - span.metrics == result + span.metrics.get(ANALYTICS_SAMPLE_RATE) == result where: rate | result - 00 | [(ANALYTICS_SAMPLE_RATE): 0] - 1 | [(ANALYTICS_SAMPLE_RATE): 1] - 0f | [(ANALYTICS_SAMPLE_RATE): 0] - 1f | [(ANALYTICS_SAMPLE_RATE): 1] - 0.1 | [(ANALYTICS_SAMPLE_RATE): 0.1] - 1.1 | [(ANALYTICS_SAMPLE_RATE): 1.1] - -1 | [(ANALYTICS_SAMPLE_RATE): -1] - 10 | [(ANALYTICS_SAMPLE_RATE): 10] - "00" | [(ANALYTICS_SAMPLE_RATE): 0] - "1" | [(ANALYTICS_SAMPLE_RATE): 1] - "1.0" | [(ANALYTICS_SAMPLE_RATE): 1] - "0" | [(ANALYTICS_SAMPLE_RATE): 0] - "0.1" | [(ANALYTICS_SAMPLE_RATE): 0.1] - "1.1" | [(ANALYTICS_SAMPLE_RATE): 1.1] - "-1" | [(ANALYTICS_SAMPLE_RATE): -1] - "str" | [:] + 00 | 0 + 1 | 1 + 0f | 0 + 1f | 1 + 0.1 | 0.1 + 1.1 | 1.1 + -1 | -1 + 10 | 10 + "00" | 0 + "1" | 1 + "1.0" | 1 + "0" | 0 + "0.1" | 0.1 + "1.1" | 1.1 + "-1" | -1 + "str" | null } def "set priority sampling via tag"() { @@ -354,6 +356,7 @@ class SpanDecoratorTest extends DDSpecification { def "set error flag when error tag reported"() { when: Tags.ERROR.set(span, error) + span.finish() then: span.isError() == error @@ -367,6 +370,7 @@ class SpanDecoratorTest extends DDSpecification { def "#attribute decorators apply to builder too"() { setup: def span = tracer.buildSpan("decorator.test").withTag(name, value).start() + span.finish() expect: span.context()."$attribute" == value @@ -381,6 +385,7 @@ class SpanDecoratorTest extends DDSpecification { def "decorators apply to builder too"() { when: def span = tracer.buildSpan("decorator.test").withTag("sn.tag1", "some val").start() + span.finish() then: span.serviceName == "some val" @@ -393,12 +398,14 @@ class SpanDecoratorTest extends DDSpecification { when: span = tracer.buildSpan("decorator.test").withTag("error", "true").start() + span.finish() then: span.error when: span = tracer.buildSpan("decorator.test").withTag(Tags.DB_STATEMENT.key, "some-statement").start() + span.finish() then: span.resourceName == "some-statement" From 7614fa42e0fab5aecf9f7af9a740e30b265bca3e Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 19 Mar 2020 19:24:01 -0700 Subject: [PATCH 08/37] Move DBStatementAsResourceName to DBStatementRule --- .../decorators/DBStatementAsResourceName.java | 37 ------------------- .../decorators/DDDecoratorsFactory.java | 1 - .../common/processor/TraceProcessor.java | 3 ++ .../processor/rule/DBStatementRule.java | 37 +++++++++++++++++++ .../decorators/SpanDecoratorTest.groovy | 22 ++++------- 5 files changed, 48 insertions(+), 52 deletions(-) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBStatementAsResourceName.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBStatementAsResourceName.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBStatementAsResourceName.java deleted file mode 100644 index d3b23bd548..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DBStatementAsResourceName.java +++ /dev/null @@ -1,37 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import datadog.trace.api.DDTags; -import io.opentracing.tag.Tags; - -public class DBStatementAsResourceName extends AbstractDecorator { - - public DBStatementAsResourceName() { - super(); - this.setMatchingTag(Tags.DB_STATEMENT.getKey()); - this.setReplacementTag(DDTags.RESOURCE_NAME); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - - // Special case: Mongo - // Skip the decorators - if (context.getTags().containsKey(Tags.COMPONENT.getKey()) - && "java-mongo".equals(context.getTags().get(Tags.COMPONENT.getKey()))) { - return true; - } - - // Assign service name - if (!super.shouldSetTag(context, tag, value)) { - // TODO: remove properly the tag (immutable at this time) - // the `db.statement` tag must be removed because it will be set - // by the Datadog Trace Agent as `sql.query`; here we're removing - // a duplicate that will not be obfuscated with the current Datadog - // Trace Agent version. - context.setTag(Tags.DB_STATEMENT.getKey(), null); - return false; - } - return true; - } -} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index f49356d306..ce6734cdf7 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -15,7 +15,6 @@ public class DDDecoratorsFactory { for (final AbstractDecorator decorator : Arrays.asList( - new DBStatementAsResourceName(), new DBTypeDecorator(), new ForceManualDropDecorator(), new ForceManualKeepDecorator(), diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 56452db0e1..3981ddd4ee 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -2,6 +2,8 @@ package datadog.trace.common.processor; import datadog.opentracing.DDSpan; import datadog.trace.api.Config; +import datadog.trace.common.processor.rule.AnalyticsSampleRateRule; +import datadog.trace.common.processor.rule.DBStatementRule; import datadog.trace.common.processor.rule.ErrorRule; import datadog.trace.common.processor.rule.ResourceNameRule; import datadog.trace.common.processor.rule.SpanTypeRule; @@ -19,6 +21,7 @@ public class TraceProcessor { final Rule[] DEFAULT_RULES = new Rule[] { // Rules are applied in order. + new DBStatementRule(), new ResourceNameRule(), new SpanTypeRule(), new Status5XXRule(), diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java new file mode 100644 index 0000000000..b4d751591d --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java @@ -0,0 +1,37 @@ +package datadog.trace.common.processor.rule; + +import datadog.opentracing.DDSpan; +import datadog.trace.common.processor.TraceProcessor; +import io.opentracing.tag.Tags; +import java.util.Collection; +import java.util.Map; + +/** + * Converts db.statement tag to resource name. This is later set to sql.query by the datadog agent + * after obfuscation. + */ +public class DBStatementRule implements TraceProcessor.Rule { + @Override + public String[] aliases() { + return new String[] {"DBStatementAsResourceName"}; + } + + @Override + public void processSpan( + final DDSpan span, final Map tags, final Collection trace) { + if (tags.containsKey(Tags.DB_STATEMENT.getKey())) { + // Special case: Mongo + // Skip the decorators + if (tags.containsKey(Tags.COMPONENT.getKey()) + && "java-mongo".equals(tags.get(Tags.COMPONENT.getKey()))) { + return; + } + + final String statement = tags.get(Tags.DB_STATEMENT.getKey()).toString(); + if (!statement.isEmpty()) { + span.setResourceName(statement); + } + span.setTag(Tags.DB_STATEMENT.getKey(), (String) null); // Remove the tag + } + } +} diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index 56be4a6b8e..c3a2871a1f 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -333,24 +333,18 @@ class SpanDecoratorTest extends DDSpecification { def "DBStatementAsResource should not interact on Mongo queries"() { when: - span.setResourceName("not-change-me") - Tags.COMPONENT.set(span, "java-mongo") - Tags.DB_STATEMENT.set(span, something) + span.setResourceName("existing") + Tags.COMPONENT.set(span, component) + Tags.DB_STATEMENT.set(span, statement) + span.finish() then: - span.getResourceName() == "not-change-me" - - - when: - span.setResourceName("change-me") - Tags.COMPONENT.set(span, "other-contrib") - Tags.DB_STATEMENT.set(span, something) - - then: - span.getResourceName() == something + span.getResourceName() == resource where: - something = "fake-query" + component | statement | resource + "java-mongo" | "some-query" | "existing" + "other" | "some-query" | "some-query" } def "set error flag when error tag reported"() { From 8170f7bc7b0d1392a0155fdd7df04a094a556387 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 20 Mar 2020 10:01:11 -0700 Subject: [PATCH 09/37] Various fixes --- .../common/processor/rule/Status404Rule.java | 5 ++++- .../processor/rule/URLAsResourceNameRule.java | 3 ++- .../datadog/trace/DDSpanContextTest.groovy | 4 +++- .../trace/api/writer/DDAgentApiTest.groovy | 22 +++++++++++++++---- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java index 5cb8102d49..7c838e549d 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status404Rule.java @@ -16,7 +16,10 @@ public class Status404Rule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { - if (!span.context().isResourceNameSet() && "404".equals(tags.get(Tags.HTTP_STATUS.getKey()))) { + final Object httpStatus = tags.get(Tags.HTTP_STATUS.getKey()); + if (!span.context().isResourceNameSet() + && httpStatus != null + && (httpStatus.equals(404) || httpStatus.equals("404"))) { span.setResourceName("404"); } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java index 6e2fb4f734..6b433e24dc 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java @@ -23,9 +23,10 @@ public class URLAsResourceNameRule implements TraceProcessor.Rule { public void processSpan( final DDSpan span, final Map tags, final Collection trace) { final DDSpanContext context = span.context(); + final Object httpStatus = tags.get(Tags.HTTP_STATUS.getKey()); if (context.isResourceNameSet() || tags.get(Tags.HTTP_URL.getKey()) == null - || "404".equals(tags.get(Tags.HTTP_STATUS.getKey()))) { + || (httpStatus != null && (httpStatus.equals(404) || httpStatus.equals("404")))) { return; } diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy index ce2b423829..0b9d6b7076 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy @@ -30,8 +30,10 @@ class DDSpanContextTest extends DDSpecification { def "special tags set certain values"() { setup: - def context = SpanFactory.newSpanOf(0).context + def span = SpanFactory.newSpanOf(0) + def context = span.context context.setTag(name, value) + span.finish() def thread = Thread.currentThread() def expectedTags = [(DDTags.THREAD_NAME): thread.name, (DDTags.THREAD_ID): thread.id] diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy index 189dca2664..289cc59930 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy @@ -3,6 +3,7 @@ package datadog.trace.api.writer import com.fasterxml.jackson.core.type.TypeReference import com.fasterxml.jackson.databind.ObjectMapper import datadog.opentracing.SpanFactory +import datadog.trace.common.sampling.RateByServiceSampler import datadog.trace.common.writer.ddagent.DDAgentApi import datadog.trace.common.writer.ddagent.DDAgentResponseListener import datadog.trace.util.test.DDSpecification @@ -98,10 +99,13 @@ class DDAgentApiTest extends DDSpecification { traces | expectedRequestBody [] | [] [[SpanFactory.newSpanOf(1L).setTag("service.name", "my-service")]] | [[new TreeMap<>([ - "duration" : 0, + "duration" : 10, "error" : 0, "meta" : ["thread.name": Thread.currentThread().getName(), "thread.id": "${Thread.currentThread().id}"], - "metrics" : [:], + "metrics" : [ + (RateByServiceSampler.SAMPLING_AGENT_RATE): 1.0, + (DDSpanContext.PRIORITY_SAMPLING_KEY) : 1, + ], "name" : "fakeOperation", "parent_id": 0, "resource" : "fakeResource", @@ -112,10 +116,13 @@ class DDAgentApiTest extends DDSpecification { "type" : "fakeType" ])]] [[SpanFactory.newSpanOf(100L).setTag("resource.name", "my-resource")]] | [[new TreeMap<>([ - "duration" : 0, + "duration" : 10, "error" : 0, "meta" : ["thread.name": Thread.currentThread().getName(), "thread.id": "${Thread.currentThread().id}"], - "metrics" : [:], + "metrics" : [ + (RateByServiceSampler.SAMPLING_AGENT_RATE): 1.0, + (DDSpanContext.PRIORITY_SAMPLING_KEY) : 1, + ], "name" : "fakeOperation", "parent_id": 0, "resource" : "my-resource", @@ -125,6 +132,13 @@ class DDAgentApiTest extends DDSpecification { "trace_id" : 1, "type" : "fakeType" ])]] + + ignore = traces.each { + it.each { + it.finish() + it.@durationNano.set(10) + } + } } def "Api ResponseListeners see 200 responses"() { From df97f90fec692dd710312e6e5954b8cd855c14d9 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 20 Mar 2020 10:32:20 -0700 Subject: [PATCH 10/37] Remove OperationDecorator --- .../aws/v0/TracingRequestHandler.java | 2 +- ...lientTest.groovy => AWS1ClientTest.groovy} | 2 +- ...lientTest.groovy => AWS0ClientTest.groovy} | 2 +- .../aws/v2/TracingExecutionInterceptor.java | 2 +- ...lientTest.groovy => Aws2ClientTest.groovy} | 2 +- .../decorators/DDDecoratorsFactory.java | 1 - .../decorators/OperationDecorator.java | 39 ------------------- .../decorators/SpanDecoratorTest.groovy | 12 ------ 8 files changed, 5 insertions(+), 57 deletions(-) rename dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/{AWSClientTest.groovy => AWS1ClientTest.groovy} (99%) rename dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/{AWSClientTest.groovy => AWS0ClientTest.groovy} (99%) rename dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/{AwsClientTest.groovy => Aws2ClientTest.groovy} (99%) delete mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java index bdb4ca44d6..5c8f208d31 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/TracingRequestHandler.java @@ -29,7 +29,7 @@ public class TracingRequestHandler extends RequestHandler2 { @Override public void beforeRequest(final Request request) { - final AgentSpan span = startSpan("aws.command"); + final AgentSpan span = startSpan("aws.http"); decorate.afterStart(span); decorate.onRequest(span, request); request.addHandlerContext(SCOPE_CONTEXT_KEY, activateSpan(span, true)); diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWSClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy similarity index 99% rename from dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWSClientTest.groovy rename to dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy index 0e66a21c8e..f332a51094 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWSClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWS1ClientTest.groovy @@ -42,7 +42,7 @@ import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan -class AWSClientTest extends AgentTestRunner { +class AWS1ClientTest extends AgentTestRunner { private static final CREDENTIALS_PROVIDER_CHAIN = new AWSCredentialsProviderChain( new EnvironmentVariableCredentialsProvider(), diff --git a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWSClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy similarity index 99% rename from dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWSClientTest.groovy rename to dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy index 6c993e8f1b..fad93941e7 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWSClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWS0ClientTest.groovy @@ -29,7 +29,7 @@ import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan -class AWSClientTest extends AgentTestRunner { +class AWS0ClientTest extends AgentTestRunner { private static final CREDENTIALS_PROVIDER_CHAIN = new AWSCredentialsProviderChain( new EnvironmentVariableCredentialsProvider(), diff --git a/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java8/datadog/trace/instrumentation/aws/v2/TracingExecutionInterceptor.java b/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java8/datadog/trace/instrumentation/aws/v2/TracingExecutionInterceptor.java index 958e579513..de39ef7318 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java8/datadog/trace/instrumentation/aws/v2/TracingExecutionInterceptor.java +++ b/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java8/datadog/trace/instrumentation/aws/v2/TracingExecutionInterceptor.java @@ -29,7 +29,7 @@ public class TracingExecutionInterceptor implements ExecutionInterceptor { @Override public void beforeExecution( final Context.BeforeExecution context, final ExecutionAttributes executionAttributes) { - final AgentSpan span = startSpan("aws.command"); + final AgentSpan span = startSpan("aws.http"); try (final AgentScope scope = activateSpan(span, false)) { DECORATE.afterStart(span); executionAttributes.putAttribute(SPAN_ATTRIBUTE, span); diff --git a/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/AwsClientTest.groovy b/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy similarity index 99% rename from dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/AwsClientTest.groovy rename to dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy index da8f200193..0a8bccef6f 100644 --- a/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/AwsClientTest.groovy +++ b/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/Aws2ClientTest.groovy @@ -35,7 +35,7 @@ import java.util.concurrent.atomic.AtomicReference import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer -class AwsClientTest extends AgentTestRunner { +class Aws2ClientTest extends AgentTestRunner { private static final StaticCredentialsProvider CREDENTIALS_PROVIDER = StaticCredentialsProvider .create(AwsBasicCredentials.create("my-access-key", "my-secret-key")) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index ce6734cdf7..bc7c673876 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -18,7 +18,6 @@ public class DDDecoratorsFactory { new DBTypeDecorator(), new ForceManualDropDecorator(), new ForceManualKeepDecorator(), - new OperationDecorator(), new PeerServiceDecorator(), new ServiceNameDecorator(), new ServiceNameDecorator("service", false), diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java deleted file mode 100644 index 9c5ad1bea1..0000000000 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/OperationDecorator.java +++ /dev/null @@ -1,39 +0,0 @@ -package datadog.opentracing.decorators; - -import datadog.opentracing.DDSpanContext; -import io.opentracing.tag.Tags; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; - -/** - * This span decorator is a simple mapping to override the operation DB tags. The operation name of - * DB decorators are handled by the DBTypeDecorator - */ -public class OperationDecorator extends AbstractDecorator { - - static final Map MAPPINGS; - - static { - final Map mappings = new HashMap<>(); - // Component name <> Operation name - mappings.put("java-aws-sdk", "aws.http"); - // FIXME: JMS ops card is low (jms-send or jms-receive), may be this mapping is useless - mappings.put("java-jms", "jms"); - // Cassandra, Mongo, JDBC are set via DBTypeDecorator - MAPPINGS = Collections.unmodifiableMap(mappings); - } - - public OperationDecorator() { - super(); - setMatchingTag(Tags.COMPONENT.getKey()); - } - - @Override - public boolean shouldSetTag(final DDSpanContext context, final String tag, final Object value) { - if (MAPPINGS.containsKey(String.valueOf(value))) { - context.setOperationName(MAPPINGS.get(String.valueOf(value))); - } - return true; - } -} diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index c3a2871a1f..012a92dbe8 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -223,18 +223,6 @@ class SpanDecoratorTest extends DDSpecification { span.serviceName == "peer-service" } - def "set operation name"() { - when: - Tags.COMPONENT.set(span, component) - - then: - span.getOperationName() == operationName - - where: - component << OperationDecorator.MAPPINGS.keySet() - operationName << OperationDecorator.MAPPINGS.values() - } - def "set resource name"() { when: span.setTag(DDTags.RESOURCE_NAME, name) From 1fb844ab5fe930ab66731bbc3b30afa62fbb99f7 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 15:57:01 -0400 Subject: [PATCH 11/37] Make `CommonTaskExecutor` periodic tasks safe * Verify that we can schedule task and catch exceptions. This should help to avoid additional exceptions on app crash during startup. * Avoid holding strong references from within executor to make sure that things can get GCed. --- .../datadog/trace/agent/tooling/Cleaner.java | 56 +++----- .../datadog/opentracing/PendingTrace.java | 22 +++- .../writer/ddagent/BatchWritingDisruptor.java | 28 ++-- .../common/exec/CommonTaskExecutor.java | 123 +++++++++++++++++- 4 files changed, 174 insertions(+), 55 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java index d4c0807823..f55ce65239 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java @@ -1,9 +1,7 @@ package datadog.trace.agent.tooling; import datadog.common.exec.CommonTaskExecutor; -import java.lang.ref.WeakReference; -import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.ScheduledFuture; +import datadog.common.exec.CommonTaskExecutor.Task; import java.util.concurrent.TimeUnit; import lombok.extern.slf4j.Slf4j; @@ -12,47 +10,25 @@ class Cleaner { void scheduleCleaning( final T target, final Adapter adapter, final long frequency, final TimeUnit unit) { - final CleanupRunnable command = new CleanupRunnable<>(target, adapter); - if (CommonTaskExecutor.INSTANCE.isShutdown()) { - log.warn( - "Cleaning scheduled but task scheduler is shutdown. Target won't be cleaned {}", target); - } else { - try { - // Schedule job and save future to allow job to be canceled if target is GC'd. - command.setFuture( - CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(command, frequency, frequency, unit)); - } catch (final RejectedExecutionException e) { - log.warn("Cleaning task rejected. Target won't be cleaned {}", target); - } + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( + new CleaningTask(adapter), target, frequency, frequency, unit, "cleaner for " + target); + } + + // Important to use explicit class to avoid implicit hard references to target + private static class CleaningTask implements Task { + private final Adapter adapter; + + public CleaningTask(final Adapter adapter) { + this.adapter = adapter; + } + + @Override + public void run(final T target) { + adapter.clean(target); } } public interface Adapter { void clean(T target); } - - private static class CleanupRunnable implements Runnable { - private final WeakReference target; - private final Adapter adapter; - private volatile ScheduledFuture future = null; - - private CleanupRunnable(final T target, final Adapter adapter) { - this.target = new WeakReference<>(target); - this.adapter = adapter; - } - - @Override - public void run() { - final T t = target.get(); - if (t != null) { - adapter.clean(t); - } else if (future != null) { - future.cancel(false); - } - } - - public void setFuture(final ScheduledFuture future) { - this.future = future; - } - } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java index b8317b0afe..2707e979e3 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java @@ -1,6 +1,7 @@ package datadog.opentracing; import datadog.common.exec.CommonTaskExecutor; +import datadog.common.exec.CommonTaskExecutor.Task; import datadog.opentracing.scopemanager.ContinuableScope; import datadog.trace.common.util.Clock; import java.io.Closeable; @@ -296,7 +297,13 @@ public class PendingTrace extends ConcurrentLinkedDeque { Collections.newSetFromMap(new ConcurrentHashMap()); public SpanCleaner() { - CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(this, 0, CLEAN_FREQUENCY, TimeUnit.SECONDS); + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( + SpanCleanerTask.INSTANCE, + this, + 0, + CLEAN_FREQUENCY, + TimeUnit.SECONDS, + "Pending trace cleaner"); } @Override @@ -312,4 +319,17 @@ public class PendingTrace extends ConcurrentLinkedDeque { run(); } } + + /* + * Important to use explicit class to avoid implicit hard references to cleaners from within executor. + */ + private static class SpanCleanerTask implements Task { + + static final SpanCleanerTask INSTANCE = new SpanCleanerTask(); + + @Override + public void run(final SpanCleaner target) { + target.run(); + } + } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java index abd3e30176..6de260cc70 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java @@ -2,6 +2,7 @@ package datadog.trace.common.writer.ddagent; import com.lmax.disruptor.EventHandler; import datadog.common.exec.CommonTaskExecutor; +import datadog.common.exec.CommonTaskExecutor.Task; import datadog.common.exec.DaemonThreadFactory; import datadog.trace.common.writer.DDAgentWriter; import java.util.ArrayList; @@ -32,17 +33,8 @@ public class BatchWritingDisruptor extends AbstractDisruptor { if (0 < flushFrequencySeconds) { // This provides a steady stream of events to enable flushing with a low throughput. - final Runnable heartbeat = - new Runnable() { - @Override - public void run() { - // Only add if the buffer is empty. - if (running && getCurrentCount() == 0) { - disruptor.getRingBuffer().tryPublishEvent(heartbeatTranslator); - } - } - }; - CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(heartbeat, 100, 100, TimeUnit.MILLISECONDS); + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( + new HeartbeatTask(), this, 100, 100, TimeUnit.MILLISECONDS, "disruptor heartbeat"); } } @@ -58,6 +50,12 @@ public class BatchWritingDisruptor extends AbstractDisruptor { return true; } + private void heartbeat() { + if (running && getCurrentCount() == 0) { + disruptor.getRingBuffer().tryPublishEvent(heartbeatTranslator); + } + } + // Intentionally not thread safe. private static class BatchWritingHandler implements EventHandler> { @@ -162,4 +160,12 @@ public class BatchWritingDisruptor extends AbstractDisruptor { } } } + + // Important to use explicit class to avoid implicit hard references to BatchWritingDisruptor + private static final class HeartbeatTask implements Task { + @Override + public void run(final BatchWritingDisruptor target) { + target.heartbeat(); + } + } } diff --git a/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java b/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java index a2f1d57960..957f5455c1 100644 --- a/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java +++ b/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java @@ -1,8 +1,11 @@ package datadog.common.exec; +import java.lang.ref.WeakReference; import java.util.List; import java.util.concurrent.AbstractExecutorService; +import java.util.concurrent.Delayed; import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -25,9 +28,48 @@ public final class CommonTaskExecutor extends AbstractExecutorService { } } - public ScheduledFuture scheduleAtFixedRate( - final Runnable command, final long initialDelay, final long period, final TimeUnit unit) { - return executorService.scheduleAtFixedRate(command, initialDelay, period, unit); + /** + * Run {@code task} periodically providing it with {@code target} + * + *

Important implementation detail here is that internally we do not hold any strong references + * to {@code target} which means it can be GCed even while periodic task is still scheduled. + * + *

If {@code target} is GCed periodic task is canceled. + * + * @param task task to run. Important: must not hold any strong references to target (or anything + * else non static) + * @param target target object to pass to task + * @param initialDelay initialDelay, see {@link + * ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, long, TimeUnit)} + * @param period period, see {@link ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, + * long, TimeUnit)} + * @param unit unit, see {@link ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, long, + * TimeUnit)} + * @param name name to use in logs when task cannot be scheduled + * @return future that can be canceled + */ + public ScheduledFuture scheduleAtFixedRate( + final Task task, + final T target, + final long initialDelay, + final long period, + final TimeUnit unit, + final String name) { + if (CommonTaskExecutor.INSTANCE.isShutdown()) { + log.warn("Periodic task scheduler is shutdown. Will not run: {}", name); + } else { + try { + final PeriodicTask periodicTask = new PeriodicTask<>(task, target); + final ScheduledFuture future = + executorService.scheduleAtFixedRate( + new PeriodicTask<>(task, target), initialDelay, period, unit); + periodicTask.setFuture(future); + return future; + } catch (final RejectedExecutionException e) { + log.warn("Cleaning task rejected. Will not run: {}", name); + } + } + return new UnscheduledFuture(name); } @Override @@ -82,4 +124,79 @@ public final class CommonTaskExecutor extends AbstractExecutorService { } } } + + public interface Task { + void run(T target); + } + + public static class PeriodicTask implements Runnable { + private final WeakReference target; + private final Task task; + private volatile ScheduledFuture future = null; + + private PeriodicTask(final Task task, final T target) { + this.target = new WeakReference<>(target); + this.task = task; + } + + @Override + public void run() { + final T t = target.get(); + if (t != null) { + task.run(t); + } else if (future != null) { + future.cancel(false); + } + } + + public void setFuture(final ScheduledFuture future) { + this.future = future; + } + } + + // Unscheduled future + @Slf4j + public static class UnscheduledFuture implements ScheduledFuture { + private final String name; + + public UnscheduledFuture(final String name) { + this.name = name; + } + + @Override + public long getDelay(final TimeUnit unit) { + return 0; + } + + @Override + public int compareTo(final Delayed o) { + return 0; + } + + @Override + public boolean cancel(final boolean mayInterruptIfRunning) { + log.debug("Cancelling future for: {}", name); + return false; + } + + @Override + public boolean isCancelled() { + return false; + } + + @Override + public boolean isDone() { + return false; + } + + @Override + public Object get() { + return null; + } + + @Override + public Object get(final long timeout, final TimeUnit unit) { + return null; + } + } } From d86093c4741d819c86a8f94880de380721e2a994 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 17:05:30 -0400 Subject: [PATCH 12/37] Minor CR fixes and some comments --- .../common/exec/CommonTaskExecutor.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java b/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java index 957f5455c1..8094501ba6 100644 --- a/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java +++ b/utils/thread-utils/src/main/java/datadog/common/exec/CommonTaskExecutor.java @@ -36,6 +36,10 @@ public final class CommonTaskExecutor extends AbstractExecutorService { * *

If {@code target} is GCed periodic task is canceled. * + *

This method should be able to schedule task in majority of cases. The only reasonable case + * when this would fail is when task is being scheduled during JVM shutdown. In this case this + * method will return 'fake' future that can still be canceled to avoid confusing callers. + * * @param task task to run. Important: must not hold any strong references to target (or anything * else non static) * @param target target object to pass to task @@ -66,9 +70,13 @@ public final class CommonTaskExecutor extends AbstractExecutorService { periodicTask.setFuture(future); return future; } catch (final RejectedExecutionException e) { - log.warn("Cleaning task rejected. Will not run: {}", name); + log.warn("Periodic task rejected. Will not run: {}", name); } } + /* + * Return a 'fake' unscheduled future to allow caller call 'cancel' on it if needed. + * We are using 'fake' object instead of null to avoid callers needing to deal with nulls. + */ return new UnscheduledFuture(name); } @@ -129,12 +137,12 @@ public final class CommonTaskExecutor extends AbstractExecutorService { void run(T target); } - public static class PeriodicTask implements Runnable { + private static class PeriodicTask implements Runnable { private final WeakReference target; private final Task task; private volatile ScheduledFuture future = null; - private PeriodicTask(final Task task, final T target) { + public PeriodicTask(final Task task, final T target) { this.target = new WeakReference<>(target); this.task = task; } @@ -156,7 +164,7 @@ public final class CommonTaskExecutor extends AbstractExecutorService { // Unscheduled future @Slf4j - public static class UnscheduledFuture implements ScheduledFuture { + private static class UnscheduledFuture implements ScheduledFuture { private final String name; public UnscheduledFuture(final String name) { @@ -175,7 +183,7 @@ public final class CommonTaskExecutor extends AbstractExecutorService { @Override public boolean cancel(final boolean mayInterruptIfRunning) { - log.debug("Cancelling future for: {}", name); + log.debug("Cancelling unscheduled future for: {}", name); return false; } From 60226086c3a36ef5281d3e06af51e60d11f8c219 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 2 Apr 2020 18:05:45 -0400 Subject: [PATCH 13/37] Catch NumberFormatException --- .../trace/common/processor/rule/Status5XXRule.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java index 107a58b2c9..507920b664 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java @@ -18,9 +18,14 @@ public class Status5XXRule implements TraceProcessor.Rule { final DDSpan span, final Map tags, final Collection trace) { if (!span.context().getErrorFlag() && tags.containsKey(Tags.HTTP_STATUS.getKey())) { final Object value = tags.get(Tags.HTTP_STATUS.getKey()); - final int responseCode = - value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); - span.setError(500 <= responseCode && responseCode < 600); + try { + final int responseCode = + value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); + span.setError(500 <= responseCode && responseCode < 600); + } catch (final NumberFormatException ex) { + // If using Tags.HTTP_STATUS, value should always be an Integer, + // but lets catch NumberFormatException just to be safe. + } } } } From d1dc756b129efeb2dc36667e935610e97ee5905b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 9 Apr 2020 15:24:58 -0400 Subject: [PATCH 14/37] Fix issues from code reviews. --- .../decorator/HttpClientDecorator.java | 4 ---- .../decorator/HttpServerDecorator.java | 4 ---- .../main/java/datadog/trace/api/Config.java | 2 +- .../decorators/DDDecoratorsFactory.java | 2 +- .../common/processor/TraceProcessor.java | 8 +++---- .../rule/AnalyticsSampleRateRule.java | 23 ++++++++++--------- .../processor/rule/DBStatementRule.java | 10 +++++--- .../common/processor/rule/ErrorRule.java | 13 ++++++----- ...s5XXRule.java => HttpStatusErrorRule.java} | 19 +++++++++++---- .../processor/rule/ResourceNameRule.java | 6 ++++- .../common/processor/rule/SpanTypeRule.java | 6 ++++- .../processor/rule/URLAsResourceNameRule.java | 11 +++++---- 12 files changed, 63 insertions(+), 45 deletions(-) rename dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/{Status5XXRule.java => HttpStatusErrorRule.java} (59%) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecorator.java index 6030d6324f..84ac4823da 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecorator.java @@ -83,10 +83,6 @@ public abstract class HttpClientDecorator extends ClientDecor final Integer status = status(response); if (status != null) { span.setTag(Tags.HTTP_STATUS, status); - - if (Config.get().getHttpClientErrorStatuses().contains(status)) { - span.setError(true); - } } } return span; diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java index a1ebdc8127..576e0d7cb5 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java @@ -108,10 +108,6 @@ public abstract class HttpServerDecorator extends final Integer status = status(response); if (status != null) { span.setTag(Tags.HTTP_STATUS, status); - - if (Config.get().getHttpServerErrorStatuses().contains(status)) { - span.setError(true); - } } } return span; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 4320b09e0d..ec36f6dd8d 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -830,7 +830,7 @@ public class Config { return jmxFetchIntegrationEnabled(integrationNames, defaultEnabled); } - public boolean isDecoratorEnabled(final String name) { + public boolean isRuleEnabled(final String name) { return getBooleanSettingFromEnvironment("trace." + name + ".enabled", true) && getBooleanSettingFromEnvironment("trace." + name.toLowerCase() + ".enabled", true); } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java index bc7c673876..4de805cd3b 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/decorators/DDDecoratorsFactory.java @@ -23,7 +23,7 @@ public class DDDecoratorsFactory { new ServiceNameDecorator("service", false), new ServletContextDecorator())) { - if (Config.get().isDecoratorEnabled(decorator.getClass().getSimpleName())) { + if (Config.get().isRuleEnabled(decorator.getClass().getSimpleName())) { decorators.add(decorator); } else { log.debug("{} disabled", decorator.getClass().getSimpleName()); diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 3981ddd4ee..4481245076 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -5,10 +5,10 @@ import datadog.trace.api.Config; import datadog.trace.common.processor.rule.AnalyticsSampleRateRule; import datadog.trace.common.processor.rule.DBStatementRule; import datadog.trace.common.processor.rule.ErrorRule; +import datadog.trace.common.processor.rule.HttpStatusErrorRule; import datadog.trace.common.processor.rule.ResourceNameRule; import datadog.trace.common.processor.rule.SpanTypeRule; import datadog.trace.common.processor.rule.Status404Rule; -import datadog.trace.common.processor.rule.Status5XXRule; import datadog.trace.common.processor.rule.URLAsResourceNameRule; import java.util.ArrayList; import java.util.Collection; @@ -24,7 +24,7 @@ public class TraceProcessor { new DBStatementRule(), new ResourceNameRule(), new SpanTypeRule(), - new Status5XXRule(), + new HttpStatusErrorRule(), new ErrorRule(), new URLAsResourceNameRule(), new Status404Rule(), @@ -44,9 +44,9 @@ public class TraceProcessor { } private static boolean isEnabled(final Rule rule) { - boolean enabled = Config.get().isDecoratorEnabled(rule.getClass().getSimpleName()); + boolean enabled = Config.get().isRuleEnabled(rule.getClass().getSimpleName()); for (final String alias : rule.aliases()) { - enabled &= Config.get().isDecoratorEnabled(alias); + enabled &= Config.get().isRuleEnabled(alias); } if (!enabled) { log.debug("{} disabled", rule.getClass().getSimpleName()); diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java index ede6778a48..d27c892227 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/AnalyticsSampleRateRule.java @@ -16,18 +16,19 @@ public class AnalyticsSampleRateRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { - if (tags.containsKey(DDTags.ANALYTICS_SAMPLE_RATE)) { - final Object value = tags.get(DDTags.ANALYTICS_SAMPLE_RATE); - if (value instanceof Number) { - span.context().setMetric(DDTags.ANALYTICS_SAMPLE_RATE, (Number) value); - } else if (value instanceof String) { - try { - span.context() - .setMetric(DDTags.ANALYTICS_SAMPLE_RATE, Double.parseDouble((String) value)); - } catch (final NumberFormatException ex) { - // ignore - } + final Object sampleRateValue = tags.get(DDTags.ANALYTICS_SAMPLE_RATE); + if (sampleRateValue instanceof Number) { + span.context().setMetric(DDTags.ANALYTICS_SAMPLE_RATE, (Number) sampleRateValue); + } else if (sampleRateValue instanceof String) { + try { + span.context() + .setMetric(DDTags.ANALYTICS_SAMPLE_RATE, Double.parseDouble((String) sampleRateValue)); + } catch (final NumberFormatException ex) { + // ignore } + } + + if (tags.containsKey(DDTags.ANALYTICS_SAMPLE_RATE)) { span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, (String) null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java index b4d751591d..e0378f782d 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/DBStatementRule.java @@ -19,18 +19,22 @@ public class DBStatementRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { - if (tags.containsKey(Tags.DB_STATEMENT.getKey())) { + final Object dbStatementValue = tags.get(Tags.DB_STATEMENT.getKey()); + + if (dbStatementValue instanceof String) { // Special case: Mongo // Skip the decorators if (tags.containsKey(Tags.COMPONENT.getKey()) && "java-mongo".equals(tags.get(Tags.COMPONENT.getKey()))) { return; } - - final String statement = tags.get(Tags.DB_STATEMENT.getKey()).toString(); + final String statement = (String) dbStatementValue; if (!statement.isEmpty()) { span.setResourceName(statement); } + } + + if (tags.containsKey(Tags.DB_STATEMENT.getKey())) { span.setTag(Tags.DB_STATEMENT.getKey(), (String) null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java index 49c0b3e112..66e971bead 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ErrorRule.java @@ -16,13 +16,14 @@ public class ErrorRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { + final Object value = tags.get(Tags.ERROR.getKey()); + if (value instanceof Boolean) { + span.setError((Boolean) value); + } else if (value != null) { + span.setError(Boolean.parseBoolean(value.toString())); + } + if (tags.containsKey(Tags.ERROR.getKey())) { - final Object value = tags.get(Tags.ERROR.getKey()); - if (value instanceof Boolean) { - span.setError((Boolean) value); - } else { - span.setError(Boolean.parseBoolean(value.toString())); - } span.setTag(Tags.ERROR, null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java similarity index 59% rename from dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java rename to dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java index 507920b664..b567dd7353 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/Status5XXRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java @@ -1,16 +1,17 @@ package datadog.trace.common.processor.rule; import datadog.opentracing.DDSpan; +import datadog.trace.api.Config; +import datadog.trace.api.DDSpanTypes; import datadog.trace.common.processor.TraceProcessor; import io.opentracing.tag.Tags; import java.util.Collection; import java.util.Map; -/** Mark all 5xx status codes as an error */ -public class Status5XXRule implements TraceProcessor.Rule { +public class HttpStatusErrorRule implements TraceProcessor.Rule { @Override public String[] aliases() { - return new String[] {"Status5XXDecorator"}; + return new String[] {}; } @Override @@ -19,9 +20,17 @@ public class Status5XXRule implements TraceProcessor.Rule { if (!span.context().getErrorFlag() && tags.containsKey(Tags.HTTP_STATUS.getKey())) { final Object value = tags.get(Tags.HTTP_STATUS.getKey()); try { - final int responseCode = + final int status = value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); - span.setError(500 <= responseCode && responseCode < 600); + if (span.getType().equals(DDSpanTypes.HTTP_SERVER)) { + if (Config.get().getHttpServerErrorStatuses().contains(status)) { + span.setError(true); + } + } else if (span.getType().equals(DDSpanTypes.HTTP_CLIENT)) { + if (Config.get().getHttpClientErrorStatuses().contains(status)) { + span.setError(true); + } + } } catch (final NumberFormatException ex) { // If using Tags.HTTP_STATUS, value should always be an Integer, // but lets catch NumberFormatException just to be safe. diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java index 0a28ce8ac6..99d44ffee4 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/ResourceNameRule.java @@ -16,8 +16,12 @@ public class ResourceNameRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { + final Object name = tags.get(DDTags.RESOURCE_NAME); + if (name != null) { + span.setResourceName(name.toString()); + } + if (tags.containsKey(DDTags.RESOURCE_NAME)) { - span.setResourceName(tags.get(DDTags.RESOURCE_NAME).toString()); span.setTag(DDTags.RESOURCE_NAME, (String) null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java index 5b1f409f84..d2cda93cfe 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/SpanTypeRule.java @@ -16,8 +16,12 @@ public class SpanTypeRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { + final Object type = tags.get(DDTags.SPAN_TYPE); + if (type != null) { + span.setSpanType(type.toString()); + } + if (tags.containsKey(DDTags.SPAN_TYPE)) { - span.setSpanType(tags.get(DDTags.SPAN_TYPE).toString()); span.setTag(DDTags.SPAN_TYPE, (String) null); // Remove the tag } } diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java index 6b433e24dc..c7b8ceb83b 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/URLAsResourceNameRule.java @@ -89,10 +89,13 @@ public class URLAsResourceNameRule implements TraceProcessor.Rule { } private String addMethodIfAvailable(final Map meta, String path) { - // if the verb (GET, POST ...) is present, add it - final Object verb = meta.get(Tags.HTTP_METHOD.getKey()); - if (verb != null && !verb.toString().isEmpty()) { - path = verb.toString().toUpperCase() + " " + path; + // if the method (GET, POST ...) is present, add it + final Object method = meta.get(Tags.HTTP_METHOD.getKey()); + if (method != null) { + final String verb = method.toString().toUpperCase().trim(); + if (!verb.isEmpty()) { + path = verb + " " + path; + } } return path; } From 683477bd601d689149f4d6c96b7003df3037c486 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 17:29:00 -0400 Subject: [PATCH 15/37] Simplify WeakMapSuppliers Also move CleanerTest to PeriodicSchedulingTest --- .../trace/agent/tooling/WeakMapSuppliers.java | 18 +++++++--- .../common/exec/PeriodicSchedulingTest.groovy | 34 ++++++++----------- utils/thread-utils/thread-utils.gradle | 2 ++ 3 files changed, 30 insertions(+), 24 deletions(-) rename dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CleanerTest.groovy => utils/thread-utils/src/test/groovy/datadog/common/exec/PeriodicSchedulingTest.groovy (60%) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index f579210d33..2e1a019c66 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -3,6 +3,8 @@ package datadog.trace.agent.tooling; import com.blogspot.mydailyjava.weaklockfree.WeakConcurrentMap; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.MapMaker; +import datadog.common.exec.CommonTaskExecutor; +import datadog.common.exec.CommonTaskExecutor.Task; import datadog.trace.bootstrap.WeakMap; import java.util.concurrent.TimeUnit; @@ -40,15 +42,23 @@ class WeakMapSuppliers { @Override public WeakMap get() { final WeakConcurrentMap map = new WeakConcurrentMap<>(false, true); - cleaner.scheduleCleaning(map, MapCleaner.CLEANER, CLEAN_FREQUENCY_SECONDS, TimeUnit.SECONDS); + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( + MapCleaningTask.INSTANCE, + map, + CLEAN_FREQUENCY_SECONDS, + CLEAN_FREQUENCY_SECONDS, + TimeUnit.SECONDS, + "cleaner for " + map); return new Adapter<>(map); } - private static class MapCleaner implements Cleaner.Adapter { - private static final MapCleaner CLEANER = new MapCleaner(); + // Important to use explicit class to avoid implicit hard references to target + private static class MapCleaningTask implements Task { + + static final MapCleaningTask INSTANCE = new MapCleaningTask(); @Override - public void clean(final WeakConcurrentMap target) { + public void run(final WeakConcurrentMap target) { target.expungeStaleEntries(); } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CleanerTest.groovy b/utils/thread-utils/src/test/groovy/datadog/common/exec/PeriodicSchedulingTest.groovy similarity index 60% rename from dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CleanerTest.groovy rename to utils/thread-utils/src/test/groovy/datadog/common/exec/PeriodicSchedulingTest.groovy index d05496f956..c9dad5899c 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CleanerTest.groovy +++ b/utils/thread-utils/src/test/groovy/datadog/common/exec/PeriodicSchedulingTest.groovy @@ -1,10 +1,8 @@ -package datadog.trace.agent.tooling +package datadog.common.exec -import datadog.common.exec.CommonTaskExecutor import datadog.trace.util.gc.GCUtils import datadog.trace.util.test.DDSpecification import spock.lang.Retry -import spock.lang.Subject import java.lang.ref.WeakReference import java.util.concurrent.CountDownLatch @@ -13,19 +11,15 @@ import java.util.concurrent.atomic.AtomicInteger import static java.util.concurrent.TimeUnit.MILLISECONDS @Retry -class CleanerTest extends DDSpecification { - - @Subject - def cleaner = new Cleaner() +class PeriodicSchedulingTest extends DDSpecification { def "test scheduling"() { setup: def latch = new CountDownLatch(2) - def target = new Object() - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - latch.countDown() + void run(CountDownLatch target) { + target.countDown() } } @@ -33,7 +27,7 @@ class CleanerTest extends DDSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(target, action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, latch, 10, 10, MILLISECONDS, "test") then: latch.await(500, MILLISECONDS) @@ -43,10 +37,10 @@ class CleanerTest extends DDSpecification { setup: def callCount = new AtomicInteger() def target = new WeakReference(new Object()) - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - callCount.incrementAndGet() + void run(Object t) { + callCount.countDown() } } @@ -54,7 +48,7 @@ class CleanerTest extends DDSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(target.get(), action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, target.get(), 10, 10, MILLISECONDS, "test") GCUtils.awaitGC(target) Thread.sleep(1) def snapshot = callCount.get() @@ -67,10 +61,10 @@ class CleanerTest extends DDSpecification { def "test null target"() { setup: def callCount = new AtomicInteger() - def action = new Cleaner.Adapter() { + def task = new CommonTaskExecutor.Task() { @Override - void clean(Object t) { - callCount.incrementAndGet() + void run(Object t) { + callCount.countDown() } } @@ -78,7 +72,7 @@ class CleanerTest extends DDSpecification { !CommonTaskExecutor.INSTANCE.isShutdown() when: - cleaner.scheduleCleaning(null, action, 10, MILLISECONDS) + CommonTaskExecutor.INSTANCE.scheduleAtFixedRate(task, null, 10, 10, MILLISECONDS, "test") Thread.sleep(11) then: diff --git a/utils/thread-utils/thread-utils.gradle b/utils/thread-utils/thread-utils.gradle index 29bb468d6e..d78196f16b 100644 --- a/utils/thread-utils/thread-utils.gradle +++ b/utils/thread-utils/thread-utils.gradle @@ -2,4 +2,6 @@ apply from: "${rootDir}/gradle/java.gradle" dependencies { compile deps.slf4j + + testCompile project(':utils:test-utils') } From eee11f5ce0f7fc29bf41e06ca0b763f8b4aa4474 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 17:34:31 -0400 Subject: [PATCH 16/37] Remove Cleaner class --- .../trace/agent/tooling/AgentTooling.java | 2 +- .../datadog/trace/agent/tooling/Cleaner.java | 34 ------------------- .../trace/agent/tooling/WeakMapSuppliers.java | 5 --- .../tooling/WeakConcurrentSupplierTest.groovy | 4 +-- 4 files changed, 2 insertions(+), 43 deletions(-) delete mode 100644 dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java index 38558b9e11..1079e2803f 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentTooling.java @@ -22,7 +22,7 @@ public class AgentTooling { private static void registerWeakMapProvider() { if (!WeakMap.Provider.isProviderRegistered()) { - WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent(new Cleaner())); + WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent()); // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.WeakConcurrent.Inline()); // WeakMap.Provider.registerIfAbsent(new WeakMapSuppliers.Guava()); } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java deleted file mode 100644 index f55ce65239..0000000000 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java +++ /dev/null @@ -1,34 +0,0 @@ -package datadog.trace.agent.tooling; - -import datadog.common.exec.CommonTaskExecutor; -import datadog.common.exec.CommonTaskExecutor.Task; -import java.util.concurrent.TimeUnit; -import lombok.extern.slf4j.Slf4j; - -@Slf4j -class Cleaner { - - void scheduleCleaning( - final T target, final Adapter adapter, final long frequency, final TimeUnit unit) { - CommonTaskExecutor.INSTANCE.scheduleAtFixedRate( - new CleaningTask(adapter), target, frequency, frequency, unit, "cleaner for " + target); - } - - // Important to use explicit class to avoid implicit hard references to target - private static class CleaningTask implements Task { - private final Adapter adapter; - - public CleaningTask(final Adapter adapter) { - this.adapter = adapter; - } - - @Override - public void run(final T target) { - adapter.clean(target); - } - } - - public interface Adapter { - void clean(T target); - } -} diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java index 2e1a019c66..818a9acab9 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/WeakMapSuppliers.java @@ -33,11 +33,6 @@ class WeakMapSuppliers { static class WeakConcurrent implements WeakMap.Implementation { @VisibleForTesting static final long CLEAN_FREQUENCY_SECONDS = 1; - private final Cleaner cleaner; - - WeakConcurrent(final Cleaner cleaner) { - this.cleaner = cleaner; - } @Override public WeakMap get() { diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index adf63a6d84..560f3c91e1 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -13,9 +13,7 @@ import java.util.concurrent.TimeUnit // These tests fail sometimes in CI. class WeakConcurrentSupplierTest extends DDSpecification { @Shared - def cleaner = new Cleaner() - @Shared - def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent(cleaner) + def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent() @Shared def weakInlineSupplier = new WeakMapSuppliers.WeakConcurrent.Inline() @Shared From 96fcb278b67904e6975e15fcd0acab5eaab6b8be Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 17:41:25 -0400 Subject: [PATCH 17/37] Add comment --- dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java index 2707e979e3..917e135c51 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java @@ -290,6 +290,8 @@ public class PendingTrace extends ConcurrentLinkedDeque { } } + // FIXME: it should be possible to simplify this logic and avod having SpanCleaner and + // SpanCleanerTask private static class SpanCleaner implements Runnable, Closeable { private static final long CLEAN_FREQUENCY = 1; From b1589c38199ea84c47dcd7099bf53866bc1b75ee Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 17:51:20 -0400 Subject: [PATCH 18/37] Fix unit test --- .../trace/agent/tooling/WeakConcurrentSupplierTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy index 560f3c91e1..bf77c51fb9 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/WeakConcurrentSupplierTest.groovy @@ -57,7 +57,7 @@ class WeakConcurrentSupplierTest extends DDSpecification { where: name | supplierSupplier - "WeakConcurrent" | { -> new WeakMapSuppliers.WeakConcurrent(cleaner) } + "WeakConcurrent" | { -> new WeakMapSuppliers.WeakConcurrent() } "WeakInline" | { -> new WeakMapSuppliers.WeakConcurrent.Inline() } "Guava" | { -> new WeakMapSuppliers.Guava() } } From 3aa803489b250a8a6f78a85051e336bf2498a49b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Mon, 6 Apr 2020 11:58:11 -0400 Subject: [PATCH 19/37] Fix tests. --- .../decorator/HttpClientDecoratorTest.groovy | 29 +-- .../decorator/HttpServerDecoratorTest.groovy | 29 +-- .../test/groovy/server/PlayServerTest.groovy | 8 +- .../test/groovy/test/TwilioClientTest.groovy | 217 +++++++----------- .../decorators/SpanDecoratorTest.groovy | 1 + .../datadog/trace/DDSpanContextTest.groovy | 24 +- .../trace/api/writer/DDAgentApiTest.groovy | 1 + .../processor/TraceProcessorTest.groovy | 31 ++- 8 files changed, 156 insertions(+), 184 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecoratorTest.groovy b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecoratorTest.groovy index adf3b1a21c..8f21c1066d 100644 --- a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecoratorTest.groovy +++ b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecoratorTest.groovy @@ -93,31 +93,26 @@ class HttpClientDecoratorTest extends ClientDecoratorTest { def decorator = newDecorator() when: - withConfigOverride(Config.HTTP_CLIENT_ERROR_STATUSES, "$errorRange") { - decorator.onResponse(span, resp) - } + decorator.onResponse(span, resp) then: if (status) { 1 * span.setTag(Tags.HTTP_STATUS, status) } - if (error) { - 1 * span.setError(true) - } 0 * _ where: - status | error | errorRange | resp - 200 | false | null | [status: 200] - 399 | false | null | [status: 399] - 400 | true | null | [status: 400] - 499 | true | null | [status: 499] - 500 | false | null | [status: 500] - 500 | true | "500" | [status: 500] - 500 | true | "400-500" | [status: 500] - 600 | false | null | [status: 600] - null | false | null | [status: null] - null | false | null | null + status | resp + 200 | [status: 200] + 399 | [status: 399] + 400 | [status: 400] + 499 | [status: 499] + 500 | [status: 500] + 500 | [status: 500] + 500 | [status: 500] + 600 | [status: 600] + null | [status: null] + null | null } def "test assert null span"() { diff --git a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy index 6c54807d48..db3aed58df 100644 --- a/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy +++ b/dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy @@ -106,31 +106,26 @@ class HttpServerDecoratorTest extends ServerDecoratorTest { def decorator = newDecorator() when: - withConfigOverride(Config.HTTP_SERVER_ERROR_STATUSES, "$errorRange") { - decorator.onResponse(span, resp) - } + decorator.onResponse(span, resp) then: if (status) { 1 * span.setTag(Tags.HTTP_STATUS, status) } - if (error) { - 1 * span.setError(true) - } 0 * _ where: - status | error | errorRange | resp - 200 | false | null | [status: 200] - 399 | false | null | [status: 399] - 400 | false | null | [status: 400] - 404 | true | "404" | [status: 404] - 404 | true | "400-500" | [status: 404] - 499 | false | null | [status: 499] - 500 | true | null | [status: 500] - 600 | false | null | [status: 600] - null | false | null | [status: null] - null | false | null | null + status | resp + 200 | [status: 200] + 399 | [status: 399] + 400 | [status: 400] + 404 | [status: 404] + 404 | [status: 404] + 499 | [status: 499] + 500 | [status: 500] + 600 | [status: 600] + null | [status: null] + null | null } def "test assert null span"() { diff --git a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy index c8ce2c32c5..8f6415ae03 100644 --- a/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy +++ b/dd-java-agent/instrumentation/play-2.3/src/test/groovy/server/PlayServerTest.groovy @@ -10,7 +10,9 @@ import datadog.trace.instrumentation.netty38.server.NettyHttpServerDecorator import datadog.trace.instrumentation.play23.PlayHttpServerDecorator import play.api.test.TestServer -import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.* +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS class PlayServerTest extends HttpServerTest { @Override @@ -62,9 +64,7 @@ class PlayServerTest extends HttpServerTest { "$Tags.HTTP_URL" String "$Tags.HTTP_METHOD" String "$Tags.HTTP_STATUS" Integer - if (endpoint == ERROR) { - "$Tags.ERROR" true - } else if (endpoint == EXCEPTION) { + if (endpoint == EXCEPTION) { errorTags(Exception, EXCEPTION.body) } if (endpoint.query) { diff --git a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy index e2249fe955..1d5cfc16e4 100644 --- a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy +++ b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy @@ -23,10 +23,7 @@ import org.apache.http.impl.client.HttpClientBuilder import java.util.concurrent.ExecutionException import java.util.concurrent.TimeUnit -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan +import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace class TwilioClientTest extends AgentTestRunner { @@ -108,23 +105,24 @@ class TwilioClientTest extends AgentTestRunner { Twilio.init(ACCOUNT_SID, AUTH_TOKEN) } + def cleanup() { + Twilio.getExecutorService().shutdown() + Twilio.setExecutorService(null) + Twilio.setRestClient(null) + } + def "synchronous message"() { setup: twilioRestClient.getObjectMapper() >> new ObjectMapper() 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(MESSAGE_RESPONSE_BODY.getBytes()), 200) - activateSpan(startSpan("test"), true) - - Message message = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).create(twilioRestClient) - - def scope = activeScope() - if (scope) { - scope.close() + Message message = runUnderTrace("test") { + Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).create(twilioRestClient) } expect: @@ -169,19 +167,14 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(CALL_RESPONSE_BODY.getBytes()), 200) - activateSpan(startSpan("test"), true) + Call call = runUnderTrace("test") { + Call.creator( + new PhoneNumber("+15558881234"), // To number + new PhoneNumber("+15559994321"), // From number - Call call = Call.creator( - new PhoneNumber("+15558881234"), // To number - new PhoneNumber("+15559994321"), // From number - - // Read TwiML at this URL when a call connects (hold music) - new URI("http://twimlets.com/holdmusic?Bucket=com.twilio.music.ambient") - ).create(twilioRestClient) - - def scope = activeScope() - if (scope) { - scope.close() + // Read TwiML at this URL when a call connects (hold music) + new URI("http://twimlets.com/holdmusic?Bucket=com.twilio.music.ambient") + ).create(twilioRestClient) } expect: @@ -250,17 +243,12 @@ class TwilioClientTest extends AgentTestRunner { .httpClient(networkHttpClient) .build() - activateSpan(startSpan("test"), true) - - Message message = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).create(realTwilioRestClient) - - def scope = activeScope() - if (scope) { - scope.close() + Message message = runUnderTrace("test") { + Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).create(realTwilioRestClient) } expect: @@ -361,21 +349,15 @@ class TwilioClientTest extends AgentTestRunner { .httpClient(networkHttpClient) .build() - activateSpan(startSpan("test"), true) - - Message message = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).create(realTwilioRestClient) - - def scope = activeScope() - if (scope) { - scope.close() + Message message = runUnderTrace("test") { + Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).create(realTwilioRestClient) } expect: - message.body == "Hello, World!" assertTraces(1) { @@ -427,14 +409,14 @@ class TwilioClientTest extends AgentTestRunner { operationName "http.request" resourceName "POST /?/Accounts/abc/Messages.json" spanType DDSpanTypes.HTTP_CLIENT - errored true + errored false tags { "$Tags.COMPONENT" "apache-httpclient" "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT - "$Tags.PEER_HOSTNAME" String - "$Tags.HTTP_URL" String - "$Tags.HTTP_METHOD" String - "$Tags.HTTP_STATUS" Integer + "$Tags.PEER_HOSTNAME" "api.twilio.com" + "$Tags.HTTP_URL" "https://api.twilio.com/2010-04-01/Accounts/abc/Messages.json" + "$Tags.HTTP_METHOD" "POST" + "$Tags.HTTP_STATUS" 500 defaultTags() } } @@ -488,29 +470,22 @@ class TwilioClientTest extends AgentTestRunner { .httpClient(networkHttpClient) .build() - activateSpan(startSpan("test"), true) + Message message = runUnderTrace("test") { + ListenableFuture future = Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).createAsync(realTwilioRestClient) - ListenableFuture future = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).createAsync(realTwilioRestClient) - - Message message - try { - message = future.get(10, TimeUnit.SECONDS) - } finally { - // Give the future callback a chance to run - Thread.sleep(1000) - def scope = activeScope() - if (scope) { - activeSpan().finish() - scope.close() + try { + return future.get(10, TimeUnit.SECONDS) + } finally { + // Give the future callback a chance to run + Thread.sleep(1000) } } expect: - message.body == "Hello, World!" assertTraces(1) { @@ -580,14 +555,14 @@ class TwilioClientTest extends AgentTestRunner { operationName "http.request" resourceName "POST /?/Accounts/abc/Messages.json" spanType DDSpanTypes.HTTP_CLIENT - errored true + errored false tags { "$Tags.COMPONENT" "apache-httpclient" "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT - "$Tags.PEER_HOSTNAME" String - "$Tags.HTTP_URL" String - "$Tags.HTTP_METHOD" String - "$Tags.HTTP_STATUS" Integer + "$Tags.PEER_HOSTNAME" "api.twilio.com" + "$Tags.HTTP_URL" "https://api.twilio.com/2010-04-01/Accounts/abc/Messages.json" + "$Tags.HTTP_METHOD" "POST" + "$Tags.HTTP_STATUS" 500 defaultTags() } } @@ -607,34 +582,29 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(ERROR_RESPONSE_BODY.getBytes()), 500) - activateSpan(startSpan("test"), true) - when: - Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).create(twilioRestClient) + runUnderTrace("test") { + Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).create(twilioRestClient) + } then: thrown(ApiException) - def scope = activeScope() - if (scope) { - scope.close() - } - expect: - assertTraces(1) { trace(0, 2) { span(0) { serviceName "unnamed-java-app" operationName "test" resourceName "test" - errored false + errored true parent() tags { + errorTags(ApiException, "Testing Failure") defaultTags() } } @@ -702,24 +672,19 @@ class TwilioClientTest extends AgentTestRunner { when: - activateSpan(startSpan("test"), true) + Message message = runUnderTrace("test") { - ListenableFuture future = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).createAsync(twilioRestClient) + ListenableFuture future = Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).createAsync(twilioRestClient) - Message message - try { - message = future.get(10, TimeUnit.SECONDS) - } finally { - // Give the future callback a chance to run - Thread.sleep(1000) - def scope = activeScope() - if (scope) { - activeSpan().finish() - scope.close() + try { + return future.get(10, TimeUnit.SECONDS) + } finally { + // Give the future callback a chance to run + Thread.sleep(1000) } } @@ -792,25 +757,18 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(ERROR_RESPONSE_BODY.getBytes()), 500) - activateSpan(startSpan("test"), true) - - ListenableFuture future = Message.creator( - new PhoneNumber("+1 555 720 5913"), // To number - new PhoneNumber("+1 555 555 5215"), // From number - "Hello world!" // SMS body - ).createAsync(twilioRestClient) - - when: - Message message - try { - message = future.get(10, TimeUnit.SECONDS) + Message message = runUnderTrace("test") { + ListenableFuture future = Message.creator( + new PhoneNumber("+1 555 720 5913"), // To number + new PhoneNumber("+1 555 555 5215"), // From number + "Hello world!" // SMS body + ).createAsync(twilioRestClient) - } finally { - Thread.sleep(1000) - def scope = activeScope() - if (scope) { - scope.close() + try { + return future.get(10, TimeUnit.SECONDS) + } finally { + Thread.sleep(1000) } } @@ -825,10 +783,11 @@ class TwilioClientTest extends AgentTestRunner { serviceName "unnamed-java-app" operationName "test" resourceName "test" - errored false + errored true parent() tags { defaultTags() + errorTags(ApiException, "Testing Failure") } } span(1) { @@ -859,11 +818,5 @@ class TwilioClientTest extends AgentTestRunner { } } } - - cleanup: - Twilio.getExecutorService().shutdown() - Twilio.setExecutorService(null) - Twilio.setRestClient(null) } - } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy index 012a92dbe8..06dd9a3b23 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/decorators/SpanDecoratorTest.groovy @@ -273,6 +273,7 @@ class SpanDecoratorTest extends DDSpecification { when: span.setTag(ANALYTICS_SAMPLE_RATE, rate) + span.finish() then: span.metrics.get(ANALYTICS_SAMPLE_RATE) == result diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy index 0b9d6b7076..17a8a91422 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/DDSpanContextTest.groovy @@ -1,24 +1,29 @@ package datadog.trace + +import datadog.opentracing.DDSpanContext import datadog.opentracing.SpanFactory import datadog.trace.api.DDTags +import datadog.trace.common.sampling.RateByServiceSampler import datadog.trace.util.test.DDSpecification class DDSpanContextTest extends DDSpecification { def "null values for tags delete existing tags"() { setup: - def context = SpanFactory.newSpanOf(0).context + def span = SpanFactory.newSpanOf(0) + def context = span.context context.setTag("some.tag", "asdf") context.setTag(name, null) context.setErrorFlag(true) + span.finish() expect: context.getTags() == tags context.serviceName == "fakeService" context.resourceName == "fakeResource" context.spanType == "fakeType" - context.toString() == "DDSpan [ t_id=1, s_id=1, p_id=0] trace=fakeService/fakeOperation/fakeResource metrics={} *errored* tags={${extra}${tags.containsKey(DDTags.SPAN_TYPE) ? "span.type=${context.getSpanType()}, " : ""}thread.id=${Thread.currentThread().id}, thread.name=${Thread.currentThread().name}}" + context.toString() == "DDSpan [ t_id=1, s_id=1, p_id=0] trace=fakeService/fakeOperation/fakeResource metrics=${defaultMetrics()} *errored* tags={${extra}${tags.containsKey(DDTags.SPAN_TYPE) ? "span.type=${context.getSpanType()}, " : ""}thread.id=${Thread.currentThread().id}, thread.name=${Thread.currentThread().name}}" where: name | extra | tags @@ -37,7 +42,7 @@ class DDSpanContextTest extends DDSpecification { def thread = Thread.currentThread() def expectedTags = [(DDTags.THREAD_NAME): thread.name, (DDTags.THREAD_ID): thread.id] - def expectedTrace = "DDSpan [ t_id=1, s_id=1, p_id=0] trace=$details metrics={} tags={thread.id=$thread.id, thread.name=$thread.name}" + def expectedTrace = "DDSpan [ t_id=1, s_id=1, p_id=0] trace=$details metrics=${defaultMetrics()} tags={thread.id=$thread.id, thread.name=$thread.name}" expect: context.getTags() == expectedTags @@ -53,8 +58,10 @@ class DDSpanContextTest extends DDSpecification { def "tags can be added to the context"() { setup: - def context = SpanFactory.newSpanOf(0).context + def span = SpanFactory.newSpanOf(0) + def context = span.context context.setTag(name, value) + span.finish() def thread = Thread.currentThread() expect: @@ -63,7 +70,7 @@ class DDSpanContextTest extends DDSpecification { (DDTags.THREAD_NAME): thread.name, (DDTags.THREAD_ID) : thread.id ] - context.toString() == "DDSpan [ t_id=1, s_id=1, p_id=0] trace=fakeService/fakeOperation/fakeResource metrics={} tags={$name=$value, thread.id=$thread.id, thread.name=$thread.name}" + context.toString() == "DDSpan [ t_id=1, s_id=1, p_id=0] trace=fakeService/fakeOperation/fakeResource metrics=${defaultMetrics()} tags={$name=$value, thread.id=$thread.id, thread.name=$thread.name}" where: name | value @@ -100,4 +107,11 @@ class DDSpanContextTest extends DDSpecification { Double | 0.5d Integer | 0x55 } + + static String defaultMetrics() { + return [ + (RateByServiceSampler.SAMPLING_AGENT_RATE): 1.0, + (DDSpanContext.PRIORITY_SAMPLING_KEY) : 1, + ] + } } diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy index 289cc59930..797672fbc5 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/api/writer/DDAgentApiTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.api.writer import com.fasterxml.jackson.core.type.TypeReference import com.fasterxml.jackson.databind.ObjectMapper +import datadog.opentracing.DDSpanContext import datadog.opentracing.SpanFactory import datadog.trace.common.sampling.RateByServiceSampler import datadog.trace.common.writer.ddagent.DDAgentApi diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy index 59cff6577e..16a19944c0 100644 --- a/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/trace/common/processor/TraceProcessorTest.groovy @@ -2,6 +2,7 @@ package datadog.trace.common.processor import datadog.opentracing.SpanFactory import datadog.trace.agent.test.utils.ConfigUtils +import datadog.trace.api.DDSpanTypes import datadog.trace.common.processor.rule.Status404Rule import datadog.trace.common.processor.rule.URLAsResourceNameRule import datadog.trace.util.test.DDSpecification @@ -60,8 +61,11 @@ class TraceProcessorTest extends DDSpecification { span.getResourceName() == "404" } - def "status #status code error=#error"() { + def "#type status #status code error=#error"() { setup: + if (type) { + span.setSpanType(DDSpanTypes."$type") + } Tags.HTTP_STATUS.set(span, status) when: @@ -71,14 +75,23 @@ class TraceProcessorTest extends DDSpecification { span.isError() == error where: - status | error - 400 | false - 404 | false - 499 | false - 500 | true - 550 | true - 599 | true - 600 | false + type | status | error + null | 400 | false + null | 500 | false + "HTTP_CLIENT" | 400 | true + "HTTP_CLIENT" | 404 | true + "HTTP_CLIENT" | 499 | true + "HTTP_CLIENT" | 500 | false + "HTTP_CLIENT" | 550 | false + "HTTP_CLIENT" | 599 | false + "HTTP_CLIENT" | 600 | false + "HTTP_SERVER" | 400 | false + "HTTP_SERVER" | 404 | false + "HTTP_SERVER" | 499 | false + "HTTP_SERVER" | 500 | true + "HTTP_SERVER" | 550 | true + "HTTP_SERVER" | 599 | true + "HTTP_SERVER" | 600 | false } def "resource name set with url path #url to #resourceName"() { From bd637732da2106f2d7e92faca99911f4547bb054 Mon Sep 17 00:00:00 2001 From: Lev Priima <1118651+lpriima@users.noreply.github.com> Date: Mon, 13 Apr 2020 15:15:24 -0700 Subject: [PATCH 20/37] No logging when using default Value when parsing (#1364) --- .../controller/ControllerFactoryTest.java | 15 +++++++++++++-- .../src/main/java/datadog/trace/api/Config.java | 1 - 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/dd-java-agent/agent-profiling/profiling-controller/src/test/java/com/datadog/profiling/controller/ControllerFactoryTest.java b/dd-java-agent/agent-profiling/profiling-controller/src/test/java/com/datadog/profiling/controller/ControllerFactoryTest.java index 972ee08a4a..b55ab4440d 100644 --- a/dd-java-agent/agent-profiling/profiling-controller/src/test/java/com/datadog/profiling/controller/ControllerFactoryTest.java +++ b/dd-java-agent/agent-profiling/profiling-controller/src/test/java/com/datadog/profiling/controller/ControllerFactoryTest.java @@ -28,10 +28,21 @@ public class ControllerFactoryTest { }); String expected = "The JFR controller could not find a supported JFR API, use OpenJDK 11+ or Azul zulu version 1.8.0_212+"; - if ("Azul Systems, Inc.".equals(System.getProperty("java.vendor"))) { + final String javaVendor = System.getProperty("java.vendor"); + final String javaRuntimeName = System.getProperty("java.runtime.name"); + final String javaVersion = System.getProperty("java.version"); + if ("Azul Systems, Inc.".equals(javaVendor)) { expected = "The JFR controller could not find a supported JFR API, use Azul zulu version 1.8.0_212+"; + } else if ("Java(TM) SE Runtime Environment".equals(javaRuntimeName) + && "Oracle Corporation".equals(javaVendor) + && javaVersion.startsWith("1.8")) { + // condition for OracleJRE8 (with proprietary JFR inside) + expected = "The JFR controller is currently not supported on the Oracle JDK <= JDK 11!"; } - assertEquals(expected, unsupportedEnvironmentException.getMessage()); + assertEquals( + expected, + unsupportedEnvironmentException.getMessage(), + "'" + javaRuntimeName + "' / '" + javaVendor + "' / '" + javaVersion + "'"); } } diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 4320b09e0d..ce5f16a608 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -1059,7 +1059,6 @@ public class Config { private static T valueOf( final String value, @NonNull final Class tClass, final T defaultValue) { if (value == null || value.trim().isEmpty()) { - log.debug("valueOf: using defaultValue '{}' for '{}' of '{}' ", defaultValue, value, tClass); return defaultValue; } try { From bef848ec2df5ba6b5083db0276636c4ad5b9e8cb Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Mon, 13 Apr 2020 18:44:18 -0400 Subject: [PATCH 21/37] Disable coverage and add TODO --- utils/thread-utils/thread-utils.gradle | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/utils/thread-utils/thread-utils.gradle b/utils/thread-utils/thread-utils.gradle index d78196f16b..fd5a0b31c8 100644 --- a/utils/thread-utils/thread-utils.gradle +++ b/utils/thread-utils/thread-utils.gradle @@ -1,5 +1,10 @@ apply from: "${rootDir}/gradle/java.gradle" +// TODO: add more tests +excludedClassesCoverage += [ + 'datadog.common.exec*' +] + dependencies { compile deps.slf4j From f4c9b73b02a85cf005784bf0dbd6364524ceff23 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Tue, 14 Apr 2020 11:47:40 -0400 Subject: [PATCH 22/37] Add thread utils to bootstrap so instumentations could use them --- dd-java-agent/agent-bootstrap/agent-bootstrap.gradle | 1 + .../src/main/java/datadog/trace/agent/tooling/Constants.java | 2 ++ .../src/main/groovy/datadog/trace/agent/test/SpockRunner.java | 1 + 3 files changed, 4 insertions(+) diff --git a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle index 7e267be9c5..4aeda043e3 100644 --- a/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle +++ b/dd-java-agent/agent-bootstrap/agent-bootstrap.gradle @@ -11,6 +11,7 @@ minimumInstructionCoverage = 0.0 dependencies { compile project(':dd-trace-api') + compile project(':utils:thread-utils') compile deps.opentracing compile deps.slf4j compile group: 'org.slf4j', name: 'slf4j-simple', version: versions.slf4j diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java index 76e5f34273..cbd0a760a0 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java @@ -14,6 +14,7 @@ public final class Constants { * datadog.trace.agent.test.SpockRunner#BOOTSTRAP_PACKAGE_PREFIXES_COPY */ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = { + "datadog.common.exec", "datadog.slf4j", "datadog.trace.api", "datadog.trace.bootstrap", @@ -24,6 +25,7 @@ public final class Constants { // This is used in IntegrationTestUtils.java public static final String[] AGENT_PACKAGE_PREFIXES = { + "datadog.common.exec", "datadog.trace.common", "datadog.trace.agent", "datadog.trace.instrumentation", diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java index 41f69ef4ca..97767891d7 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java @@ -32,6 +32,7 @@ public class SpockRunner extends Sputnik { * references bootstrap classes (e.g. DatadogClassLoader). */ public static final String[] BOOTSTRAP_PACKAGE_PREFIXES_COPY = { + "datadog.common.exec", "datadog.slf4j", "datadog.trace.api", "datadog.trace.bootstrap", From 11c71e1d5d0a4f97d16a025085d15ae941e08eac Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 14 Apr 2020 12:20:34 -0400 Subject: [PATCH 23/37] Avoid setting the error tag and test fixes. --- .../java/datadog/trace/agent/tooling/OpenTracing32.java | 7 ++++++- .../twilio/src/test/groovy/test/TwilioClientTest.groovy | 2 +- .../datadog/trace/common/processor/TraceProcessor.java | 2 +- .../trace/common/processor/rule/HttpStatusErrorRule.java | 4 ++-- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java index db51e06bcf..3464072270 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java @@ -5,6 +5,7 @@ import static io.opentracing.propagation.Format.Builtin.TEXT_MAP_EXTRACT; import static io.opentracing.propagation.Format.Builtin.TEXT_MAP_INJECT; import static java.util.Collections.singletonMap; +import datadog.opentracing.DDSpan; import datadog.trace.api.interceptor.MutableSpan; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation; import datadog.trace.bootstrap.instrumentation.api.AgentPropagation.Getter; @@ -165,7 +166,11 @@ public final class OpenTracing32 implements TracerAPI { @Override public AgentSpan setError(final boolean error) { - Tags.ERROR.set(span, error); + if (span instanceof DDSpan) { + ((DDSpan) span).setError(error); + } else { + Tags.ERROR.set(span, error); + } return this; } diff --git a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy index 1d5cfc16e4..758587a529 100644 --- a/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy +++ b/dd-java-agent/instrumentation/twilio/src/test/groovy/test/TwilioClientTest.groovy @@ -758,7 +758,7 @@ class TwilioClientTest extends AgentTestRunner { 1 * twilioRestClient.request(_) >> new Response(new ByteArrayInputStream(ERROR_RESPONSE_BODY.getBytes()), 500) when: - Message message = runUnderTrace("test") { + runUnderTrace("test") { ListenableFuture future = Message.creator( new PhoneNumber("+1 555 720 5913"), // To number new PhoneNumber("+1 555 555 5215"), // From number diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java index 4481245076..f0375b0bc8 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/TraceProcessor.java @@ -24,8 +24,8 @@ public class TraceProcessor { new DBStatementRule(), new ResourceNameRule(), new SpanTypeRule(), - new HttpStatusErrorRule(), new ErrorRule(), + new HttpStatusErrorRule(), new URLAsResourceNameRule(), new Status404Rule(), new AnalyticsSampleRateRule(), diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java index b567dd7353..786800b5a2 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/processor/rule/HttpStatusErrorRule.java @@ -17,8 +17,8 @@ public class HttpStatusErrorRule implements TraceProcessor.Rule { @Override public void processSpan( final DDSpan span, final Map tags, final Collection trace) { - if (!span.context().getErrorFlag() && tags.containsKey(Tags.HTTP_STATUS.getKey())) { - final Object value = tags.get(Tags.HTTP_STATUS.getKey()); + final Object value = tags.get(Tags.HTTP_STATUS.getKey()); + if (value != null && !span.context().getErrorFlag()) { try { final int status = value instanceof Integer ? (int) value : Integer.parseInt(value.toString()); From 64161d93c9c9ea1ecd95ee77ef72baccd6bd996f Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 9 Apr 2020 14:26:33 -0400 Subject: [PATCH 24/37] Add instrumentation to detect the route at the beginning of the spring request Instead of waiting till the handler is called, otherwise if a response is returned by a filter then the proper name wouldn't be set and would fall back to the URL. --- .../servlet3/Servlet3Decorator.java | 25 ++- .../DispatcherServletInstrumentation.java | 64 ++++++- .../HandlerMappingResourceNameFilter.java | 65 +++++++ .../groovy/test/{ => boot}/AppConfig.groovy | 2 +- .../{ => boot}/SpringBootBasedTest.groovy | 9 +- .../test/{ => boot}/TestController.groovy | 12 +- .../test/filter/FilteredAppConfig.groovy | 112 ++++++++++++ .../test/filter/ServletFilterTest.groovy | 160 ++++++++++++++++++ .../groovy/test/filter/TestController.groovy | 74 ++++++++ .../agent/test/base/HttpServerTest.groovy | 45 ++++- 10 files changed, 553 insertions(+), 15 deletions(-) create mode 100644 dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HandlerMappingResourceNameFilter.java rename dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/{ => boot}/AppConfig.groovy (98%) rename dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/{ => boot}/SpringBootBasedTest.groovy (96%) rename dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/{ => boot}/TestController.groovy (85%) create mode 100644 dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy create mode 100644 dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy create mode 100644 dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java index 955bdbe3ff..0c9822d6c3 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java @@ -2,8 +2,11 @@ package datadog.trace.instrumentation.servlet3; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; +import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; +import javax.servlet.Filter; +import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -58,12 +61,32 @@ public class Servlet3Decorator public AgentSpan onRequest(final AgentSpan span, final HttpServletRequest request) { assert span != null; if (request != null) { - span.setTag("servlet.context", request.getContextPath()); span.setTag("servlet.path", request.getServletPath()); + span.setTag("servlet.context", request.getContextPath()); + onContext(span, request, request.getServletContext()); } return super.onRequest(span, request); } + /** + * This method executes the filter created by + * datadog.trace.instrumentation.springweb.DispatcherServletInstrumentation$HandlerMappingAdvice. + * This was easier and less "hacky" than other ways to add the filter to the front of the filter + * chain. + */ + private void onContext( + final AgentSpan span, final HttpServletRequest request, final ServletContext context) { + final Object attribute = context.getAttribute("dd.dispatcher-filter"); + if (attribute instanceof Filter) { + final Filter filter = (Filter) attribute; + try { + request.setAttribute(DD_SPAN_ATTRIBUTE, span); + filter.doFilter(request, null, null); + } catch (final IOException | ServletException e) { + } + } + } + @Override public AgentSpan onError(final AgentSpan span, final Throwable throwable) { if (throwable instanceof ServletException && throwable.getCause() != null) { diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/DispatcherServletInstrumentation.java b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/DispatcherServletInstrumentation.java index 5450bb6410..be2bf9dd5f 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/DispatcherServletInstrumentation.java +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/DispatcherServletInstrumentation.java @@ -5,23 +5,31 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; import static datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator.DECORATE; import static datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator.DECORATE_RENDER; +import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isProtected; import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.util.HashMap; +import java.util.List; import java.util.Map; +import javax.servlet.ServletContext; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; -import org.springframework.web.method.HandlerMethod; +import org.springframework.context.ApplicationContext; +import org.springframework.web.servlet.DispatcherServlet; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.ModelAndView; @AutoService(Instrumenter.class) @@ -36,23 +44,38 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default return named("org.springframework.web.servlet.DispatcherServlet"); } + @Override + public Map contextStore() { + return singletonMap( + "org.springframework.web.servlet.DispatcherServlet", + packageName + ".HandlerMappingResourceNameFilter"); + } + @Override public String[] helperClassNames() { return new String[] { packageName + ".SpringWebHttpServerDecorator", packageName + ".SpringWebHttpServerDecorator$1", + packageName + ".HandlerMappingResourceNameFilter", }; } @Override public Map, String> transformers() { final Map, String> transformers = new HashMap<>(); + transformers.put( + isMethod() + .and(isProtected()) + .and(named("onRefresh")) + .and(takesArgument(0, named("org.springframework.context.ApplicationContext"))) + .and(takesArguments(1)), + DispatcherServletInstrumentation.class.getName() + "$HandlerMappingAdvice"); transformers.put( isMethod() .and(isProtected()) .and(named("render")) .and(takesArgument(0, named("org.springframework.web.servlet.ModelAndView"))), - DispatcherServletInstrumentation.class.getName() + "$DispatcherAdvice"); + DispatcherServletInstrumentation.class.getName() + "$RenderAdvice"); transformers.put( isMethod() .and(isProtected()) @@ -62,7 +85,37 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default return transformers; } - public static class DispatcherAdvice { + /** + * This advice creates a filter that has reference to the handlerMappings from DispatcherServlet + * which allows the mappings to be evaluated at the beginning of the filter chain. This evaluation + * is done inside the Servlet3Decorator.onContext method. + */ + public static class HandlerMappingAdvice { + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void afterRefresh( + @Advice.This final DispatcherServlet dispatcher, + @Advice.Argument(0) final ApplicationContext springCtx, + @Advice.FieldValue("handlerMappings") final List handlerMappings, + @Advice.Thrown final Throwable throwable) { + final ServletContext servletContext = springCtx.getBean(ServletContext.class); + if (handlerMappings != null && servletContext != null) { + final ContextStore contextStore = + InstrumentationContext.get( + DispatcherServlet.class, HandlerMappingResourceNameFilter.class); + HandlerMappingResourceNameFilter filter = contextStore.get(dispatcher); + if (filter == null) { + filter = new HandlerMappingResourceNameFilter(); + contextStore.put(dispatcher, filter); + } + filter.setHandlerMappings(handlerMappings); + servletContext.setAttribute( + "dd.dispatcher-filter", filter); // used by Servlet3Decorator.onContext + } + } + } + + public static class RenderAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static AgentScope onEnter(@Advice.Argument(0) final ModelAndView mv) { @@ -79,11 +132,6 @@ public final class DispatcherServletInstrumentation extends Instrumenter.Default DECORATE_RENDER.beforeFinish(scope); scope.close(); } - - // Make this advice match consistently with HandlerAdapterInstrumentation - private void muzzleCheck(final HandlerMethod method) { - method.getMethod(); - } } public static class ErrorHandlerAdvice { diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HandlerMappingResourceNameFilter.java b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HandlerMappingResourceNameFilter.java new file mode 100644 index 0000000000..268a02ed41 --- /dev/null +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/main/java/datadog/trace/instrumentation/springweb/HandlerMappingResourceNameFilter.java @@ -0,0 +1,65 @@ +package datadog.trace.instrumentation.springweb; + +import static datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; +import static datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator.DECORATE; + +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import java.util.List; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import org.springframework.web.servlet.HandlerExecutionChain; +import org.springframework.web.servlet.HandlerMapping; + +public class HandlerMappingResourceNameFilter implements Filter { + private volatile List handlerMappings; + + @Override + public void init(final FilterConfig filterConfig) {} + + @Override + public void doFilter( + final ServletRequest servletRequest, + final ServletResponse servletResponse, + final FilterChain filterChain) { + if (servletRequest instanceof HttpServletRequest && handlerMappings != null) { + final HttpServletRequest request = (HttpServletRequest) servletRequest; + try { + if (findMapping(request)) { + // Name the parent span based on the matching pattern + final Object parentSpan = request.getAttribute(DD_SPAN_ATTRIBUTE); + if (parentSpan instanceof AgentSpan) { + // Let the parent span resource name be set with the attribute set in findMapping. + DECORATE.onRequest((AgentSpan) parentSpan, request); + } + } + } catch (final Exception e) { + } + } + } + + @Override + public void destroy() {} + + /** + * When a HandlerMapping matches a request, it sets HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE + * as an attribute on the request. This attribute is read by + * SpringWebHttpServerDecorator.onRequest and set as the resource name. + */ + private boolean findMapping(final HttpServletRequest request) throws Exception { + for (final HandlerMapping mapping : handlerMappings) { + final HandlerExecutionChain handler = mapping.getHandler(request); + if (handler != null) { + return true; + } + } + return false; + } + + public void setHandlerMappings(final List handlerMappings) { + this.handlerMappings = handlerMappings; + } +} diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy similarity index 98% rename from dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy rename to dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy index 1d7a41b5a8..fce7d0d2ab 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/AppConfig.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy @@ -1,4 +1,4 @@ -package test +package test.boot import org.apache.catalina.connector.Connector import org.springframework.boot.autoconfigure.SpringBootApplication diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy similarity index 96% rename from dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy rename to dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy index 860f643845..c58ca3078c 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -1,4 +1,4 @@ -package test +package test.boot import datadog.opentracing.DDSpan import datadog.trace.agent.test.asserts.ListWriterAssert @@ -51,6 +51,11 @@ class SpringBootBasedTest extends HttpServerTest true } + @Override + String testPathParam() { + "/path/{id}/param" + } + @Override boolean testNotFound() { // FIXME: the instrumentation adds an extra controller span which is not consistent. @@ -116,7 +121,7 @@ class SpringBootBasedTest extends HttpServerTest trace.span(index) { serviceName expectedServiceName() operationName expectedOperationName() - resourceName endpoint.status == 404 ? "404" : "$method ${endpoint.resolve(address).path}" + resourceName endpoint.resource(method, address, testPathParam()) spanType DDSpanTypes.HTTP_SERVER errored endpoint.errored if (parentID != null) { diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy similarity index 85% rename from dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy rename to dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy index 3380af9522..b768574995 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/TestController.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/TestController.groovy @@ -1,10 +1,11 @@ -package test +package test.boot import datadog.trace.agent.test.base.HttpServerTest import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity import org.springframework.stereotype.Controller import org.springframework.web.bind.annotation.ExceptionHandler +import org.springframework.web.bind.annotation.PathVariable import org.springframework.web.bind.annotation.RequestMapping import org.springframework.web.bind.annotation.RequestParam import org.springframework.web.bind.annotation.ResponseBody @@ -12,6 +13,7 @@ import org.springframework.web.servlet.view.RedirectView import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -35,6 +37,14 @@ class TestController { } } + @RequestMapping("/path/{id}/param") + @ResponseBody + String path_param(@PathVariable Integer id) { + HttpServerTest.controller(PATH_PARAM) { + "$id" + } + } + @RequestMapping("/redirect") @ResponseBody RedirectView redirect() { diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy new file mode 100644 index 0000000000..46367b50ae --- /dev/null +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy @@ -0,0 +1,112 @@ +package test.filter + +import datadog.trace.agent.test.base.HttpServerTest +import org.apache.catalina.connector.Connector +import org.springframework.boot.autoconfigure.SpringBootApplication +import org.springframework.boot.context.embedded.EmbeddedServletContainerFactory +import org.springframework.boot.context.embedded.tomcat.TomcatConnectorCustomizer +import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory +import org.springframework.context.annotation.Bean +import org.springframework.http.MediaType +import org.springframework.web.HttpMediaTypeNotAcceptableException +import org.springframework.web.accept.ContentNegotiationStrategy +import org.springframework.web.context.request.NativeWebRequest +import org.springframework.web.servlet.config.annotation.ContentNegotiationConfigurer +import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter + +import javax.servlet.Filter +import javax.servlet.FilterChain +import javax.servlet.FilterConfig +import javax.servlet.ServletException +import javax.servlet.ServletRequest +import javax.servlet.ServletResponse +import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +@SpringBootApplication +class FilteredAppConfig extends WebMvcConfigurerAdapter { + + @Override + void configureContentNegotiation(ContentNegotiationConfigurer configurer) { + configurer.favorPathExtension(false) + .favorParameter(true) + .ignoreAcceptHeader(true) + .useJaf(false) + .defaultContentTypeStrategy(new ContentNegotiationStrategy() { + @Override + List resolveMediaTypes(NativeWebRequest webRequest) throws HttpMediaTypeNotAcceptableException { + return [MediaType.TEXT_PLAIN, MediaType.APPLICATION_JSON] + } + }) + } + + @Bean + EmbeddedServletContainerFactory servletContainerFactory() { + def factory = new TomcatEmbeddedServletContainerFactory() + + factory.addConnectorCustomizers( + new TomcatConnectorCustomizer() { + @Override + void customize(final Connector connector) { + connector.setEnableLookups(true) + } + }) + + return factory + } + + @Bean + Filter servletFilter() { + return new Filter() { + + @Override + void init(FilterConfig filterConfig) throws ServletException { + } + + @Override + void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { + HttpServletRequest req = (HttpServletRequest) request + HttpServletResponse resp = (HttpServletResponse) response + HttpServerTest.ServerEndpoint endpoint = HttpServerTest.ServerEndpoint.forPath(req.servletPath) + HttpServerTest.controller(endpoint) { + resp.contentType = "text/plain" + switch (endpoint) { + case SUCCESS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break + case QUERY_PARAM: + resp.status = endpoint.status + resp.writer.print(req.queryString) + break + case PATH_PARAM: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break + case REDIRECT: + resp.sendRedirect(endpoint.body) + break + case ERROR: + resp.sendError(endpoint.status, endpoint.body) + break + case EXCEPTION: + throw new Exception(endpoint.body) + default: + chain.doFilter(request, response) + } + } + } + + @Override + void destroy() { + } + } + } +} diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy new file mode 100644 index 0000000000..13eb458918 --- /dev/null +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -0,0 +1,160 @@ +package test.filter + +import datadog.opentracing.DDSpan +import datadog.trace.agent.test.asserts.ListWriterAssert +import datadog.trace.agent.test.asserts.SpanAssert +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.api.DDTags +import datadog.trace.bootstrap.instrumentation.api.Tags +import datadog.trace.instrumentation.servlet3.Servlet3Decorator +import datadog.trace.instrumentation.springweb.SpringWebHttpServerDecorator +import groovy.transform.stc.ClosureParams +import groovy.transform.stc.SimpleType +import org.apache.catalina.core.ApplicationFilterChain +import org.springframework.boot.SpringApplication +import org.springframework.context.ConfigurableApplicationContext +import org.springframework.web.servlet.view.RedirectView + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS +import static java.util.Collections.singletonMap + +class ServletFilterTest extends HttpServerTest { + + @Override + ConfigurableApplicationContext startServer(int port) { + def app = new SpringApplication(FilteredAppConfig) + app.setDefaultProperties(singletonMap("server.port", port)) + def context = app.run() + return context + } + + @Override + void stopServer(ConfigurableApplicationContext ctx) { + ctx.close() + } + + @Override + String component() { + return Servlet3Decorator.DECORATE.component() + } + + @Override + String expectedOperationName() { + return "servlet.request" + } + + @Override + boolean hasHandlerSpan() { + false + } + + @Override + String testPathParam() { + "/path/{id}/param" + } + + @Override + boolean testExceptionBody() { + false + } + + @Override + boolean testNotFound() { + // FIXME: the instrumentation adds an extra controller span which is not consistent. + // Fix tests or remove extra span. + false + } + + void cleanAndAssertTraces( + final int size, + @ClosureParams(value = SimpleType, options = "datadog.trace.agent.test.asserts.ListWriterAssert") + @DelegatesTo(value = ListWriterAssert, strategy = Closure.DELEGATE_FIRST) + final Closure spec) { + + // If this is failing, make sure HttpServerTestAdvice is applied correctly. + TEST_WRITER.waitForTraces(size * 2) + + TEST_WRITER.each { + def renderSpan = it.find { + it.operationName == "response.render" + } + if (renderSpan) { + SpanAssert.assertSpan(renderSpan) { + operationName "response.render" + resourceName "response.render" + spanType "web" + errored false + tags { + "$Tags.COMPONENT" "spring-webmvc" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "view.type" RedirectView.name + defaultTags() + } + } + it.remove(renderSpan) + } + } + + super.cleanAndAssertTraces(size, spec) + } + + @Override + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + serviceName expectedServiceName() + operationName "spring.handler" + resourceName "TestController.${endpoint.name().toLowerCase()}" + spanType DDSpanTypes.HTTP_SERVER + errored endpoint == EXCEPTION + childOf(parent as DDSpan) + tags { + "$Tags.COMPONENT" SpringWebHttpServerDecorator.DECORATE.component() + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + if (endpoint == EXCEPTION) { + errorTags(Exception, EXCEPTION.body) + } + defaultTags() + } + } + } + + @Override + void serverSpan(TraceAssert trace, int index, BigInteger traceID = null, BigInteger parentID = null, String method = "GET", ServerEndpoint endpoint = SUCCESS) { + trace.span(index) { + serviceName expectedServiceName() + operationName expectedOperationName() + resourceName endpoint.resource(method, address, testPathParam()) + spanType DDSpanTypes.HTTP_SERVER + errored endpoint.errored + if (parentID != null) { + traceId traceID + parentId parentID + } else { + parent() + } + tags { + "$Tags.COMPONENT" component + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional + "$Tags.PEER_PORT" Integer + "$Tags.HTTP_URL" "${endpoint.resolve(address)}" + "$Tags.HTTP_METHOD" method + "$Tags.HTTP_STATUS" endpoint.status + "span.origin.type" ApplicationFilterChain.name + "servlet.path" endpoint.path + if (endpoint.errored) { + "error.msg" { it == null || it == EXCEPTION.body } + "error.type" { it == null || it == Exception.name } + "error.stack" { it == null || it instanceof String } + } + if (endpoint.query) { + "$DDTags.HTTP_QUERY" endpoint.query + } + defaultTags(true) + } + } + } +} diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy new file mode 100644 index 0000000000..3a33cd10e1 --- /dev/null +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/TestController.groovy @@ -0,0 +1,74 @@ +package test.filter + +import datadog.trace.agent.test.base.HttpServerTest +import org.springframework.http.HttpStatus +import org.springframework.http.ResponseEntity +import org.springframework.stereotype.Controller +import org.springframework.web.bind.annotation.ExceptionHandler +import org.springframework.web.bind.annotation.PathVariable +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.RequestParam +import org.springframework.web.bind.annotation.ResponseBody +import org.springframework.web.servlet.view.RedirectView + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +@Controller +class TestController { + + @RequestMapping("/success") + @ResponseBody + String success() { + HttpServerTest.controller(SUCCESS) { + SUCCESS.body + } + } + + @RequestMapping("/query") + @ResponseBody + String query_param(@RequestParam("some") String param) { + HttpServerTest.controller(QUERY_PARAM) { + "some=$param" + } + } + + @RequestMapping("/path/{id}/param") + @ResponseBody + String path_param(@PathVariable Integer id) { + HttpServerTest.controller(PATH_PARAM) { + "$id" + } + } + + @RequestMapping("/redirect") + @ResponseBody + RedirectView redirect() { + HttpServerTest.controller(REDIRECT) { + new RedirectView(REDIRECT.body) + } + } + + @RequestMapping("/error-status") + ResponseEntity error() { + HttpServerTest.controller(ERROR) { + new ResponseEntity(ERROR.body, HttpStatus.valueOf(ERROR.status)) + } + } + + @RequestMapping("/exception") + ResponseEntity exception() { + HttpServerTest.controller(EXCEPTION) { + throw new Exception(EXCEPTION.body) + } + } + + @ExceptionHandler + ResponseEntity handleException(Throwable throwable) { + new ResponseEntity(throwable.message, HttpStatus.INTERNAL_SERVER_ERROR) + } +} diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 67d3fcd5b1..52462ca71e 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -27,6 +27,7 @@ import static datadog.trace.agent.test.asserts.TraceAssert.assertTrace import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS @@ -92,7 +93,7 @@ abstract class HttpServerTest extends AgentTestRunner { false } - // Return the handler span's name + /** Return the handler span's name */ String reorderHandlerSpan() { null } @@ -113,6 +114,11 @@ abstract class HttpServerTest extends AgentTestRunner { true } + /** Return the expected resource name */ + String testPathParam() { + null + } + enum ServerEndpoint { SUCCESS("success", 200, "success"), REDIRECT("redirect", 302, "/redirected"), @@ -157,6 +163,10 @@ abstract class HttpServerTest extends AgentTestRunner { return address.resolve(path) } + String resource(String method, URI address, String pathParam) { + return status == 404 ? "404" : "$method ${pathParam ? pathParam : resolve(address).path}" + } + private static final Map PATH_MAP = values().collectEntries { [it.path, it] } static ServerEndpoint forPath(String path) { @@ -288,6 +298,37 @@ abstract class HttpServerTest extends AgentTestRunner { endpoint << [SUCCESS, QUERY_PARAM] } + def "test path param"() { + setup: + assumeTrue(testPathParam() != null) + def request = request(PATH_PARAM, method, body).build() + def response = client.newCall(request).execute() + + expect: + response.code() == PATH_PARAM.status + response.body().string() == PATH_PARAM.body + + and: + cleanAndAssertTraces(1) { + if (hasHandlerSpan()) { + trace(0, 3) { + serverSpan(it, 0, null, null, method, PATH_PARAM) + handlerSpan(it, 1, span(0), PATH_PARAM) + controllerSpan(it, 2, span(1)) + } + } else { + trace(0, 2) { + serverSpan(it, 0, null, null, method, PATH_PARAM) + controllerSpan(it, 1, span(0)) + } + } + } + + where: + method = "GET" + body = null + } + def "test success with multiple header attached parent"() { setup: def traceId = 123G @@ -521,7 +562,7 @@ abstract class HttpServerTest extends AgentTestRunner { trace.span(index) { serviceName expectedServiceName() operationName expectedOperationName() - resourceName endpoint.status == 404 ? "404" : "$method ${endpoint.resolve(address).path}" + resourceName endpoint.resource(method, address, testPathParam()) spanType DDSpanTypes.HTTP_SERVER errored endpoint.errored if (parentID != null) { From ce006e14056443ee856e888fe313844e7563d4c1 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 14 Apr 2020 14:31:46 -0400 Subject: [PATCH 25/37] Ensure RequestDispatcher span is part of expected trace --- .../instrumentation/api/AgentSpan.java | 2 ++ .../instrumentation/api/AgentTracer.java | 6 ++++ .../trace/agent/tooling/OpenTracing32.java | 14 +++++++++ .../servlet3/Servlet3Decorator.java | 8 +++-- .../RequestDispatcherInstrumentation.java | 19 ++++++++++-- .../test/groovy/RequestDispatcherTest.groovy | 3 ++ .../spring-webmvc-3.1.gradle | 1 + .../test/groovy/test/boot/AppConfig.groovy | 2 +- .../test/boot/SpringBootBasedTest.groovy | 15 +++++++++ .../test/filter/FilteredAppConfig.groovy | 31 ++++++++++++++++++- .../test/filter/ServletFilterTest.groovy | 15 +++++++++ .../agent/test/base/HttpServerTest.groovy | 4 ++- 12 files changed, 112 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index c049b31d0b..3d5b82cd40 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -19,6 +19,8 @@ public interface AgentSpan { AgentSpan getLocalRootSpan(); + boolean isSameTrace(AgentSpan otherSpan); + Context context(); void finish(); diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java index 0d208577be..fb992597d5 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java @@ -182,6 +182,12 @@ public class AgentTracer { return this; } + @Override + public boolean isSameTrace(final AgentSpan otherSpan) { + // Not sure if this is the best idea... + return otherSpan instanceof NoopAgentSpan; + } + @Override public Context context() { return NoopContext.INSTANCE; diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java index 3464072270..77fa060595 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java @@ -198,6 +198,20 @@ public final class OpenTracing32 implements TracerAPI { return this; } + @Override + public boolean isSameTrace(final AgentSpan otherAgentSpan) { + if (otherAgentSpan instanceof OT32Span) { + final Span otherSpan = ((OT32Span) otherAgentSpan).span; + if (span instanceof DDSpan && otherSpan instanceof DDSpan) { + // minor optimization to avoid BigInteger.toString() + return ((DDSpan) span).getTraceId().equals(((DDSpan) otherSpan).getTraceId()); + } else { + return span.context().toTraceId().equals(otherSpan.context().toTraceId()); + } + } + return false; + } + @Override public OT32Context context() { final SpanContext context = span.context(); diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java index 0c9822d6c3..d939ae2764 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java @@ -10,7 +10,9 @@ import javax.servlet.ServletContext; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import lombok.extern.slf4j.Slf4j; +@Slf4j public class Servlet3Decorator extends HttpServerDecorator { public static final Servlet3Decorator DECORATE = new Servlet3Decorator(); @@ -78,11 +80,11 @@ public class Servlet3Decorator final AgentSpan span, final HttpServletRequest request, final ServletContext context) { final Object attribute = context.getAttribute("dd.dispatcher-filter"); if (attribute instanceof Filter) { - final Filter filter = (Filter) attribute; + request.setAttribute(DD_SPAN_ATTRIBUTE, span); try { - request.setAttribute(DD_SPAN_ATTRIBUTE, span); - filter.doFilter(request, null, null); + ((Filter) attribute).doFilter(request, null, null); } catch (final IOException | ServletException e) { + log.debug("Exception unexpectedly thrown by filter", e); } } } diff --git a/dd-java-agent/instrumentation/servlet/src/main/java/datadog/trace/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java b/dd-java-agent/instrumentation/servlet/src/main/java/datadog/trace/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java index 6ff07cb0af..55be3188f9 100644 --- a/dd-java-agent/instrumentation/servlet/src/main/java/datadog/trace/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/src/main/java/datadog/trace/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java @@ -79,12 +79,27 @@ public final class RequestDispatcherInstrumentation extends Instrumenter.Default @Advice.This final RequestDispatcher dispatcher, @Advice.Local("_requestSpan") Object requestSpan, @Advice.Argument(0) final ServletRequest request) { - if (activeSpan() == null) { + final AgentSpan parentSpan = activeSpan(); + + final Object servletSpanObject = request.getAttribute(DD_SPAN_ATTRIBUTE); + final AgentSpan servletSpan = + servletSpanObject instanceof AgentSpan ? (AgentSpan) servletSpanObject : null; + + if (parentSpan == null && servletSpan == null) { // Don't want to generate a new top-level span return null; } + final AgentSpan.Context parent; + if (servletSpan == null || (parentSpan != null && servletSpan.isSameTrace(parentSpan))) { + // Use the parentSpan if the servletSpan is null or part of the same trace. + parent = parentSpan.context(); + } else { + // parentSpan is part of a different trace, so lets ignore it. + // This can happen with the way Tomcat does error handling. + parent = servletSpan.context(); + } - final AgentSpan span = startSpan("servlet." + method); + final AgentSpan span = startSpan("servlet." + method, parent); DECORATE.afterStart(span); final String target = diff --git a/dd-java-agent/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy b/dd-java-agent/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy index 04c1389f97..9804eab5c5 100644 --- a/dd-java-agent/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy +++ b/dd-java-agent/instrumentation/servlet/src/test/groovy/RequestDispatcherTest.groovy @@ -25,6 +25,7 @@ class RequestDispatcherTest extends AgentTestRunner { dispatcher.include("") then: + 2 * request.getAttribute(DD_SPAN_ATTRIBUTE) assertTraces(2) { trace(0, 1) { basicSpan(it, 0, "forward-child") @@ -45,6 +46,7 @@ class RequestDispatcherTest extends AgentTestRunner { } then: + 1 * request.getAttribute(DD_SPAN_ATTRIBUTE) assertTraces(1) { trace(0, 3) { basicSpan(it, 0, "parent") @@ -97,6 +99,7 @@ class RequestDispatcherTest extends AgentTestRunner { def th = thrown(ServletException) th == ex + 1 * request.getAttribute(DD_SPAN_ATTRIBUTE) assertTraces(1) { trace(0, 3) { basicSpan(it, 0, "parent", null, ex) diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle b/dd-java-agent/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle index cf71c6fea7..b2c76e8668 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/spring-webmvc-3.1.gradle @@ -48,6 +48,7 @@ dependencies { } // Include servlet instrumentation for verifying the tomcat requests + testCompile project(':dd-java-agent:instrumentation:servlet') testCompile project(':dd-java-agent:instrumentation:servlet:request-3') testCompile group: 'javax.validation', name: 'validation-api', version: '1.1.0.Final' diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy index fce7d0d2ab..ddc7251604 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/AppConfig.groovy @@ -25,7 +25,7 @@ class AppConfig extends WebMvcConfigurerAdapter { .defaultContentTypeStrategy(new ContentNegotiationStrategy() { @Override List resolveMediaTypes(NativeWebRequest webRequest) throws HttpMediaTypeNotAcceptableException { - return [MediaType.TEXT_PLAIN, MediaType.APPLICATION_JSON] + return [MediaType.TEXT_PLAIN] } }) } diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy index c58ca3078c..a3de856098 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -91,6 +91,21 @@ class SpringBootBasedTest extends HttpServerTest } it.remove(renderSpan) } + def responseSpan = it.find { + it.operationName == "servlet.response" + } + if (responseSpan) { + SpanAssert.assertSpan(responseSpan) { + operationName "servlet.response" + resourceName { it == "HttpServletResponse.sendRedirect" || it == "HttpServletResponse.sendError" } + errored false + tags { + "$Tags.COMPONENT" "java-web-servlet-response" + defaultTags() + } + } + it.remove(responseSpan) + } } super.cleanAndAssertTraces(size, spec) diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy index 46367b50ae..66bd9676bf 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/FilteredAppConfig.groovy @@ -1,5 +1,6 @@ package test.filter +import com.google.common.base.Charsets import datadog.trace.agent.test.base.HttpServerTest import org.apache.catalina.connector.Connector import org.springframework.boot.autoconfigure.SpringBootApplication @@ -7,7 +8,14 @@ import org.springframework.boot.context.embedded.EmbeddedServletContainerFactory import org.springframework.boot.context.embedded.tomcat.TomcatConnectorCustomizer import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory import org.springframework.context.annotation.Bean +import org.springframework.http.HttpInputMessage +import org.springframework.http.HttpOutputMessage import org.springframework.http.MediaType +import org.springframework.http.converter.AbstractHttpMessageConverter +import org.springframework.http.converter.HttpMessageConverter +import org.springframework.http.converter.HttpMessageNotReadableException +import org.springframework.http.converter.HttpMessageNotWritableException +import org.springframework.util.StreamUtils import org.springframework.web.HttpMediaTypeNotAcceptableException import org.springframework.web.accept.ContentNegotiationStrategy import org.springframework.web.context.request.NativeWebRequest @@ -42,7 +50,7 @@ class FilteredAppConfig extends WebMvcConfigurerAdapter { .defaultContentTypeStrategy(new ContentNegotiationStrategy() { @Override List resolveMediaTypes(NativeWebRequest webRequest) throws HttpMediaTypeNotAcceptableException { - return [MediaType.TEXT_PLAIN, MediaType.APPLICATION_JSON] + return [MediaType.TEXT_PLAIN] } }) } @@ -62,6 +70,27 @@ class FilteredAppConfig extends WebMvcConfigurerAdapter { return factory } + @Bean + HttpMessageConverter> createPlainMapMessageConverter() { + return new AbstractHttpMessageConverter>(MediaType.TEXT_PLAIN) { + + @Override + protected boolean supports(Class clazz) { + return Map.isAssignableFrom(clazz) + } + + @Override + protected Map readInternal(Class> clazz, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { + return null + } + + @Override + protected void writeInternal(Map stringObjectMap, HttpOutputMessage outputMessage) throws IOException, HttpMessageNotWritableException { + StreamUtils.copy(stringObjectMap.get("message"), Charsets.UTF_8, outputMessage.getBody()) + } + } + } + @Bean Filter servletFilter() { return new Filter() { diff --git a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy index 13eb458918..0644ad2672 100644 --- a/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy +++ b/dd-java-agent/instrumentation/spring-webmvc-3.1/src/test/groovy/test/filter/ServletFilterTest.groovy @@ -96,6 +96,21 @@ class ServletFilterTest extends HttpServerTest { } it.remove(renderSpan) } + def responseSpan = it.find { + it.operationName == "servlet.response" + } + if (responseSpan) { + SpanAssert.assertSpan(responseSpan) { + operationName "servlet.response" + resourceName { it == "HttpServletResponse.sendRedirect" || it == "HttpServletResponse.sendError" } + errored false + tags { + "$Tags.COMPONENT" "java-web-servlet-response" + defaultTags() + } + } + it.remove(responseSpan) + } } super.cleanAndAssertTraces(size, spec) diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy index 52462ca71e..3b14193ce8 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy @@ -140,6 +140,7 @@ abstract class HttpServerTest extends AgentTestRunner { final int status final String body final Boolean errored + final boolean hasPathParam ServerEndpoint(String uri, int status, String body) { def uriObj = URI.create(uri) @@ -149,6 +150,7 @@ abstract class HttpServerTest extends AgentTestRunner { this.status = status this.body = body this.errored = status >= 500 + this.hasPathParam = body == "123" } String getPath() { @@ -164,7 +166,7 @@ abstract class HttpServerTest extends AgentTestRunner { } String resource(String method, URI address, String pathParam) { - return status == 404 ? "404" : "$method ${pathParam ? pathParam : resolve(address).path}" + return status == 404 ? "404" : "$method ${hasPathParam ? pathParam : resolve(address).path}" } private static final Map PATH_MAP = values().collectEntries { [it.path, it] } From 8e111e49ad9aac626a634434c9ecda6e42fecae0 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Wed, 15 Apr 2020 09:57:55 -0400 Subject: [PATCH 26/37] Revert feature to calculate sample rate based on failed reporting --- .../common/writer/ddagent/TraceProcessingDisruptor.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java index 16a5889c8f..7c79b9f10b 100644 --- a/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java +++ b/dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java @@ -3,7 +3,6 @@ package datadog.trace.common.writer.ddagent; import com.lmax.disruptor.EventHandler; import datadog.common.exec.DaemonThreadFactory; import datadog.opentracing.DDSpan; -import datadog.opentracing.DDSpanContext; import datadog.trace.common.processor.TraceProcessor; import datadog.trace.common.writer.DDAgentWriter; import java.util.List; @@ -64,12 +63,7 @@ public class TraceProcessingDisruptor extends AbstractDisruptor> { final DisruptorEvent> event, final long sequence, final boolean endOfBatch) { try { if (event.data != null) { - if (1 < event.representativeCount && !event.data.isEmpty()) { - // attempt to have agent scale the metrics properly - ((DDSpan) event.data.get(0).getLocalRootSpan()) - .context() - .setMetric(DDSpanContext.SAMPLE_RATE_KEY, 1d / event.representativeCount); - } + // TODO populate `_sample_rate` metric in a way that accounts for lost/dropped traces try { event.data = processor.onTraceComplete(event.data); final byte[] serializedTrace = api.serializeTrace(event.data); From ebb9b5da6537c7bb56490149ef4f2b5cd6e49149 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 16 Apr 2020 14:23:16 -0400 Subject: [PATCH 27/37] Properly handle resource name for nested JAX-RS calls Previously the last called one was winning. Now we check if the resource name is already set before applying the new name. --- .../instrumentation/api/AgentSpan.java | 2 + .../instrumentation/api/AgentTracer.java | 5 ++ .../trace/agent/tooling/OpenTracing32.java | 8 +++ .../jaxrs1/JaxRsAnnotationsDecorator.java | 21 ++++--- .../JaxRsAnnotationsInstrumentation.java | 2 +- .../src/test/groovy/JerseyTest.groovy | 49 ++++++++++++++++ .../src/test/java/Resource.java | 6 ++ .../jaxrs2/JaxRsAnnotationsDecorator.java | 18 +++--- .../src/test/groovy/JaxRsFilterTest.groovy | 56 +++++++++++++++++++ .../src/test/java/Resource.java | 6 ++ .../datadog/opentracing/DDSpanContext.java | 4 ++ 11 files changed, 162 insertions(+), 15 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index 3d5b82cd40..9d42f196f2 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -29,5 +29,7 @@ public interface AgentSpan { void setSpanName(String spanName); + boolean hasResourceName(); + interface Context {} } diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java index fb992597d5..856bca5b42 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java @@ -203,6 +203,11 @@ public class AgentTracer { @Override public void setSpanName(final String spanName) {} + + @Override + public boolean hasResourceName() { + return false; + } } static class NoopAgentScope implements AgentScope { diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java index 77fa060595..214725f174 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/OpenTracing32.java @@ -234,6 +234,14 @@ public final class OpenTracing32 implements TracerAPI { span.setOperationName(spanName); } + @Override + public boolean hasResourceName() { + if (span instanceof DDSpan) { + return ((DDSpan) span).context().hasResourceName(); + } + return false; + } + private Span getSpan() { return span; } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java index bd39af4b7b..8cbaf29c78 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java @@ -36,10 +36,11 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { return "jax-rs-controller"; } - public void onControllerStart( + public void onJaxRsSpan( final AgentSpan span, final AgentSpan parent, final Class target, final Method method) { + final String resourceName = getPathResourceName(target, method); - updateParent(parent, resourceName); + updateRootSpan(parent, resourceName); span.setTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_SERVER); @@ -48,19 +49,24 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { if (isRootScope && !resourceName.isEmpty()) { span.setTag(DDTags.RESOURCE_NAME, resourceName); } else { - span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(target) + "." + method.getName()); + span.setTag( + DDTags.RESOURCE_NAME, + DECORATE.spanNameForClass(target) + (method == null ? "" : "." + method.getName())); } } - private void updateParent(AgentSpan span, final String resourceName) { + private void updateRootSpan(AgentSpan span, final String resourceName) { if (span == null) { return; } span = span.getLocalRootSpan(); - span.setTag(Tags.COMPONENT, "jax-rs"); - if (!resourceName.isEmpty()) { - span.setTag(DDTags.RESOURCE_NAME, resourceName); + if (!span.hasResourceName()) { + span.setTag(Tags.COMPONENT, "jax-rs"); + + if (!resourceName.isEmpty()) { + span.setTag(DDTags.RESOURCE_NAME, resourceName); + } } } @@ -72,6 +78,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { */ private String getPathResourceName(final Class target, final Method method) { Map classMap = resourceNames.get(target); + if (classMap == null) { resourceNames.putIfAbsent(target, new ConcurrentHashMap()); classMap = resourceNames.get(target); diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java index 0c71b36215..f13c86c7af 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java @@ -84,7 +84,7 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default final AgentSpan parent = activeSpan(); final AgentSpan span = startSpan(JAX_ENDPOINT_OPERATION_NAME); - DECORATE.onControllerStart(span, parent, target.getClass(), method); + DECORATE.onJaxRsSpan(span, parent, target.getClass(), method); DECORATE.afterStart(span); final AgentScope scope = activateSpan(span, true); diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JerseyTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JerseyTest.groovy index daeeca315c..6ffbadc507 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JerseyTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JerseyTest.groovy @@ -57,4 +57,53 @@ class JerseyTest extends AgentTestRunner { "/test2/hello/bob" | "POST /test2/hello/{name}" | "Test2.hello" | "Test2 bob!" "/test3/hi/bob" | "POST /test3/hi/{name}" | "Test3.hello" | "Test3 bob!" } + + def "test nested call"() { + + when: + // start a trace because the test doesn't go through any servlet or other instrumentation. + def response = runUnderTrace("test.span") { + resources.client().resource(resource).post(String) + } + + then: + response == expectedResponse + + assertTraces(1) { + trace(0, 3) { + span(0) { + operationName "test.span" + resourceName parentResourceName + tags { + "$Tags.COMPONENT" "jax-rs" + defaultTags() + } + } + span(1) { + childOf span(0) + operationName "jax-rs.request" + resourceName controller1Name + spanType DDSpanTypes.HTTP_SERVER + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + span(2) { + childOf span(1) + operationName "jax-rs.request" + resourceName controller2Name + spanType DDSpanTypes.HTTP_SERVER + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + } + } + + where: + resource | parentResourceName | controller1Name | controller2Name | expectedResponse + "/test3/nested" | "POST /test3/nested" | "Test3.nested" | "Test3.hello" | "Test3 nested!" + } } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/Resource.java b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/Resource.java index 0a505ee794..73df13fd49 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/Resource.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/Resource.java @@ -39,5 +39,11 @@ public interface Resource { public String hello(@PathParam("name") final String name) { return "Test3 " + name + "!"; } + + @POST + @Path("/nested") + public String nested() { + return hello("nested"); + } } } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java index c229a6c492..70cba43faf 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.jaxrs2; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; + import datadog.trace.agent.tooling.ClassHierarchyIterable; import datadog.trace.api.DDSpanTypes; import datadog.trace.api.DDTags; @@ -25,8 +27,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { public static final JaxRsAnnotationsDecorator DECORATE = new JaxRsAnnotationsDecorator(); - private final WeakMap, Map> resourceNames = - WeakMap.Provider.newWeakMap(); + private final WeakMap, Map> resourceNames = newWeakMap(); @Override protected String[] instrumentationNames() { @@ -47,7 +48,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { final AgentSpan span, final AgentSpan parent, final Class target, final Method method) { final String resourceName = getPathResourceName(target, method); - updateParent(parent, resourceName); + updateRootSpan(parent, resourceName); span.setTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_SERVER); @@ -62,15 +63,18 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { } } - private void updateParent(AgentSpan span, final String resourceName) { + private void updateRootSpan(AgentSpan span, final String resourceName) { if (span == null) { return; } span = span.getLocalRootSpan(); - span.setTag(Tags.COMPONENT, "jax-rs"); - if (!resourceName.isEmpty()) { - span.setTag(DDTags.RESOURCE_NAME, resourceName); + if (!span.hasResourceName()) { + span.setTag(Tags.COMPONENT, "jax-rs"); + + if (!resourceName.isEmpty()) { + span.setTag(DDTags.RESOURCE_NAME, resourceName); + } } } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsFilterTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsFilterTest.groovy index b11687316c..eaf8ab7559 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsFilterTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsFilterTest.groovy @@ -96,6 +96,62 @@ abstract class JaxRsFilterTest extends AgentTestRunner { "/test3/hi/bob" | false | true | "test.span" | "PrematchRequestFilter.filter" | "Aborted Prematch" } + def "test nested call"() { + given: + simpleRequestFilter.abort = false + prematchRequestFilter.abort = false + + when: + def responseText + def responseStatus + + // start a trace because the test doesn't go through any servlet or other instrumentation. + runUnderTrace("test.span") { + (responseText, responseStatus) = makeRequest(resource) + } + + then: + responseStatus == Response.Status.OK.statusCode + responseText == expectedResponse + + assertTraces(1) { + trace(0, 3) { + span(0) { + operationName "test.span" + resourceName parentResourceName + tags { + "$Tags.COMPONENT" "jax-rs" + defaultTags() + } + } + span(1) { + childOf span(0) + operationName "jax-rs.request" + resourceName controller1Name + spanType DDSpanTypes.HTTP_SERVER + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + span(2) { + childOf span(1) + operationName "jax-rs.request" + resourceName controller2Name + spanType DDSpanTypes.HTTP_SERVER + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + } + } + + where: + resource | parentResourceName | controller1Name | controller2Name | expectedResponse + "/test3/nested" | "POST /test3/nested" | "Test3.nested" | "Test3.hello" | "Test3 nested!" + } + @Provider class SimpleRequestFilter implements ContainerRequestFilter { boolean abort = false diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/java/Resource.java b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/java/Resource.java index 0a505ee794..73df13fd49 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/java/Resource.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/java/Resource.java @@ -39,5 +39,11 @@ public interface Resource { public String hello(@PathParam("name") final String name) { return "Test3 " + name + "!"; } + + @POST + @Path("/nested") + public String nested() { + return hello("nested"); + } } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java index 240c4358d7..cc029507fb 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpanContext.java @@ -174,6 +174,10 @@ public class DDSpanContext implements io.opentracing.SpanContext { return !(resourceName == null || resourceName.isEmpty()); } + public boolean hasResourceName() { + return isResourceNameSet() || tags.containsKey(DDTags.RESOURCE_NAME); + } + public void setResourceName(final String resourceName) { this.resourceName = resourceName; } From 07025dd9aaac8bff871474e07ad4987bc78b0881 Mon Sep 17 00:00:00 2001 From: Lev Priima <1118651+lpriima@users.noreply.github.com> Date: Thu, 16 Apr 2020 12:59:57 -0700 Subject: [PATCH 28/37] no dd.* prefix in "env" and "version" tags (#1378) --- dd-trace-api/src/main/java/datadog/trace/api/Config.java | 2 +- .../src/test/groovy/datadog/trace/api/ConfigTest.groovy | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/Config.java b/dd-trace-api/src/main/java/datadog/trace/api/Config.java index 4f512bb6c8..ba1bbc7e80 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/Config.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/Config.java @@ -1205,7 +1205,7 @@ public class Config { final Map map, final String propName) { final String val = getSettingFromEnvironment(propName, null); if (val != null) { - return !val.equals(map.put(propertyNameToSystemPropertyName(propName), val)); + return !val.equals(map.put(propName, val)); } return false; } diff --git a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy index 267abf6381..6b71342e31 100644 --- a/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy +++ b/dd-trace-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy @@ -1246,13 +1246,13 @@ class ConfigTest extends DDSpecification { setup: environmentVariables.set(DD_ENV_ENV, "test_env") environmentVariables.set(DD_VERSION_ENV, "1.2.3") - environmentVariables.set(DD_TAGS_ENV, "dd.env:production , dd.version:3.2.1") + environmentVariables.set(DD_TAGS_ENV, "env:production , version:3.2.1") when: Config config = new Config() then: - config.mergedSpanTags == ["dd.env": "test_env", "dd.version": "1.2.3"] + config.mergedSpanTags == ["env": "test_env", "version": "1.2.3"] } def "propertyNameToEnvironmentVariableName unit test"() { From 7c1f166048d08bfa2e71547c26b1b01adf228037 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 09:31:57 -0400 Subject: [PATCH 29/37] Use newer version of jnr-unixsocket --- dd-trace-ot/dd-trace-ot.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index fe14a57d17..5676c00d80 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -45,7 +45,7 @@ dependencies { compile deps.okhttp compile group: 'org.msgpack', name: 'msgpack-core', version: '0.8.20' compile group: 'com.squareup.moshi', name: 'moshi', version: '1.9.2' - compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.23' + compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.28' compile group: 'com.lmax', name: 'disruptor', version: '3.4.2' // We have autoservices defined in test subtree, looks like we need this to be able to properly rebuild this From ad0960a922fd8d4ad431823a45c97f7e67a54faf Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 09:29:09 -0400 Subject: [PATCH 30/37] Use newer version of statsd client --- dd-trace-ot/dd-trace-ot.gradle | 2 +- gradle/dependencies.gradle | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index 5676c00d80..d638b75bc3 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -39,7 +39,7 @@ dependencies { compile deps.opentracing compile group: 'io.opentracing.contrib', name: 'opentracing-tracerresolver', version: '0.1.0' - compile group: 'com.datadoghq', name: 'java-dogstatsd-client', version: '2.1.1' + compile group: 'com.datadoghq', name: 'java-dogstatsd-client', version: "${versions.dogstatsd}" compile deps.slf4j compile deps.okhttp diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 1a2aa072f6..88ea4ecde7 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -17,7 +17,8 @@ ext { bytebuddy : "1.10.6", scala : "2.11.12", // Last version to support Java 7 (2.12+ require Java 8+) kotlin : "1.3.61", - coroutines : "1.3.0" + coroutines : "1.3.0", + dogstatsd : "2.9.0" ] deps = [ @@ -67,9 +68,7 @@ ext { // Shared between agent tooling and instrumentation and JMXFetch shared : [ - dependencies.create(group: 'com.datadoghq', name: 'java-dogstatsd-client', version: '2.8'), - // "Explicit override jnr version because .22 caused issues" - mar-kolya - dependencies.create(group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.23'), + dependencies.create(group: 'com.datadoghq', name: 'java-dogstatsd-client', version: "${versions.dogstatsd}"), dependencies.create(group: 'com.google.guava', name: 'guava', version: "${versions.guava}") ], From 00ac902a8ff9d4bcfd3d64b1ad45e638f57c46f7 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 09:56:26 -0400 Subject: [PATCH 31/37] Include specific unixsocket lib version into shared jar --- dd-trace-ot/dd-trace-ot.gradle | 2 +- gradle/dependencies.gradle | 30 ++++++++++++++++-------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/dd-trace-ot/dd-trace-ot.gradle b/dd-trace-ot/dd-trace-ot.gradle index d638b75bc3..8d81c1a031 100644 --- a/dd-trace-ot/dd-trace-ot.gradle +++ b/dd-trace-ot/dd-trace-ot.gradle @@ -45,7 +45,7 @@ dependencies { compile deps.okhttp compile group: 'org.msgpack', name: 'msgpack-core', version: '0.8.20' compile group: 'com.squareup.moshi', name: 'moshi', version: '1.9.2' - compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: '0.28' + compile group: 'com.github.jnr', name: 'jnr-unixsocket', version: "${versions.jnr_unixsocket}" compile group: 'com.lmax', name: 'disruptor', version: '3.4.2' // We have autoservices defined in test subtree, looks like we need this to be able to properly rebuild this diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 88ea4ecde7..0dbe2e3f21 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -3,22 +3,23 @@ def spockGroovyVer = groovyVer.replaceAll(/\.\d+$/, '') ext { versions = [ - opentracing: '0.32.0', + opentracing : '0.32.0', - slf4j : "1.7.29", - guava : "20.0", // Last version to support Java 7 - okhttp : "3.12.8", // 3.12.x is last version to support Java7) + slf4j : "1.7.29", + guava : "20.0", // Last version to support Java 7 + okhttp : "3.12.8", // 3.12.x is last version to support Java7) - spock : "1.3-groovy-$spockGroovyVer", - groovy : groovyVer, - junit5 : "5.5.2", - logback : "1.2.3", - lombok : "1.18.10", - bytebuddy : "1.10.6", - scala : "2.11.12", // Last version to support Java 7 (2.12+ require Java 8+) - kotlin : "1.3.61", - coroutines : "1.3.0", - dogstatsd : "2.9.0" + spock : "1.3-groovy-$spockGroovyVer", + groovy : groovyVer, + junit5 : "5.5.2", + logback : "1.2.3", + lombok : "1.18.10", + bytebuddy : "1.10.6", + scala : "2.11.12", // Last version to support Java 7 (2.12+ require Java 8+) + kotlin : "1.3.61", + coroutines : "1.3.0", + dogstatsd : "2.9.0", + jnr_unixsocket: "0.28" ] deps = [ @@ -69,6 +70,7 @@ ext { // Shared between agent tooling and instrumentation and JMXFetch shared : [ dependencies.create(group: 'com.datadoghq', name: 'java-dogstatsd-client', version: "${versions.dogstatsd}"), + dependencies.create(group: 'com.github.jnr', name: 'jnr-unixsocket', version: "${versions.jnr_unixsocket}"), dependencies.create(group: 'com.google.guava', name: 'guava', version: "${versions.guava}") ], From 1ec76d68b78057efdb2ee7a9bb28eb8a9ad26a18 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 10:33:50 -0400 Subject: [PATCH 32/37] update okhttp --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 1a2aa072f6..52fc60be12 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -7,7 +7,7 @@ ext { slf4j : "1.7.29", guava : "20.0", // Last version to support Java 7 - okhttp : "3.12.8", // 3.12.x is last version to support Java7) + okhttp : "3.12.10", // 3.12.x is last version to support Java7) spock : "1.3-groovy-$spockGroovyVer", groovy : groovyVer, From 2fc06d9c8508e8feb940996739f12e2d22256f1d Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 10:34:34 -0400 Subject: [PATCH 33/37] update slf4j --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 52fc60be12..d5ecf9dcc7 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -5,7 +5,7 @@ ext { versions = [ opentracing: '0.32.0', - slf4j : "1.7.29", + slf4j : "1.7.30", guava : "20.0", // Last version to support Java 7 okhttp : "3.12.10", // 3.12.x is last version to support Java7) From 20bbc6d9da34d8bc481a2636e2ebcb29ce643f13 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 10:36:01 -0400 Subject: [PATCH 34/37] update junit5 --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index d5ecf9dcc7..071f86fa5a 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -11,7 +11,7 @@ ext { spock : "1.3-groovy-$spockGroovyVer", groovy : groovyVer, - junit5 : "5.5.2", + junit5 : "5.6.2", logback : "1.2.3", lombok : "1.18.10", bytebuddy : "1.10.6", From 5f9bd1e0828cb341f5f5917890dc226757a11c6a Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 10:36:51 -0400 Subject: [PATCH 35/37] update kotlin --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 071f86fa5a..257dd0fb90 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -16,7 +16,7 @@ ext { lombok : "1.18.10", bytebuddy : "1.10.6", scala : "2.11.12", // Last version to support Java 7 (2.12+ require Java 8+) - kotlin : "1.3.61", + kotlin : "1.3.72", coroutines : "1.3.0" ] From 0d11207123b17ebce056a0593c279e3838b63c39 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Fri, 17 Apr 2020 10:38:47 -0400 Subject: [PATCH 36/37] update groovy --- gradle/dependencies.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 257dd0fb90..9d25939100 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -1,4 +1,4 @@ -def groovyVer = "2.5.10" +def groovyVer = "2.5.11" def spockGroovyVer = groovyVer.replaceAll(/\.\d+$/, '') ext { From 185ad185dec35848b22053bb670faf6dd467ab8d Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Fri, 17 Apr 2020 18:14:54 +0200 Subject: [PATCH 37/37] PROF-1037: Capture thread CPU time elapsed in scope (#1358) --- .../java/datadog/trace/bootstrap/Agent.java | 31 +++++-- .../profiling/ProfilingTestApplication.java | 22 ++++- ...ngIntegrationContinuousProfilesTest.groovy | 13 ++- dd-trace-ot/jfr-openjdk/jfr-openjdk.gradle | 1 + .../opentracing/jfr/openjdk/ScopeEvent.java | 11 +++ .../jfr/openjdk}/ScopeEventFactoryTest.groovy | 4 +- .../jfr/openjdk}/ScopeEventTest.groovy | 85 ++++++++++++++++++- .../opentracing/jfr/openjdk}/JfrHelper.java | 2 + .../common/util/JmxThreadCpuTimeProvider.java | 22 +++++ .../util/NoneThreadCpuTimeProvider.java | 8 ++ .../common/util/ThreadCpuTimeAccess.java | 59 +++++++++++++ .../common/util/ThreadCpuTimeProvider.java | 16 ++++ .../util/ThreadCpuTimeAccessTest.groovy | 83 ++++++++++++++++++ 13 files changed, 345 insertions(+), 12 deletions(-) rename dd-trace-ot/jfr-openjdk/src/test/groovy/{ => datadog/opentracing/jfr/openjdk}/ScopeEventFactoryTest.groovy (85%) rename dd-trace-ot/jfr-openjdk/src/test/groovy/{ => datadog/opentracing/jfr/openjdk}/ScopeEventTest.groovy (51%) rename dd-trace-ot/jfr-openjdk/src/test/{groovy => java/datadog/opentracing/jfr/openjdk}/JfrHelper.java (94%) create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/util/JmxThreadCpuTimeProvider.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/util/NoneThreadCpuTimeProvider.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeAccess.java create mode 100644 dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeProvider.java create mode 100644 dd-trace-ot/src/test/groovy/datadog/trace/common/util/ThreadCpuTimeAccessTest.groovy diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 2ffc541f6e..898933771f 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -72,9 +72,9 @@ public class Agent { */ if (appUsingCustomLogManager) { log.debug("Custom logger detected. Delaying JMXFetch initialization."); - registerLogManagerCallback(new StartJmxFetchCallback(bootstrapURL)); + registerLogManagerCallback(new StartJmxCallback(bootstrapURL)); } else { - startJmxFetch(bootstrapURL); + startJmx(bootstrapURL); } /* @@ -151,8 +151,8 @@ public class Agent { public abstract void execute(); } - protected static class StartJmxFetchCallback extends ClassLoadCallBack { - StartJmxFetchCallback(final URL bootstrapURL) { + protected static class StartJmxCallback extends ClassLoadCallBack { + StartJmxCallback(final URL bootstrapURL) { super(bootstrapURL); } @@ -163,7 +163,7 @@ public class Agent { @Override public void execute() { - startJmxFetch(bootstrapURL); + startJmx(bootstrapURL); } } @@ -262,6 +262,27 @@ public class Agent { } } + private static synchronized void startJmx(final URL bootstrapURL) { + startJmxFetch(bootstrapURL); + initializeJmxThreadCpuTimeProvider(); + } + + /** Enable JMX based thread CPU time provider once it is safe to touch JMX */ + private static synchronized void initializeJmxThreadCpuTimeProvider() { + log.info("Initializing JMX thread CPU time provider"); + if (AGENT_CLASSLOADER == null) { + throw new IllegalStateException("Datadog agent should have been started already"); + } + try { + final Class tracerInstallerClass = + AGENT_CLASSLOADER.loadClass("datadog.trace.common.util.ThreadCpuTimeAccess"); + final Method enableJmxMethod = tracerInstallerClass.getMethod("enableJmx"); + enableJmxMethod.invoke(null); + } catch (final Throwable ex) { + log.error("Throwable thrown while initializing JMX thread CPU time provider", ex); + } + } + private static synchronized void startJmxFetch(final URL bootstrapURL) { if (JMXFETCH_CLASSLOADER == null) { final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); diff --git a/dd-smoke-tests/profiling-integration-tests/src/main/java/datadog/smoketest/profiling/ProfilingTestApplication.java b/dd-smoke-tests/profiling-integration-tests/src/main/java/datadog/smoketest/profiling/ProfilingTestApplication.java index 3d693a4008..c9bf3a4053 100644 --- a/dd-smoke-tests/profiling-integration-tests/src/main/java/datadog/smoketest/profiling/ProfilingTestApplication.java +++ b/dd-smoke-tests/profiling-integration-tests/src/main/java/datadog/smoketest/profiling/ProfilingTestApplication.java @@ -1,9 +1,13 @@ package datadog.smoketest.profiling; import datadog.trace.api.Trace; +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; +import java.util.Random; import java.util.concurrent.TimeUnit; public class ProfilingTestApplication { + private static final ThreadMXBean THREAD_MX_BEAN = ManagementFactory.getThreadMXBean(); public static void main(final String[] args) throws InterruptedException { long exitDelay = -1; @@ -23,6 +27,22 @@ public class ProfilingTestApplication { @Trace private static void tracedMethod() throws InterruptedException { System.out.println("Tracing"); - Thread.sleep(100); + tracedBusyMethod(); + Thread.sleep(50); + } + + @Trace + private static void tracedBusyMethod() { + long startTime = THREAD_MX_BEAN.getCurrentThreadCpuTime(); + Random random = new Random(); + long accumulator = 0L; + while (true) { + accumulator += random.nextInt(113); + if (THREAD_MX_BEAN.getCurrentThreadCpuTime() - startTime > 10_000_000L) { + // looking for at least 10ms CPU time + break; + } + } + System.out.println("accumulated: " + accumulator); } } diff --git a/dd-smoke-tests/profiling-integration-tests/src/test/groovy/datadog/smoketest/ProfilingIntegrationContinuousProfilesTest.groovy b/dd-smoke-tests/profiling-integration-tests/src/test/groovy/datadog/smoketest/ProfilingIntegrationContinuousProfilesTest.groovy index a76b2ee42f..7643d86885 100644 --- a/dd-smoke-tests/profiling-integration-tests/src/test/groovy/datadog/smoketest/ProfilingIntegrationContinuousProfilesTest.groovy +++ b/dd-smoke-tests/profiling-integration-tests/src/test/groovy/datadog/smoketest/ProfilingIntegrationContinuousProfilesTest.groovy @@ -6,8 +6,11 @@ import net.jpountz.lz4.LZ4FrameInputStream import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest +import org.openjdk.jmc.common.item.Aggregators +import org.openjdk.jmc.common.item.Attribute import org.openjdk.jmc.common.item.IItemCollection import org.openjdk.jmc.common.item.ItemFilters +import org.openjdk.jmc.common.unit.UnitLookup import org.openjdk.jmc.flightrecorder.JfrLoaderToolkit import java.time.Instant @@ -101,6 +104,14 @@ class ProfilingIntegrationContinuousProfilesTest extends AbstractSmokeTest { IItemCollection scopeEvents = events.apply(ItemFilters.type("datadog.Scope")) scopeEvents.size() > 0 - } + def cpuTimeAttr = Attribute.attr("cpuTime", "cpuTime", UnitLookup.TIMESPAN) + + // filter out scope events without CPU time data + def filteredScopeEvents = scopeEvents.apply(ItemFilters.more(cpuTimeAttr, UnitLookup.NANOSECOND.quantity(Long.MIN_VALUE))) + // make sure there is at least one scope event with CPU time data + filteredScopeEvents.size() > 0 + + filteredScopeEvents.getAggregate(Aggregators.min("datadog.Scope", cpuTimeAttr)).longValue() >= 10_000L + } } diff --git a/dd-trace-ot/jfr-openjdk/jfr-openjdk.gradle b/dd-trace-ot/jfr-openjdk/jfr-openjdk.gradle index 3500efa998..8f37a36d41 100644 --- a/dd-trace-ot/jfr-openjdk/jfr-openjdk.gradle +++ b/dd-trace-ot/jfr-openjdk/jfr-openjdk.gradle @@ -9,6 +9,7 @@ apply plugin: 'idea' dependencies { compile deps.slf4j compile project(':dd-trace-ot') + testCompile project(':dd-java-agent:testing') } /* diff --git a/dd-trace-ot/jfr-openjdk/src/main/java/datadog/opentracing/jfr/openjdk/ScopeEvent.java b/dd-trace-ot/jfr-openjdk/src/main/java/datadog/opentracing/jfr/openjdk/ScopeEvent.java index cdefad81ea..55b1298599 100644 --- a/dd-trace-ot/jfr-openjdk/src/main/java/datadog/opentracing/jfr/openjdk/ScopeEvent.java +++ b/dd-trace-ot/jfr-openjdk/src/main/java/datadog/opentracing/jfr/openjdk/ScopeEvent.java @@ -2,12 +2,14 @@ package datadog.opentracing.jfr.openjdk; import datadog.opentracing.DDSpanContext; import datadog.opentracing.jfr.DDScopeEvent; +import datadog.trace.common.util.ThreadCpuTimeAccess; import jdk.jfr.Category; import jdk.jfr.Description; import jdk.jfr.Event; import jdk.jfr.Label; import jdk.jfr.Name; import jdk.jfr.StackTrace; +import jdk.jfr.Timespan; @Name("datadog.Scope") @Label("Scope") @@ -38,6 +40,11 @@ public final class ScopeEvent extends Event implements DDScopeEvent { @Label("Operation Name") private String operationName; + @Label("Thread CPU Time") + @Timespan + // does not need to be volatile since the event is created and committed from the same thread + private long cpuTime = 0L; + ScopeEvent(final DDSpanContext spanContext) { this.spanContext = spanContext; } @@ -45,6 +52,7 @@ public final class ScopeEvent extends Event implements DDScopeEvent { @Override public void start() { if (isEnabled()) { + cpuTime = ThreadCpuTimeAccess.getCurrentThreadCpuTime(); begin(); } } @@ -53,6 +61,9 @@ public final class ScopeEvent extends Event implements DDScopeEvent { public void finish() { end(); if (shouldCommit()) { + if (cpuTime > 0) { + cpuTime = ThreadCpuTimeAccess.getCurrentThreadCpuTime() - cpuTime; + } traceId = spanContext.getTraceId().toString(IDS_RADIX); spanId = spanContext.getSpanId().toString(IDS_RADIX); parentId = spanContext.getParentId().toString(IDS_RADIX); diff --git a/dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventFactoryTest.groovy b/dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventFactoryTest.groovy similarity index 85% rename from dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventFactoryTest.groovy rename to dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventFactoryTest.groovy index ea30c156c3..85a52c9f9b 100644 --- a/dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventFactoryTest.groovy +++ b/dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventFactoryTest.groovy @@ -1,6 +1,6 @@ +package datadog.opentracing.jfr.openjdk + import datadog.opentracing.jfr.DDNoopScopeEvent -import datadog.opentracing.jfr.openjdk.ScopeEvent -import datadog.opentracing.jfr.openjdk.ScopeEventFactory import spock.lang.Requires import spock.lang.Specification diff --git a/dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventTest.groovy b/dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventTest.groovy similarity index 51% rename from dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventTest.groovy rename to dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventTest.groovy index a687ac9655..e7f020137e 100644 --- a/dd-trace-ot/jfr-openjdk/src/test/groovy/ScopeEventTest.groovy +++ b/dd-trace-ot/jfr-openjdk/src/test/groovy/datadog/opentracing/jfr/openjdk/ScopeEventTest.groovy @@ -1,21 +1,26 @@ +package datadog.opentracing.jfr.openjdk + import datadog.opentracing.DDSpanContext import datadog.opentracing.DDTracer import datadog.opentracing.PendingTrace +import datadog.trace.agent.test.utils.ConfigUtils +import datadog.trace.api.Config import datadog.trace.api.sampling.PrioritySampling import datadog.trace.common.sampling.RateByServiceSampler +import datadog.trace.common.util.ThreadCpuTimeAccess import datadog.trace.common.writer.ListWriter import datadog.trace.context.TraceScope +import datadog.trace.util.test.DDSpecification import io.opentracing.Scope import io.opentracing.Span import spock.lang.Requires -import spock.lang.Specification import java.time.Duration import static datadog.trace.api.Config.DEFAULT_SERVICE_NAME @Requires({ jvm.java11Compatible }) -class ScopeEventTest extends Specification { +class ScopeEventTest extends DDSpecification { private static final int IDS_RADIX = 16 private static final Duration SLEEP_DURATION = Duration.ofSeconds(1) @@ -45,8 +50,12 @@ class ScopeEventTest extends Specification { .withServiceName("test service") .withResourceName("test resource") - def "Scope event is written"() { + def "Scope event is written with thread CPU time"() { setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "true") + } + ThreadCpuTimeAccess.enableJmx() def recording = JfrHelper.startRecording() when: @@ -68,6 +77,76 @@ class ScopeEventTest extends Specification { event.getString("serviceName") == "test service" event.getString("resourceName") == "test resource" event.getString("operationName") == "test operation" + event.getLong("cpuTime") != Long.MIN_VALUE + + cleanup: + ThreadCpuTimeAccess.disableJmx() + } + + def "Scope event is written without thread CPU time - profiling enabled"() { + setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "true") + } + ThreadCpuTimeAccess.disableJmx() + def recording = JfrHelper.startRecording() + + when: + Scope scope = builder.startActive(false) + Span span = scope.span() + sleep(SLEEP_DURATION.toMillis()) + scope.close() + def events = JfrHelper.stopRecording(recording) + span.finish() + + then: + events.size() == 1 + def event = events[0] + event.eventType.name == "datadog.Scope" + event.duration >= SLEEP_DURATION + event.getString("traceId") == span.context().traceId.toString(IDS_RADIX) + event.getString("spanId") == span.context().spanId.toString(IDS_RADIX) + event.getString("parentId") == span.context().parentId.toString(IDS_RADIX) + event.getString("serviceName") == "test service" + event.getString("resourceName") == "test resource" + event.getString("operationName") == "test operation" + event.getLong("cpuTime") == Long.MIN_VALUE + + cleanup: + ThreadCpuTimeAccess.disableJmx() + } + + def "Scope event is written without thread CPU time - profiling disabled"() { + setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "false") + } + ThreadCpuTimeAccess.enableJmx() + def recording = JfrHelper.startRecording() + + when: + Scope scope = builder.startActive(false) + Span span = scope.span() + sleep(SLEEP_DURATION.toMillis()) + scope.close() + def events = JfrHelper.stopRecording(recording) + span.finish() + + then: + events.size() == 1 + def event = events[0] + event.eventType.name == "datadog.Scope" + event.duration >= SLEEP_DURATION + event.getString("traceId") == span.context().traceId.toString(IDS_RADIX) + event.getString("spanId") == span.context().spanId.toString(IDS_RADIX) + event.getString("parentId") == span.context().parentId.toString(IDS_RADIX) + event.getString("serviceName") == "test service" + event.getString("resourceName") == "test resource" + event.getString("operationName") == "test operation" + event.getLong("cpuTime") == Long.MIN_VALUE + + cleanup: + ThreadCpuTimeAccess.disableJmx() } def "Scope event is written after continuation activation"() { diff --git a/dd-trace-ot/jfr-openjdk/src/test/groovy/JfrHelper.java b/dd-trace-ot/jfr-openjdk/src/test/java/datadog/opentracing/jfr/openjdk/JfrHelper.java similarity index 94% rename from dd-trace-ot/jfr-openjdk/src/test/groovy/JfrHelper.java rename to dd-trace-ot/jfr-openjdk/src/test/java/datadog/opentracing/jfr/openjdk/JfrHelper.java index 9bbf5d6678..d91785fe6d 100644 --- a/dd-trace-ot/jfr-openjdk/src/test/groovy/JfrHelper.java +++ b/dd-trace-ot/jfr-openjdk/src/test/java/datadog/opentracing/jfr/openjdk/JfrHelper.java @@ -1,3 +1,5 @@ +package datadog.opentracing.jfr.openjdk; + import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/util/JmxThreadCpuTimeProvider.java b/dd-trace-ot/src/main/java/datadog/trace/common/util/JmxThreadCpuTimeProvider.java new file mode 100644 index 0000000000..f1c8ac2a79 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/util/JmxThreadCpuTimeProvider.java @@ -0,0 +1,22 @@ +package datadog.trace.common.util; + +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; + +/** Thread CPU time provider based on {@linkplain ThreadMXBean#getCurrentThreadCpuTime()} */ +final class JmxThreadCpuTimeProvider implements ThreadCpuTimeProvider { + private final ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean(); + + public static final JmxThreadCpuTimeProvider INSTANCE = new JmxThreadCpuTimeProvider(); + + private JmxThreadCpuTimeProvider() {} + + /** + * @return the actual thread CPU time as reported by {@linkplain + * ThreadMXBean#getCurrentThreadCpuTime()} + */ + @Override + public long getThreadCpuTime() { + return threadMXBean.getCurrentThreadCpuTime(); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/util/NoneThreadCpuTimeProvider.java b/dd-trace-ot/src/main/java/datadog/trace/common/util/NoneThreadCpuTimeProvider.java new file mode 100644 index 0000000000..aaef7ddce8 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/util/NoneThreadCpuTimeProvider.java @@ -0,0 +1,8 @@ +package datadog.trace.common.util; + +final class NoneThreadCpuTimeProvider implements ThreadCpuTimeProvider { + @Override + public long getThreadCpuTime() { + return Long.MIN_VALUE; + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeAccess.java b/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeAccess.java new file mode 100644 index 0000000000..dc7480a9ad --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeAccess.java @@ -0,0 +1,59 @@ +package datadog.trace.common.util; + +import datadog.trace.api.Config; +import lombok.extern.slf4j.Slf4j; + +/** + * Thread CPU time accessor.
+ * This class abstracts away the actual method used to get the current thread CPU time. + */ +@Slf4j +public final class ThreadCpuTimeAccess { + private static volatile ThreadCpuTimeProvider cpuTimeProvider = ThreadCpuTimeProvider.NONE; + + /** + * Disable JMX based thread CPU time. Will flip back to the {@linkplain + * ThreadCpuTimeProvider#NONE} implementation. + */ + public static void disableJmx() { + log.debug("Disabling JMX thread CPU time provider"); + cpuTimeProvider = ThreadCpuTimeProvider.NONE; + } + + /** Enable JMX based thread CPU time */ + public static void enableJmx() { + if (!Config.get().isProfilingEnabled()) { + log.debug("Will not enable thread CPU time access. Profiling is disabled."); + return; + } + try { + log.debug("Enabling JMX thread CPU time provider"); + /* + * Can not use direct class reference to JmxThreadCpuTimeProvider since on some rare JVM implementations + * using eager class resolution that class could be resolved at the moment when ThreadCpuTime is being loaded, + * potentially triggering j.u.l initialization which is potentially dangerous and can be done only at certain + * point in time. + * Using reflection should alleviate this problem - no class constant to resolve during class load. The JMX + * thread cpu time provider will be loaded at exact moment when the reflection code is executed. Then it is up + * to the caller to ensure that it is safe to use JMX. + */ + cpuTimeProvider = + (ThreadCpuTimeProvider) + Class.forName("datadog.trace.common.util.JmxThreadCpuTimeProvider") + .getField("INSTANCE") + .get(null); + } catch (ClassNotFoundException | NoSuchFieldException | IllegalAccessException e) { + log.info("Unable to initialize JMX thread CPU time provider", e); + } + } + + /** + * Get the current thread CPU time + * + * @return the actual current thread CPU time or {@linkplain Long#MIN_VALUE} if the JMX provider + * is not available + */ + public static long getCurrentThreadCpuTime() { + return cpuTimeProvider.getThreadCpuTime(); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeProvider.java b/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeProvider.java new file mode 100644 index 0000000000..380b3f8241 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/trace/common/util/ThreadCpuTimeProvider.java @@ -0,0 +1,16 @@ +package datadog.trace.common.util; + +/** + * A pluggable thread CPU time provider used by {@linkplain ThreadCpuTimeAccess}. {@linkplain + * ThreadCpuTimeAccess} may not use JMX classes (even via transitive dependencies) due to potential + * race in j.u.l initialization. Therefore it uses an abstract {@linkplain ThreadCpuTimeProvider} + * type to hold the actual implementation which may be switched between the {@linkplain + * ThreadCpuTimeProvider#NONE} and {@linkplain JmxThreadCpuTimeProvider} on-the-fly once JMX is safe + * to use. + */ +interface ThreadCpuTimeProvider { + ThreadCpuTimeProvider NONE = new NoneThreadCpuTimeProvider(); + + /** Get the current thread CPU time */ + long getThreadCpuTime(); +} diff --git a/dd-trace-ot/src/test/groovy/datadog/trace/common/util/ThreadCpuTimeAccessTest.groovy b/dd-trace-ot/src/test/groovy/datadog/trace/common/util/ThreadCpuTimeAccessTest.groovy new file mode 100644 index 0000000000..88f6f1e85b --- /dev/null +++ b/dd-trace-ot/src/test/groovy/datadog/trace/common/util/ThreadCpuTimeAccessTest.groovy @@ -0,0 +1,83 @@ +package datadog.trace.common.util + +import datadog.trace.agent.test.utils.ConfigUtils +import datadog.trace.api.Config +import datadog.trace.util.test.DDSpecification + +class ThreadCpuTimeAccessTest extends DDSpecification { + def "No thread CPU time provider - profiling enabled"() { + setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "true") + } + ThreadCpuTimeAccess.disableJmx() + + when: + def threadCpuTime1 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + // burn some cpu + def sum = 0 + for (int i = 0; i < 10_000; i++) { + sum += i + } + def threadCpuTime2 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + + then: + sum > 0 + threadCpuTime1 == Long.MIN_VALUE + threadCpuTime2 == Long.MIN_VALUE + + cleanup: + ThreadCpuTimeAccess.disableJmx() + } + + def "No thread CPU time provider - profiling disabled"() { + setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "false") + } + ThreadCpuTimeAccess.enableJmx() + + when: + def threadCpuTime1 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + // burn some cpu + def sum = 0 + for (int i = 0; i < 10_000; i++) { + sum += i + } + def threadCpuTime2 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + + then: + sum > 0 + threadCpuTime1 == Long.MIN_VALUE + threadCpuTime2 == Long.MIN_VALUE + + cleanup: + ThreadCpuTimeAccess.disableJmx() + } + + def "JMX thread CPU time provider"() { + setup: + ConfigUtils.updateConfig { + System.properties.setProperty("dd.${Config.PROFILING_ENABLED}", "true") + } + ThreadCpuTimeAccess.enableJmx() + + when: + def threadCpuTime1 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + // burn some cpu + def sum = 0 + for (int i = 0; i < 10_000; i++) { + sum += i + } + def threadCpuTime2 = ThreadCpuTimeAccess.getCurrentThreadCpuTime() + + then: + sum > 0 + threadCpuTime1 != Long.MIN_VALUE + threadCpuTime2 != Long.MIN_VALUE + threadCpuTime2 > threadCpuTime1 + + cleanup: + ThreadCpuTimeAccess.disableJmx() + } +}