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 08cbdd2a36..e5d6ed3136 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 @@ -3,6 +3,7 @@ package datadog.trace.instrumentation.jaxrs2; import static datadog.trace.bootstrap.WeakMap.Provider.newWeakMap; import datadog.trace.agent.decorator.BaseDecorator; +import datadog.trace.agent.tooling.ClassHierarchyIterable; import datadog.trace.api.DDSpanTypes; import datadog.trace.api.DDTags; import datadog.trace.bootstrap.WeakMap; @@ -10,8 +11,6 @@ import datadog.trace.instrumentation.api.AgentSpan; import datadog.trace.instrumentation.api.Tags; import java.lang.annotation.Annotation; import java.lang.reflect.Method; -import java.util.ArrayList; -import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import javax.ws.rs.HttpMethod; @@ -83,9 +82,31 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { String resourceName = classMap.get(method); if (resourceName == null) { - final String httpMethod = locateHttpMethod(method); - final List paths = gatherPaths(target, method); - resourceName = buildResourceName(httpMethod, paths); + String httpMethod = null; + Path methodPath = null; + final Path classPath = findClassPath(target); + for (final Class currentClass : new ClassHierarchyIterable(target)) { + final Method currentMethod; + if (currentClass.equals(target)) { + currentMethod = method; + } else { + currentMethod = findMatchingMethod(method, currentClass.getDeclaredMethods()); + } + + if (currentMethod != null) { + if (httpMethod == null) { + httpMethod = locateHttpMethod(currentMethod); + } + if (methodPath == null) { + methodPath = findMethodPath(currentMethod); + } + + if (httpMethod != null && methodPath != null) { + break; + } + } + } + resourceName = buildResourceName(httpMethod, classPath, methodPath); classMap.put(method, resourceName); } @@ -102,40 +123,77 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { return httpMethod; } - private List gatherPaths(Class target, final Method method) { - final List paths = new ArrayList(); - while (target != null && target != Object.class) { - final Path annotation = target.getAnnotation(Path.class); - if (annotation != null) { - paths.add(annotation); - break; // Annotation overridden, no need to continue. - } - target = target.getSuperclass(); - } - final Path methodPath = method.getAnnotation(Path.class); - if (methodPath != null) { - paths.add(methodPath); - } - return paths; + private Path findMethodPath(final Method method) { + return method.getAnnotation(Path.class); } - private String buildResourceName(final String httpMethod, final List paths) { + private Path findClassPath(final Class target) { + for (final Class currentClass : new ClassHierarchyIterable(target)) { + final Path annotation = currentClass.getAnnotation(Path.class); + if (annotation != null) { + // Annotation overridden, no need to continue. + return annotation; + } + } + + return null; + } + + private Method findMatchingMethod(final Method baseMethod, final Method[] methods) { + nextMethod: + for (final Method method : methods) { + if (!baseMethod.getReturnType().equals(method.getReturnType())) { + continue; + } + + if (!baseMethod.getName().equals(method.getName())) { + continue; + } + + final Class[] baseParameterTypes = baseMethod.getParameterTypes(); + final Class[] parameterTypes = method.getParameterTypes(); + if (baseParameterTypes.length != parameterTypes.length) { + continue; + } + for (int i = 0; i < baseParameterTypes.length; i++) { + if (!baseParameterTypes[i].equals(parameterTypes[i])) { + continue nextMethod; + } + } + return method; + } + return null; + } + + private String buildResourceName( + final String httpMethod, final Path classPath, final Path methodPath) { 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 { + boolean skipSlash = false; + if (classPath != null) { + if (!classPath.value().startsWith("/")) { resourceNameBuilder.append("/"); - resourceNameBuilder.append(path.value()); } - last = path; + resourceNameBuilder.append(classPath.value()); + skipSlash = classPath.value().endsWith("/"); } + + if (methodPath != null) { + String path = methodPath.value(); + if (skipSlash) { + if (path.startsWith("/")) { + path = path.length() == 1 ? "" : path.substring(1); + } + } else if (!path.startsWith("/")) { + resourceNameBuilder.append("/"); + } + resourceNameBuilder.append(path); + } + resourceName = resourceNameBuilder.toString().trim(); return resourceName; } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java index e5c6c41cff..8d3a66e388 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.jaxrs2; +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.hasSuperMethod; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; @@ -8,6 +9,7 @@ import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.DEC import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.declaresMethod; import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.named; import com.google.auto.service.AutoService; @@ -47,21 +49,27 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default @Override public String[] helperClassNames() { return new String[] { - "datadog.trace.agent.decorator.BaseDecorator", packageName + ".JaxRsAnnotationsDecorator", + "datadog.trace.agent.decorator.BaseDecorator", + "datadog.trace.agent.tooling.ClassHierarchyIterable", + "datadog.trace.agent.tooling.ClassHierarchyIterable$ClassIterator", + packageName + ".JaxRsAnnotationsDecorator", }; } @Override public Map, String> transformers() { return singletonMap( - isAnnotatedWith( - named("javax.ws.rs.Path") - .or(named("javax.ws.rs.DELETE")) - .or(named("javax.ws.rs.GET")) - .or(named("javax.ws.rs.HEAD")) - .or(named("javax.ws.rs.OPTIONS")) - .or(named("javax.ws.rs.POST")) - .or(named("javax.ws.rs.PUT"))), + isMethod() + .and( + hasSuperMethod( + isAnnotatedWith( + named("javax.ws.rs.Path") + .or(named("javax.ws.rs.DELETE")) + .or(named("javax.ws.rs.GET")) + .or(named("javax.ws.rs.HEAD")) + .or(named("javax.ws.rs.OPTIONS")) + .or(named("javax.ws.rs.POST")) + .or(named("javax.ws.rs.PUT"))))), JaxRsAnnotationsAdvice.class.getName()); } diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy index 752e233ae4..45b40fba0d 100644 --- a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy @@ -2,6 +2,7 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.bootstrap.WeakMap import datadog.trace.instrumentation.api.Tags import datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator + import javax.ws.rs.DELETE import javax.ws.rs.GET import javax.ws.rs.HEAD @@ -9,7 +10,6 @@ import javax.ws.rs.OPTIONS import javax.ws.rs.POST import javax.ws.rs.PUT import javax.ws.rs.Path - import java.lang.reflect.Method import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace @@ -86,53 +86,53 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner { resourceNames.get(obj.class).size() == 1 where: - name | obj - "/a" | new Jax() { + name | obj + "/a" | new Jax() { @Path("/a") void call() { } } - "GET /b" | new Jax() { + "GET /b" | new Jax() { @GET @Path("/b") void call() { } } - "POST /c" | new InterfaceWithPath() { + "POST /interface/c" | new InterfaceWithPath() { @POST @Path("/c") void call() { } } - "HEAD" | new InterfaceWithPath() { + "HEAD /interface" | new InterfaceWithPath() { @HEAD void call() { } } - "POST /abstract/d" | new AbstractClassWithPath() { + "POST /abstract/d" | new AbstractClassWithPath() { @POST @Path("/d") void call() { } } - "PUT /abstract" | new AbstractClassWithPath() { + "PUT /abstract" | new AbstractClassWithPath() { @PUT void call() { } } - "OPTIONS /child/e" | new ChildClassWithPath() { + "OPTIONS /child/e" | new ChildClassWithPath() { @OPTIONS @Path("/e") void call() { } } - "DELETE /child" | new ChildClassWithPath() { + "DELETE /child/call" | new ChildClassWithPath() { @DELETE void call() { } } - "POST /child/call" | new ChildClassWithPath() - "GET /child/call" | new JavaInterfaces.ChildClassOnInterface() + "POST /child/call" | new ChildClassWithPath() + "GET /child/call" | new JavaInterfaces.ChildClassOnInterface() // TODO: uncomment when we drop support for Java 7 // "GET /child/invoke" | new JavaInterfaces.DefaultChildClassOnInterface() @@ -168,18 +168,6 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner { void call() { } } | _ - new InterfaceWithPath() { - void call() { - } - } | _ - new AbstractClassWithPath() { - void call() { - } - } | _ - new ChildClassWithPath() { - void call() { - } - } | _ } interface Jax { diff --git a/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JerseyTest.groovy b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JerseyTest.groovy new file mode 100644 index 0000000000..8e82e01b54 --- /dev/null +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JerseyTest.groovy @@ -0,0 +1,62 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.api.Tags +import io.dropwizard.testing.junit.ResourceTestRule +import org.junit.ClassRule +import spock.lang.Shared + +import javax.ws.rs.client.Entity + +import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace + +class JerseyTest extends AgentTestRunner { + + @Shared + @ClassRule + ResourceTestRule resources = ResourceTestRule.builder() + .addResource(new Resource.Test1()) + .addResource(new Resource.Test2()) + .addResource(new Resource.Test3()) + .build() + + def "test #resource"() { + when: + // start a trace because the test doesn't go through any servlet or other instrumentation. + def response = runUnderTrace("test.span") { + resources.client().target(resource).request().post(Entity.text(""), String) + } + + then: + response == expectedResponse + + assertTraces(1) { + trace(0, 2) { + span(0) { + operationName "test.span" + resourceName expectedResourceName + tags { + "$Tags.COMPONENT" "jax-rs" + defaultTags() + } + } + + span(1) { + childOf span(0) + operationName "jax-rs.request" + resourceName controllerName + spanType DDSpanTypes.HTTP_SERVER + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + } + } + + where: + resource | expectedResourceName | controllerName | expectedResponse + "/test/hello/bob" | "POST /test/hello/{name}" | "Test1.hello" | "Test1 bob!" + "/test2/hello/bob" | "POST /test2/hello/{name}" | "Test2.hello" | "Test2 bob!" + "/test3/hi/bob" | "POST /test3/hi/{name}" | "Test3.hello" | "Test3 bob!" + } +} 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 new file mode 100644 index 0000000000..0a505ee794 --- /dev/null +++ b/dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/java/Resource.java @@ -0,0 +1,43 @@ +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; + +// Originally had this as a groovy class but was getting some weird errors. +@Path("/ignored") +public interface Resource { + @Path("ignored") + String hello(final String name); + + @Path("/test") + interface SubResource extends Cloneable, Resource { + @Override + @POST + @Path("/hello/{name}") + String hello(@PathParam("name") final String name); + } + + class Test1 implements SubResource { + @Override + public String hello(final String name) { + return "Test1 " + name + "!"; + } + } + + @Path("/test2") + class Test2 implements SubResource { + @Override + public String hello(final String name) { + return "Test2 " + name + "!"; + } + } + + @Path("/test3") + class Test3 implements SubResource { + @Override + @POST + @Path("/hi/{name}") + public String hello(@PathParam("name") final String name) { + return "Test3 " + name + "!"; + } + } +}