From a413b0d08d0ec4b1c700b52ce13806e9a7e08f4c Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Wed, 27 Feb 2019 11:13:14 -0800 Subject: [PATCH] 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(