smallrye / smallrye-opentelemetry

SmallRye OpenTelemetry - A CDI and Jakarta REST implementation of OpenTelemetry Tracing
Apache License 2.0
19 stars 13 forks source link

OpenTelemetryClientFilter causes memory leak #344

Open Olard opened 1 month ago

Olard commented 1 month ago

The OpenTelemetryClientFilter causes a memory leak (and broken spans) by not handling failed requests (UnknownHostException, SocketTimeException, ...) as the filter method of the ClientResponseFilter interface is not called when there is no response.

The filter automatically registers itself with the JAX-RS client implementation via the jakarta.ws.rs.ext.Provider annotation and there is no way via the API to unregister a filter. Which means there is no way to prevent the leak unless you do something vendor specific.

It could be that the same thing happens for the OpenTelemetryServerFilter, but there it is mitigated by the fact that the exception mapper is called first which means you have a response again.

radcortez commented 1 month ago

I'll have a look.

Olard commented 1 month ago

Related issues I've found: https://github.com/jakartaee/rest/issues/684 https://github.com/opentracing-contrib/java-jaxrs/issues/30

radcortez commented 1 month ago

Yes, I remember having this issue before but never invested the time to fix it.

Unfortunately, we don't have a good way to fix it (if any). From what I investigated, I couldn't find a way to catch the Exception and close the trace in RESTEasy directly. Using a CDI interceptor could work, but the issue will still apply to clients created programmatically.

@jamezp how about if we add something to RESTEasy that allows you to override the client builder? Consumers could then put some code around it, like it is done with the CDI Interceptor, but without requiring CDI. I'm more than happy to contribute that piece.

@Olard I don't have a good workaround, except for catching the exceptions and closing the OpenTelemetry resources manually.

jamezp commented 1 month ago

@radcortez What did you have in mind? I'm not too sure ow we could allow overriding the ClientBuilder. I've got some ideas, but they would currently be RESTEasy specific. TBH I just created a way to do this in a different project I'm working on with something like:

public interface RestClientBuilderProvider {

    /**
     * A provider which can supply a {@link ClientBuilder} for create the
     * {@linkplain jakarta.ws.rs.client.Client REST client}.
     *
     * @return the {@link ClientBuilder} to use for creating the REST client, cannot be {@code null}
     */
    default ClientBuilder getClientBuilder() {
        final ServiceLoader<RestClientBuilderProvider> loader = ServiceLoader.load(RestClientBuilderProvider.class);
        if (loader.iterator().hasNext()) {
            return loader.iterator().next().getClientBuilder();
        }
        return ClientBuilder.newBuilder();
    }
}
radcortez commented 1 month ago

Yes, something similar, RESTEasy specific.

That provider could be plugged into the RestClientBuilderResolver implementation so that if you provide your own RestClientBuilderProvider, you could override the builder or just fallback to the default builder.