Add outcome tag to HttpClient metrics (micrometer-metrics#3732)

Aligns the HTTP client instrumentations by consistently including an outcome tag. This can make querying these metrics easier as it groups a class of HTTP status codes into a single value that can be directly queried instead of using regex or ranges.
beatfreaker · Apr 4, 2023 · 63cfcbc · 63cfcbc
1 parent 9fbf990
commit 63cfcbc
Show file tree

Hide file tree

Showing 17 changed files with 131 additions and 33 deletions.
diff --git a/...meter/core/instrument/binder/httpcomponents/ApacheHttpClientObservationDocumentation.java b/...meter/core/instrument/binder/httpcomponents/ApacheHttpClientObservationDocumentation.java
@@ -52,6 +52,12 @@ public String asString() {
                 return "status";
             }
         },
+        OUTCOME {
+            @Override
+            public String asString() {
+                return "outcome";
+            }
+        },
         METHOD {
             @Override
             public String asString() {

diff --git a/...r/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java b/...r/core/instrument/binder/httpcomponents/DefaultApacheHttpClientObservationConvention.java
@@ -17,6 +17,7 @@
 
 import io.micrometer.common.KeyValues;
 import io.micrometer.common.lang.Nullable;
+import io.micrometer.core.instrument.binder.http.Outcome;
 import org.apache.http.HttpException;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpResponse;
@@ -63,13 +64,19 @@ public KeyValues getLowCardinalityKeyValues(ApacheHttpClientContext context) {
                 ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.URI
                     .withValue(context.getUriMapper().apply(context.getCarrier())),
                 ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.STATUS
-                    .withValue(getStatusValue(context.getResponse(), context.getError())));
+                    .withValue(getStatusValue(context.getResponse(), context.getError())),
+                ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.OUTCOME
+                    .withValue(getStatusOutcome(context.getResponse()).name()));
         if (context.shouldExportTagsForRoute()) {
             keyValues = keyValues.and(HttpContextUtils.generateTagStringsForRoute(context.getApacheHttpContext()));
         }
         return keyValues;
     }
 
+    Outcome getStatusOutcome(@Nullable HttpResponse response) {
+        return response != null ? Outcome.forStatus(response.getStatusLine().getStatusCode()) : Outcome.UNKNOWN;
+    }
+
     String getStatusValue(@Nullable HttpResponse response, Throwable error) {
         if (error instanceof IOException || error instanceof HttpException || error instanceof RuntimeException) {
             return "IO_ERROR";

diff --git a/...va/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java b/...va/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
@@ -21,6 +21,7 @@
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
 import io.micrometer.core.instrument.Timer;
+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;
@@ -118,11 +119,13 @@ public HttpResponse execute(HttpRequest request, HttpClientConnection conn, Http
                     () -> new ApacheHttpClientContext(request, context, uriMapper, exportTagsForRoute), convention,
                     DefaultApacheHttpClientObservationConvention.INSTANCE);
         String statusCodeOrError = "UNKNOWN";
+        Outcome statusOutcome = Outcome.UNKNOWN;
 
         try {
             HttpResponse response = super.execute(request, conn, context);
             sample.setResponse(response);
             statusCodeOrError = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusValue(response, null);
+            statusOutcome = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusOutcome(response);
             return response;
         }
         catch (IOException | HttpException | RuntimeException e) {
@@ -132,10 +135,11 @@ public HttpResponse execute(HttpRequest request, HttpClientConnection conn, Http
         }
         finally {
             String status = statusCodeOrError;
+            String outcome = statusOutcome.name();
             sample.stop(METER_NAME, "Duration of Apache HttpClient request execution",
                     () -> Tags
                         .of("method", DefaultApacheHttpClientObservationConvention.INSTANCE.getMethodString(request),
-                                "uri", uriMapper.apply(request), "status", status)
+                                "uri", uriMapper.apply(request), "status", status, "outcome", outcome)
                         .and(exportTagsForRoute ? HttpContextUtils.generateTagsForRoute(context) : Tags.empty())
                         .and(extraTags));
         }

diff --git a/...r/core/instrument/binder/httpcomponents/hc5/ApacheHttpClientObservationDocumentation.java b/...r/core/instrument/binder/httpcomponents/hc5/ApacheHttpClientObservationDocumentation.java
@@ -48,6 +48,12 @@ public String asString() {
                 return "status";
             }
         },
+        OUTCOME {
+            @Override
+            public String asString() {
+                return "outcome";
+            }
+        },
         METHOD {
             @Override
             public String asString() {

diff --git a/...re/instrument/binder/httpcomponents/hc5/DefaultApacheHttpClientObservationConvention.java b/...re/instrument/binder/httpcomponents/hc5/DefaultApacheHttpClientObservationConvention.java
@@ -17,6 +17,7 @@
 
 import io.micrometer.common.KeyValues;
 import io.micrometer.common.lang.Nullable;
+import io.micrometer.core.instrument.binder.http.Outcome;
 import org.apache.hc.core5.http.HttpException;
 import org.apache.hc.core5.http.HttpRequest;
 import org.apache.hc.core5.http.HttpResponse;
@@ -59,13 +60,19 @@ public KeyValues getLowCardinalityKeyValues(ApacheHttpClientContext context) {
                 ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.URI
                     .withValue(context.getUriMapper().apply(context.getCarrier())),
                 ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.STATUS
-                    .withValue(getStatusValue(context.getResponse(), context.getError())));
+                    .withValue(getStatusValue(context.getResponse(), context.getError())),
+                ApacheHttpClientObservationDocumentation.ApacheHttpClientKeyNames.OUTCOME
+                    .withValue(getStatusOutcome(context.getResponse()).name()));
         if (context.shouldExportTagsForRoute()) {
             keyValues = keyValues.and(HttpContextUtils.generateTagStringsForRoute(context.getApacheHttpContext()));
         }
         return keyValues;
     }
 
+    Outcome getStatusOutcome(@Nullable HttpResponse response) {
+        return response != null ? Outcome.forStatus(response.getCode()) : Outcome.UNKNOWN;
+    }
+
     String getStatusValue(@Nullable HttpResponse response, Throwable error) {
         if (error instanceof IOException || error instanceof HttpException || error instanceof RuntimeException) {
             return "IO_ERROR";

diff --git a/...o/micrometer/core/instrument/binder/httpcomponents/hc5/MicrometerHttpRequestExecutor.java b/...o/micrometer/core/instrument/binder/httpcomponents/hc5/MicrometerHttpRequestExecutor.java
@@ -20,6 +20,7 @@
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
 import io.micrometer.core.instrument.Timer;
+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;
@@ -105,11 +106,13 @@ public ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientConnect
                     () -> new ApacheHttpClientContext(request, context, uriMapper, exportTagsForRoute), convention,
                     DefaultApacheHttpClientObservationConvention.INSTANCE);
         String statusCodeOrError = "UNKNOWN";
+        Outcome statusOutcome = Outcome.UNKNOWN;
 
         try {
             ClassicHttpResponse response = super.execute(request, conn, context);
             sample.setResponse(response);
             statusCodeOrError = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusValue(response, null);
+            statusOutcome = DefaultApacheHttpClientObservationConvention.INSTANCE.getStatusOutcome(response);
             return response;
         }
         catch (IOException | HttpException | RuntimeException e) {
@@ -119,10 +122,11 @@ public ClassicHttpResponse execute(ClassicHttpRequest request, HttpClientConnect
         }
         finally {
             String status = statusCodeOrError;
+            String outcome = statusOutcome.name();
             sample.stop(METER_NAME, "Duration of Apache HttpClient request execution",
                     () -> Tags
                         .of("method", DefaultApacheHttpClientObservationConvention.INSTANCE.getMethodString(request),
-                                "uri", uriMapper.apply(request), "status", status)
+                                "uri", uriMapper.apply(request), "status", status, "outcome", outcome)
                         .and(exportTagsForRoute ? HttpContextUtils.generateTagsForRoute(context) : Tags.empty())
                         .and(extraTags));
         }

diff --git a/...java/io/micrometer/core/instrument/binder/okhttp3/DefaultOkHttpObservationConvention.java b/...java/io/micrometer/core/instrument/binder/okhttp3/DefaultOkHttpObservationConvention.java
@@ -22,6 +22,7 @@
 import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
+import io.micrometer.core.instrument.binder.http.Outcome;
 import okhttp3.Request;
 import okhttp3.Response;
 
@@ -88,7 +89,8 @@ public KeyValues getLowCardinalityKeyValues(OkHttpContext context) {
         KeyValues keyValues = KeyValues
             .of(METHOD.withValue(requestAvailable ? request.method() : TAG_VALUE_UNKNOWN),
                     URI.withValue(getUriTag(urlMapper, state, request)),
-                    STATUS.withValue(getStatusMessage(state.response, state.exception)))
+                    STATUS.withValue(getStatusMessage(state.response, state.exception)),
+                    OUTCOME.withValue(getStatusOutcome(state.response).name()))
             .and(extraTags)
             .and(stream(contextSpecificTags.spliterator(), false)
                 .map(contextTag -> contextTag.apply(request, state.response))
@@ -112,6 +114,14 @@ private String getUriTag(Function<Request, String> urlMapper, OkHttpObservationI
                 : urlMapper.apply(request);
     }
 
+    private Outcome getStatusOutcome(@Nullable Response response) {
+        if (response == null) {
+            return Outcome.UNKNOWN;
+        }
+
+        return Outcome.forStatus(response.code());
+    }
+
     private String getStatusMessage(@Nullable Response response, @Nullable IOException exception) {
         if (exception != null) {
             return "IO_ERROR";

diff --git a/...rc/main/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListener.java b/...rc/main/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListener.java
@@ -22,6 +22,7 @@
 import io.micrometer.core.instrument.Tag;
 import io.micrometer.core.instrument.Tags;
 import io.micrometer.core.instrument.Timer;
+import io.micrometer.core.instrument.binder.http.Outcome;
 import okhttp3.EventListener;
 import okhttp3.*;
 
@@ -168,6 +169,7 @@ void time(CallState state) {
         Iterable<Tag> tags = Tags
             .of("method", requestAvailable ? request.method() : TAG_VALUE_UNKNOWN, "uri", getUriTag(state, request),
                     "status", getStatusMessage(state.response, state.exception))
+            .and(getStatusOutcome(state.response).asTag())
             .and(extraTags)
             .and(stream(contextSpecificTags.spliterator(), false)
                 .map(contextTag -> contextTag.apply(request, state.response))
@@ -219,6 +221,14 @@ private Iterable<Tag> getRequestTags(@Nullable Request request) {
         return Tags.empty();
     }
 
+    private Outcome getStatusOutcome(@Nullable Response response) {
+        if (response == null) {
+            return Outcome.UNKNOWN;
+        }
+
+        return Outcome.forStatus(response.code());
+    }
+
     private String getStatusMessage(@Nullable Response response, @Nullable IOException exception) {
         if (exception != null) {
             return "IO_ERROR";

diff --git a/...ain/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpObservationDocumentation.java b/...ain/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpObservationDocumentation.java
@@ -95,6 +95,13 @@ public String asString() {
             public String asString() {
                 return "status";
             }
+        },
+
+        OUTCOME {
+            @Override
+            public String asString() {
+                return "outcome";
+            }
         }
 
     }

diff --git a/...va11/io/micrometer/core/instrument/binder/jdk/DefaultHttpClientObservationConvention.java b/...va11/io/micrometer/core/instrument/binder/jdk/DefaultHttpClientObservationConvention.java
@@ -18,6 +18,7 @@
 import io.micrometer.common.KeyValues;
 import io.micrometer.common.lang.NonNull;
 import io.micrometer.common.lang.Nullable;
+import io.micrometer.core.instrument.binder.http.Outcome;
 
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
@@ -47,8 +48,11 @@ public KeyValues getLowCardinalityKeyValues(HttpClientContext context) {
                 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())));
+            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;
     }

diff --git a/...n/java11/io/micrometer/core/instrument/binder/jdk/HttpClientObservationDocumentation.java b/...n/java11/io/micrometer/core/instrument/binder/jdk/HttpClientObservationDocumentation.java
@@ -60,6 +60,13 @@ public String asString() {
             }
         },
 
+        OUTCOME {
+            @Override
+            public String asString() {
+                return "outcome";
+            }
+        },
+
         /**
          * HTTP URI.
          */

diff --git a/...r-core/src/main/java11/io/micrometer/core/instrument/binder/jdk/MicrometerHttpClient.java b/...r-core/src/main/java11/io/micrometer/core/instrument/binder/jdk/MicrometerHttpClient.java
@@ -19,6 +19,7 @@
 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;
@@ -238,8 +239,11 @@ private <T> void stopObservationOrTimer(
                             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())));
+                        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;
                 });

diff --git a/...o/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutorTest.java b/...o/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutorTest.java
@@ -96,12 +96,18 @@ void httpStatusCodeIsTagged(boolean configureObservationRegistry, @WiremockResol
         EntityUtils.consume(client.execute(new HttpGet(server.baseUrl() + "/ok")).getEntity());
         EntityUtils.consume(client.execute(new HttpGet(server.baseUrl() + "/notfound")).getEntity());
         EntityUtils.consume(client.execute(new HttpGet(server.baseUrl() + "/error")).getEntity());
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "200").timer().count())
-            .isEqualTo(2L);
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "404").timer().count())
-            .isEqualTo(1L);
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "500").timer().count())
-            .isEqualTo(1L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "200", "outcome", "SUCCESS")
+            .timer()
+            .count()).isEqualTo(2L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "404", "outcome", "CLIENT_ERROR")
+            .timer()
+            .count()).isEqualTo(1L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "500", "outcome", "SERVER_ERROR")
+            .timer()
+            .count()).isEqualTo(1L);
     }
 
     @ParameterizedTest
@@ -329,8 +335,10 @@ void httpStatusCodeIsTaggedWithIoError(boolean 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);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "IO_ERROR", "outcome", "UNKNOWN")
+            .timer()
+            .count()).isEqualTo(1L);
     }
 
     static class CustomGlobalApacheHttpConvention extends DefaultApacheHttpClientObservationConvention

diff --git a/...crometer/core/instrument/binder/httpcomponents/hc5/MicrometerHttpRequestExecutorTest.java b/...crometer/core/instrument/binder/httpcomponents/hc5/MicrometerHttpRequestExecutorTest.java
@@ -107,12 +107,18 @@ void httpStatusCodeIsTagged(boolean configureObservationRegistry, @WiremockResol
         execute(client, new HttpGet(server.baseUrl() + "/ok"));
         execute(client, new HttpGet(server.baseUrl() + "/notfound"));
         execute(client, new HttpGet(server.baseUrl() + "/error"));
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "200").timer().count())
-            .isEqualTo(2L);
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "404").timer().count())
-            .isEqualTo(1L);
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "500").timer().count())
-            .isEqualTo(1L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "200", "outcome", "SUCCESS")
+            .timer()
+            .count()).isEqualTo(2L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "404", "outcome", "CLIENT_ERROR")
+            .timer()
+            .count()).isEqualTo(1L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "500", "outcome", "SERVER_ERROR")
+            .timer()
+            .count()).isEqualTo(1L);
     }
 
     @ParameterizedTest
@@ -323,8 +329,10 @@ void httpStatusCodeIsTaggedWithIoError(boolean configureObservationRegistry,
         CloseableHttpClient client = client(executor(false, configureObservationRegistry));
         assertThatThrownBy(() -> execute(client, new HttpGet(server.baseUrl() + "/error")))
             .isInstanceOf(ClientProtocolException.class);
-        assertThat(registry.get(EXPECTED_METER_NAME).tags("method", "GET", "status", "IO_ERROR").timer().count())
-            .isEqualTo(1L);
+        assertThat(registry.get(EXPECTED_METER_NAME)
+            .tags("method", "GET", "status", "IO_ERROR", "outcome", "UNKNOWN")
+            .timer()
+            .count()).isEqualTo(1L);
     }
 
     static class CustomGlobalApacheHttpConvention extends DefaultApacheHttpClientObservationConvention

diff --git a/...est/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListenerTest.java b/...est/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListenerTest.java
@@ -72,8 +72,8 @@ void timeSuccessful(@WiremockResolver.Wiremock WireMockServer server) throws IOE
         client.newCall(request).execute().close();
 
         assertThat(registry.get("okhttp.requests")
-            .tags("foo", "bar", "status", "200", "uri", URI_EXAMPLE_VALUE, "target.host", "localhost", "target.port",
-                    String.valueOf(server.port()), "target.scheme", "http")
+            .tags("foo", "bar", "status", "200", "outcome", "SUCCESS", "uri", URI_EXAMPLE_VALUE, "target.host",
+                    "localhost", "target.port", String.valueOf(server.port()), "target.scheme", "http")
             .timer()
             .count()).isEqualTo(1L);
     }
@@ -86,8 +86,8 @@ void timeNotFound(@WiremockResolver.Wiremock WireMockServer server) throws IOExc
         client.newCall(request).execute().close();
 
         assertThat(registry.get("okhttp.requests")
-            .tags("foo", "bar", "status", "404", "uri", "NOT_FOUND", "target.host", "localhost", "target.port",
-                    String.valueOf(server.port()), "target.scheme", "http")
+            .tags("foo", "bar", "status", "404", "outcome", "CLIENT_ERROR", "uri", "NOT_FOUND", "target.host",
+                    "localhost", "target.port", String.valueOf(server.port()), "target.scheme", "http")
             .timer()
             .count()).isEqualTo(1L);
     }
@@ -114,7 +114,8 @@ void timeFailureDueToTimeout(@WiremockResolver.Wiremock WireMockServer server) {
         }
 
         assertThat(registry.get("okhttp.requests")
-            .tags("foo", "bar", "uri", URI_EXAMPLE_VALUE, "status", "IO_ERROR", "target.host", "localhost")
+            .tags("foo", "bar", "uri", URI_EXAMPLE_VALUE, "status", "IO_ERROR", "outcome", "UNKNOWN", "target.host",
+                    "localhost")
             .timer()
             .count()).isEqualTo(1L);
     }