From b87852612b642c296d3528cc8a39e12ac823235c Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 12 Dec 2023 14:41:41 +0100 Subject: [PATCH] Improve "active" metrics handling in WebClient observations Prior to this commit, the WebClient observations would have a specific lifecycle where the observation context is build with a `ClientRequest.Builder` as tracing needs to add an outgoing request header before the request is made immutable. With this setup, the metrics observation handler processes the start event by increasing the "http.client.requests.active" counter and collecting tags at this point. Because then the immutable request is not yet fully built or set on the context, the keyvalues collected by the observation convention at that point can be incomplete. This commit ensures that a request is always made available in the context, even if it is updated right after the observation start. The only difference between the two should be additional tracing headers and a request attribute holding the current observation context. Closes gh-31702 --- .../ClientRequestObservationContext.java | 15 ++++++++++++ .../function/client/DefaultWebClient.java | 5 ++-- ...ientRequestObservationConventionTests.java | 23 ++++++++----------- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientRequestObservationContext.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientRequestObservationContext.java index 4d0f2fdc39..46ce43fc3e 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientRequestObservationContext.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientRequestObservationContext.java @@ -50,10 +50,25 @@ public class ClientRequestObservationContext extends RequestReplySenderContext headers.set(name, value)); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java index 5786a21b55..ccb57b713c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java @@ -438,12 +438,11 @@ final class DefaultWebClient implements WebClient { @SuppressWarnings("deprecation") @Override public Mono exchange() { - ClientRequestObservationContext observationContext = new ClientRequestObservationContext(); ClientRequest.Builder requestBuilder = initRequestBuilder(); + ClientRequestObservationContext observationContext = new ClientRequestObservationContext(requestBuilder); return Mono.deferContextual(contextView -> { Observation observation = ClientHttpObservationDocumentation.HTTP_REACTIVE_CLIENT_EXCHANGES.observation(observationConvention, DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, observationRegistry); - observationContext.setCarrier(requestBuilder); observation .parentObservation(contextView.getOrDefault(ObservationThreadLocalAccessor.KEY, null)) .start(); @@ -452,7 +451,7 @@ final class DefaultWebClient implements WebClient { filterFunction = filterFunctions.andThen(filterFunction); } ClientRequest request = requestBuilder - .attribute(ClientRequestObservationContext.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observation.getContext()) + .attribute(ClientRequestObservationContext.CURRENT_OBSERVATION_CONTEXT_ATTRIBUTE, observationContext) .build(); observationContext.setUriTemplate((String) request.attribute(URI_TEMPLATE_ATTRIBUTE).orElse(null)); observationContext.setRequest(request); 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 74bbb755e2..d1becc26e5 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 @@ -43,19 +43,19 @@ class DefaultClientRequestObservationConventionTests { @Test void shouldHaveContextualName() { - ClientRequestObservationContext context = new ClientRequestObservationContext(); - context.setCarrier(ClientRequest.create(HttpMethod.GET, URI.create("/test"))); - context.setRequest(context.getCarrier().build()); + ClientRequestObservationContext context = new ClientRequestObservationContext(ClientRequest.create(HttpMethod.GET, URI.create("/test"))); assertThat(this.observationConvention.getContextualName(context)).isEqualTo("http get"); } @Test void shouldOnlySupportWebClientObservationContext() { - assertThat(this.observationConvention.supportsContext(new ClientRequestObservationContext())).isTrue(); + ClientRequest.Builder request = ClientRequest.create(HttpMethod.GET, URI.create("/test")); + assertThat(this.observationConvention.supportsContext(new ClientRequestObservationContext(request))).isTrue(); assertThat(this.observationConvention.supportsContext(new Observation.Context())).isFalse(); } @Test + @SuppressWarnings("removal") void shouldAddKeyValuesForNullExchange() { ClientRequestObservationContext context = new ClientRequestObservationContext(); assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).hasSize(6) @@ -68,13 +68,14 @@ class DefaultClientRequestObservationConventionTests { @Test void shouldAddKeyValuesForExchangeWithException() { - ClientRequestObservationContext context = new ClientRequestObservationContext(); + ClientRequest.Builder request = ClientRequest.create(HttpMethod.GET, URI.create("/test")); + ClientRequestObservationContext context = new ClientRequestObservationContext(request); context.setError(new IllegalStateException("Could not create client request")); assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).hasSize(6) - .contains(KeyValue.of("method", "none"), KeyValue.of("uri", "none"), KeyValue.of("status", "CLIENT_ERROR"), + .contains(KeyValue.of("method", "GET"), KeyValue.of("uri", "none"), KeyValue.of("status", "CLIENT_ERROR"), KeyValue.of("client.name", "none"), KeyValue.of("exception", "IllegalStateException"), KeyValue.of("outcome", "UNKNOWN")); assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).hasSize(1) - .contains(KeyValue.of("http.url", "none")); + .contains(KeyValue.of("http.url", "/test")); } @Test @@ -83,7 +84,6 @@ class DefaultClientRequestObservationConventionTests { .attribute(WebClient.class.getName() + ".uriTemplate", "/resource/{id}"); ClientRequestObservationContext context = createContext(request); context.setUriTemplate("/resource/{id}"); - context.setRequest(context.getCarrier().build()); 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", "none"), KeyValue.of("outcome", "SUCCESS")); @@ -94,7 +94,6 @@ class DefaultClientRequestObservationConventionTests { @Test void shouldAddKeyValuesForRequestWithoutUriTemplate() { ClientRequestObservationContext context = createContext(ClientRequest.create(HttpMethod.GET, URI.create("/resource/42"))); - context.setRequest(context.getCarrier().build()); assertThat(this.observationConvention.getLowCardinalityKeyValues(context)) .contains(KeyValue.of("method", "GET"), KeyValue.of("uri", "none")); assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).hasSize(1).contains(KeyValue.of("http.url", "/resource/42")); @@ -103,14 +102,12 @@ class DefaultClientRequestObservationConventionTests { @Test void shouldAddClientNameKeyValueForRequestWithHost() { ClientRequestObservationContext context = createContext(ClientRequest.create(HttpMethod.GET, URI.create("https://localhost:8080/resource/42"))); - context.setRequest(context.getCarrier().build()); assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("client.name", "localhost")); } @Test void shouldAddRootUriEvenIfTemplateMissing() { ClientRequestObservationContext context = createContext(ClientRequest.create(HttpMethod.GET, URI.create("https://example.org/"))); - context.setRequest(context.getCarrier().build()); assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).contains(KeyValue.of("uri", "/")); } @@ -118,13 +115,11 @@ class DefaultClientRequestObservationConventionTests { 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); + ClientRequestObservationContext context = new ClientRequestObservationContext(request); context.setResponse(ClientResponse.create(HttpStatus.OK).build()); return context; }