open-telemetry
/
opentelemetry-java-instrumentation
mirror of https://github.com/open-telemetry/opentelemetry-java-instrumentation.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1061 from fujigon/feature/fix-npe-in-jaxrs-inst-for-default-impl 

fixes NPE in jaxrs instrument with interface with default method and its impl class.
This commit is contained in:
Tyler Benson 2019-10-30 16:36:19 -07:00 committed by GitHub
parent 733342de5e 8e48a70c6d
commit 6885849254
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 251 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsDecorator.java
View File

@ -10,7 +10,8 @@ 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.LinkedList;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import javax.ws.rs.HttpMethod;
@ -36,8 +37,9 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return "jax-rs-controller";
}
public void onControllerStart(final AgentSpan span, final AgentSpan parent, final Method method) {
final String resourceName = getPathResourceName(method);
public void onControllerStart(
final AgentSpan span, final AgentSpan parent, final Class target, final Method method) {
final String resourceName = getPathResourceName(target, method);
updateParent(parent, resourceName);
span.setTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_SERVER);
@ -47,14 +49,15 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
if (isRootScope && !resourceName.isEmpty()) {
span.setTag(DDTags.RESOURCE_NAME, resourceName);
} else {
span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method));
span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(target) + "." + method.getName());
}
}
private void updateParent(final AgentSpan span, final String resourceName) {
private void updateParent(AgentSpan span, final String resourceName) {
if (span == null) {
return;
}
span = span.getLocalRootSpan();
span.setTag(Tags.COMPONENT, "jax-rs");
if (!resourceName.isEmpty()) {
@ -68,8 +71,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
*
* @return The result can be an empty string but will never be {@code null}.
*/
private String getPathResourceName(final Method method) {
final Class<?> target = method.getDeclaringClass();
private String getPathResourceName(final Class target, final Method method) {
Map<Method, String> classMap = resourceNames.get(target);
if (classMap == null) {
@ -82,7 +84,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
String resourceName = classMap.get(method);
if (resourceName == null) {
final String httpMethod = locateHttpMethod(method);
final LinkedList<Path> paths = gatherPaths(method);
final List<Path> paths = gatherPaths(target, method);
resourceName = buildResourceName(httpMethod, paths);
classMap.put(method, resourceName);
}
@ -100,13 +102,13 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return httpMethod;
}
private LinkedList<Path> gatherPaths(final Method method) {
Class<?> target = method.getDeclaringClass();
final LinkedList<Path> paths = new LinkedList<>();
while (target != Object.class) {
private List<Path> gatherPaths(Class<Object> target, final Method method) {
final List<Path> paths = new ArrayList();
while (target != null && target != Object.class) {
final Path annotation = target.getAnnotation(Path.class);
if (annotation != null) {
paths.push(annotation);
paths.add(annotation);
break; // Annotation overridden, no need to continue.
}
target = target.getSuperclass();
}
@ -117,7 +119,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return paths;
}
private String buildResourceName(final String httpMethod, final LinkedList<Path> paths) {
private String buildResourceName(final String httpMethod, final List<Path> paths) {
final String resourceName;
final StringBuilder resourceNameBuilder = new StringBuilder();
if (httpMethod != null) {

5
dd-java-agent/instrumentation/jax-rs-annotations-1/src/main/java/datadog/trace/instrumentation/jaxrs1/JaxRsAnnotationsInstrumentation.java
View File

@ -69,12 +69,13 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default
public static class JaxRsAnnotationsAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static AgentScope nameSpan(@Advice.Origin final Method method) {
public static AgentScope nameSpan(
@Advice.This final Object target, @Advice.Origin final Method method) {
// Rename the parent span according to the path represented by these annotations.
final AgentSpan parent = activeSpan();
final AgentSpan span = startSpan(JAX_ENDPOINT_OPERATION_NAME);
DECORATE.onControllerStart(span, parent, method);
DECORATE.onControllerStart(span, parent, target.getClass(), method);
DECORATE.afterStart(span);
final AgentScope scope = activateSpan(span, true);

63
dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JavaInterfaces.java Normal file
View File

@ -0,0 +1,63 @@
import javax.ws.rs.GET;
import javax.ws.rs.Path;
public class JavaInterfaces {
interface Jax {
void call();
}
@Path("interface")
interface InterfaceWithClassMethodPath extends Jax {
@Override
@GET
@Path("invoke")
void call();
}
@Path("abstract")
abstract class AbstractClassOnInterfaceWithClassPath implements InterfaceWithClassMethodPath {
@GET
@Path("call")
@Override
public void call() {
// do nothing
}
abstract void actual();
}
@Path("child")
class ChildClassOnInterface extends AbstractClassOnInterfaceWithClassPath {
@Override
void actual() {
// do nothing
}
}
// TODO: uncomment when we drop support for Java 7
// @Path("interface")
// interface DefaultInterfaceWithClassMethodPath extends Jax {
//
// @GET
// @Path("invoke")
// default void call() {
// actual();
// }
//
// void actual();
// }
//
// @Path("child")
// class DefaultChildClassOnInterface implements DefaultInterfaceWithClassMethodPath {
//
// @Override
// public void actual() {
// // do nothing
// }
// }
}

43
dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy
View File

@ -1,6 +1,7 @@
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.bootstrap.WeakMap
import datadog.trace.instrumentation.api.Tags
import datadog.trace.instrumentation.jaxrs1.JaxRsAnnotationsDecorator
import javax.ws.rs.DELETE
import javax.ws.rs.GET
import javax.ws.rs.HEAD
@ -9,8 +10,9 @@ 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
import static datadog.trace.instrumentation.jaxrs1.JaxRsAnnotationsDecorator.DECORATE
class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@ -41,7 +43,7 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
def "span named '#name' from annotations on class when is not root span"() {
setup:
def startingCacheSize = DECORATE.resourceNames.size()
def startingCacheSize = resourceNames.size()
runUnderTrace("test") {
obj.call()
}
@ -70,8 +72,8 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
}
}
}
DECORATE.resourceNames.size() == startingCacheSize + 1
DECORATE.resourceNames.get(obj.class).size() == 1
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
when: "multiple calls to the same method"
runUnderTrace("test") {
@ -80,58 +82,65 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
}
}
then: "doesn't increase the cache size"
DECORATE.resourceNames.size() == startingCacheSize + 1
DECORATE.resourceNames.get(obj.class).size() == 1
resourceNames.size() == startingCacheSize + 1
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 /c" | new InterfaceWithPath() {
@POST
@Path("/c")
void call() {
}
}
"HEAD" | new InterfaceWithPath() {
"HEAD" | 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 /abstract/child/e" | new ChildClassWithPath() {
"OPTIONS /child/e" | new ChildClassWithPath() {
@OPTIONS
@Path("/e")
void call() {
}
}
"DELETE /abstract/child" | new ChildClassWithPath() {
"DELETE /child" | new ChildClassWithPath() {
@DELETE
void call() {
}
}
"POST /abstract/child/call" | new ChildClassWithPath()
"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()
className = getName(obj.class)
// JavaInterfaces classes are loaded on a different classloader, so we need to find the right cache instance.
decorator = obj.class.classLoader.loadClass(JaxRsAnnotationsDecorator.name).getField("DECORATE").get(null)
resourceNames = (WeakMap<Class, Map<Method, String>>) decorator.resourceNames
}
def "no annotations has no effect"() {

2
dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/groovy/JerseyTest.groovy
View File

@ -9,7 +9,7 @@ class JerseyTest extends AgentTestRunner {
@Shared
@ClassRule
ResourceTestRule resources = ResourceTestRule.builder().addResource(new TestResource()).build()
ResourceTestRule resources = ResourceTestRule.builder().addResource(new Resource.Test()).build()
def "test resource"() {
setup:

17
dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/Resource.java Normal file
View File

@ -0,0 +1,17 @@
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("/test")
class Test implements Resource {
@POST
@Path("/hello/{name}")
public String addBook(@PathParam("name") final String name) {
return "Hello " + name + "!";
}
}
}

13
dd-java-agent/instrumentation/jax-rs-annotations-1/src/test/java/TestResource.java
View File

@ -1,13 +0,0 @@
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("/test")
public class TestResource {
@POST
@Path("/hello/{name}")
public String addBook(@PathParam("name") final String name) {
return "Hello " + name + "!";
}
}

31
dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java
View File

@ -7,10 +7,11 @@ import datadog.trace.api.DDSpanTypes;
import datadog.trace.api.DDTags;
import datadog.trace.bootstrap.WeakMap;
import datadog.trace.instrumentation.api.AgentSpan;
import io.opentracing.tag.Tags;
import datadog.trace.instrumentation.api.Tags;
import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.LinkedList;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import javax.ws.rs.HttpMethod;
@ -36,8 +37,9 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return "jax-rs-controller";
}
public void onControllerStart(final AgentSpan span, final AgentSpan parent, final Method method) {
final String resourceName = getPathResourceName(method);
public void onControllerStart(
final AgentSpan span, final AgentSpan parent, final Class target, final Method method) {
final String resourceName = getPathResourceName(target, method);
updateParent(parent, resourceName);
span.setTag(DDTags.SPAN_TYPE, DDSpanTypes.HTTP_SERVER);
@ -47,7 +49,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
if (isRootScope && !resourceName.isEmpty()) {
span.setTag(DDTags.RESOURCE_NAME, resourceName);
} else {
span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForMethod(method));
span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(target) + "." + method.getName());
}
}
@ -56,7 +58,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return;
}
span = span.getLocalRootSpan();
span.setTag(Tags.COMPONENT.getKey(), "jax-rs");
span.setTag(Tags.COMPONENT, "jax-rs");
if (!resourceName.isEmpty()) {
span.setTag(DDTags.RESOURCE_NAME, resourceName);
@ -69,8 +71,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
*
* @return The result can be an empty string but will never be {@code null}.
*/
private String getPathResourceName(final Method method) {
final Class<?> target = method.getDeclaringClass();
private String getPathResourceName(final Class target, final Method method) {
Map<Method, String> classMap = resourceNames.get(target);
if (classMap == null) {
@ -83,7 +84,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
String resourceName = classMap.get(method);
if (resourceName == null) {
final String httpMethod = locateHttpMethod(method);
final LinkedList<Path> paths = gatherPaths(method);
final List<Path> paths = gatherPaths(target, method);
resourceName = buildResourceName(httpMethod, paths);
classMap.put(method, resourceName);
}
@ -101,13 +102,13 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return httpMethod;
}
private LinkedList<Path> gatherPaths(final Method method) {
Class<?> target = method.getDeclaringClass();
final LinkedList<Path> paths = new LinkedList<>();
while (target != Object.class) {
private List<Path> gatherPaths(Class<Object> target, final Method method) {
final List<Path> paths = new ArrayList();
while (target != null && target != Object.class) {
final Path annotation = target.getAnnotation(Path.class);
if (annotation != null) {
paths.push(annotation);
paths.add(annotation);
break; // Annotation overridden, no need to continue.
}
target = target.getSuperclass();
}
@ -118,7 +119,7 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator {
return paths;
}
private String buildResourceName(final String httpMethod, final LinkedList<Path> paths) {
private String buildResourceName(final String httpMethod, final List<Path> paths) {
final String resourceName;
final StringBuilder resourceNameBuilder = new StringBuilder();
if (httpMethod != null) {

7
dd-java-agent/instrumentation/jax-rs-annotations-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java
View File

@ -68,13 +68,14 @@ public final class JaxRsAnnotationsInstrumentation extends Instrumenter.Default
public static class JaxRsAnnotationsAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static AgentScope nameSpan(@Advice.Origin final Method method) {
public static AgentScope nameSpan(
@Advice.This final Object target, @Advice.Origin final Method method) {
// Rename the parent span according to the path represented by these annotations.
final AgentSpan parent = activeSpan();
final AgentSpan span = startSpan(JAX_ENDPOINT_OPERATION_NAME);
JaxRsAnnotationsDecorator.DECORATE.onControllerStart(span, parent, method);
JaxRsAnnotationsDecorator.DECORATE.afterStart(span);
DECORATE.onControllerStart(span, parent, target.getClass(), method);
DECORATE.afterStart(span);
final AgentScope scope = activateSpan(span, false);
scope.setAsyncPropagation(true);

14
dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/DropwizardTest.groovy
View File

@ -140,8 +140,11 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport, Servlet3Decor
}
}
@Path("/")
static class ServiceResource {
@Path("/ignored1")
static interface TestInterface {}
@Path("/ignored2")
static abstract class AbstractClass implements TestInterface {
@GET
@Path("success")
@ -158,6 +161,10 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport, Servlet3Decor
Response.status(REDIRECT.status).location(new URI(REDIRECT.body)).build()
}
}
}
@Path("/ignored3")
static class ParentClass extends AbstractClass {
@GET
@Path("error-status")
@ -176,4 +183,7 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport, Servlet3Decor
return null
}
}
@Path("/")
static class ServiceResource extends ParentClass {}
}

63
dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JavaInterfaces.java Normal file
View File

@ -0,0 +1,63 @@
import javax.ws.rs.GET;
import javax.ws.rs.Path;
public class JavaInterfaces {
interface Jax {
void call();
}
@Path("interface")
interface InterfaceWithClassMethodPath extends Jax {
@Override
@GET
@Path("invoke")
void call();
}
@Path("abstract")
abstract class AbstractClassOnInterfaceWithClassPath implements InterfaceWithClassMethodPath {
@GET
@Path("call")
@Override
public void call() {
// do nothing
}
abstract void actual();
}
@Path("child")
class ChildClassOnInterface extends AbstractClassOnInterfaceWithClassPath {
@Override
void actual() {
// do nothing
}
}
// TODO: uncomment when we drop support for Java 7
// @Path("interface")
// interface DefaultInterfaceWithClassMethodPath extends Jax {
//
// @GET
// @Path("invoke")
// default void call() {
// actual();
// }
//
// void actual();
// }
//
// @Path("child")
// class DefaultChildClassOnInterface implements DefaultInterfaceWithClassMethodPath {
//
// @Override
// public void actual() {
// // do nothing
// }
// }
}

50
dd-java-agent/instrumentation/jax-rs-annotations-2/src/test/groovy/JaxRsAnnotationsInstrumentationTest.groovy
View File

@ -1,5 +1,7 @@
import datadog.trace.agent.test.AgentTestRunner
import io.opentracing.tag.Tags
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
@ -8,8 +10,9 @@ 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
import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.DECORATE
class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
@ -30,7 +33,7 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
resourceName "POST /a"
spanType "web"
tags {
"$Tags.COMPONENT.key" "jax-rs-controller"
"$Tags.COMPONENT" "jax-rs-controller"
defaultTags()
}
}
@ -40,7 +43,7 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
def "span named '#name' from annotations on class when is not root span"() {
setup:
def startingCacheSize = DECORATE.resourceNames.size()
def startingCacheSize = resourceNames.size()
runUnderTrace("test") {
obj.call()
}
@ -53,7 +56,7 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
resourceName name
parent()
tags {
"$Tags.COMPONENT.key" "jax-rs"
"$Tags.COMPONENT" "jax-rs"
defaultTags()
}
}
@ -63,14 +66,14 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
spanType "web"
childOf span(0)
tags {
"$Tags.COMPONENT.key" "jax-rs-controller"
"$Tags.COMPONENT" "jax-rs-controller"
defaultTags()
}
}
}
}
DECORATE.resourceNames.size() == startingCacheSize + 1
DECORATE.resourceNames.get(obj.class).size() == 1
resourceNames.size() == startingCacheSize + 1
resourceNames.get(obj.class).size() == 1
when: "multiple calls to the same method"
runUnderTrace("test") {
@ -79,58 +82,65 @@ class JaxRsAnnotationsInstrumentationTest extends AgentTestRunner {
}
}
then: "doesn't increase the cache size"
DECORATE.resourceNames.size() == startingCacheSize + 1
DECORATE.resourceNames.get(obj.class).size() == 1
resourceNames.size() == startingCacheSize + 1
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 /c" | new InterfaceWithPath() {
@POST
@Path("/c")
void call() {
}
}
"HEAD" | new InterfaceWithPath() {
"HEAD" | 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 /abstract/child/e" | new ChildClassWithPath() {
"OPTIONS /child/e" | new ChildClassWithPath() {
@OPTIONS
@Path("/e")
void call() {
}
}
"DELETE /abstract/child" | new ChildClassWithPath() {
"DELETE /child" | new ChildClassWithPath() {
@DELETE
void call() {
}
}
"POST /abstract/child/call" | new ChildClassWithPath()
"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()
className = getName(obj.class)
// JavaInterfaces classes are loaded on a different classloader, so we need to find the right cache instance.
decorator = obj.class.classLoader.loadClass(JaxRsAnnotationsDecorator.name).getField("DECORATE").get(null)
resourceNames = (WeakMap<Class, Map<Method, String>>) decorator.resourceNames
}
def "no annotations has no effect"() {