Add instrumentation for jax-ws frameworks #2314
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
|
Can you split this PR? Reviewing almost 2k lines at once won't be easy |
|
@mateuszrzeszutek It really isn't that bad, there is a 500 line xml configuration file for axis, 6 .gradle files, some tests files, the amount of actual code isn't that much + it is the same thing for 3 frameworks and it is in new files which should make it easier to review. |
|
Okay, I'll take a look at it tomorrow then. |
|
@mateuszrzeszutek thanks |
...axis2-1.6/library/src/main/java/io/opentelemetry/instrumentation/axis2/Axis2JaxWsTracer.java
Show resolved
Hide resolved
...ooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java
Show resolved
Hide resolved
| group = "org.apache.axis2" | ||
| module = "axis2-jaxws" | ||
| versions = "[1.6.0,)" | ||
| assertInverse |
There was a problem hiding this comment.
Nice, I didn't know it works without = true
There was a problem hiding this comment.
it wasn't intentional, so might not really work
There was a problem hiding this comment.
🤔
Can you verify that it works?
...axis2-1.6/library/src/main/java/io/opentelemetry/instrumentation/axis2/Axis2JaxWsTracer.java
Outdated
Show resolved
Hide resolved
| dependencies { | ||
| implementation project(':instrumentation:jaxws:jaxws-2.0-cxf-3.0:library') | ||
|
|
||
| api group: 'javax.xml.ws', name: 'jaxws-api', version: '2.3.1' |
There was a problem hiding this comment.
Won't using api include this lib in the javaagent? Consider using compileOnly + testImplementation instead
| testInstrumentation project(':instrumentation:servlet:servlet-3.0:javaagent') | ||
| testInstrumentation project(':instrumentation:jetty-8.0:javaagent') | ||
|
|
||
| latestDepTestLibrary group: 'com.sun.xml.ws', name: 'jaxws-rt', version: '2+' |
There was a problem hiding this comment.
If tests don't work for [3,) shouldn't we add a limit in muzzle too?
There was a problem hiding this comment.
added classLoaderMatcher
| } | ||
|
|
||
| public void end(Packet packet, Throwable throwable) { | ||
| Scope scope = (Scope) packet.get(TracingPropertySet.SCOPE_KEY); |
There was a problem hiding this comment.
Properties are not removed from the packet (unlike other JAX-WS instrumentations), is that okay?
There was a problem hiding this comment.
For axis and cxf I removed the properties to ensure that we won't end up closing the same span twice in case these interceptors are called twice for whatever reason. On metro these properties can not be removed, remove method exists, but calling it will complain that the property is read only. I guess if needed I could add a property with mutable value that could track whether end has already been called.
…a/io/opentelemetry/instrumentation/axis2/Axis2JaxWsTracer.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Adds instrumentation for cxf, metro and axis2 jax-ws frameworks. Currently instrumentation only creates a span for web service invocations and updates server span name.
There is already an instrumentation that creates spans for jax-ws methods based on annotations. I believe it is slightly flawed as it is not able to identify all webservice methods and also identifies some methods as webservice methods although they are not.