From 681cf0dae7c739125479168b5a7ce7e716eba2f5 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Wed, 5 Oct 2022 11:40:28 +0200 Subject: [PATCH] Provide full request URL for "http.url" keyvalue Prior to this commit, the Observation filter for Servlet applications would only use the request pathInfo as an "http.url" high cardinality keyvalue. This commit ensures that we're using the full request URL as a value there. This also polishes gh-29254. Fixes gh-29257 See gh-29254 --- .../DefaultClientHttpObservationConvention.java | 4 ++-- .../DefaultHttpRequestsObservationConvention.java | 7 +++---- .../DefaultHttpRequestsObservationConvention.java | 8 ++++---- .../DefaultHttpRequestsObservationConventionTests.java | 5 ----- .../client/DefaultClientObservationConvention.java | 8 ++++---- 5 files changed, 13 insertions(+), 19 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientHttpObservationConvention.java b/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientHttpObservationConvention.java index e039cb26fa..bce76cc602 100644 --- a/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientHttpObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/http/client/observation/DefaultClientHttpObservationConvention.java @@ -46,7 +46,7 @@ public class DefaultClientHttpObservationConvention implements ClientHttpObserva private static final KeyValue EXCEPTION_NONE = KeyValue.of(ClientHttpObservation.LowCardinalityKeyNames.EXCEPTION, "none"); - private static final KeyValue URI_EXPANDED_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none"); + private static final KeyValue HTTP_URL_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none"); private static final KeyValue CLIENT_NAME_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.CLIENT_NAME, "none"); @@ -141,7 +141,7 @@ public class DefaultClientHttpObservationConvention implements ClientHttpObserva if (context.getCarrier() != null) { return KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().getURI().toASCIIString()); } - return URI_EXPANDED_NONE; + return HTTP_URL_NONE; } protected KeyValue clientName(ClientHttpObservationContext context) { diff --git a/spring-web/src/main/java/org/springframework/web/observation/DefaultHttpRequestsObservationConvention.java b/spring-web/src/main/java/org/springframework/web/observation/DefaultHttpRequestsObservationConvention.java index 33b6030b9b..85df4e3947 100644 --- a/spring-web/src/main/java/org/springframework/web/observation/DefaultHttpRequestsObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/web/observation/DefaultHttpRequestsObservationConvention.java @@ -47,7 +47,7 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs private static final KeyValue EXCEPTION_NONE = KeyValue.of(HttpRequestsObservation.LowCardinalityKeyNames.EXCEPTION, "none"); - private static final KeyValue URI_EXPANDED_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); + private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); private final String name; @@ -138,10 +138,9 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs protected KeyValue uriExpanded(HttpRequestsObservationContext context) { if (context.getCarrier() != null) { - String uriExpanded = (context.getCarrier().getPathInfo() != null) ? context.getCarrier().getPathInfo() : "/"; - return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, uriExpanded); + return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().getRequestURI()); } - return URI_EXPANDED_UNKNOWN; + return HTTP_URL_UNKNOWN; } } diff --git a/spring-web/src/main/java/org/springframework/web/observation/reactive/DefaultHttpRequestsObservationConvention.java b/spring-web/src/main/java/org/springframework/web/observation/reactive/DefaultHttpRequestsObservationConvention.java index fdfd4cac29..b117637e3b 100644 --- a/spring-web/src/main/java/org/springframework/web/observation/reactive/DefaultHttpRequestsObservationConvention.java +++ b/spring-web/src/main/java/org/springframework/web/observation/reactive/DefaultHttpRequestsObservationConvention.java @@ -48,7 +48,7 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs private static final KeyValue EXCEPTION_NONE = KeyValue.of(HttpRequestsObservation.LowCardinalityKeyNames.EXCEPTION, "none"); - private static final KeyValue URI_EXPANDED_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); + private static final KeyValue HTTP_URL_UNKNOWN = KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, "UNKNOWN"); private final String name; @@ -85,7 +85,7 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs @Override public KeyValues getHighCardinalityKeyValues(HttpRequestsObservationContext context) { - return KeyValues.of(uriExpanded(context)); + return KeyValues.of(httpUrl(context)); } protected KeyValue method(HttpRequestsObservationContext context) { @@ -143,12 +143,12 @@ public class DefaultHttpRequestsObservationConvention implements HttpRequestsObs return HttpOutcome.UNKNOWN.asKeyValue(); } - protected KeyValue uriExpanded(HttpRequestsObservationContext context) { + protected KeyValue httpUrl(HttpRequestsObservationContext context) { if (context.getCarrier() != null) { String uriExpanded = context.getCarrier().getPath().toString(); return KeyValue.of(HttpRequestsObservation.HighCardinalityKeyNames.HTTP_URL, uriExpanded); } - return URI_EXPANDED_UNKNOWN; + return HTTP_URL_UNKNOWN; } } diff --git a/spring-web/src/test/java/org/springframework/web/observation/DefaultHttpRequestsObservationConventionTests.java b/spring-web/src/test/java/org/springframework/web/observation/DefaultHttpRequestsObservationConventionTests.java index af410f58d1..14ac6c3abe 100644 --- a/spring-web/src/test/java/org/springframework/web/observation/DefaultHttpRequestsObservationConventionTests.java +++ b/spring-web/src/test/java/org/springframework/web/observation/DefaultHttpRequestsObservationConventionTests.java @@ -60,7 +60,6 @@ class DefaultHttpRequestsObservationConventionTests { void addsKeyValuesForExchange() { this.request.setMethod("POST"); this.request.setRequestURI("/test/resource"); - this.request.setPathInfo("/test/resource"); assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5) .contains(KeyValue.of("method", "POST"), KeyValue.of("uri", "UNKNOWN"), KeyValue.of("status", "200"), @@ -72,7 +71,6 @@ class DefaultHttpRequestsObservationConventionTests { @Test void addsKeyValuesForExchangeWithPathPattern() { this.request.setRequestURI("/test/resource"); - this.request.setPathInfo("/test/resource"); this.context.setPathPattern("/test/{name}"); assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5) @@ -85,7 +83,6 @@ class DefaultHttpRequestsObservationConventionTests { @Test void addsKeyValuesForErrorExchange() { this.request.setRequestURI("/test/resource"); - this.request.setPathInfo("/test/resource"); this.context.setError(new IllegalArgumentException("custom error")); this.response.setStatus(500); @@ -99,7 +96,6 @@ class DefaultHttpRequestsObservationConventionTests { @Test void addsKeyValuesForRedirectExchange() { this.request.setRequestURI("/test/redirect"); - this.request.setPathInfo("/test/redirect"); this.response.setStatus(302); this.response.addHeader("Location", "https://example.org/other"); @@ -113,7 +109,6 @@ class DefaultHttpRequestsObservationConventionTests { @Test void addsKeyValuesForNotFoundExchange() { this.request.setRequestURI("/test/notFound"); - this.request.setPathInfo("/test/notFound"); this.response.setStatus(404); assertThat(this.convention.getLowCardinalityKeyValues(this.context)).hasSize(5) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientObservationConvention.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientObservationConvention.java index 794cbb7fbb..35591ff360 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientObservationConvention.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientObservationConvention.java @@ -47,7 +47,7 @@ public class DefaultClientObservationConvention implements ClientObservationConv private static final KeyValue EXCEPTION_NONE = KeyValue.of(ClientObservation.LowCardinalityKeyNames.EXCEPTION, "none"); - private static final KeyValue URI_EXPANDED_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none"); + private static final KeyValue HTTP_URL_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.HTTP_URL, "none"); private static final KeyValue CLIENT_NAME_NONE = KeyValue.of(ClientHttpObservation.HighCardinalityKeyNames.CLIENT_NAME, "none"); @@ -135,14 +135,14 @@ public class DefaultClientObservationConvention implements ClientObservationConv @Override public KeyValues getHighCardinalityKeyValues(ClientObservationContext context) { - return KeyValues.of(uriExpanded(context), clientName(context)); + return KeyValues.of(httpUrl(context), clientName(context)); } - protected KeyValue uriExpanded(ClientObservationContext context) { + protected KeyValue httpUrl(ClientObservationContext context) { if (context.getCarrier() != null) { return KeyValue.of(ClientObservation.HighCardinalityKeyNames.HTTP_URL, context.getCarrier().url().toASCIIString()); } - return URI_EXPANDED_NONE; + return HTTP_URL_NONE; } protected KeyValue clientName(ClientObservationContext context) {