From 71d31e381e8f4d868a689010e9755800bcfff53f Mon Sep 17 00:00:00 2001 From: Johnny Lim Date: Thu, 30 Jan 2025 01:36:05 +0900 Subject: [PATCH] Remove special handling of 404/301 from JDK HTTP client instrumentation See gh-5812 Signed-off-by: Johnny Lim --- ...efaultHttpClientObservationConvention.java | 3 +- .../binder/jdk/MicrometerHttpClientTests.java | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/micrometer-java11/src/main/java/io/micrometer/java11/instrument/binder/jdk/DefaultHttpClientObservationConvention.java b/micrometer-java11/src/main/java/io/micrometer/java11/instrument/binder/jdk/DefaultHttpClientObservationConvention.java index 75259f8618..7ad657c90f 100644 --- a/micrometer-java11/src/main/java/io/micrometer/java11/instrument/binder/jdk/DefaultHttpClientObservationConvention.java +++ b/micrometer-java11/src/main/java/io/micrometer/java11/instrument/binder/jdk/DefaultHttpClientObservationConvention.java @@ -55,8 +55,7 @@ public KeyValues getLowCardinalityKeyValues(HttpClientContext context) { String getUri(HttpRequest request, @Nullable HttpResponse httpResponse, Function uriMapper) { - return httpResponse != null && (httpResponse.statusCode() == 404 || httpResponse.statusCode() == 301) - ? "NOT_FOUND" : uriMapper.apply(request); + return uriMapper.apply(request); } String getStatus(@Nullable HttpResponse response) { diff --git a/micrometer-java11/src/test/java/io/micrometer/java11/instrument/binder/jdk/MicrometerHttpClientTests.java b/micrometer-java11/src/test/java/io/micrometer/java11/instrument/binder/jdk/MicrometerHttpClientTests.java index 0c867b9990..91918bfd08 100644 --- a/micrometer-java11/src/test/java/io/micrometer/java11/instrument/binder/jdk/MicrometerHttpClientTests.java +++ b/micrometer-java11/src/test/java/io/micrometer/java11/instrument/binder/jdk/MicrometerHttpClientTests.java @@ -57,6 +57,7 @@ void setup() { stubFor(any(urlEqualTo("/metrics")).willReturn(ok().withBody("body"))); stubFor(any(urlEqualTo("/test-fault")) .willReturn(new ResponseDefinitionBuilder().withFault(Fault.CONNECTION_RESET_BY_PEER))); + stubFor(any(urlEqualTo("/resources/1")).willReturn(notFound())); } @Test @@ -98,6 +99,46 @@ void shouldInstrumentHttpClientWithTimer(WireMockRuntimeInfo wmInfo) throws IOEx thenMeterRegistryContainsHttpClientTags(); } + @Test + void shouldInstrumentHttpClientWhenNotFound(WireMockRuntimeInfo wmInfo) throws IOException, InterruptedException { + HttpRequest request = HttpRequest.newBuilder() + .GET() + .uri(URI.create(wmInfo.getHttpBaseUrl() + "/resources/1")) + .build(); + + HttpClient observedClient = MicrometerHttpClient.instrumentationBuilder(httpClient, meterRegistry).build(); + observedClient.send(request, HttpResponse.BodyHandlers.ofString()); + + then(meterRegistry.get("http.client.requests") + .tag("method", "GET") + .tag("status", "404") + .tag("outcome", "CLIENT_ERROR") + .tag("uri", "UNKNOWN") + .timer()).isNotNull(); + } + + @Test + void shouldInstrumentHttpClientWithUriPatternHeaderWhenNotFound(WireMockRuntimeInfo wmInfo) + throws IOException, InterruptedException { + String uriPattern = "/resources/{id}"; + + HttpRequest request = HttpRequest.newBuilder() + .header(MicrometerHttpClient.URI_PATTERN_HEADER, uriPattern) + .GET() + .uri(URI.create(wmInfo.getHttpBaseUrl() + "/resources/1")) + .build(); + + HttpClient observedClient = MicrometerHttpClient.instrumentationBuilder(httpClient, meterRegistry).build(); + observedClient.send(request, HttpResponse.BodyHandlers.ofString()); + + then(meterRegistry.get("http.client.requests") + .tag("method", "GET") + .tag("status", "404") + .tag("outcome", "CLIENT_ERROR") + .tag("uri", uriPattern) + .timer()).isNotNull(); + } + @Test @Issue("#5136") void shouldThrowErrorFromSendAsync(WireMockRuntimeInfo wmInfo) {