Support JAX-WS transaction naming #486
Conversation
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
============================================
+ Coverage 64.39% 69.48% +5.09%
- Complexity 68 1414 +1346
============================================
Files 165 156 -9
Lines 6235 5755 -480
Branches 719 614 -105
============================================
- Hits 4015 3999 -16
+ Misses 1974 1510 -464
Partials 246 246Continue to review full report at Codecov.
|
| public ElementMatcher<? super NamedElement> getTypeMatcherPreFilter() { | ||
| // setting application_packages makes this matcher more performant but is not required | ||
| // could lead to false negative matches when importing a 3rd party library whose JAX-WS resources are exposed | ||
| return isInAnyPackage(applicationPackages, ElementMatchers.<NamedElement>any()); |
There was a problem hiding this comment.
Is this false negative interesting? I assume mostly it would be application code, right?
There was a problem hiding this comment.
Yes, this would only be an edge case. I expect that mostly, the JAX-WS resources are inside of the application. If they're not, then they can just add the 3rd party packages as well.
|
|
||
| @Override | ||
| public Collection<String> getInstrumentationGroupNames() { | ||
| return Arrays.asList("jax-ws", "jax-ws-annotations"); |
There was a problem hiding this comment.
Why both? It would make sense if we want to disable annotation-based instrumentation in general and then have annotations both here and in jax-rs, but this just seems like a duplication
There was a problem hiding this comment.
I did it mainly to do it in a similar way to the JAX-RS instrumentation. I think my initial intention was to have another category for the client instrumentation. But we ended up not instrumenting the JAX-RS client, so it's kind of useless as it is right now. I'll remove the jax-*-annotations category. We can always re-add them later if needed.
| if (tracer != null) { | ||
| final Transaction transaction = tracer.currentTransaction(); | ||
| if (transaction != null && transaction.getName().length() == 0) { | ||
| transaction.withName(signature); |
There was a problem hiding this comment.
SOAP clients calls will not be captured?
There was a problem hiding this comment.
This is not about capturing SOAP calls, it's just about naming SOAP transactions
There was a problem hiding this comment.
I meant- would this instrumentation be executed for SOAP client invocations, thus changing the wrong transaction names, or the annotation in the type matching eliminates that?
There was a problem hiding this comment.
That's actually a good question. I think it works because the stub created for the WS request does not contain the annotations.
There was a problem hiding this comment.
The test I suggested would verify that
| import javax.jws.WebService; | ||
| import javax.jws.soap.SOAPBinding; | ||
|
|
||
| import static org.assertj.core.api.Java6Assertions.assertThat; |
There was a problem hiding this comment.
Ask my IDE 😄
But seriously, good catch 👍
| assertThat(response.body().string()).isNotEmpty(); | ||
|
|
||
| final List<JsonNode> reportedTransactions = test.getReportedTransactions(); | ||
| assertThat(reportedTransactions.stream().map(node -> node.get("name").textValue())).contains("HelloWorldServiceImpl#sayHello"); |
There was a problem hiding this comment.
Full test should be:
- First transaction is
HelloWorldServiceImpl#sayHello - second transaction is the SOAP client servlet one
- the client span is reported
And even though tested within the app, for clarity I would test that both transactions have the same trace ID
|
Also - update changelog |
4 participants
closes #332