Skip to content

Exclude $view proxies from JAX-RS instrumentation #600

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

Merged
merged 2 commits into from
Apr 25, 2019
Merged

Exclude $view proxies from JAX-RS instrumentation #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4ab0f7
Exclude $view proxies from JAX-RS instrumentation
mdindoffer Apr 18, 2019
432f04a
Merge branch 'master' into excludeView
felixbarny Apr 23, 2019
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
1 change: 1 addition & 0 deletions ...-plugin/src/main/java/co/elastic/apm/agent/jaxrs/JaxRsTransactionNameInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public ElementMatcher<? super TypeDescription> getTypeMatcher() {
if (configuration.isEnableJaxrsAnnotationInheritance()) {
return not(isInterface())
.and(not(ElementMatchers.<TypeDescription>nameContains("$Proxy")))
.and(not(ElementMatchers.<TypeDescription>nameContains("$view")))
.and(isAnnotatedWith(named("javax.ws.rs.Path"))
.or(hasSuperType(isAnnotatedWith(named("javax.ws.rs.Path"))))
);
Expand Down
36 changes: 32 additions & 4 deletions ...gin/src/test/java/co/elastic/apm/agent/jaxrs/JaxRsTransactionNameInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,20 @@ public void testJaxRsTransactionNameWithJaxrsAnnotationInheritance() {
assertThat(actualTransactions.get(2).getName().toString()).isEqualTo("ResourceWithPathOnAbstract#testMethod");
}

@Test
public void testProxyClassInstrumentationExclusion() {
when(config.getConfig(JaxRsConfiguration.class).isEnableJaxrsAnnotationInheritance()).thenReturn(true);
ElasticApmAgent.initInstrumentation(tracer, ByteBuddyAgent.install());

doRequest("testViewProxy");
doRequest("testProxyProxy");

List<Transaction> actualTransactions = reporter.getTransactions();
assertThat(actualTransactions).hasSize(2);
Copy link
Contributor

@eyalkoren eyalkoren Apr 24, 2019

Choose a reason for hiding this comment

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

Why do we capture them at all? Through a base servlet or servlet filter?

Copy link
Member

@felixbarny felixbarny Apr 24, 2019

Choose a reason for hiding this comment

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

I guess these are just EJB proxies for the actual resource class. But maybe we should rather exclude those proxies globally here:

apm-agent-java/apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

Lines 229 to 246 in 4ab2b6f

private final ConfigurationOption<List<WildcardMatcher>> classesExcludedFromInstrumentation = ConfigurationOption
.builder(new ListValueConverter<>(new WildcardMatcherValueConverter()), List.class)
.key("classes_excluded_from_instrumentation")
.configurationCategory(CORE_CATEGORY)
.tags("internal")
.description("\n" +
"\n" +
WildcardMatcher.DOCUMENTATION)
.dynamic(true)
.buildWithDefault(Arrays.asList(
WildcardMatcher.valueOf("(?-i)org.infinispan*"),
WildcardMatcher.valueOf("(?-i)org.apache.xerces*"),
WildcardMatcher.valueOf("(?-i)org.jboss.as.*"),
WildcardMatcher.valueOf("(?-i)io.undertow.core*"),
WildcardMatcher.valueOf("(?-i)org.eclipse.jdt.ecj*"),
WildcardMatcher.valueOf("(?-i)org.wildfly.extension.*"),
WildcardMatcher.valueOf("(?-i)org.wildfly.security*")
));

by adding WildcardMatcher.caseSensitiveMatcher("*$Proxy") and WildcardMatcher.caseSensitiveMatcher("*$view")

Copy link
Contributor

@eyalkoren eyalkoren Apr 24, 2019

Choose a reason for hiding this comment

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

I meant- why are they captured in the test?

Copy link
Member

@felixbarny felixbarny Apr 24, 2019

Choose a reason for hiding this comment

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

In a real-world scenario, the transaction would be created by the servlet instrumentation. The JAX-RS instrumentation does not create transactions, it only sets the name. In this test, the transaction is manually created by the private doRequest method.

WDYT about moving it to the default classes_excluded_from_instrumentation?

Copy link
Contributor

@eyalkoren eyalkoren Apr 24, 2019

Choose a reason for hiding this comment

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

Ahh, missed the manual transaction creation.

I am not sure about moving to classes_excluded_from_instrumentation. Here it causes a transaction name explosion. Maybe in some scenarios it will capture things that otherwise won't be captured? Like some implementation that creates $Proxy implementations to interface stubs without a resource class in between? If you don't think this is a valid concern, go ahead and move it there.

assertThat(actualTransactions.get(0).getName().toString()).isEqualTo("unnamed");
assertThat(actualTransactions.get(1).getName().toString()).isEqualTo("unnamed");
}

@Test
public void testJaxRsTransactionNameNonSampledTransactions() throws IOException {
config.getConfig(CoreConfiguration.class).getSampleRate().update(0.0, SpyConfiguration.CONFIG_SOURCE_NAME);
Expand All @@ -121,8 +135,13 @@ public void testJaxRsTransactionNameNonSampledTransactions() throws IOException
/**
* @return configuration for the jersey test server. Includes all resource classes in the co.elastic.apm.agent.jaxrs.resources package.
*/
@Override
protected Application configure() {
return new ResourceConfig(ResourceWithPath.class, ResourceWithPathOnInterface.class, ResourceWithPathOnAbstract.class);
return new ResourceConfig(ResourceWithPath.class,
ResourceWithPathOnInterface.class,
ResourceWithPathOnAbstract.class,
ProxiedClass$view.class,
ProxiedClass$Proxy.class);
}

/**
Expand All @@ -147,29 +166,38 @@ public interface SuperResourceInterface {
@Path("testInterface")
public interface ResourceInterfaceWithPath extends SuperResourceInterface {
String testMethod();

}

public interface ResourceInterfaceWithoutPath extends SuperResourceInterface {
String testMethod();
}

public abstract static class AbstractResourceClassWithoutPath implements ResourceInterfaceWithoutPath {

}

@Path("testAbstract")
public abstract static class AbstractResourceClassWithPath implements ResourceInterfaceWithoutPath {
}

@Path("testViewProxy")
public static class ProxiedClass$view implements SuperResourceInterface {
public String testMethod() {
return "ok";
}
}

@Path("testProxyProxy")
public static class ProxiedClass$Proxy implements SuperResourceInterface {
public String testMethod() {
return "ok";
}
}

@Path("test")
public static class ResourceWithPath extends AbstractResourceClassWithoutPath {
public String testMethod() {
return "ok";
}

}

public static class ResourceWithPathOnAbstract extends AbstractResourceClassWithPath {
Expand Down