Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenTelemetryClientFilter causes memory leak #344

Open
Olard opened this issue Jul 25, 2024 · 5 comments
Open

OpenTelemetryClientFilter causes memory leak #344

Olard opened this issue Jul 25, 2024 · 5 comments

Comments

@Olard
Copy link

Olard commented Jul 25, 2024

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
Copy link
Member

I'll have a look.

@Olard
Copy link
Author

Olard commented Jul 26, 2024

Related issues I've found:
jakartaee/rest#684
opentracing-contrib/java-jaxrs#30

@radcortez
Copy link
Member

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
Copy link

jamezp commented Jul 26, 2024

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants