Add support for JAX-RS-based transaction names #224

felixbarny · 2018-09-24T18:58:24Z

closes #67

codecov-io · 2018-09-25T08:39:13Z

Codecov Report

Merging #224 into master will increase coverage by 0.01%.
The diff coverage is 79.04%.

@@             Coverage Diff              @@
##             master     #224      +/-   ##
============================================
+ Coverage     74.58%   74.59%   +0.01%     
- Complexity     1190     1217      +27     
============================================
  Files           123      127       +4     
  Lines          4391     4488      +97     
  Branches        437      451      +14     
============================================
+ Hits           3275     3348      +73     
- Misses          917      935      +18     
- Partials        199      205       +6

Impacted Files	Coverage Δ	Complexity Δ
...stic/apm/bci/bytebuddy/ClassLoaderNameMatcher.java	`100% <100%> (ø)`	`6 <5> (ø)`	⬇️
.../co/elastic/apm/bci/ElasticApmInstrumentation.java	`100% <100%> (ø)`	`7 <1> (+1)`	⬆️
...stic/apm/bci/bytebuddy/MethodHierarchyMatcher.java	`100% <100%> (ø)`	`11 <11> (?)`
...c/apm/impl/stacktrace/StacktraceConfiguration.java	`100% <100%> (ø)`	`4 <0> (ø)`	⬇️
...ddy/SimpleMethodSignatureOffsetMappingFactory.java	`28.57% <28.57%> (ø)`	`2 <2> (?)`
...o/elastic/apm/configuration/CoreConfiguration.java	`91.97% <36.36%> (-4.86%)`	`15 <1> (+1)`
...java/co/elastic/apm/configuration/StartupInfo.java	`79.48% <50%> (-1.6%)`	`12 <0> (ø)`
.../main/java/co/elastic/apm/bci/ElasticApmAgent.java	`71.31% <60%> (-0.69%)`	`19 <0> (ø)`
...apm/jaxrs/JaxRsTransactionNameInstrumentation.java	`79.16% <79.16%> (ø)`	`7 <7> (?)`
...astic/apm/bci/bytebuddy/CustomElementMatchers.java	`92% <92%> (ø)`	`6 <6> (?)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ed21af...56459ea. Read the comment docs.

eyalkoren

There are some issues I commented about that maybe are due to some misunderstanding.
Regardless, I assumed that the support for JAX-RS would actually be a combination of the path and the HTTP method. I think these are much more meaningful than class and method name. For sure that's the case to someone monitoring the application and not familiar with the code. I think the path is ALWAYS better than code, and I understand we are reluctant to rely on it due to path params, but the support for JAX-RS can rely on the @Path and @PathParams in order to avoid that, can't it?

eyalkoren · 2018-09-25T11:45:21Z

apm-agent-core/src/main/java/co/elastic/apm/bci/bytebuddy/CustomElementMatchers.java

+    private static boolean canLoadClass(ClassLoader target, String className) {
+        boolean result;
+        try {
+            target.loadClass(className);


That's a very "intrusive" thing to do just to check something. It may cause premature class loading, which may or may not have side effects.

What side effects are you thinking about? What could be an alternative to this approach?

I think a slightly better way would be trying to load the class via java.lang.Class#forName(java.lang.String, boolean, java.lang.ClassLoader) and setting the initialize flag to false.

We have direct control over which classes are affected. In this case, it is just the javax.ws.rs.Path class, which is also just an annotation.

OK, so at least you should add an ALL CAPITALS warning that this check should be used with caution and only for classes that have no dependencies (like a small interface or annotation)

eyalkoren · 2018-09-25T11:52:39Z

...apm-jaxrs-plugin/src/main/java/co/elastic/apm/jaxrs/JaxRsTransactionNameInstrumentation.java

+
+    @Override
+    public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+        return isInAnyPackage(applicationPackages, any())


Don't you think it is likely that there are frameworks that use JAX-RS? I am not sure what we want to do if there are, but this means we will miss them, right?

...apm-jaxrs-plugin/src/main/java/co/elastic/apm/jaxrs/JaxRsTransactionNameInstrumentation.java

apm-agent-core/src/main/java/co/elastic/apm/bci/bytebuddy/CustomElementMatchers.java

eyalkoren · 2018-09-25T12:17:45Z

...apm-jaxrs-plugin/src/main/java/co/elastic/apm/jaxrs/JaxRsTransactionNameInstrumentation.java

+    @Override
+    public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+        // setting application_packages makes this matcher more performant but is not required
+        return isInAnyPackage(applicationPackages, ElementMatchers.<TypeDescription>any())


Don't you think it is likely that there are frameworks that use JAX-RS? I am not sure what we want to do if there are, but this means we will miss them, right?

Good point. Not sure if that is a likely case. The use case would be when importing a 3rd party library which also contains JAX-RS resources, right?

Yes, that what I meant

I am not sure if this check provides any value and it is an expensive one. I think it better be removed, but in any case the order of the checks should probably be reversed

Quite the contrary. This isInAnyPackage matcher is a really cheap one because the class file does not have to be parsed at all. In general, the matchers which match based on the class names are the cheapest ones. Without this check, each and every class has to be parsed to see if it is annotated with @Path. Prepending the isInAnyPackage matcher considerably limits the amount classes which have to be checked with a more expensive matcher :). Also, typically, application_packages is only set to one or a handful of packages.

In the case you are importing a 3rd party library containing JAX-RS resources, it probably makes sense to add that to the application_packages namespace anyway. Because then stack traces also highlight those classes. But I can see that this may be confusing for users which wonder why some endpoints are not picked up. It's a tradeoff but I think with proper documentation (right at the supported technologies page), this would be the tradeoff I prefer.

Without this check, each and every class has to be parsed to see if it is annotated with @Path. Prepending the isInAnyPackage matcher considerably limits the amount classes which have to be checked with a more expensive matcher :)

Right, my mistake, I confused with the CL matcher that use caches...

eyalkoren · 2018-09-26T02:17:19Z

...apm-jaxrs-plugin/src/main/java/co/elastic/apm/jaxrs/JaxRsTransactionNameInstrumentation.java

+    public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+        // setting application_packages makes this matcher more performant but is not required
+        return isInAnyPackage(applicationPackages, ElementMatchers.<TypeDescription>any())
+            .and(isAnnotatedWith(named("javax.ws.rs.Path")));


Are you sure the class must be annotated with javax.ws.rs.Path? Isn't it allowed to only annotate methods?

Some answers on Stack Overflow suggested@Path is required on the class level.

But this quote from the JAX-RS 2.0 spec lets me doubt this:

A resource class is a Java class that uses JAX-RS annotations to implement a corresponding Web resource.
Resource classes are POJOs that have at least one method annotated with @Path or a request method designator.

I'll investigate

If you have a test app maybe just testing would be fastest

eyalkoren · 2018-09-26T02:24:31Z

Some additional minor notes about the filtering.
However, unless you think relying on @PathParam is still not enough to guard us from dynamic URLs, I believe the end result of this PR should be that transaction names are the combination of the class @Path and the method @Path. That's the real essence of RESTful API, which is what this is all about.

felixbarny · 2018-09-26T07:31:25Z

I think the path is ALWAYS better than code

Well, I guess it depends on who is viewing the data and also how the controllers and methods are named. The verb and method don't always convey the semantics of an operation

Example: POST /users/$id can mean a lot of things and not all applications are actually conforming to the semantics of the verbs. Does it mean update user? Or maybe even delete user? Something else? Especially business people may not know the subtle differences between POST and PUT, for example. So I think sometimes it's easier for them to display the method name, as it might carry more information and tells what is actually happening. Of course, that requires the developers to use meaningful method names.

Maybe we can discuss this in today's agent meeting and see what other agents are doing. Also for Spring MVC, we already use ClassName#methodName as the transaction name, so it makes sense to be consistent in the JAX-RS instrumentation.

I understand we are reluctant to rely on it due to path params, but the support for JAX-RS can rely on the @path and @PathParams in order to avoid that, can't it?

Sure, we would not have problems with transaction name explosions when relying on the value of @Path.

eyalkoren · 2018-09-26T08:00:13Z

When I designed a RESTful server, I put a lot of thought in the paths structure, as they are really the API for the system, even though I am a developer. Of course I named classes and methods with proper names, but they had no real importance. Think about it this way - you can easily refactor your server changing the class and method names and then upgrade without affecting the system. That's because they are not part of the API.
Regarding to who our customers are - I don't know well enough. I can only guess that some APM users may not have any knowledge about the code.
Anyway, let's not hold it for that. We can discuss and revisit later on.

felixbarny · 2018-09-26T09:49:39Z

Another important quote from the spec (http://download.oracle.com/otn-pub/jcp/jaxrs-2_0-fr-eval-spec/jsr339-jaxrs-2.0-final-spec.pdf section 3.6 Annotation Inheritance):

Note that inheritance of class or interface annotations is not supported.

So it seems that the @Path annotation has to be on the concrete resource class and a @Path on a superclass or an implemented interface is not sufficient. However, method and method parameter-level annotations can be inherited from superclasses/interfaces:

JAX-RS annotations may be used on the methods and method parameters of a super-class or an implemented interface.

JAX-RS annotations may be used on the methods and method parameters of a super-class or an implemented interface.

felixbarny · 2018-09-27T12:20:46Z

Added annotation search on the method hierarchy to satisfy

JAX-RS annotations may be used on the methods and method parameters of a super-class or an implemented interface.

I feel like we should probably have a type pool cache before merging...

felixbarny · 2018-10-05T06:05:15Z

The performance of matching annotations of a specific type does not seem to be great :/
Enabling the JAX-RS instrumentation increases the type matching overhead by a factor of ~3.

The good news is that if users specify their application_packages, the instrumentation will only look for JAX-RS resources in those packages. Another good news is that the instrumentation will only affect the matching performance if the user has a dependency to JAX-RS at all.

enable_type_matching_name_pre_filtering	excluded_from_instrumentation	Total time spent matching
true	jax-rs	2,555ms
true		6,893ms
false	jax-rs	3,497ms
false		12,351ms

I gathered these metrics by executing the WildFlyIT 13.0.0.Final.

felixbarny · 2018-10-15T17:04:55Z

We concluded we should log a warning if the user did not configure the application_packages. In addition, we will also extend the default list of excluded packages.

felixbarny · 2018-10-16T09:59:39Z

Jenkins retest this please

Add support for JAX-RS-based transaction names

645d45f

closes elastic#67

felixbarny added the [zube]: In Review label Sep 24, 2018

felixbarny self-assigned this Sep 24, 2018

Fix Java 7 and generics-related compilation errors

4135335

felixbarny requested a review from eyalkoren September 25, 2018 08:37

eyalkoren reviewed Sep 25, 2018

View reviewed changes

apm-agent-core/src/main/java/co/elastic/apm/bci/bytebuddy/CustomElementMatchers.java Outdated Show resolved Hide resolved

eyalkoren reviewed Sep 25, 2018

View reviewed changes

felixbarny added 2 commits September 25, 2018 15:51

Use Class.forName in canLoadClass

f8d4822

Merge remote-tracking branch 'origin/master' into jaxrs-annotations

4a2eb91

eyalkoren reviewed Sep 26, 2018

View reviewed changes

eyalkoren approved these changes Sep 26, 2018

View reviewed changes

Match @HttpMethod annotations in method hierarchy

12ec7d7

JAX-RS annotations may be used on the methods and method parameters of a super-class or an implemented interface.

felixbarny and others added 2 commits October 1, 2018 14:39

Add warning to CustomElementMatchers#classLoaderCanLoadClass

813bb7f

Merge branch 'master' into jaxrs-annotations

cbb0713

felixbarny and others added 2 commits October 5, 2018 08:08

Move isInAnyPackage matcher to getTypeMatcherPreFilter

a8e9887

Merge branch 'master' into jaxrs-annotations

a2520b0

felixbarny and others added 3 commits October 15, 2018 19:05

Merge branch 'master' into jaxrs-annotations

ab177cf

Log a warning when application_packages has not been configured

a07470d

Merge branch 'master' into jaxrs-annotations

e3dd92d

Fix tests

56459ea

felixbarny merged commit 936d1ba into elastic:master Oct 16, 2018

felixbarny deleted the jaxrs-annotations branch October 16, 2018 14:35

zube bot added [zube]: Done and removed [zube]: In Review labels Oct 16, 2018

alvarolobato removed the [zube]: Done label Oct 22, 2018

Add support for JAX-RS-based transaction names #224

Add support for JAX-RS-based transaction names #224

Uh oh!

Conversation

felixbarny commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren commented Sep 26, 2018

Uh oh!

felixbarny commented Sep 26, 2018

Uh oh!

eyalkoren commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixbarny commented Sep 26, 2018

Uh oh!

felixbarny commented Sep 27, 2018

Uh oh!

felixbarny commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixbarny commented Oct 15, 2018

Uh oh!

felixbarny commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felixbarny commented Sep 24, 2018 •

edited

Loading

codecov-io commented Sep 25, 2018 •

edited

Loading

eyalkoren Sep 26, 2018 •

edited

Loading

eyalkoren commented Sep 26, 2018 •

edited

Loading

felixbarny commented Oct 5, 2018 •

edited

Loading