Remove special handling of 404/301 from OkHttp instrumentation

See micrometer-metricsgh-5812
shakuzen · Jan 20, 2025 · 809f26f · 809f26f
1 parent 14d0cd8
commit 809f26f
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 11 deletions.
diff --git a/...java/io/micrometer/core/instrument/binder/okhttp3/DefaultOkHttpObservationConvention.java b/...java/io/micrometer/core/instrument/binder/okhttp3/DefaultOkHttpObservationConvention.java
@@ -88,7 +88,7 @@ public KeyValues getLowCardinalityKeyValues(OkHttpContext context) {
         // TODO: Tags to key values and back - maybe we can improve this?
         KeyValues keyValues = KeyValues
             .of(METHOD.withValue(requestAvailable ? request.method() : TAG_VALUE_UNKNOWN),
-                    URI.withValue(getUriTag(urlMapper, state, request)),
+                    URI.withValue(getUriTag(urlMapper, request)),
                     STATUS.withValue(getStatusMessage(state.response, state.exception)),
                     OUTCOME.withValue(getStatusOutcome(state.response).name()))
             .and(extraTags)
@@ -105,13 +105,11 @@ public KeyValues getLowCardinalityKeyValues(OkHttpContext context) {
         return keyValues;
     }
 
-    private String getUriTag(Function<Request, String> urlMapper, OkHttpObservationInterceptor.CallState state,
-            @Nullable Request request) {
+    private String getUriTag(Function<Request, String> urlMapper, @Nullable Request request) {
         if (request == null) {
             return TAG_VALUE_UNKNOWN;
         }
-        return state.response != null && (state.response.code() == 404 || state.response.code() == 301) ? "NOT_FOUND"
-                : urlMapper.apply(request);
+        return urlMapper.apply(request);
     }
 
     private Outcome getStatusOutcome(@Nullable Response response) {

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
@@ -167,8 +167,8 @@ void time(CallState state) {
         boolean requestAvailable = request != null;
 
         Iterable<Tag> tags = Tags
-            .of("method", requestAvailable ? request.method() : TAG_VALUE_UNKNOWN, "uri", getUriTag(state, request),
-                    "status", getStatusMessage(state.response, state.exception))
+            .of("method", requestAvailable ? request.method() : TAG_VALUE_UNKNOWN, "uri", getUriTag(request), "status",
+                    getStatusMessage(state.response, state.exception))
             .and(getStatusOutcome(state.response).asTag())
             .and(extraTags)
             .and(stream(contextSpecificTags.spliterator(), false)
@@ -196,12 +196,11 @@ private Tags generateTagsForRoute(@Nullable Request request) {
                 TAG_TARGET_PORT, Integer.toString(request.url().port()));
     }
 
-    private String getUriTag(CallState state, @Nullable Request request) {
+    private String getUriTag(@Nullable Request request) {
         if (request == null) {
             return TAG_VALUE_UNKNOWN;
         }
-        return state.response != null && (state.response.code() == 404 || state.response.code() == 301) ? "NOT_FOUND"
-                : urlMapper.apply(request);
+        return urlMapper.apply(request);
     }
 
     private Iterable<Tag> getRequestTags(@Nullable Request request) {

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