Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uncomment and fix JAX-RS default method tests #2930

Merged
merged 3 commits into from
May 11, 2021
Merged

Uncomment and fix JAX-RS default method tests #2930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb18f3f
Uncomment and fix JAX-RS default method tests
trask May 8, 2021
1caf03c
Fix unit tests
trask May 8, 2021
8b0fe8c
Merge remote-tracking branch 'upstream/main' into jaxrs-default-metho…
trask May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ...opentelemetry/javaagent/instrumentation/apachecamel/ApacheCamelInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static java.util.Collections.singletonList;
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

import com.google.auto.service.AutoService;
Expand Down Expand Up @@ -51,7 +49,7 @@ public ElementMatcher<ClassLoader> classLoaderOptimization() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return not(isAbstract()).and(implementsInterface(named("org.apache.camel.CamelContext")));
return implementsInterface(named("org.apache.camel.CamelContext"));
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...n/java/io/opentelemetry/javaagent/instrumentation/grizzly/HttpHandlerInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.grizzly;

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.safeHasSuperType;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
Expand All @@ -28,7 +28,7 @@ public ElementMatcher<ClassLoader> classLoaderOptimization() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return safeHasSuperType(named("org.glassfish.grizzly.http.server.HttpHandler"));
return extendsClass(named("org.glassfish.grizzly.http.server.HttpHandler"));
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...o/opentelemetry/javaagent/instrumentation/grpc/v1_6/GrpcServerBuilderInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.grpc.v1_6;

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.safeHasSuperType;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
Expand All @@ -31,7 +31,7 @@ public ElementMatcher<ClassLoader> classLoaderOptimization() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return safeHasSuperType(named("io.grpc.ServerBuilder"));
return extendsClass(named("io.grpc.ServerBuilder"));
}

@Override
Expand Down
5 changes: 2 additions & 3 deletions ...ion/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentInstrumentationSpecifica
}

@Unroll
def "span named '#paramName' from annotations on class when is not root span"() {
def "span named '#paramName' from annotations on class '#className' when is not root span"() {
setup:
runUnderServerTrace("test") {
obj.call()
Expand Down Expand Up @@ -128,8 +128,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentInstrumentationSpecifica
}
"/child/call" | new ChildClassWithPath()
"/child/call" | new JavaInterfaces.ChildClassOnInterface()
// TODO: uncomment when we drop support for Java 7
// "/child/invoke" | new JavaInterfaces.DefaultChildClassOnInterface()
"/child/call" | new JavaInterfaces.DefaultChildClassOnInterface()

className = getClassName(obj.class)
}
Expand Down
47 changes: 24 additions & 23 deletions instrumentation/jaxrs/jaxrs-1.0/javaagent/src/test/java/JavaInterfaces.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ interface InterfaceWithClassMethodPath extends Jax {
}

@Path("abstract")
abstract class AbstractClassOnInterfaceWithClassPath implements InterfaceWithClassMethodPath {
abstract static class AbstractClassOnInterfaceWithClassPath
implements InterfaceWithClassMethodPath {

@GET
@Path("call")
Expand All @@ -36,33 +37,33 @@ public void call() {
}

@Path("child")
class ChildClassOnInterface extends AbstractClassOnInterfaceWithClassPath {
static 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
// }
// }
@Path("interface")
interface DefaultInterfaceWithClassMethodPath extends Jax {

@Override
@GET
@Path("call")
default void call() {
actual();
}

void actual();
}

@Path("child")
static class DefaultChildClassOnInterface implements DefaultInterfaceWithClassMethodPath {

@Override
public void actual() {
// do nothing
}
}
}
5 changes: 2 additions & 3 deletions ...rs/jaxrs-2.0/jaxrs-2.0-testing/src/main/groovy/JaxRsAnnotationsInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ abstract class JaxRsAnnotationsInstrumentationTest extends AgentInstrumentationS
}

@Unroll
def "span named '#paramName' from annotations on class when is not root span"() {
def "span named '#paramName' from annotations on class '#className' when is not root span"() {
setup:
runUnderServerTrace("test") {
obj.call()
Expand Down Expand Up @@ -128,8 +128,7 @@ abstract class JaxRsAnnotationsInstrumentationTest extends AgentInstrumentationS
}
"/child/call" | new ChildClassWithPath()
"/child/call" | new JavaInterfaces.ChildClassOnInterface()
// TODO: uncomment when we drop support for Java 7
// "GET /child/invoke" | new JavaInterfaces.DefaultChildClassOnInterface()
"/child/call" | new JavaInterfaces.DefaultChildClassOnInterface()

className = getClassName(obj.class)
}
Expand Down
47 changes: 24 additions & 23 deletions instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-testing/src/main/java/JavaInterfaces.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ interface InterfaceWithClassMethodPath extends Jax {
}

@Path("abstract")
abstract class AbstractClassOnInterfaceWithClassPath implements InterfaceWithClassMethodPath {
abstract static class AbstractClassOnInterfaceWithClassPath
implements InterfaceWithClassMethodPath {

@GET
@Path("call")
Expand All @@ -36,33 +37,33 @@ public void call() {
}

@Path("child")
class ChildClassOnInterface extends AbstractClassOnInterfaceWithClassPath {
static 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
// }
// }
@Path("interface")
interface DefaultInterfaceWithClassMethodPath extends Jax {

@Override
@GET
@Path("call")
default void call() {
actual();
}

void actual();
}

@Path("child")
static class DefaultChildClassOnInterface implements DefaultInterfaceWithClassMethodPath {

@Override
public void actual() {
// do nothing
}
}
}
4 changes: 2 additions & 2 deletions ...ain/java/io/opentelemetry/javaagent/instrumentation/khttp/KHttpInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.javaagent.instrumentation.khttp;

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.safeHasSuperType;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
Expand Down Expand Up @@ -45,7 +45,7 @@ public ElementMatcher<ClassLoader> classLoaderOptimization() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return safeHasSuperType(named("khttp.KHttp"));
return extendsClass(named("khttp.KHttp"));
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions .../java/io/opentelemetry/javaagent/instrumentation/vertx/VertxWebInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@

package io.opentelemetry.javaagent.instrumentation.vertx;

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.safeHasSuperType;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isInterface;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import com.google.auto.service.AutoService;
Expand Down Expand Up @@ -47,7 +45,7 @@ public ElementMatcher<ClassLoader> classLoaderOptimization() {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return not(isInterface()).and(safeHasSuperType(named("io.vertx.ext.web.Route")));
return implementsInterface(named("io.vertx.ext.web.Route"));
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions ...-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ public static ElementMatcher.Junction<TypeDescription> extendsClass(

public static ElementMatcher.Junction<TypeDescription> implementsInterface(
ElementMatcher<TypeDescription> matcher) {
return not(isInterface())
.and(new SafeHasSuperTypeMatcher(new SafeErasureMatcher<>(matcher), true));
return new SafeHasSuperTypeMatcher(new SafeErasureMatcher<>(matcher), true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change was not needed for JAX-RS default methods, but I looked through callers and not sure why we wouldn't also want to instrument interface default methods, so removed not(isInterface()) here too

}

public static ElementMatcher.Junction<TypeDescription> hasInterface(
Expand All @@ -36,8 +35,7 @@ public static ElementMatcher.Junction<TypeDescription> hasInterface(

public static ElementMatcher.Junction<TypeDescription> safeHasSuperType(
ElementMatcher<TypeDescription> matcher) {
return not(isInterface())
.and(new SafeHasSuperTypeMatcher(new SafeErasureMatcher<>(matcher), false));
return new SafeHasSuperTypeMatcher(new SafeErasureMatcher<>(matcher), false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, interface default methods are not instrumented. I looked through the callers and changed some of them to extendsClass(), and the remaining seem more correct taking interface default methods in to account.

}

/**
Expand Down
4 changes: 3 additions & 1 deletion ...tooling/src/main/java/io/opentelemetry/javaagent/tooling/context/FieldBackedProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.safeHasSuperType;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.BOOTSTRAP_CLASSLOADER;
import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;

import io.opentelemetry.instrumentation.api.caching.Cache;
import io.opentelemetry.instrumentation.api.config.Config;
Expand Down Expand Up @@ -385,7 +387,7 @@ public AgentBuilder.Identified.Extendable additionalInstrumentation(
*/
builder =
builder
.type(safeHasSuperType(named(entry.getKey())))
.type(not(isAbstract()).and(safeHasSuperType(named(entry.getKey()))))
.and(safeToInjectFieldsMatcher())
.and(ActualInstrumentationExtensionImplementation.NOT_DECORATOR_MATCHER)
.transform(NoOpTransformer.INSTANCE);
Expand Down
8 changes: 5 additions & 3 deletions ...roovy/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/HasInterfaceMatcherTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@ import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.
import static net.bytebuddy.matcher.ElementMatchers.named

import io.opentelemetry.javaagent.tooling.AgentTooling
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.*
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.A
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.B
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.E
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.F
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.G
import net.bytebuddy.description.type.TypeDescription
import net.bytebuddy.description.type.TypeList
import net.bytebuddy.jar.asm.Opcodes
import spock.lang.Shared
import spock.lang.Specification

Expand Down Expand Up @@ -57,7 +60,6 @@ class HasInterfaceMatcherTest extends Specification {
then:
!result // default to false
noExceptionThrown()
1 * type.getModifiers() >> Opcodes.ACC_ABSTRACT
1 * type.isInterface() >> true
1 * type.asGenericType() >> typeGeneric
1 * typeGeneric.asErasure() >> { throw new Exception("asErasure exception") }
Expand Down
15 changes: 8 additions & 7 deletions ...o/opentelemetry/javaagent/tooling/bytebuddy/matcher/ImplementsInterfaceMatcherTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,13 @@ import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.
import static net.bytebuddy.matcher.ElementMatchers.named

import io.opentelemetry.javaagent.tooling.AgentTooling
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.*
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.A
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.B
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.E
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.F
import io.opentelemetry.javaagent.tooling.bytebuddy.matcher.testclasses.G
import net.bytebuddy.description.type.TypeDescription
import net.bytebuddy.description.type.TypeList
import net.bytebuddy.jar.asm.Opcodes
import spock.lang.Shared
import spock.lang.Specification

Expand All @@ -28,10 +31,10 @@ class ImplementsInterfaceMatcherTest extends Specification {

where:
matcherClass | type | result
A | A | false
A | B | false
A | A | true
A | B | true
B | A | false
A | E | false
A | E | true
A | F | true
A | G | true
F | A | false
Expand All @@ -54,7 +57,6 @@ class ImplementsInterfaceMatcherTest extends Specification {
then:
!result // default to false
noExceptionThrown()
1 * type.getModifiers() >> Opcodes.ACC_ABSTRACT
1 * type.isInterface() >> true
1 * type.asGenericType() >> typeGeneric
1 * typeGeneric.asErasure() >> { throw new Exception("asErasure exception") }
Expand All @@ -79,7 +81,6 @@ class ImplementsInterfaceMatcherTest extends Specification {
then:
!result // default to false
noExceptionThrown()
1 * type.getModifiers() >> Opcodes.ACC_ABSTRACT
1 * type.isInterface() >> true
1 * type.asGenericType() >> typeGeneric
1 * typeGeneric.asErasure() >> { throw new Exception("asErasure exception") }
Expand Down
Loading