From 34242e4849e51bf2b81cac8610caa60381fa9ff6 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 26 Feb 2019 16:21:50 -0800 Subject: [PATCH 1/9] Migrate method tracing instrumentation to Decorator --- .../trace/agent/decorator/BaseDecorator.java | 43 ++++++- .../agent/decorator/BaseDecoratorTest.groovy | 115 ++++++++++++++---- .../DatabaseClientDecoratorTest.groovy | 2 +- .../trace_annotation/TraceAdvice.java | 31 +---- .../TraceAnnotationsInstrumentation.java | 7 ++ .../TraceConfigInstrumentation.java | 7 ++ .../trace_annotation/TraceDecorator.java | 22 ++++ 7 files changed, 173 insertions(+), 54 deletions(-) create mode 100644 dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceDecorator.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java index 6a265a88ac..2d83f4c839 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java @@ -4,8 +4,10 @@ import static io.opentracing.log.Fields.ERROR_OBJECT; import datadog.trace.api.Config; import datadog.trace.api.DDTags; +import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.tag.Tags; +import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collections; import java.util.TreeSet; @@ -16,11 +18,13 @@ public abstract class BaseDecorator { protected final float traceAnalyticsSampleRate; protected BaseDecorator() { + final String[] instrumentationNames = instrumentationNames(); traceAnalyticsEnabled = - Config.traceAnalyticsIntegrationEnabled( - new TreeSet<>(Arrays.asList(instrumentationNames())), traceAnalyticsDefault()); + instrumentationNames.length > 0 + && Config.traceAnalyticsIntegrationEnabled( + new TreeSet<>(Arrays.asList(instrumentationNames)), traceAnalyticsDefault()); float rate = 1.0f; - for (final String name : instrumentationNames()) { + for (final String name : instrumentationNames) { rate = Config.getFloatSettingFromEnvironment( "integration." + name + ".analytics.sample-rate", rate); @@ -38,6 +42,12 @@ public abstract class BaseDecorator { return false; } + public Scope afterStart(final Scope scope) { + assert scope != null; + afterStart(scope.span()); + return scope; + } + public Span afterStart(final Span span) { assert span != null; span.setTag(DDTags.SPAN_TYPE, spanType()); @@ -48,11 +58,23 @@ public abstract class BaseDecorator { return span; } + public Scope beforeFinish(final Scope scope) { + assert scope != null; + beforeFinish(scope.span()); + return scope; + } + public Span beforeFinish(final Span span) { assert span != null; return span; } + public Scope onError(final Scope scope, final Throwable throwable) { + assert scope != null; + onError(scope.span(), throwable); + return scope; + } + public Span onError(final Span span, final Throwable throwable) { assert span != null; if (throwable != null) { @@ -61,4 +83,19 @@ public abstract class BaseDecorator { } return span; } + + public String spanNameForMethod(final Method method) { + final Class declaringClass = method.getDeclaringClass(); + String className = declaringClass.getSimpleName(); + if (className.isEmpty()) { + className = declaringClass.getName(); + if (declaringClass.getPackage() != null) { + final String pkgName = declaringClass.getPackage().getName(); + if (!pkgName.isEmpty()) { + className = declaringClass.getName().replace(pkgName, "").substring(1); + } + } + } + return className + "." + method.getName(); + } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy index 7d87742b44..27d24031d8 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy @@ -1,6 +1,7 @@ package datadog.trace.agent.decorator import datadog.trace.api.DDTags +import io.opentracing.Scope import io.opentracing.Span import io.opentracing.tag.Tags import spock.lang.Shared @@ -52,36 +53,81 @@ class BaseDecoratorTest extends Specification { def "test assert null span"() { when: - decorator.afterStart(null) + decorator.afterStart((Span) null) then: thrown(AssertionError) when: - decorator.onError(null, null) + decorator.onError((Span) null, null) then: thrown(AssertionError) when: - decorator.beforeFinish(null) + decorator.beforeFinish((Span) null) then: thrown(AssertionError) } + def "test assert null scope"() { + when: + decorator.afterStart((Scope) null) + + then: + thrown(AssertionError) + + when: + decorator.onError((Scope) null, null) + + then: + thrown(AssertionError) + + when: + decorator.beforeFinish((Scope) null) + + then: + thrown(AssertionError) + } + + def "test assert non-null scope"() { + setup: + def span = Mock(Span) + def scope = Mock(Scope) + + when: + decorator.afterStart(scope) + + then: + 1 * scope.span() >> span + + when: + decorator.onError(scope, null) + + then: + 1 * scope.span() >> span + + when: + decorator.beforeFinish(scope) + + then: + 1 * scope.span() >> span + } + def "test analytics rate default disabled"() { when: - BaseDecorator dec = newDecorator(defaultEnabled) + BaseDecorator dec = newDecorator(defaultEnabled, hasConfigNames) then: dec.traceAnalyticsEnabled == defaultEnabled - dec.traceAnalyticsSampleRate == sampleRate + dec.traceAnalyticsSampleRate == sampleRate.floatValue() where: - defaultEnabled | sampleRate - true | 1.0 - false | 1.0 + defaultEnabled | hasConfigNames | sampleRate + true | false | 1.0 + false | false | 1.0 + false | true | 1.0 } def "test analytics rate enabled"() { @@ -112,12 +158,12 @@ class BaseDecoratorTest extends Specification { return newDecorator(false) } - def newDecorator(final Boolean analyticsEnabledDefault) { - return analyticsEnabledDefault ? + def newDecorator(boolean analyticsEnabledDefault, boolean emptyInstrumentationNames = false) { + return emptyInstrumentationNames ? new BaseDecorator() { @Override protected String[] instrumentationNames() { - return ["test1", "test2"] + return [] } @Override @@ -134,21 +180,42 @@ class BaseDecoratorTest extends Specification { return true } } : - new BaseDecorator() { - @Override - protected String[] instrumentationNames() { - return ["test1", "test2"] - } + analyticsEnabledDefault ? + new BaseDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } - @Override - protected String spanType() { - return "test-type" - } + @Override + protected String spanType() { + return "test-type" + } - @Override - protected String component() { - return "test-component" + @Override + protected String component() { + return "test-component" + } + + protected boolean traceAnalyticsDefault() { + return true + } + } : + new BaseDecorator() { + @Override + protected String[] instrumentationNames() { + return ["test1", "test2"] + } + + @Override + protected String spanType() { + return "test-type" + } + + @Override + protected String component() { + return "test-component" + } } - } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/DatabaseClientDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/DatabaseClientDecoratorTest.groovy index cb1de42b14..e1bae7ae91 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/DatabaseClientDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/DatabaseClientDecoratorTest.groovy @@ -75,7 +75,7 @@ class DatabaseClientDecoratorTest extends ClientDecoratorTest { def decorator = newDecorator() when: - decorator.afterStart(null) + decorator.afterStart((Span) null) then: thrown(AssertionError) diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAdvice.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAdvice.java index a3cb44f4b5..a070eb9aac 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAdvice.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAdvice.java @@ -1,14 +1,11 @@ package datadog.trace.instrumentation.trace_annotation; -import static io.opentracing.log.Fields.ERROR_OBJECT; +import static datadog.trace.instrumentation.trace_annotation.TraceDecorator.DECORATE; import datadog.trace.api.Trace; import io.opentracing.Scope; -import io.opentracing.Span; -import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import java.lang.reflect.Method; -import java.util.Collections; import net.bytebuddy.asm.Advice; public class TraceAdvice { @@ -18,34 +15,16 @@ public class TraceAdvice { final Trace trace = method.getAnnotation(Trace.class); String operationName = trace == null ? null : trace.operationName(); if (operationName == null || operationName.isEmpty()) { - final Class declaringClass = method.getDeclaringClass(); - String className = declaringClass.getSimpleName(); - if (className.isEmpty()) { - className = declaringClass.getName(); - if (declaringClass.getPackage() != null) { - final String pkgName = declaringClass.getPackage().getName(); - if (!pkgName.isEmpty()) { - className = declaringClass.getName().replace(pkgName, "").substring(1); - } - } - } - operationName = className + "." + method.getName(); + operationName = DECORATE.spanNameForMethod(method); } - - return GlobalTracer.get() - .buildSpan(operationName) - .withTag(Tags.COMPONENT.getKey(), "trace") - .startActive(true); + return DECORATE.afterStart(GlobalTracer.get().buildSpan(operationName).startActive(true)); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { - if (throwable != null) { - final Span span = scope.span(); - Tags.ERROR.set(span, true); - span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); - } + DECORATE.onError(scope, throwable); + DECORATE.beforeFinish(scope); scope.close(); } } diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java index 931d02261e..ac250de924 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceAnnotationsInstrumentation.java @@ -82,6 +82,13 @@ public final class TraceAnnotationsInstrumentation extends Instrumenter.Default return safeHasSuperType(declaresMethod(isAnnotatedWith(methodTraceMatcher))); } + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", packageName + ".TraceDecorator", + }; + } + @Override public Map, String> transformers() { return Collections.singletonMap( diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java index e6583bb944..fe2f560816 100644 --- a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java @@ -120,6 +120,13 @@ public class TraceConfigInstrumentation implements Instrumenter { return safeHasSuperType(named(className)); } + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", packageName + ".TraceDecorator", + }; + } + @Override public Map, String> transformers() { ElementMatcher.Junction methodMatchers = null; diff --git a/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceDecorator.java b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceDecorator.java new file mode 100644 index 0000000000..5cc11b3e42 --- /dev/null +++ b/dd-java-agent/instrumentation/trace-annotation/src/main/java/datadog/trace/instrumentation/trace_annotation/TraceDecorator.java @@ -0,0 +1,22 @@ +package datadog.trace.instrumentation.trace_annotation; + +import datadog.trace.agent.decorator.BaseDecorator; + +public class TraceDecorator extends BaseDecorator { + public static TraceDecorator DECORATE = new TraceDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[0]; + } + + @Override + protected String spanType() { + return null; + } + + @Override + protected String component() { + return "trace"; + } +} From cd9dc94b2489db3cd585a5ce19c97f5996b27fec Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 26 Feb 2019 16:22:49 -0800 Subject: [PATCH 2/9] Migrate Hystrix instrumentation to Decorator --- .../HystrixCommandInstrumentation.java | 40 ++++++----------- .../hystrix/HystrixDecorator.java | 43 +++++++++++++++++++ 2 files changed, 57 insertions(+), 26 deletions(-) create mode 100644 dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixDecorator.java diff --git a/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixCommandInstrumentation.java b/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixCommandInstrumentation.java index 268dd00a90..d56c7af59c 100644 --- a/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixCommandInstrumentation.java +++ b/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixCommandInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.hystrix; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static io.opentracing.log.Fields.ERROR_OBJECT; +import static datadog.trace.instrumentation.hystrix.HystrixDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -11,12 +11,8 @@ import static net.bytebuddy.matcher.ElementMatchers.not; import com.google.auto.service.AutoService; import com.netflix.hystrix.HystrixCommand; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.api.DDTags; import io.opentracing.Scope; -import io.opentracing.Span; -import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; -import java.util.Collections; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -34,11 +30,16 @@ public class HystrixCommandInstrumentation extends Instrumenter.Default { @Override public ElementMatcher typeMatcher() { - // Not adding a version restriction because this should work with any version and add some - // benefit. return not(isInterface()).and(safeHasSuperType(named("com.netflix.hystrix.HystrixCommand"))); } + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", packageName + ".HystrixDecorator", + }; + } + @Override public Map, String> transformers() { return singletonMap( @@ -52,30 +53,17 @@ public class HystrixCommandInstrumentation extends Instrumenter.Default { @Advice.This final HystrixCommand command, @Advice.Origin("#m") final String methodName) { - final String commandName = command.getCommandKey().name(); - final String groupName = command.getCommandGroup().name(); - final boolean circuitOpen = command.isCircuitBreakerOpen(); - - final String resourceName = groupName + "." + commandName + "." + methodName; - - return GlobalTracer.get() - .buildSpan(OPERATION_NAME) - .withTag(DDTags.RESOURCE_NAME, resourceName) - .withTag(Tags.COMPONENT.getKey(), "hystrix") - .withTag("hystrix.command", commandName) - .withTag("hystrix.group", groupName) - .withTag("hystrix.circuit-open", circuitOpen) - .startActive(true); + final Scope scope = GlobalTracer.get().buildSpan(OPERATION_NAME).startActive(true); + DECORATE.afterStart(scope); + DECORATE.onCommand(scope, command, methodName); + return scope; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { - if (throwable != null) { - final Span span = scope.span(); - Tags.ERROR.set(span, true); - span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); - } + DECORATE.onError(scope, throwable); + DECORATE.beforeFinish(scope); scope.close(); } } diff --git a/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixDecorator.java b/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixDecorator.java new file mode 100644 index 0000000000..d6203d512f --- /dev/null +++ b/dd-java-agent/instrumentation/hystrix-1.4/src/main/java/datadog/trace/instrumentation/hystrix/HystrixDecorator.java @@ -0,0 +1,43 @@ +package datadog.trace.instrumentation.hystrix; + +import com.netflix.hystrix.HystrixCommand; +import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.api.DDTags; +import io.opentracing.Scope; +import io.opentracing.Span; + +public class HystrixDecorator extends BaseDecorator { + public static HystrixDecorator DECORATE = new HystrixDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[0]; + } + + @Override + protected String spanType() { + return null; + } + + @Override + protected String component() { + return "hystrix"; + } + + public void onCommand( + final Scope scope, final HystrixCommand command, final String methodName) { + if (command != null) { + final String commandName = command.getCommandKey().name(); + final String groupName = command.getCommandGroup().name(); + final boolean circuitOpen = command.isCircuitBreakerOpen(); + + final String resourceName = groupName + "." + commandName + "." + methodName; + + final Span span = scope.span(); + span.setTag(DDTags.RESOURCE_NAME, resourceName); + span.setTag("hystrix.command", commandName); + span.setTag("hystrix.group", groupName); + span.setTag("hystrix.circuit-open", circuitOpen); + } + } +} From 4e9449db8189f590c30e2b62919abe516b91071b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 26 Feb 2019 16:24:26 -0800 Subject: [PATCH 3/9] Migrate Jax-rs annotation instrumentation to Decorator --- .../java/datadog/trace/bootstrap/WeakMap.java | 15 +++ .../trace/agent/tooling/WeakMapSuppliers.java | 5 + .../jaxrs/JaxRsAnnotationsDecorator.java | 99 +++++++++++++++++++ .../JaxRsAnnotationsInstrumentation.java | 85 ++-------------- ...JaxRsAnnotationsInstrumentationTest.groovy | 14 +++ 5 files changed, 140 insertions(+), 78 deletions(-) create mode 100644 dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java index 778426e2a5..8e24c44003 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/WeakMap.java @@ -16,6 +16,8 @@ public interface WeakMap { void put(K key, V value); + void putIfAbsent(K key, V value); + @Slf4j class Provider { private static final AtomicReference provider = @@ -81,6 +83,19 @@ public interface WeakMap { map.put(key, value); } + @Override + public void putIfAbsent(final K key, final V value) { + // We can't use putIfAbsent since it was added in 1.8. + // As a result, we must use double check locking. + if (!map.containsKey(key)) { + synchronized (this) { + if (!map.containsKey(key)) { + map.put(key, value); + } + } + } + } + @Override public String toString() { return map.toString(); 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 daa050c130..42b1be611a 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 @@ -170,6 +170,11 @@ class WeakMapSuppliers { public void put(final K key, final V value) { map.put(key, value); } + + @Override + public void putIfAbsent(final K key, final V value) { + map.putIfAbsent(key, value); + } } static class Inline implements WeakMap.Supplier { diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java new file mode 100644 index 0000000000..b3c4e4a83a --- /dev/null +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java @@ -0,0 +1,99 @@ +package datadog.trace.instrumentation.jaxrs; + +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; + +import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.api.DDTags; +import datadog.trace.bootstrap.WeakMap; +import io.opentracing.Scope; +import io.opentracing.Span; +import io.opentracing.tag.Tags; +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; +import java.util.LinkedList; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import javax.ws.rs.HttpMethod; +import javax.ws.rs.Path; + +public class JaxRsAnnotationsDecorator extends BaseDecorator { + public static JaxRsAnnotationsDecorator DECORATE = new JaxRsAnnotationsDecorator(); + + private final WeakMap> resourceNames = newWeakMap(); + + @Override + protected String[] instrumentationNames() { + return new String[0]; + } + + @Override + protected String spanType() { + return null; + } + + @Override + protected String component() { + return "jax-rs-controller"; + } + + public void updateParent(final Scope scope, final Method method) { + if (scope == null) { + return; + } + final Span span = scope.span(); + Tags.COMPONENT.set(span, "jax-rs"); + + Class target = method.getDeclaringClass(); + Map classMap = resourceNames.get(target); + + if (classMap == null) { + resourceNames.putIfAbsent(target, new ConcurrentHashMap()); + classMap = resourceNames.get(target); + } + + final String methodName = method.toString(); + String resourceName = classMap.get(methodName); + if (resourceName == null) { + final LinkedList paths = new LinkedList<>(); + while (target != Object.class) { + final Path annotation = target.getAnnotation(Path.class); + if (annotation != null) { + paths.push(annotation); + } + target = target.getSuperclass(); + } + final Path methodPath = method.getAnnotation(Path.class); + if (methodPath != null) { + paths.add(methodPath); + } + String httpMethod = null; + for (final Annotation ann : method.getDeclaredAnnotations()) { + if (ann.annotationType().getAnnotation(HttpMethod.class) != null) { + httpMethod = ann.annotationType().getSimpleName(); + } + } + + final StringBuilder resourceNameBuilder = new StringBuilder(); + if (httpMethod != null) { + resourceNameBuilder.append(httpMethod); + resourceNameBuilder.append(" "); + } + Path last = null; + for (final Path path : paths) { + if (path.value().startsWith("/") || (last != null && last.value().endsWith("/"))) { + resourceNameBuilder.append(path.value()); + } else { + resourceNameBuilder.append("/"); + resourceNameBuilder.append(path.value()); + } + last = path; + } + resourceName = resourceNameBuilder.toString().trim(); + classMap.put(methodName, resourceName); + } + + if (!resourceName.isEmpty()) { + span.setTag(DDTags.RESOURCE_NAME, resourceName); + } + } +} diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java index 7f1775732e..8922ab789d 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.jaxrs; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static io.opentracing.log.Fields.ERROR_OBJECT; +import static datadog.trace.instrumentation.jaxrs.JaxRsAnnotationsDecorator.DECORATE; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.declaresMethod; import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith; @@ -9,18 +9,10 @@ import static net.bytebuddy.matcher.ElementMatchers.named; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.api.DDTags; import io.opentracing.Scope; -import io.opentracing.Span; -import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; -import java.lang.annotation.Annotation; import java.lang.reflect.Method; -import java.util.Collections; -import java.util.LinkedList; import java.util.Map; -import javax.ws.rs.HttpMethod; -import javax.ws.rs.Path; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -58,83 +50,20 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Advice.OnMethodEnter(suppress = Throwable.class) public static Scope nameSpan(@Advice.Origin final Method method) { - - // TODO: do we need caching for this? - final LinkedList paths = new LinkedList<>(); - Class target = method.getDeclaringClass(); - while (target != Object.class) { - final Path annotation = target.getAnnotation(Path.class); - if (annotation != null) { - paths.push(annotation); - } - target = target.getSuperclass(); - } - final Path methodPath = method.getAnnotation(Path.class); - if (methodPath != null) { - paths.add(methodPath); - } - String httpMethod = null; - for (final Annotation ann : method.getDeclaredAnnotations()) { - if (ann.annotationType().getAnnotation(HttpMethod.class) != null) { - httpMethod = ann.annotationType().getSimpleName(); - } - } - - final StringBuilder resourceNameBuilder = new StringBuilder(); - if (httpMethod != null) { - resourceNameBuilder.append(httpMethod); - resourceNameBuilder.append(" "); - } - Path last = null; - for (final Path path : paths) { - if (path.value().startsWith("/") || (last != null && last.value().endsWith("/"))) { - resourceNameBuilder.append(path.value()); - } else { - resourceNameBuilder.append("/"); - resourceNameBuilder.append(path.value()); - } - last = path; - } - final String resourceName = resourceNameBuilder.toString().trim(); - + // Rename the parent span according to the path represented by these annotations. final Scope scope = GlobalTracer.get().scopeManager().active(); - if (scope != null && !resourceName.isEmpty()) { - scope.span().setTag(DDTags.RESOURCE_NAME, resourceName); - Tags.COMPONENT.set(scope.span(), "jax-rs"); - } + DECORATE.updateParent(scope, method); // Now create a span representing the method execution. - - final Class clazz = method.getDeclaringClass(); - final String methodName = method.getName(); - - String className = clazz.getSimpleName(); - if (className.isEmpty()) { - className = clazz.getName(); - if (clazz.getPackage() != null) { - final String pkgName = clazz.getPackage().getName(); - if (!pkgName.isEmpty()) { - className = clazz.getName().replace(pkgName, "").substring(1); - } - } - } - - final String operationName = className + "." + methodName; - - return GlobalTracer.get() - .buildSpan(operationName) - .withTag(Tags.COMPONENT.getKey(), "jax-rs-controller") - .startActive(true); + final String operationName = DECORATE.spanNameForMethod(method); + return DECORATE.afterStart(GlobalTracer.get().buildSpan(operationName).startActive(true)); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Enter final Scope scope, @Advice.Thrown final Throwable throwable) { - if (throwable != null) { - final Span span = scope.span(); - Tags.ERROR.set(span, true); - span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); - } + DECORATE.onError(scope, throwable); + DECORATE.beforeFinish(scope); scope.close(); } } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy index 1206e762ae..5233c940e4 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy @@ -10,11 +10,13 @@ import javax.ws.rs.PUT import javax.ws.rs.Path import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace +import static datadog.trace.instrumentation.jaxrs.JaxRsAnnotationsDecorator.DECORATE class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner { def "span named '#name' from annotations on class"() { setup: + def startingCacheSize = DECORATE.resourceNames.size() runUnderTrace("test") { obj.call() } @@ -42,6 +44,18 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner { } } } + DECORATE.resourceNames.size() == startingCacheSize + 1 + DECORATE.resourceNames.get(obj.class).size() == 1 + + when: "multiple calls to the same method" + runUnderTrace("test") { + (1..10).each { + obj.call() + } + } + then: "doesn't increase the cache size" + DECORATE.resourceNames.size() == startingCacheSize + 1 + DECORATE.resourceNames.get(obj.class).size() == 1 where: name | obj From 1644de39695835d2b80f40a2b7f3379a61d29cfa Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Tue, 26 Feb 2019 16:25:24 -0800 Subject: [PATCH 4/9] Migrate Jax-rs client instrumentation to Decorator --- .../AkkaHttpClientInstrumentationTest.groovy | 10 ++--- .../src/test/groovy/UrlConnectionTest.groovy | 7 ++- .../jaxrs/ClientTracingFilter.java | 15 +++---- .../jaxrs/JaxRsClientDecorator.java | 45 +++++++++++++++++++ .../src/test/groovy/JaxRsClientTest.groovy | 14 +++--- .../trace/agent/test/utils/PortUtils.java | 2 + 6 files changed, 68 insertions(+), 25 deletions(-) create mode 100644 dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientDecorator.java 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 bdae803760..8d497d8a15 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 @@ -20,13 +20,13 @@ import java.util.concurrent.CompletionStage import java.util.concurrent.ExecutionException import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer +import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride class AkkaHttpClientInstrumentationTest extends AgentTestRunner { private static final String MESSAGE = "an\nmultiline\nhttp\nresponse" private static final long TIMEOUT = 10000L - private static final int UNUSED_PORT = 61 // this port should always be closed @AutoCleanup @Shared @@ -109,7 +109,7 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { def "error request trace"() { setup: - def url = new URL("http://localhost:$UNUSED_PORT/test") + def url = new URL("http://localhost:$UNUSABLE_PORT/test") HttpRequest request = HttpRequest.create(url.toString()) CompletionStage responseFuture = @@ -139,7 +139,7 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT "$Tags.PEER_HOSTNAME.key" server.address.host - "$Tags.PEER_PORT.key" UNUSED_PORT + "$Tags.PEER_PORT.key" UNUSABLE_PORT "$Tags.COMPONENT.key" "akka-http-client" "$Tags.ERROR.key" true errorTags(StreamTcpException, { it.contains("Tcp command") }) @@ -241,7 +241,7 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { def "error request pool trace"() { setup: // Use port number that really should be closed - def url = new URL("http://localhost:$UNUSED_PORT/test") + def url = new URL("http://localhost:$UNUSABLE_PORT/test") def response = withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { Source @@ -272,7 +272,7 @@ class AkkaHttpClientInstrumentationTest extends AgentTestRunner { "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT "$Tags.PEER_HOSTNAME.key" server.address.host - "$Tags.PEER_PORT.key" UNUSED_PORT + "$Tags.PEER_PORT.key" UNUSABLE_PORT "$Tags.COMPONENT.key" "akka-http-client" "$Tags.ERROR.key" true errorTags(StreamTcpException, { it.contains("Tcp command") }) diff --git a/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy b/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy index 00ef7d371d..6b976d6683 100644 --- a/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy +++ b/dd-java-agent/instrumentation/http-url-connection/src/test/groovy/UrlConnectionTest.groovy @@ -6,14 +6,13 @@ import datadog.trace.instrumentation.http_url_connection.UrlInstrumentation import io.opentracing.tag.Tags import io.opentracing.util.GlobalTracer +import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace import static datadog.trace.agent.test.utils.TraceUtils.withConfigOverride import static datadog.trace.instrumentation.http_url_connection.HttpUrlConnectionInstrumentation.HttpUrlState.OPERATION_NAME class UrlConnectionTest extends AgentTestRunner { - private static final int UNUSED_PORT = 61 // this port should always be closed - def "trace request with connection failure #scheme"() { when: withConfigOverride(Config.HTTP_CLIENT_HOST_SPLIT_BY_DOMAIN, "$renameService") { @@ -53,7 +52,7 @@ class UrlConnectionTest extends AgentTestRunner { "$Tags.HTTP_URL.key" "$url" "$Tags.HTTP_METHOD.key" "GET" "$Tags.PEER_HOSTNAME.key" "localhost" - "$Tags.PEER_PORT.key" UNUSED_PORT + "$Tags.PEER_PORT.key" UNUSABLE_PORT errorTags ConnectException, String defaultTags() } @@ -66,7 +65,7 @@ class UrlConnectionTest extends AgentTestRunner { "http" | true "https" | false - url = new URI("$scheme://localhost:$UNUSED_PORT").toURL() + url = new URI("$scheme://localhost:$UNUSABLE_PORT").toURL() } def "trace request with connection failure to a local file with broken url path"() { diff --git a/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java index c90ca63f71..be9a3d34f1 100644 --- a/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java +++ b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/ClientTracingFilter.java @@ -1,10 +1,10 @@ package datadog.trace.instrumentation.jaxrs; -import datadog.trace.api.DDSpanTypes; +import static datadog.trace.instrumentation.jaxrs.JaxRsClientDecorator.DECORATE; + import datadog.trace.api.DDTags; import io.opentracing.Span; import io.opentracing.propagation.Format; -import io.opentracing.tag.Tags; import io.opentracing.util.GlobalTracer; import javax.annotation.Priority; import javax.ws.rs.Priorities; @@ -25,13 +25,10 @@ public class ClientTracingFilter implements ClientRequestFilter, ClientResponseF final Span span = GlobalTracer.get() .buildSpan("jax-rs.client.call") - .withTag(Tags.COMPONENT.getKey(), "jax-rs.client") - .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT) - .withTag(Tags.HTTP_METHOD.getKey(), requestContext.getMethod()) - .withTag(Tags.HTTP_URL.getKey(), requestContext.getUri().toString()) - .withTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_CLIENT) .withTag(DDTags.RESOURCE_NAME, requestContext.getMethod() + " jax-rs.client.call") .start(); + DECORATE.afterStart(span); + DECORATE.onRequest(span, requestContext); log.debug("{} - client span started", span); @@ -50,8 +47,8 @@ public class ClientTracingFilter implements ClientRequestFilter, ClientResponseF final Object spanObj = requestContext.getProperty(SPAN_PROPERTY_NAME); if (spanObj instanceof Span) { final Span span = (Span) spanObj; - Tags.HTTP_STATUS.set(span, responseContext.getStatus()); - + DECORATE.onResponse(span, responseContext); + DECORATE.beforeFinish(span); span.finish(); log.debug("{} - client spanObj finished", spanObj); } diff --git a/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientDecorator.java b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientDecorator.java new file mode 100644 index 0000000000..47f642ab01 --- /dev/null +++ b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientDecorator.java @@ -0,0 +1,45 @@ +package datadog.trace.instrumentation.jaxrs; + +import datadog.trace.agent.decorator.HttpClientDecorator; +import javax.ws.rs.client.ClientRequestContext; +import javax.ws.rs.client.ClientResponseContext; + +public class JaxRsClientDecorator + extends HttpClientDecorator { + public static final JaxRsClientDecorator DECORATE = new JaxRsClientDecorator(); + + @Override + protected String[] instrumentationNames() { + return new String[] {"jax-rs", "jaxrs", "jax-rs-client"}; + } + + @Override + protected String component() { + return "jax-rs.client"; + } + + @Override + protected String method(final ClientRequestContext httpRequest) { + return httpRequest.getMethod(); + } + + @Override + protected String url(final ClientRequestContext httpRequest) { + return httpRequest.getUri().toString(); + } + + @Override + protected String hostname(final ClientRequestContext httpRequest) { + return httpRequest.getUri().getHost(); + } + + @Override + protected Integer port(final ClientRequestContext httpRequest) { + return httpRequest.getUri().getPort(); + } + + @Override + protected Integer status(final ClientResponseContext httpResponse) { + return httpResponse.getStatus(); + } +} diff --git a/dd-java-agent/instrumentation/jax-rs-client/src/test/groovy/JaxRsClientTest.groovy b/dd-java-agent/instrumentation/jax-rs-client/src/test/groovy/JaxRsClientTest.groovy index e4a7befa2f..0a4e5b7ba1 100644 --- a/dd-java-agent/instrumentation/jax-rs-client/src/test/groovy/JaxRsClientTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-client/src/test/groovy/JaxRsClientTest.groovy @@ -1,5 +1,4 @@ import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.agent.test.utils.PortUtils import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags import io.opentracing.tag.Tags @@ -19,12 +18,10 @@ import javax.ws.rs.core.Response import java.util.concurrent.ExecutionException import static datadog.trace.agent.test.server.http.TestHttpServer.httpServer +import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT class JaxRsClientTest extends AgentTestRunner { - @Shared - def emptyPort = PortUtils.randomOpenPort() - @AutoCleanup @Shared def server = httpServer { @@ -61,12 +58,13 @@ class JaxRsClientTest extends AgentTestRunner { parent() errored false tags { - "$Tags.COMPONENT.key" "jax-rs.client" "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$Tags.HTTP_METHOD.key" "GET" "$Tags.HTTP_STATUS.key" 200 "$Tags.HTTP_URL.key" "$server.address/ping" + "$Tags.PEER_HOSTNAME.key" "localhost" + "$Tags.PEER_PORT.key" server.address.port "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT defaultTags() } @@ -90,7 +88,7 @@ class JaxRsClientTest extends AgentTestRunner { def "#lib connection failure creates errored span"() { when: Client client = builder.build() - WebTarget service = client.target("http://localhost:$emptyPort/ping") + WebTarget service = client.target("http://localhost:$UNUSABLE_PORT/ping") if (async) { AsyncInvoker request = service.request(MediaType.TEXT_PLAIN).async() request.get().get() @@ -115,7 +113,9 @@ class JaxRsClientTest extends AgentTestRunner { "$Tags.COMPONENT.key" "jax-rs.client" "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$Tags.HTTP_METHOD.key" "GET" - "$Tags.HTTP_URL.key" "http://localhost:$emptyPort/ping" + "$Tags.HTTP_URL.key" "http://localhost:$UNUSABLE_PORT/ping" + "$Tags.PEER_HOSTNAME.key" "localhost" + "$Tags.PEER_PORT.key" UNUSABLE_PORT "$DDTags.SPAN_TYPE" DDSpanTypes.HTTP_CLIENT errorTags ProcessingException, String defaultTags() diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/PortUtils.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/PortUtils.java index 4fd97ed801..05d0fffac6 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/PortUtils.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/PortUtils.java @@ -5,6 +5,8 @@ import java.net.ServerSocket; public class PortUtils { + public static int UNUSABLE_PORT = 61; + /** Open up a random, reusable port. */ public static int randomOpenPort() { final ServerSocket socket; From a413b0d08d0ec4b1c700b52ce13806e9a7e08f4c Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 11:13:14 -0800 Subject: [PATCH 5/9] Review changes. --- .../trace/agent/decorator/BaseDecorator.java | 13 ++- .../agent/decorator/BaseDecoratorTest.groovy | 24 +++++ .../agent/decorator/SampleJavaClass.java | 14 +++ .../tooling/WeakConcurrentSupplierTest.groovy | 4 +- .../jaxrs/JaxRsAnnotationsDecorator.java | 101 ++++++++++-------- .../JaxRsAnnotationsInstrumentation.java | 7 ++ 6 files changed, 118 insertions(+), 45 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/SampleJavaClass.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java index 2d83f4c839..21c3792674 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java @@ -84,10 +84,17 @@ public abstract class BaseDecorator { return span; } + /** + * This method is used to generate an acceptable span (operation) name based on a given method + * reference. Anonymous classes are named based on their parent. + * + * @param method + * @return + */ public String spanNameForMethod(final Method method) { final Class declaringClass = method.getDeclaringClass(); - String className = declaringClass.getSimpleName(); - if (className.isEmpty()) { + String className; + if (declaringClass.isAnonymousClass()) { className = declaringClass.getName(); if (declaringClass.getPackage() != null) { final String pkgName = declaringClass.getPackage().getName(); @@ -95,6 +102,8 @@ public abstract class BaseDecorator { className = declaringClass.getName().replace(pkgName, "").substring(1); } } + } else { + className = declaringClass.getSimpleName(); } return className + "." + method.getName(); } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy index 27d24031d8..fe94dbe768 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/BaseDecoratorTest.groovy @@ -154,6 +154,22 @@ class BaseDecoratorTest extends Specification { true | "test2" | "" | true | 1.0 } + def "test spanNameForMethod"() { + when: + def result = decorator.spanNameForMethod(method) + + then: + result == "${name}.run" + + where: + target | name + SomeInnerClass | "SomeInnerClass" + SomeNestedClass | "SomeNestedClass" + SampleJavaClass.anonymousClass | "SampleJavaClass\$1" + + method = target.getDeclaredMethod("run") + } + def newDecorator() { return newDecorator(false) } @@ -218,4 +234,12 @@ class BaseDecoratorTest extends Specification { } } } + + class SomeInnerClass implements Runnable { + void run() {} + } + + static class SomeNestedClass implements Runnable { + void run() {} + } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/SampleJavaClass.java b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/SampleJavaClass.java new file mode 100644 index 0000000000..0db4f72aee --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/decorator/SampleJavaClass.java @@ -0,0 +1,14 @@ +package datadog.trace.agent.decorator; + +/** + * Used by {@link BaseDecoratorTest}. Groovy with Java 10+ doesn't seem to treat it properly as an + * anonymous class, so use a Java class instead. + */ +public class SampleJavaClass { + public static Class anonymousClass = + new Runnable() { + + @Override + public void run() {} + }.getClass(); +} 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 a883c73fd2..a05fa25ff0 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 @@ -1,14 +1,16 @@ package datadog.trace.agent.tooling - import datadog.trace.bootstrap.WeakMap import datadog.trace.util.gc.GCUtils +import spock.lang.Retry import spock.lang.Shared import spock.lang.Specification import java.lang.ref.WeakReference import java.util.concurrent.TimeUnit +@Retry +// These tests fail sometimes in CI. class WeakConcurrentSupplierTest extends Specification { @Shared def weakConcurrentSupplier = new WeakMapSuppliers.WeakConcurrent() diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java index b3c4e4a83a..e6a68bfd79 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsDecorator.java @@ -19,7 +19,7 @@ import javax.ws.rs.Path; public class JaxRsAnnotationsDecorator extends BaseDecorator { public static JaxRsAnnotationsDecorator DECORATE = new JaxRsAnnotationsDecorator(); - private final WeakMap> resourceNames = newWeakMap(); + private final WeakMap> resourceNames = newWeakMap(); @Override protected String[] instrumentationNames() { @@ -43,57 +43,74 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { final Span span = scope.span(); Tags.COMPONENT.set(span, "jax-rs"); - Class target = method.getDeclaringClass(); - Map classMap = resourceNames.get(target); + final Class target = method.getDeclaringClass(); + Map classMap = resourceNames.get(target); if (classMap == null) { - resourceNames.putIfAbsent(target, new ConcurrentHashMap()); + resourceNames.putIfAbsent(target, new ConcurrentHashMap()); classMap = resourceNames.get(target); + // classMap should not be null at this point because we have a + // strong reference to target and don't manually clear the map. } - final String methodName = method.toString(); - String resourceName = classMap.get(methodName); + String resourceName = classMap.get(method); if (resourceName == null) { - final LinkedList paths = new LinkedList<>(); - while (target != Object.class) { - final Path annotation = target.getAnnotation(Path.class); - if (annotation != null) { - paths.push(annotation); - } - target = target.getSuperclass(); - } - final Path methodPath = method.getAnnotation(Path.class); - if (methodPath != null) { - paths.add(methodPath); - } - String httpMethod = null; - for (final Annotation ann : method.getDeclaredAnnotations()) { - if (ann.annotationType().getAnnotation(HttpMethod.class) != null) { - httpMethod = ann.annotationType().getSimpleName(); - } - } - - final StringBuilder resourceNameBuilder = new StringBuilder(); - if (httpMethod != null) { - resourceNameBuilder.append(httpMethod); - resourceNameBuilder.append(" "); - } - Path last = null; - for (final Path path : paths) { - if (path.value().startsWith("/") || (last != null && last.value().endsWith("/"))) { - resourceNameBuilder.append(path.value()); - } else { - resourceNameBuilder.append("/"); - resourceNameBuilder.append(path.value()); - } - last = path; - } - resourceName = resourceNameBuilder.toString().trim(); - classMap.put(methodName, resourceName); + final String httpMethod = locateHttpMethod(method); + final LinkedList paths = gatherPaths(method); + resourceName = buildResourceName(httpMethod, paths); + classMap.put(method, resourceName); } if (!resourceName.isEmpty()) { span.setTag(DDTags.RESOURCE_NAME, resourceName); } } + + private String locateHttpMethod(final Method method) { + String httpMethod = null; + for (final Annotation ann : method.getDeclaredAnnotations()) { + if (ann.annotationType().getAnnotation(HttpMethod.class) != null) { + httpMethod = ann.annotationType().getSimpleName(); + } + } + return httpMethod; + } + + private LinkedList gatherPaths(final Method method) { + Class target = method.getDeclaringClass(); + final LinkedList paths = new LinkedList<>(); + while (target != Object.class) { + final Path annotation = target.getAnnotation(Path.class); + if (annotation != null) { + paths.push(annotation); + } + target = target.getSuperclass(); + } + final Path methodPath = method.getAnnotation(Path.class); + if (methodPath != null) { + paths.add(methodPath); + } + return paths; + } + + private String buildResourceName(final String httpMethod, final LinkedList paths) { + final String resourceName; + final StringBuilder resourceNameBuilder = new StringBuilder(); + if (httpMethod != null) { + resourceNameBuilder.append(httpMethod); + resourceNameBuilder.append(" "); + } + Path last = null; + for (final Path path : paths) { + if (path.value().startsWith("/") || (last != null && last.value().endsWith("/"))) { + resourceNameBuilder.append(path.value()); + } else { + resourceNameBuilder.append("/"); + resourceNameBuilder.append(path.value()); + } + last = path; + } + resourceName = resourceNameBuilder.toString().trim(); + return resourceName; + } } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java index 8922ab789d..cb3916cf58 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsAnnotationsInstrumentation.java @@ -32,6 +32,13 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default .or(safeHasSuperType(declaresMethod(isAnnotatedWith(named("javax.ws.rs.Path")))))); } + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", packageName + ".JaxRsAnnotationsDecorator", + }; + } + @Override public Map, String> transformers() { return singletonMap( From 9c9e74e0d00c1702d03d531d44848fe37f2c9046 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 12:53:33 -0800 Subject: [PATCH 6/9] Missing helpers --- .../JerseyClientConnectionErrorInstrumentation.java | 8 +++++--- .../ResteasyClientConnectionErrorInstrumentation.java | 4 +++- .../jaxrs/JaxRsClientInstrumentation.java | 10 +++++++--- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-jersey/src/main/java/datadog/trace/instrumentation/connection_error/jersey/JerseyClientConnectionErrorInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-jersey/src/main/java/datadog/trace/instrumentation/connection_error/jersey/JerseyClientConnectionErrorInstrumentation.java index 3274147d13..266eaea96f 100644 --- a/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-jersey/src/main/java/datadog/trace/instrumentation/connection_error/jersey/JerseyClientConnectionErrorInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-jersey/src/main/java/datadog/trace/instrumentation/connection_error/jersey/JerseyClientConnectionErrorInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.connection_error.jersey; +import static io.opentracing.log.Fields.ERROR_OBJECT; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -42,7 +43,9 @@ public final class JerseyClientConnectionErrorInstrumentation extends Instrument @Override public String[] helperClassNames() { - return new String[] {getClass().getName() + "$WrappedFuture"}; + return new String[] { + getClass().getName() + "$WrappedFuture", + }; } @Override @@ -62,12 +65,11 @@ public final class JerseyClientConnectionErrorInstrumentation extends Instrument @Advice.FieldValue("requestContext") final ClientRequest context, @Advice.Thrown final Throwable throwable) { if (throwable != null) { - final Object prop = context.getProperty(ClientTracingFilter.SPAN_PROPERTY_NAME); if (prop instanceof Span) { final Span span = (Span) prop; Tags.ERROR.set(span, true); - span.log(Collections.singletonMap(Fields.ERROR_OBJECT, throwable)); + span.log(Collections.singletonMap(ERROR_OBJECT, throwable)); span.finish(); } } diff --git a/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-resteasy/src/main/java/datadog/trace/instrumentation/connection_error/resteasy/ResteasyClientConnectionErrorInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-resteasy/src/main/java/datadog/trace/instrumentation/connection_error/resteasy/ResteasyClientConnectionErrorInstrumentation.java index 827090c491..5f286250a8 100644 --- a/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-resteasy/src/main/java/datadog/trace/instrumentation/connection_error/resteasy/ResteasyClientConnectionErrorInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client/connection-error-handling-resteasy/src/main/java/datadog/trace/instrumentation/connection_error/resteasy/ResteasyClientConnectionErrorInstrumentation.java @@ -43,7 +43,9 @@ public final class ResteasyClientConnectionErrorInstrumentation extends Instrume @Override public String[] helperClassNames() { - return new String[] {getClass().getName() + "$WrappedFuture"}; + return new String[] { + getClass().getName() + "$WrappedFuture", + }; } @Override diff --git a/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java index 0f1aee1868..b29884fb1c 100644 --- a/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-client/src/main/java/datadog/trace/instrumentation/jaxrs/JaxRsClientInstrumentation.java @@ -29,9 +29,13 @@ public final class JaxRsClientInstrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - "datadog.trace.instrumentation.jaxrs.ClientTracingFeature", - "datadog.trace.instrumentation.jaxrs.ClientTracingFilter", - "datadog.trace.instrumentation.jaxrs.InjectAdapter" + "datadog.trace.agent.decorator.BaseDecorator", + "datadog.trace.agent.decorator.ClientDecorator", + "datadog.trace.agent.decorator.HttpClientDecorator", + packageName + ".JaxRsClientDecorator", + packageName + ".ClientTracingFeature", + packageName + ".ClientTracingFilter", + packageName + ".InjectAdapter", }; } From f7e8980d08737c26adfe8044584f5a3e03c28bab Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 15:35:15 -0800 Subject: [PATCH 7/9] Add null check on span type Null spanTypes cause the tag to be removed. --- .../java/datadog/trace/agent/decorator/BaseDecorator.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java index 21c3792674..fc4960d7ad 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/decorator/BaseDecorator.java @@ -50,7 +50,9 @@ public abstract class BaseDecorator { public Span afterStart(final Span span) { assert span != null; - span.setTag(DDTags.SPAN_TYPE, spanType()); + if (spanType() != null) { + span.setTag(DDTags.SPAN_TYPE, spanType()); + } Tags.COMPONENT.set(span, component()); if (traceAnalyticsEnabled) { span.setTag(DDTags.ANALYTICS_SAMPLE_RATE, traceAnalyticsSampleRate); From 133460a79ac472e9729703e19bfff5af941c82b0 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 17:44:53 -0800 Subject: [PATCH 8/9] Stop making hard references in HelperInjector Also make awaitGC interruptable. --- .../trace/agent/tooling/HelperInjector.java | 23 ++++-- .../agent/test/HelperInjectionTest.groovy | 76 ++++++++++++++----- .../classloading/ClassLoadingTest.groovy | 2 + .../trace/agent/test/AgentTestRunner.java | 6 +- .../agent/test/utils/ClasspathUtils.java | 21 +++++ .../java/muzzle/MuzzleWeakReferenceTest.java | 2 +- .../java/datadog/trace/util/gc/GCUtils.java | 7 +- 7 files changed, 105 insertions(+), 32 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java index ea2597ba21..d61dcb16e8 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/HelperInjector.java @@ -1,14 +1,16 @@ package datadog.trace.agent.tooling; import static datadog.trace.agent.tooling.ClassLoaderMatcher.BOOTSTRAP_CLASSLOADER; +import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; +import datadog.trace.bootstrap.WeakMap; import java.io.File; import java.io.IOException; +import java.security.SecureClassLoader; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; @@ -24,9 +26,13 @@ import net.bytebuddy.utility.JavaModule; /** Injects instrumentation helper classes into the user's classloader. */ @Slf4j public class HelperInjector implements Transformer { + // Need this because we can't put null into the injectedClassLoaders map. + private static final ClassLoader BOOTSTRAP_CLASSLOADER_PLACEHOLDER = + new SecureClassLoader(null) {}; + private final Set helperClassNames; private Map helperMap = null; - private final Set injectedClassLoaders = new HashSet<>(); + private final WeakMap injectedClassLoaders = newWeakMap(); /** * Construct HelperInjector. @@ -80,15 +86,18 @@ public class HelperInjector implements Transformer { public DynamicType.Builder transform( final DynamicType.Builder builder, final TypeDescription typeDescription, - final ClassLoader classLoader, + ClassLoader classLoader, final JavaModule module) { if (helperClassNames.size() > 0) { synchronized (this) { - if (!injectedClassLoaders.contains(classLoader)) { + if (classLoader == BOOTSTRAP_CLASSLOADER) { + classLoader = BOOTSTRAP_CLASSLOADER_PLACEHOLDER; + } + if (!injectedClassLoaders.containsKey(classLoader)) { try { final Map helperMap = getHelperMap(); log.debug("Injecting classes onto classloader {} -> {}", classLoader, helperClassNames); - if (classLoader == BOOTSTRAP_CLASSLOADER) { + if (classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER) { final Map> injected = ClassInjector.UsingInstrumentation.of( new File(System.getProperty("java.io.tmpdir")), @@ -108,13 +117,13 @@ public class HelperInjector implements Transformer { + ". Failed to inject helper classes into instance " + classLoader + " of type " - + (classLoader == BOOTSTRAP_CLASSLOADER + + (classLoader == BOOTSTRAP_CLASSLOADER_PLACEHOLDER ? "" : classLoader.getClass().getName()), e); throw new RuntimeException(e); } - injectedClassLoaders.add(classLoader); + injectedClassLoaders.put(classLoader, true); } } } diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy index f59a7d4ec4..79b7e8a488 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/HelperInjectionTest.groovy @@ -1,38 +1,54 @@ package datadog.trace.agent.test + import datadog.trace.agent.tooling.AgentInstaller import datadog.trace.agent.tooling.HelperInjector import datadog.trace.agent.tooling.Utils import net.bytebuddy.agent.ByteBuddyAgent +import net.bytebuddy.description.type.TypeDescription +import net.bytebuddy.dynamic.ClassFileLocator +import net.bytebuddy.dynamic.loading.ClassInjector import spock.lang.Specification +import spock.lang.Timeout -import java.lang.reflect.Method +import java.lang.ref.WeakReference +import java.util.concurrent.atomic.AtomicReference +import static datadog.trace.agent.test.utils.ClasspathUtils.isClassLoaded import static datadog.trace.agent.tooling.ClassLoaderMatcher.BOOTSTRAP_CLASSLOADER +import static datadog.trace.util.gc.GCUtils.awaitGC class HelperInjectionTest extends Specification { + @Timeout(10) def "helpers injected to non-delegating classloader"() { setup: String helperClassName = HelperInjectionTest.getPackage().getName() + '.HelperClass' HelperInjector injector = new HelperInjector(helperClassName) - URLClassLoader emptyLoader = new URLClassLoader(new URL[0], (ClassLoader) null) + AtomicReference emptyLoader = new AtomicReference<>(new URLClassLoader(new URL[0], (ClassLoader) null)) when: - emptyLoader.loadClass(helperClassName) + emptyLoader.get().loadClass(helperClassName) then: thrown ClassNotFoundException when: - injector.transform(null, null, emptyLoader, null) - emptyLoader.loadClass(helperClassName) + injector.transform(null, null, emptyLoader.get(), null) + emptyLoader.get().loadClass(helperClassName) then: - isClassLoaded(helperClassName, emptyLoader) + isClassLoaded(helperClassName, emptyLoader.get()) // injecting into emptyLoader should not load on agent's classloader !isClassLoaded(helperClassName, Utils.getAgentClassLoader()) - cleanup: - emptyLoader?.close() + when: "references to emptyLoader are gone" + emptyLoader.get().close() // cleanup + def ref = new WeakReference(emptyLoader.get()) + emptyLoader.set(null) + + awaitGC(ref) + + then: "HelperInjector doesn't prevent it from being collected" + null == ref.get() } def "helpers injected on bootstrap classloader"() { @@ -55,16 +71,38 @@ class HelperInjectionTest extends Specification { helperClass.getClassLoader() == BOOTSTRAP_CLASSLOADER } - private static boolean isClassLoaded(String className, ClassLoader classLoader) { - final Method findLoadedClassMethod = ClassLoader.getDeclaredMethod("findLoadedClass", String) - try { - findLoadedClassMethod.setAccessible(true) - Class loadedClass = (Class) findLoadedClassMethod.invoke(classLoader, className) - return null != loadedClass && loadedClass.getClassLoader() == classLoader - } catch (Exception e) { - throw new IllegalStateException(e) - } finally { - findLoadedClassMethod.setAccessible(false) - } + def "check hard references on class injection"() { + setup: + String helperClassName = HelperInjectionTest.getPackage().getName() + '.HelperClass' + + // Copied from HelperInjector: + final ClassFileLocator locator = + ClassFileLocator.ForClassLoader.of(Utils.getAgentClassLoader()) + final byte[] classBytes = locator.locate(helperClassName).resolve() + final TypeDescription typeDesc = + new TypeDescription.Latent( + helperClassName, 0, null, Collections. emptyList()) + + AtomicReference emptyLoader = new AtomicReference<>(new URLClassLoader(new URL[0], (ClassLoader) null)) + AtomicReference injector = new AtomicReference<>(new ClassInjector.UsingReflection(emptyLoader.get())) + injector.get().inject([(typeDesc): classBytes]) + + when: + def injectorRef = new WeakReference(injector.get()) + injector.set(null) + + awaitGC(injectorRef) + + then: + null == injectorRef.get() + + when: + def loaderRef = new WeakReference(emptyLoader.get()) + emptyLoader.set(null) + + awaitGC(loaderRef) + + then: + null == loaderRef.get() } } diff --git a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy index 02cd9a8511..883730a769 100644 --- a/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy +++ b/dd-java-agent/src/test/groovy/datadog/trace/agent/integration/classloading/ClassLoadingTest.groovy @@ -6,11 +6,13 @@ import datadog.trace.agent.test.IntegrationTestUtils import datadog.trace.api.Trace import datadog.trace.util.gc.GCUtils import spock.lang.Specification +import spock.lang.Timeout import java.lang.ref.WeakReference import static datadog.trace.agent.test.IntegrationTestUtils.createJarWithClasses +@Timeout(10) class ClassLoadingTest extends Specification { final URL[] classpath = [createJarWithClasses(ClassToInstrument, ClassToInstrumentChild, Trace)] diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java index 6c8317c6fe..254e3765bb 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/AgentTestRunner.java @@ -178,13 +178,13 @@ public abstract class AgentTestRunner extends Specification { @AfterClass public static synchronized void agentCleanup() { - assert INSTRUMENTATION_ERROR_COUNT.get() == 0 - : INSTRUMENTATION_ERROR_COUNT.get() + " Instrumentation errors during test"; - if (null != activeTransformer) { INSTRUMENTATION.removeTransformer(activeTransformer); activeTransformer = null; } + // Cleanup before assertion. + assert INSTRUMENTATION_ERROR_COUNT.get() == 0 + : INSTRUMENTATION_ERROR_COUNT.get() + " Instrumentation errors during test"; } public static void assertTraces( diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ClasspathUtils.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ClasspathUtils.java index 4c40616872..b398454f71 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ClasspathUtils.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/utils/ClasspathUtils.java @@ -13,6 +13,7 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; +import java.lang.reflect.Method; import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; @@ -156,4 +157,24 @@ public class ClasspathUtils { } return new URLClassLoader(urls.build().toArray(new URL[0]), null); } + + // Moved this to a java class because groovy was adding a hard ref to classLoader + public static boolean isClassLoaded(final String className, final ClassLoader classLoader) { + try { + final Method findLoadedClassMethod = + ClassLoader.class.getDeclaredMethod("findLoadedClass", String.class); + try { + findLoadedClassMethod.setAccessible(true); + final Class loadedClass = + (Class) findLoadedClassMethod.invoke(classLoader, className); + return null != loadedClass && loadedClass.getClassLoader() == classLoader; + } catch (final Exception e) { + throw new IllegalStateException(e); + } finally { + findLoadedClassMethod.setAccessible(false); + } + } catch (final NoSuchMethodException e) { + throw new IllegalStateException(e); + } + } } diff --git a/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java b/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java index 8420aa5b71..cf9ddcb16c 100644 --- a/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java +++ b/dd-java-agent/testing/src/test/java/muzzle/MuzzleWeakReferenceTest.java @@ -14,7 +14,7 @@ public class MuzzleWeakReferenceTest { * * Even returning a WeakReference is enough for spock to create a strong ref. */ - public static boolean classLoaderRefIsGarbageCollected() { + public static boolean classLoaderRefIsGarbageCollected() throws InterruptedException { ClassLoader loader = new URLClassLoader(new URL[0], null); final WeakReference clRef = new WeakReference<>(loader); final Reference[] refs = diff --git a/utils/gc-utils/src/main/java/datadog/trace/util/gc/GCUtils.java b/utils/gc-utils/src/main/java/datadog/trace/util/gc/GCUtils.java index 5b899897e9..3df49f5ce5 100644 --- a/utils/gc-utils/src/main/java/datadog/trace/util/gc/GCUtils.java +++ b/utils/gc-utils/src/main/java/datadog/trace/util/gc/GCUtils.java @@ -4,15 +4,18 @@ import java.lang.ref.WeakReference; public abstract class GCUtils { - public static void awaitGC() { + public static void awaitGC() throws InterruptedException { Object obj = new Object(); final WeakReference ref = new WeakReference<>(obj); obj = null; awaitGC(ref); } - public static void awaitGC(final WeakReference ref) { + public static void awaitGC(final WeakReference ref) throws InterruptedException { while (ref.get() != null) { + if (Thread.interrupted()) { + throw new InterruptedException(); + } System.gc(); System.runFinalization(); } From d53d3fe4c8cbcb655660fe3797d1cf0848f891f2 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 20:56:47 -0800 Subject: [PATCH 9/9] Add @Retry to tests that fail randomly in CI. Should eventually revisit this to try and remove them. Latest failure was on: ``` at ExecutorInstrumentationTest.#poolImpl '#name' reports after canceled jobs(ExecutorInstrumentationTest.groovy:202) ``` --- .../src/test/groovy/AkkaExecutorInstrumentationTest.groovy | 2 ++ .../src/test/groovy/ScalaExecutorInstrumentationTest.groovy | 2 ++ .../src/test/groovy/ExecutorInstrumentationTest.groovy | 2 ++ 3 files changed, 6 insertions(+) diff --git a/dd-java-agent/instrumentation/java-concurrent/akka-2.5-testing/src/test/groovy/AkkaExecutorInstrumentationTest.groovy b/dd-java-agent/instrumentation/java-concurrent/akka-2.5-testing/src/test/groovy/AkkaExecutorInstrumentationTest.groovy index d2e337da5f..5507b6fa7a 100644 --- a/dd-java-agent/instrumentation/java-concurrent/akka-2.5-testing/src/test/groovy/AkkaExecutorInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/java-concurrent/akka-2.5-testing/src/test/groovy/AkkaExecutorInstrumentationTest.groovy @@ -5,6 +5,7 @@ import datadog.opentracing.scopemanager.ContinuableScope import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.Trace import io.opentracing.util.GlobalTracer +import spock.lang.Retry import spock.lang.Shared import java.lang.reflect.InvocationTargetException @@ -19,6 +20,7 @@ import java.util.concurrent.TimeUnit * Test executor instrumentation for Akka specific classes. * This is to large extent a copy of ExecutorInstrumentationTest. */ +@Retry class AkkaExecutorInstrumentationTest extends AgentTestRunner { @Shared diff --git a/dd-java-agent/instrumentation/java-concurrent/scala-testing/src/test/groovy/ScalaExecutorInstrumentationTest.groovy b/dd-java-agent/instrumentation/java-concurrent/scala-testing/src/test/groovy/ScalaExecutorInstrumentationTest.groovy index 78780d8fb1..517c2e6c6b 100644 --- a/dd-java-agent/instrumentation/java-concurrent/scala-testing/src/test/groovy/ScalaExecutorInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/java-concurrent/scala-testing/src/test/groovy/ScalaExecutorInstrumentationTest.groovy @@ -5,6 +5,7 @@ import datadog.trace.api.Trace import io.opentracing.util.GlobalTracer import scala.concurrent.forkjoin.ForkJoinPool import scala.concurrent.forkjoin.ForkJoinTask +import spock.lang.Retry import spock.lang.Shared import java.lang.reflect.InvocationTargetException @@ -19,6 +20,7 @@ import java.util.concurrent.TimeUnit * Test executor instrumentation for Scala specific classes. * This is to large extent a copy of ExecutorInstrumentationTest. */ +@Retry class ScalaExecutorInstrumentationTest extends AgentTestRunner { @Shared diff --git a/dd-java-agent/instrumentation/java-concurrent/src/test/groovy/ExecutorInstrumentationTest.groovy b/dd-java-agent/instrumentation/java-concurrent/src/test/groovy/ExecutorInstrumentationTest.groovy index 2897f02d95..5f7313679a 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/test/groovy/ExecutorInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/java-concurrent/src/test/groovy/ExecutorInstrumentationTest.groovy @@ -5,6 +5,7 @@ import datadog.trace.api.Trace import datadog.trace.bootstrap.instrumentation.java.concurrent.CallableWrapper import datadog.trace.bootstrap.instrumentation.java.concurrent.RunnableWrapper import io.opentracing.util.GlobalTracer +import spock.lang.Retry import spock.lang.Shared import java.lang.reflect.InvocationTargetException @@ -18,6 +19,7 @@ import java.util.concurrent.ScheduledThreadPoolExecutor import java.util.concurrent.ThreadPoolExecutor import java.util.concurrent.TimeUnit +@Retry class ExecutorInstrumentationTest extends AgentTestRunner { @Shared