Align http client tags between httpclient and okhttp (#1702)

Apache http client metrics prefixed tags about the target being called with `target.`, while the OkHttp client used `host`. Not only is `host` likely to conflict with a configured common tag, but it differs from the other http client implementation we have metrics for, which can be confusing for users looking at and possibly comparing these metrics.

This adds the same `target.` prefixed tags for the OkHttp metrics, namely `host`, `port`, `scheme`. It keeps the previous `host` tag by default for backwards compatibility. However, the `host` tag can be opted out of by using the new `includeHostTag` method on the Builder.

Co-authored-by: Tommy Ludwig <[email protected]>

Author: kubamarchwicki

micrometer-core/src/main/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListener.java

@@ -22,7 +22,11 @@
 import io.micrometer.core.lang.NonNullApi;
 import io.micrometer.core.lang.NonNullFields;
 import io.micrometer.core.lang.Nullable;
-import okhttp3.*;
+import okhttp3.Call;
+import okhttp3.EventListener;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+import okhttp3.Response;
 
 import java.io.IOException;
 import java.lang.reflect.Method;
@@ -77,18 +81,26 @@ private static Method getMethod(String name, Class<?>... parameterTypes) {
     private final Function<Request, String> urlMapper;
     private final Iterable<Tag> extraTags;
     private final Iterable<BiFunction<Request, Response, Tag>> contextSpecificTags;
+    private final boolean includeHostTag;
 
     // VisibleForTesting
     final ConcurrentMap<Call, CallState> callState = new ConcurrentHashMap<>();
 
     protected OkHttpMetricsEventListener(MeterRegistry registry, String requestsMetricName, Function<Request, String> urlMapper,
                                          Iterable<Tag> extraTags,
                                          Iterable<BiFunction<Request, Response, Tag>> contextSpecificTags) {
+        this(registry, requestsMetricName, urlMapper, extraTags, contextSpecificTags, true);
+    }
+
+    OkHttpMetricsEventListener(MeterRegistry registry, String requestsMetricName, Function<Request, String> urlMapper,
+                               Iterable<Tag> extraTags,
+                               Iterable<BiFunction<Request, Response, Tag>> contextSpecificTags, boolean includeHostTag) {
         this.registry = registry;
         this.requestsMetricName = requestsMetricName;
         this.urlMapper = urlMapper;
         this.extraTags = extraTags;
         this.contextSpecificTags = contextSpecificTags;
+        this.includeHostTag = includeHostTag;
     }
 
     public static Builder builder(MeterRegistry registry, String name) {
@@ -130,19 +142,19 @@ private void time(CallState state) {
         String uri = state.response != null && (state.response.code() == 404 || state.response.code() == 301)
                 ? "NOT_FOUND" : urlMapper.apply(request);
 
-        Tags requestTags = requestAvailable ? getRequestTags(request) : Tags.empty();
+        Tags requestTags = requestAvailable ? getRequestTags(request).and(generateTagsForRoute(request)) : Tags.empty();
 
         Iterable<Tag> tags = Tags.of(
                         "method", requestAvailable ? request.method() : "UNKNOWN",
                         "uri", requestAvailable ? uri : "UNKNOWN",
-                        "status", getStatusMessage(state.response, state.exception),
-                        "host", requestAvailable ? request.url().host() : "UNKNOWN"
+                        "status", getStatusMessage(state.response, state.exception)
                 )
                 .and(extraTags)
                 .and(stream(contextSpecificTags.spliterator(), false)
                         .map(contextTag -> contextTag.apply(request, state.response))
                         .collect(toList()))
                 .and(requestTags);
+        tags = includeHostTag ? Tags.of(tags).and("host", requestAvailable ? request.url().host() : "UNKNOWN") : tags;
 
         Timer.builder(this.requestsMetricName)
                 .tags(tags)
@@ -151,6 +163,14 @@ private void time(CallState state) {
                 .record(registry.config().clock().monotonicTime() - state.startTime, TimeUnit.NANOSECONDS);
     }
 
+    private Tags generateTagsForRoute(Request request) {
+        return Tags.of(
+                "target.scheme", request.url().scheme(),
+                "target.host", request.url().host(),
+                "target.port", Integer.toString(request.url().port())
+        );
+    }
+
     private Tags getRequestTags(Request request) {
         if (REQUEST_TAG_CLASS_EXISTS) {
             Tags requestTag = request.tag(Tags.class);
@@ -198,6 +218,7 @@ public static class Builder {
         private Function<Request, String> uriMapper = (request) -> Optional.ofNullable(request.header(URI_PATTERN)).orElse("none");
         private Tags tags = Tags.empty();
         private Collection<BiFunction<Request, Response, Tag>> contextSpecificTags = new ArrayList<>();
+        private boolean includeHostTag = true;
 
         Builder(MeterRegistry registry, String name) {
             this.registry = registry;
@@ -238,8 +259,23 @@ public Builder uriMapper(Function<Request, String> uriMapper) {
             return this;
         }
 
+        /**
+         * Historically, OkHttp Metrics provided by {@link OkHttpMetricsEventListener} included a
+         * {@code host} tag for the target host being called. To align with other HTTP client metrics,
+         * this was changed to {@code target.host}, but to maintain backwards compatibility the {@code host}
+         * tag can also be included. By default, {@code includeHostTag} is {@literal true} so both tags are included.
+         *
+         * @param includeHostTag whether to include the {@code host} tag
+         * @return this builder
+         * @since 1.5.0
+         */
+        public Builder includeHostTag(boolean includeHostTag) {
+            this.includeHostTag = includeHostTag;
+            return this;
+        }
+
         public OkHttpMetricsEventListener build() {
-            return new OkHttpMetricsEventListener(registry, name, uriMapper, tags, contextSpecificTags);
+            return new OkHttpMetricsEventListener(registry, name, uriMapper, tags, contextSpecificTags, includeHostTag);
         }
     }
 }

micrometer-core/src/test/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpMetricsEventListenerTest.java

@@ -73,7 +73,7 @@ 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)
+                .tags("foo", "bar", "status", "200", "uri", URI_EXAMPLE_VALUE, "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
                 .timer().count()).isEqualTo(1L);
     }
 
@@ -87,7 +87,7 @@ 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")
+                .tags("foo", "bar", "status", "404", "uri", "NOT_FOUND", "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
                 .timer().count()).isEqualTo(1L);
     }
 
@@ -115,7 +115,7 @@ void timeFailureDueToTimeout(@WiremockResolver.Wiremock WireMockServer server) {
         }
 
         assertThat(registry.get("okhttp.requests")
-                .tags("foo", "bar", "uri", URI_EXAMPLE_VALUE, "status", "IO_ERROR")
+                .tags("foo", "bar", "uri", URI_EXAMPLE_VALUE, "status", "IO_ERROR", "target.host", "localhost")
                 .timer().count()).isEqualTo(1L);
     }
 
@@ -136,7 +136,7 @@ void uriTagWorksWithUriPatternHeader(@WiremockResolver.Wiremock WireMockServer s
         client.newCall(request).execute().close();
 
         assertThat(registry.get("okhttp.requests")
-                .tags("foo", "bar", "uri", "/", "status", "200")
+                .tags("foo", "bar", "uri", "/", "status", "200", "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
                 .timer().count()).isEqualTo(1L);
     }
 
@@ -157,7 +157,7 @@ void uriTagWorksWithUriMapper(@WiremockResolver.Wiremock WireMockServer server)
         client.newCall(request).execute().close();
 
         assertThat(registry.get("okhttp.requests")
-                .tags("foo", "bar", "uri", "/helloworld.txt", "status", "200")
+                .tags("foo", "bar", "uri", "/helloworld.txt", "status", "200", "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
                 .timer().count()).isEqualTo(1L);
     }
 
@@ -222,6 +222,25 @@ void requestTagsWithoutClass(@WiremockResolver.Wiremock WireMockServer server) t
         testRequestTags(server, request);
     }
 
+    @Test
+    void hostTagCanBeDisabled(@WiremockResolver.Wiremock WireMockServer server) throws IOException {
+        server.stubFor(any(anyUrl()));
+        OkHttpClient client = new OkHttpClient.Builder()
+                .eventListener(OkHttpMetricsEventListener.builder(registry, "okhttp.requests")
+                        .includeHostTag(false)
+                        .build())
+                .build();
+        Request request = new Request.Builder()
+                .url(server.baseUrl())
+                .build();
+
+        client.newCall(request).execute().close();
+
+        assertThat(registry.get("okhttp.requests")
+                .tags("status", "200", "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
+                .timer().getId().getTags()).doesNotContain(Tag.of("host", "localhost"));
+    }
+
     private void testRequestTags(@WiremockResolver.Wiremock WireMockServer server, Request request) throws IOException {
         server.stubFor(any(anyUrl()));
         OkHttpClient client = new OkHttpClient.Builder()
@@ -234,7 +253,7 @@ private void testRequestTags(@WiremockResolver.Wiremock WireMockServer server, R
         client.newCall(request).execute().close();
 
         assertThat(registry.get("okhttp.requests")
-                .tags("foo", "bar", "uri", "/helloworld.txt", "status", "200", "requestTag1", "tagValue1")
+                .tags("foo", "bar", "uri", "/helloworld.txt", "status", "200", "requestTag1", "tagValue1", "target.host", "localhost", "target.port", "" + server.port(), "target.scheme", "http")
                 .timer().count()).isEqualTo(1L);
     }