CXF 4.0 Instrumentation #11971
CXF 4.0 Instrumentation #11971
Conversation
| group.set("org.apache.cxf") | ||
| module.set("cxf-rt-frontend-jaxws") | ||
| versions.set("[4.0.0,)") | ||
| extraDependency("jakarta.servlet:jakarta.servlet-api:5.0.0") |
There was a problem hiding this comment.
We usually suggest to add assertInverse.set(true) here. If can't do it, it had better to add necessary comment like
There was a problem hiding this comment.
(sorry, I unresolved this discussion as I wasn't clear what the resolution was, thanks)
...n/java/io/opentelemetry/javaagent/instrumentation/jaxws/v3_0/JaxWsInstrumentationModule.java
Outdated
Show resolved
Hide resolved
....0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/cxf/CxfSingletons.java
Outdated
Show resolved
Hide resolved
| @@ -64,7 +64,7 @@ Download | |||
| the [latest version](https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases/latest/download/opentelemetry-javaagent.jar). | |||
|
|
|||
| This package includes the instrumentation agent as well as | |||
| instrumentations for all supported libraries and all available data exporters. | |||
| instrumentation for all supported libraries and all available data exporters. | |||
There was a problem hiding this comment.
@theletterf could you review this change
There was a problem hiding this comment.
IntelliJ complained about the grammar here, and I changed it. I have other similar changes uncommitted locally. Please advise on how to proceed.
|
Do I understand correctly that for cxf instrumentation this is copy paste from the existing code and the only change is that in https://github.com/ammachado/opentelemetry-java-instrumentation/blob/b4dd73c7e91f40add167978d8733d382bcf7bacf/instrumentation/jaxws/jaxws-3.0-cxf-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/cxf/CxfServerSpanNaming.java#L12 you are using jakarta servlet api instead of the javax one? |
|
|
||
| import jakarta.servlet.ServletConfig | ||
|
|
||
| class TestWsServlet extends CXFNonSpringServlet { |
There was a problem hiding this comment.
we have been attempting to convert all of our groovy tests to java (see #7195), is there a reason these tests need to be in groovy, or could you write them in java instead?
There was a problem hiding this comment.
Since these tests are copy pasted from older cxf tests it is in my opinion fine to keep them in groovy.
Yes, that's correct. I can refactor the tests to Java, or even try to extract a common CXF library to improve reuse, but the original idea was just duplicate the existing instrumentation and move packages from |
Have a look at how this is implemented for metro. There is a separate testing module for jakarta version https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/jaxws/jaxws-3.0-metro-2.2-testing The implementation for the only class (as far as I remember) that depends on servlet api https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jaxws/jaxws-metro-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/metro/MetroServerSpanNameUpdater.java uses reflection. Instead of reflection you could also add a compile dependency on both versions of servlet api. Use servlet api only in methods annotated with |
|
I made an alternative PR for this #12077 |
|
Closing this PR since #12077 was merged. |
Adds CXF 4.0 instrumentation