-
Notifications
You must be signed in to change notification settings - Fork 327
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.
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #600 +/- ##
=========================================
Coverage 63.13% 63.13%
Complexity 68 68
=========================================
Files 188 188
Lines 7278 7278
Branches 845 845
=========================================
Hits 4595 4595
Misses 2413 2413
Partials 270 270Continue to review full report at Codecov.
|
| doRequest("testProxyProxy"); | ||
|
|
||
| List<Transaction> actualTransactions = reporter.getTransactions(); | ||
| assertThat(actualTransactions).hasSize(2); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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")
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Excluding
$viewproxy classes prevents explosion of transaction names in certain scenarios, see #512.