From 5dfa61eb0bb868230ac89295d9ad12472e6422c3 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 30 Jan 2023 10:09:45 +0100 Subject: [PATCH] Restrict "uri" KeyValue for client observations Prior to this commit, the "uri" KeyValue for low cardinality metadata would contain the entire uri template given to the HTTP client when creating the request. This was a breaking change for existing metrics dashboards, as previous support was removing the protocol, host and port parts of the URI. Indeed, this information is available in the "client.name" and "http.uri" KayValue. This commit parses and removes the protocol+host+port information from the uri template for the "uri" KeyValue. Fixes gh-29885 --- .../src/docs/asciidoc/integration/observability.adoc | 4 ++-- .../ClientHttpObservationDocumentation.java | 1 + .../DefaultClientRequestObservationConvention.java | 10 +++++++++- ...faultClientRequestObservationConventionTests.java | 12 ++++++++++++ .../web/client/RestTemplateObservationTests.java | 4 ++-- .../client/ClientHttpObservationDocumentation.java | 3 ++- .../DefaultClientRequestObservationConvention.java | 10 +++++++++- ...faultClientRequestObservationConventionTests.java | 8 ++++++++ 8 files changed, 45 insertions(+), 7 deletions(-) diff --git a/framework-docs/src/docs/asciidoc/integration/observability.adoc b/framework-docs/src/docs/asciidoc/integration/observability.adoc index 20d0d94669..ff7faa2eaf 100644 --- a/framework-docs/src/docs/asciidoc/integration/observability.adoc +++ b/framework-docs/src/docs/asciidoc/integration/observability.adoc @@ -132,7 +132,7 @@ Instrumentation is using the `org.springframework.http.client.observation.Client |=== |Name | Description |`method` _(required)_|Name of HTTP request method or `"none"` if the request could not be created. -|`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. +|`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. Only the path part of the URI is considered. |`client.name` _(required)_|Client name derived from the request URI host. |`status` _(required)_|HTTP response raw status code, or `"IO_ERROR"` in case of `IOException`, or `"CLIENT_ERROR"` if no response was received. |`outcome` _(required)_|Outcome of the HTTP client exchange. @@ -158,7 +158,7 @@ Instrumentation is using the `org.springframework.web.reactive.function.client.C |=== |Name | Description |`method` _(required)_|Name of HTTP request method or `"none"` if the request could not be created. -|`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. +|`uri` _(required)_|URI template used for HTTP request, or `"none"` if none was provided. Only the path part of the URI is considered. |`client.name` _(required)_|Client name derived from the request URI host. |`status` _(required)_|HTTP response raw status code, or `"IO_ERROR"` in case of `IOException`, or `"CLIENT_ERROR"` if no response was received. |`outcome` _(required)_|Outcome of the HTTP client exchange. diff --git a/spring-web/src/main/java/org/springframework/http/client/observation/ClientHttpObservationDocumentation.java b/spring-web/src/main/java/org/springframework/http/client/observation/ClientHttpObservationDocumentation.java index 58bb2ed287..6efcf48e2c 100644 --- a/spring-web/src/main/java/org/springframework/http/client/observation/ClientHttpObservationDocumentation.java +++ b/spring-web/src/main/java/org/springframework/http/client/observation/ClientHttpObservationDocumentation.java @@ -70,6 +70,7 @@ public enum ClientHttpObservationDocumentation implements ObservationDocumentati /** * URI template used for HTTP request, or {@value KeyValue#NONE_VALUE} if none was provided. + * Only the path part of the URI is considered. */ URI { @Override diff --git a/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientRequestObservationConvention.java b/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientRequestObservationConvention.java index 442a7efdfe..019852dbcc 100644 --- a/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientRequestObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientRequestObservationConvention.java @@ -17,6 +17,7 @@ package org.springframework.http.client.observation; import java.io.IOException; +import java.util.regex.Pattern; import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; @@ -39,6 +40,8 @@ public class DefaultClientRequestObservationConvention implements ClientRequestO private static final String DEFAULT_NAME = "http.client.requests"; + private static final Pattern PATTERN_BEFORE_PATH = Pattern.compile("^https?://[^/]+/"); + private static final KeyValue URI_NONE = KeyValue.of(LowCardinalityKeyNames.URI, KeyValue.NONE_VALUE); private static final KeyValue METHOD_NONE = KeyValue.of(LowCardinalityKeyNames.METHOD, KeyValue.NONE_VALUE); @@ -92,11 +95,16 @@ public class DefaultClientRequestObservationConvention implements ClientRequestO protected KeyValue uri(ClientRequestObservationContext context) { if (context.getUriTemplate() != null) { - return KeyValue.of(LowCardinalityKeyNames.URI, context.getUriTemplate()); + return KeyValue.of(LowCardinalityKeyNames.URI, extractPath(context.getUriTemplate())); } return URI_NONE; } + private static String extractPath(String uriTemplate) { + String path = PATTERN_BEFORE_PATH.matcher(uriTemplate).replaceFirst(""); + return (path.startsWith("/") ? path : "/" + path); + } + protected KeyValue method(ClientRequestObservationContext context) { if (context.getCarrier() != null) { return KeyValue.of(LowCardinalityKeyNames.METHOD, context.getCarrier().getMethod().name()); diff --git a/spring-web/src/test/java/org/springframework/http/client/observation/DefaultClientRequestObservationConventionTests.java b/spring-web/src/test/java/org/springframework/http/client/observation/DefaultClientRequestObservationConventionTests.java index 14ee942883..6bed961f9a 100644 --- a/spring-web/src/test/java/org/springframework/http/client/observation/DefaultClientRequestObservationConventionTests.java +++ b/spring-web/src/test/java/org/springframework/http/client/observation/DefaultClientRequestObservationConventionTests.java @@ -73,6 +73,18 @@ class DefaultClientRequestObservationConventionTests { assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("http.url", "/resource/42")); } + @Test + void addsKeyValuesForRequestWithUriTemplateWithHost() { + ClientRequestObservationContext context = createContext( + new MockClientHttpRequest(HttpMethod.GET, "https://example.org/resource/{id}", 42), new MockClientHttpResponse()); + context.setUriTemplate("https://example.org/resource/{id}"); + assertThat(this.observationConvention.getLowCardinalityKeyValues(context)) + .contains(KeyValue.of("exception", "none"), KeyValue.of("method", "GET"), KeyValue.of("uri", "/resource/{id}"), + KeyValue.of("status", "200"), KeyValue.of("client.name", "example.org"), KeyValue.of("outcome", "SUCCESS")); + assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).contains(KeyValue.of("http.url", "https://example.org/resource/42")); + } + + @Test void addsKeyValuesForRequestWithoutUriTemplate() { ClientRequestObservationContext context = createContext( diff --git a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java index e87756f614..97eb7fc5fe 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java @@ -88,7 +88,7 @@ class RestTemplateObservationTests { template.execute("https://example.com/hotels/{hotel}/bookings/{booking}", GET, null, null, "42", "21"); - assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "https://example.com/hotels/{hotel}/bookings/{booking}"); + assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "/hotels/{hotel}/bookings/{booking}"); } @Test @@ -101,7 +101,7 @@ class RestTemplateObservationTests { template.execute("https://example.com/hotels/{hotel}/bookings/{booking}", GET, null, null, vars); - assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "https://example.com/hotels/{hotel}/bookings/{booking}"); + assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "/hotels/{hotel}/bookings/{booking}"); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientHttpObservationDocumentation.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientHttpObservationDocumentation.java index aa895b4abc..6850e2a15c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientHttpObservationDocumentation.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientHttpObservationDocumentation.java @@ -67,6 +67,7 @@ public enum ClientHttpObservationDocumentation implements ObservationDocumentati /** * URI template used for HTTP request, or {@value KeyValue#NONE_VALUE} if none was provided. + * Only the path part of the URI is considered. */ URI { @Override @@ -123,7 +124,7 @@ public enum ClientHttpObservationDocumentation implements ObservationDocumentati public enum HighCardinalityKeyNames implements KeyName { /** - * HTTP request URI. + * The full HTTP request URI. */ HTTP_URL { @Override diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConvention.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConvention.java index 760bd13591..bacaba388a 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConvention.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConvention.java @@ -17,6 +17,7 @@ package org.springframework.web.reactive.function.client; import java.io.IOException; +import java.util.regex.Pattern; import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; @@ -40,6 +41,8 @@ public class DefaultClientRequestObservationConvention implements ClientRequestO private static final String ROOT_PATH = "/"; + private static final Pattern PATTERN_BEFORE_PATH = Pattern.compile("^https?://[^/]+/"); + private static final KeyValue URI_NONE = KeyValue.of(LowCardinalityKeyNames.URI, KeyValue.NONE_VALUE); private static final KeyValue METHOD_NONE = KeyValue.of(LowCardinalityKeyNames.METHOD, KeyValue.NONE_VALUE); @@ -94,7 +97,7 @@ public class DefaultClientRequestObservationConvention implements ClientRequestO protected KeyValue uri(ClientRequestObservationContext context) { if (context.getUriTemplate() != null) { - return KeyValue.of(LowCardinalityKeyNames.URI, context.getUriTemplate()); + return KeyValue.of(LowCardinalityKeyNames.URI, extractPath(context.getUriTemplate())); } ClientRequest request = context.getRequest(); if (request != null && ROOT_PATH.equals(request.url().getPath())) { @@ -103,6 +106,11 @@ public class DefaultClientRequestObservationConvention implements ClientRequestO return URI_NONE; } + private static String extractPath(String uriTemplate) { + String path = PATTERN_BEFORE_PATH.matcher(uriTemplate).replaceFirst(""); + return (path.startsWith("/") ? path : "/" + path); + } + protected KeyValue method(ClientRequestObservationContext context) { if (context.getRequest() != null) { return KeyValue.of(LowCardinalityKeyNames.METHOD, context.getRequest().method().name()); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConventionTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConventionTests.java index 28d31c7ce6..74bbb755e2 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConventionTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConventionTests.java @@ -114,6 +114,14 @@ class DefaultClientRequestObservationConventionTests { assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("uri", "/")); } + @Test + void shouldOnlyConsiderPathForUriKeyValue() { + ClientRequestObservationContext context = createContext(ClientRequest.create(HttpMethod.GET, URI.create("https://example.org/resource/42"))); + context.setUriTemplate("https://example.org/resource/{id}"); + context.setRequest(context.getCarrier().build()); + assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("uri", "/resource/{id}")); + } + private ClientRequestObservationContext createContext(ClientRequest.Builder request) { ClientRequestObservationContext context = new ClientRequestObservationContext(); context.setCarrier(request);