Apply gh-5553 to io.micrometer.core.instrument.binder.jdk (#5704)

* Apply gh-5553 to io.micrometer.core.instrument.binder.jdk

* Fix missing error on context in MicrometerHttpClient.sendAsync()

This commit also applies gh-5705.

Author: izeye

micrometer-core/src/main/java11/io/micrometer/core/instrument/binder/jdk/DefaultHttpClientObservationConvention.java

@@ -43,29 +43,36 @@ public KeyValues getLowCardinalityKeyValues(HttpClientContext context) {
             return KeyValues.empty();
         }
         HttpRequest httpRequest = context.getCarrier().build();
-        KeyValues keyValues = KeyValues.of(
+        return KeyValues.of(
                 HttpClientObservationDocumentation.LowCardinalityKeys.METHOD.withValue(httpRequest.method()),
                 HttpClientObservationDocumentation.LowCardinalityKeys.URI
-                    .withValue(getUriTag(httpRequest, context.getResponse(), context.getUriMapper())));
-        if (context.getResponse() != null) {
-            keyValues = keyValues
-                .and(HttpClientObservationDocumentation.LowCardinalityKeys.STATUS
-                    .withValue(String.valueOf(context.getResponse().statusCode())))
-                .and(HttpClientObservationDocumentation.LowCardinalityKeys.OUTCOME
-                    .withValue(Outcome.forStatus(context.getResponse().statusCode()).name()));
-        }
-        return keyValues;
+                    .withValue(getUriTag(httpRequest, context.getResponse(), context.getUriMapper())),
+                HttpClientObservationDocumentation.LowCardinalityKeys.STATUS
+                    .withValue(getStatus(context.getResponse())),
+                HttpClientObservationDocumentation.LowCardinalityKeys.OUTCOME
+                    .withValue(getOutcome(context.getResponse())));
     }
 
-    String getUriTag(@Nullable HttpRequest request, @Nullable HttpResponse<?> httpResponse,
+    String getUriTag(HttpRequest request, @Nullable HttpResponse<?> httpResponse,
             Function<HttpRequest, String> uriMapper) {
-        if (request == null) {
-            return null;
-        }
         return httpResponse != null && (httpResponse.statusCode() == 404 || httpResponse.statusCode() == 301)
                 ? "NOT_FOUND" : uriMapper.apply(request);
     }
 
+    String getStatus(@Nullable HttpResponse<?> response) {
+        if (response == null) {
+            return "UNKNOWN";
+        }
+        return String.valueOf(response.statusCode());
+    }
+
+    String getOutcome(@Nullable HttpResponse<?> response) {
+        if (response == null) {
+            return Outcome.UNKNOWN.name();
+        }
+        return Outcome.forStatus(response.statusCode()).name();
+    }
+
     @Override
     @NonNull
     public String getName() {

micrometer-core/src/main/java11/io/micrometer/core/instrument/binder/jdk/MicrometerHttpClient.java

@@ -17,9 +17,7 @@
 
 import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.instrument.MeterRegistry;
-import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
-import io.micrometer.core.instrument.binder.http.Outcome;
 import io.micrometer.core.instrument.observation.ObservationOrTimerCompatibleInstrumentation;
 import io.micrometer.observation.Observation;
 import io.micrometer.observation.ObservationRegistry;
@@ -36,6 +34,7 @@
 import java.time.Duration;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
 import java.util.concurrent.Executor;
 import java.util.function.Function;
 
@@ -234,19 +233,13 @@ private <T> void stopObservationOrTimer(
             ObservationOrTimerCompatibleInstrumentation<HttpClientContext> instrumentation, HttpRequest request,
             @Nullable HttpResponse<T> res) {
         instrumentation.stop(DefaultHttpClientObservationConvention.INSTANCE.getName(), "Timer for JDK's HttpClient",
-                () -> {
-                    Tags tags = Tags.of(HttpClientObservationDocumentation.LowCardinalityKeys.METHOD.asString(),
-                            request.method(), HttpClientObservationDocumentation.LowCardinalityKeys.URI.asString(),
-                            DefaultHttpClientObservationConvention.INSTANCE.getUriTag(request, res, uriMapper));
-                    if (res != null) {
-                        tags = tags
-                            .and(Tag.of(HttpClientObservationDocumentation.LowCardinalityKeys.STATUS.asString(),
-                                    String.valueOf(res.statusCode())))
-                            .and(Tag.of(HttpClientObservationDocumentation.LowCardinalityKeys.OUTCOME.asString(),
-                                    Outcome.forStatus(res.statusCode()).name()));
-                    }
-                    return tags;
-                });
+                () -> Tags.of(HttpClientObservationDocumentation.LowCardinalityKeys.METHOD.asString(), request.method(),
+                        HttpClientObservationDocumentation.LowCardinalityKeys.URI.asString(),
+                        DefaultHttpClientObservationConvention.INSTANCE.getUriTag(request, res, uriMapper),
+                        HttpClientObservationDocumentation.LowCardinalityKeys.STATUS.asString(),
+                        DefaultHttpClientObservationConvention.INSTANCE.getStatus(res),
+                        HttpClientObservationDocumentation.LowCardinalityKeys.OUTCOME.asString(),
+                        DefaultHttpClientObservationConvention.INSTANCE.getOutcome(res)));
     }
 
     private ObservationOrTimerCompatibleInstrumentation<HttpClientContext> observationOrTimer(
@@ -272,12 +265,16 @@ public <T> CompletableFuture<HttpResponse<T>> sendAsync(HttpRequest httpRequest,
                 httpRequestBuilder);
         HttpRequest request = httpRequestBuilder.build();
         return client.sendAsync(request, bodyHandler, pushPromiseHandler).handle((response, throwable) -> {
+            instrumentation.setResponse(response);
             if (throwable != null) {
                 instrumentation.setThrowable(throwable);
+                stopObservationOrTimer(instrumentation, request, response);
+                throw new CompletionException(throwable);
+            }
+            else {
+                stopObservationOrTimer(instrumentation, request, response);
+                return response;
             }
-            instrumentation.setResponse(response);
-            stopObservationOrTimer(instrumentation, request, response);
-            return response;
         });
     }
 

micrometer-core/src/test/java11/io/micrometer/core/instrument/binder/jdk/MicrometerHttpClientTests.java

@@ -15,6 +15,8 @@
  */
 package io.micrometer.core.instrument.binder.jdk;
 
+import com.github.tomakehurst.wiremock.client.ResponseDefinitionBuilder;
+import com.github.tomakehurst.wiremock.http.Fault;
 import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
 import com.github.tomakehurst.wiremock.junit5.WireMockTest;
 import io.micrometer.core.instrument.MeterRegistry;
@@ -33,8 +35,10 @@
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.time.Duration;
+import java.util.concurrent.CompletionException;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
+import static org.assertj.core.api.Assertions.*;
 import static org.assertj.core.api.BDDAssertions.then;
 
 @WireMockTest
@@ -47,6 +51,8 @@ class MicrometerHttpClientTests {
     @BeforeEach
     void setup() {
         stubFor(any(urlEqualTo("/metrics")).willReturn(ok().withBody("body")));
+        stubFor(any(urlEqualTo("/test-fault"))
+            .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER)));
     }
 
     @Test
@@ -84,6 +90,43 @@ void shouldInstrumentHttpClientWithTimer(WireMockRuntimeInfo wmInfo) throws IOEx
         thenMeterRegistryContainsHttpClientTags();
     }
 
+    @Test
+    void shouldThrowErrorFromSendAsync(WireMockRuntimeInfo wmInfo) {
+        var client = MicrometerHttpClient.instrumentationBuilder(httpClient, meterRegistry).build();
+
+        String uri = "/test-fault";
+        var request = HttpRequest.newBuilder(URI.create(wmInfo.getHttpBaseUrl() + uri))
+            .header(MicrometerHttpClient.URI_PATTERN_HEADER, uri)
+            .GET()
+            .build();
+
+        var response = client.sendAsync(request, HttpResponse.BodyHandlers.ofString());
+
+        assertThatThrownBy(response::join).isInstanceOf(CompletionException.class);
+        assertThatNoException().isThrownBy(() -> meterRegistry.get("http.client.requests")
+            .tag("method", "GET")
+            .tag("uri", uri)
+            .tag("status", "UNKNOWN")
+            .tag("outcome", "UNKNOWN")
+            .timer());
+    }
+
+    @Test
+    void sendAsyncShouldSetErrorInContext(WireMockRuntimeInfo wmInfo) {
+        ObservationRegistry observationRegistry = TestObservationRegistry.create();
+        StoreContextObservationHandler storeContextObservationHandler = new StoreContextObservationHandler();
+        observationRegistry.observationConfig().observationHandler(storeContextObservationHandler);
+
+        var request = HttpRequest.newBuilder(URI.create(wmInfo.getHttpBaseUrl() + "/test-fault")).GET().build();
+
+        HttpClient observedClient = MicrometerHttpClient.instrumentationBuilder(httpClient, meterRegistry)
+            .observationRegistry(observationRegistry)
+            .build();
+        var response = observedClient.sendAsync(request, HttpResponse.BodyHandlers.ofString());
+        assertThatThrownBy(response::join).isInstanceOf(CompletionException.class);
+        assertThat(storeContextObservationHandler.context.getError()).isInstanceOf(CompletionException.class);
+    }
+
     private void thenMeterRegistryContainsHttpClientTags() {
         then(meterRegistry.find("http.client.requests")
             .tag("method", "GET")
@@ -108,4 +151,20 @@ public void onStart(HttpClientContext context) {
         };
     }
 
+    static class StoreContextObservationHandler implements ObservationHandler<HttpClientContext> {
+
+        HttpClientContext context;
+
+        @Override
+        public boolean supportsContext(Observation.Context context) {
+            return context instanceof HttpClientContext;
+        }
+
+        @Override
+        public void onStart(HttpClientContext context) {
+            this.context = context;
+        }
+
+    }
+
 }