micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.45k stars 979 forks source link

Instrumented Java 11 HttpClient does not re-throw exceptions in sendAsync call #5136

Open alex-inv opened 3 months ago

alex-inv commented 3 months ago

Describe the bug After instrumenting our java.net.HttpClient with newly released MicrometerHttpClient decorator, we've noticed our tests started failing. It seems that if any exception (e.g. network error) occurs, it will be caught in MicrometerHttpClient and not propagated further to the next CompletionStage available to the user, contrary to what happens in the original HttpClient implementation.

Implementation of sendAsync decorator in MicrometerHttpClient does not re-throw exception in the CompletableFuture.handle() method, but returns only response. In case handle got an exception as its input, null result will be returned instead:

return client.sendAsync(request, bodyHandler, pushPromiseHandler).handle((response, throwable) -> {
            if (throwable != null) {
                instrumentation.setThrowable(throwable);
            }
            instrumentation.setResponse(response);
            stopObservationOrTimer(instrumentation, request, response);
            return response;
        });

Environment

To Reproduce The easiest way to reproduce the bug is to write a test simulating a faulty HTTP server, for example using WireMock library. I've attached a minimal demo application (built with JDK 17 and Gradle) with a failing test.

Here I show two tests, first passing and second failing, highlighting the reported issue:

@Test
public void testError() {
        WireMock.stubFor(WireMock.get(WireMock.urlEqualTo("/test-url"))
                .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER)));

        var client = HttpClient.newBuilder().build();

        var request = HttpRequest.newBuilder(URI.create(wireMockServer.baseUrl() + "/test-url"))
                .GET().build();

        var response = client.sendAsync(request, HttpResponse.BodyHandlers.ofString());

        assertThrows(CompletionException.class, response::join);
}

@Test
public void testErrorMicrometer() {
        WireMock.stubFor(WireMock.get(WireMock.urlEqualTo("/test-url"))
                .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER)));

        var client = MicrometerHttpClient.instrumentationBuilder(
                HttpClient.newBuilder().build(),
                new SimpleMeterRegistry()
        ).build();

        var request = HttpRequest.newBuilder(URI.create(wireMockServer.baseUrl() + "/test-url"))
                .GET().build();

        var response = client.sendAsync(request, HttpResponse.BodyHandlers.ofString());

        // This test fails - nothing is thrown
        assertThrows(CompletionException.class, response::join);
}

Expected behavior Underlying error should be re-thrown and CompletionStage returned to the user should be completed exceptionally too, as per original API behavior.

While it's not possible to re-throw a generic Throwable in CompletableFuture.handle method due to limitations of checked exceptions handling, it's possible to wrap the original Throwable in a CompletionException, which would not break the CompletableFuture flow of executions.

shakuzen commented 2 weeks ago

Thanks for all the analysis and code. Would you be willing to submit a pull request for this?