Added Jakarta and Javax instrumentations

[marcingrzejszczak]

to have that working we've added additional convention and contexts around javax and jakarta classes. Added tests for RestEasy that use the instrumentations. Also added ObservedValve that is an instrumentation for Tomcat (we currently support Tomcat up till version 8 - no jakarta imports).

cc @bclozel

[bclozel]

Thanks Marcin for working on this!

I'm wondering about the instrumentation for the JAX RS client bits. This is using the ClientRequestFilter and ClientResponseFilter contracts for this. It seems that if the connection somehow fails, or if a filter fails, a javax.ws.rs.client.ResponseProcessingException is thrown directly and this exception does not go through the response filter. This means that observations would not be stopped in those cases and that the instrumentation would miss a bunch of cases: filter errors and I/O errors in general. This reminds me of the work we recently did on the HttpComponents client where we switched to a different approach. Is there a better way to instrument clients here. Maybe through the *Invoker contracts by wrapping? We would need to see some concrete usage with a client to see if that's feasible.

I'm also wondering about the same problems on the server side with the ContainerRequestFilter and ContainerResponseFilter contracts. There, it seems that any exception (thrown by a JAX-RS endpoint or a filter) will be handled by an ExceptionMapper. In this case we might not need a different instrumentation approach but we might need a consistent ordering of that filter in the chain. Should it be ordered so that all other filters had a chance to process the response so that we don't record the wrong information? How should that be enforced?

Now about the Tomcat Valve. I'm not super familiar with this contract, but I remember that the asyncSupported flag is off by default and that this can play a important part since requests might not be instrumented for the async I/O case. I think that for regular async support, you might not need to do something special for async dispatches (like in the Servlet filter case), but I'm not entirely sure and I think this should be checked.

In general, in the Spring Framework instrumentation I tried to provide a lot of flexibility with custom names for observations and custom conventions (by providing constructor variants), but I'm wondering if this doesn't clutter the API in the end. Maybe this would be better handled with other contracts like the observation filter? I think this really depends on the use case. Maybe here JAX RS implementations would like to have a choice to come up with their own custom name?

[marcingrzejszczak]

I'm wondering about the instrumentation for the JAX RS client bits. This is using the ClientRequestFilter and ClientResponseFilter contracts for this. It seems that if the connection somehow fails, or if a filter fails, a javax.ws.rs.client.ResponseProcessingException is thrown directly and this exception does not go through the response filter. This means that observations would not be stopped in those cases and that the instrumentation would miss a bunch of cases: filter errors and I/O errors in general. This reminds me of the work we recently did on the HttpComponents client where we switched to a different approach. Is there a better way to instrument clients here. Maybe through the *Invoker contracts by wrapping? We would need to see some concrete usage with a client to see if that's feasible.
I'm also wondering about the same problems on the server side with the ContainerRequestFilter and ContainerResponseFilter contracts. There, it seems that any exception (thrown by a JAX-RS endpoint or a filter) will be handled by an ExceptionMapper. In this case we might not need a different instrumentation approach but we might need a consistent ordering of that filter in the chain. Should it be ordered so that all other filters had a chance to process the response so that we don't record the wrong information? How should that be enforced?

Yup I fully agree. I don't know if these are the right contracts to be used for proper wrapping. I think I'll need some help from people working with Jakarta API directly. I asked about it in the RestEASY tracker https://issues.redhat.com/browse/RESTEASY-3356?focusedId=22781362&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-22781362 and still am waiting for some feedback.

Now about the Tomcat Valve. I'm not super familiar with this contract, but I remember that the asyncSupported flag is off by default and that this can play a important part since requests might not be instrumented for the async I/O case. I think that for regular async support, you might not need to do something special for async dispatches (like in the Servlet filter case), but I'm not entirely sure and I think this should be checked.

We had it turned on for Sleuth for ages and it worked well 🤷

In general, in the Spring Framework instrumentation I tried to provide a lot of flexibility with custom names for observations and custom conventions (by providing constructor variants), but I'm wondering if this doesn't clutter the API in the end. Maybe this would be better handled with other contracts like the observation filter? I think this really depends on the use case. Maybe here JAX RS implementations would like to have a choice to come up with their own custom name?

I see. I guess it would be good for someone using JAX RS to chime in and give us feedback. Do you know anyone we can mention here? :)

[bclozel]

Now about the Tomcat Valve. I'm not super familiar with this contract, but I remember that the asyncSupported flag is off by default and that this can play a important part since requests might not be instrumented for the async I/O case. I think that for regular async support, you might not need to do something special for async dispatches (like in the Servlet filter case), but I'm not entirely sure and I think this should be checked.

We had it turned on for Sleuth for ages and it worked well 🤷

My bad, I missed that it was calling setAsyncSupported(true) already in the constructor.

In general, in the Spring Framework instrumentation I tried to provide a lot of flexibility with custom names for observations and custom conventions (by providing constructor variants), but I'm wondering if this doesn't clutter the API in the end. Maybe this would be better handled with other contracts like the observation filter? I think this really depends on the use case. Maybe here JAX RS implementations would like to have a choice to come up with their own custom name?

I see. I guess it would be good for someone using JAX RS to chime in and give us feedback. Do you know anyone we can mention here? :)

Not really unfortunately. I think Helidon is integrating with tracing libraries directly. Maybe something there that we can take a look at?

[marcingrzejszczak]

So I think we're using the proper APIs here. We just need to add support for error cases https://github.com/helidon-io/helidon/blob/helidon-3.x/tracing/jersey/src/main/java/io/helidon/tracing/jersey/AbstractTracingFilter.java#L125C9-L139

[marcingrzejszczak]

However for the client side there's also the interceptor https://github.com/helidon-io/helidon/blob/helidon-3.x/tracing/jersey-client/src/main/java/io/helidon/tracing/jersey/client/ClientTracingInterceptor.java . I'll check it out

[jonatan-ivanov]

Shouldn't we add the user-agent too just like we did with the client?

[shakuzen]

method might be null here, which would result in http null as the contextual name. Is that what we want?

[shakuzen]

Similar to the client instrumentation, I think this should be named more specific to JAX-RS, since it requires JAX-RS classes. Perhaps JaxRsServerObservationContext, and other classes that use this should be similarly renamed. The JavaDoc should also be updated to not mention Servlet but JAX-RS instead. We should also guide users on instrumenting at the right level. If they are using a framework, they should use the instrumentation there. If they are not using a framework or it doesn't have Micrometer instrumentation, but they are using JAX-RS, they should instrument at the JAX-RS level. If they are not using JAX-RS but are using Servlet, they should instrument at the Servlet level.

[shakuzen]

I've updated the naming of all related classes. The JavaDocs still need an update.

[shakuzen]

I'm not familiar with how libraries provide these AutoDiscoverable implementations, but the JavaDoc mentions declaring the implementation in a file in META-INF/services named org.glassfish.jersey.internal.spi.AutoDiscoverable. Otherwise I guess this won't be auto-discovered, but as I said, I'm not familiar with this.

Looking at the documentation, it says this is internal, so I wonder if we should be providing this implementation at all.

Auto discovery functionality is in Jersey supported by implementing an internal AutoDiscoverable Jersey SPI. This interface is not public at the moment, and is subject to change in the future, so be careful when trying to use it.

[shakuzen]

This should be named more specifically to Jersey, since it implements a Jersey SPI.

[shakuzen]

I suppose this would be a problem in an environment that is using a JAX-RS implementation not including Jersey. I don't know how much of a problem that is, but as far as I understand, there will be issues with the filter linked in the JavaDoc if there are exceptions and an interceptor like this isn't configured.

[shakuzen]

Ultimately, this is a blocker for providing JAX-RS client instrumentation. This was ran into by the OTel Java instrumentation as well in open-telemetry/opentelemetry-java-instrumentation#5430. There's more information about the limitation in JAX-RS itself in jakartaee/rest#684. So I don't know that we can provide JAX-RS client instrumentation without it being buggy. For now, we should not merge this code until/unless we can find an alternative solution.

[shakuzen]

@marcingrzejszczak The existing tests do not demonstrate the issue mentioned in the linked issues. Running the tests against a WireMock endpoint stubbed as follows demonstrates the issue:

wmRuntimeInfo.getWireMock().register(WireMock.get("/connectionReset").willReturn(WireMock.aResponse().withFault(Fault.CONNECTION_RESET_BY_PEER)));

The observation will be started but never stopped in this case. The Jersey specific ObservationJerseyClientInterceptor doesn't have any effect in the ObservationJaxRsHttpClientFilterTests because RESTEasy is used. This demonstrates why I don't think we can offer JAX-RS client instrumentation: it leaks observations when there is no response from the server. Rather we can add Jersey client instrumentation to jersey-micrometer, where with the Jersey interceptor we can ensure no leaks. It's unfortunate we can't instrument at the JAX-RS level for the client side, but we don't want to put out instrumentation with a known leak.

Full test code and failure

Test method added to ObservationJaxRsHttpClientFilterTests

@Test
void clientFilterShouldWorkWithJakartaHttpClientForExceptionsWithoutResponse(WireMockRuntimeInfo wmRuntimeInfo) {
    wmRuntimeInfo.getWireMock().register(WireMock.get("/connectionReset").willReturn(WireMock.aResponse().withFault(Fault.CONNECTION_RESET_BY_PEER)));

    try (Client client = ClientBuilder.newClient()) {
        client.register(new ObservationJaxRsHttpClientFilter(observationRegistry, null));
        final WebTarget target = client
            .target("http://localhost:" + wmRuntimeInfo.getHttpPort() + "/connectionReset");
        Exception exception = null;
        try (Response response = target.request().get()) {
            BDDAssertions.fail("Response should not be returned");
        } catch (Exception e) {
            exception = e;
        }
        BDDAssertions.then(exception).isNotNull().isInstanceOf(ProcessingException.class);
    }

    wmRuntimeInfo.getWireMock()
        .verifyThat(WireMock.getRequestedFor(WireMock.urlEqualTo("/connectionReset"))
            .withHeader("foo", WireMock.equalTo("bar")));

    TestObservationRegistryAssert.then(observationRegistry)
        .hasSingleObservationThat()
        .hasBeenStarted()
        .hasBeenStopped()
        .hasError();
}

Test outcome

java.lang.AssertionError: Observation is not stopped

	at io.micrometer.jakarta9.instrument.binder.http.jaxrs.client.ObservationJaxRsHttpClientFilterTests.clientFilterShouldWorkWithJakartaHttpClientForExceptionsWithoutResponses(ObservationJaxRsHttpClientFilterTests.java:112)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[shakuzen]

I'm confused why this is needed now but it wasn't prior to this pull request. I get a compilation error in JerseyTags when I remove this, but that class in micrometer-core is unchanged.

[shakuzen]

There are no assertions on the observation produced by the instrumentation. I was looking to add an implementation of our HttpServerTimingInstrumentationVerificationTests for this instrumentation, but it looks like the uri tag always has the value UNKNOWN because setPathPattern is never called on JaxRsContainerObservationContext

[mkjensen]

Hi,

What is the status of this?

It looks pretty interesting to us. We have a product using Spring Boot 3.2 consisting of both legacy code using RESTEasy (via RESTEasy Spring Boot Starter) and newer code using Spring Web MVC.

It would be nice if we could easily get the same metrics for both, specifically the http.server.requests metric.

Thanks.