Handle IO_ERROR for Apache HTTP client with observation API (#3418)

See #3312 (review)
micrometer-metrics · Sep 20, 2022 · 2e1fb6c · 2e1fb6c
1 parent d054e63
commit 2e1fb6c
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 5 deletions.
diff --git a/...r/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java b/...r/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java
@@ -17,9 +17,12 @@
 
 import io.micrometer.common.KeyValues;
 import io.micrometer.common.lang.Nullable;
+import org.apache.http.HttpException;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
 
+import java.io.IOException;
+
 /**
  * Default implementation of {@link ApacheHttpClientObservationConvention}.
  *
@@ -56,14 +59,18 @@ public KeyValues getLowCardinalityKeyValues(ApacheHttpClientContext context) {
                 ApacheHttpClientDocumentedObservation.ApacheHttpClientKeyNames.URI
                         .withValue(context.getUriMapper().apply(context.getCarrier())),
                 ApacheHttpClientDocumentedObservation.ApacheHttpClientKeyNames.STATUS
-                        .withValue(getStatusValue(context.getResponse())));
+                        .withValue(getStatusValue(context.getResponse(), context.getError().orElse(null))));
         if (context.shouldExportTagsForRoute()) {
             keyValues = keyValues.and(HttpContextUtils.generateTagStringsForRoute(context.getApacheHttpContext()));
         }
         return keyValues;
     }
 
-    String getStatusValue(@Nullable HttpResponse response) {
+    String getStatusValue(@Nullable HttpResponse response, Throwable error) {
+        if (error instanceof IOException || error instanceof HttpException || error instanceof RuntimeException) {
+            return "IO_ERROR";
+        }
+
         return response != null ? Integer.toString(response.getStatusLine().getStatusCode()) : "CLIENT_ERROR";
     }
 

diff --git a/...va/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java b/...va/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
@@ -117,11 +117,12 @@ public HttpResponse execute(HttpRequest request, HttpClientConnection conn, Http
         try {
             HttpResponse response = super.execute(request, conn, context);
             sample.setResponse(response);
-            statusCodeOrError = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusValue(response);
+            statusCodeOrError = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusValue(response, null);
             return response;
         }
         catch (IOException | HttpException | RuntimeException e) {
             statusCodeOrError = "IO_ERROR";
+            sample.setThrowable(e);
             throw e;
         }
         finally {

diff --git a/...o/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutorTest.java b/...o/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutorTest.java
@@ -26,6 +26,7 @@
 import io.micrometer.observation.GlobalObservationConvention;
 import io.micrometer.observation.tck.TestObservationRegistry;
 import io.micrometer.observation.tck.TestObservationRegistryAssert;
+import org.apache.http.client.ClientProtocolException;
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.methods.HttpEntityEnclosingRequestBase;
 import org.apache.http.client.methods.HttpGet;
@@ -45,7 +46,7 @@
 import java.util.stream.Collectors;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.*;
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.*;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
@@ -298,7 +299,18 @@ public String getMethod() {
 
     }
 
-    // TODO add test for status = IO_ERROR case.
+    @ParameterizedTest
+    @ValueSource(booleans = { false, true })
+    void httpStatusCodeIsTaggedWithIoError(boolean configureObservationRegistry,
+            @WiremockResolver.Wiremock WireMockServer server) {
+        server.stubFor(any(urlEqualTo("/error")).willReturn(aResponse().withStatus(1)));
+        HttpClient client = client(executor(false, configureObservationRegistry));
+        assertThatThrownBy(
+                () -> EntityUtils.consume(client.execute(new HttpGet(server.baseUrl() + "/error")).getEntity()))
+                        .isInstanceOf(ClientProtocolException.class);
+        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "IO_ERROR").timer().count())
+                .isEqualTo(1L);
+    }
 
     static class CustomGlobalApacheHttpConvention extends DefaultApacheHttpClientObservationConvention
             implements GlobalObservationConvention<ApacheHttpClientContext> {