root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

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
This commit is contained in:
Brian Clozel 2023-12-12 14:41:41 +01:00
parent 7adc2f0779
commit b87852612b
3 changed files with 26 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientRequestObservationContext.java
View File

@ -50,10 +50,25 @@ public class ClientRequestObservationContext extends RequestReplySenderContext<C
private ClientRequest request;
/**
* Create a new Observation context for HTTP client observations.
* @deprecated as of 6.1, in favor of {@link #ClientRequestObservationContext(ClientRequest.Builder)}
*/
@Deprecated(since = "6.1", forRemoval = true)
public ClientRequestObservationContext() {
super(ClientRequestObservationContext::setRequestHeader);
}
/**
* Create a new Observation context for HTTP client observations.
*/
public ClientRequestObservationContext(ClientRequest.Builder request) {
super(ClientRequestObservationContext::setRequestHeader);
setCarrier(request);
setRequest(request.build());
}
private static void setRequestHeader(@Nullable ClientRequest.Builder request, String name, String value) {
if (request != null) {
request.headers(headers -> headers.set(name, value));

5
spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java
View File

@ -438,12 +438,11 @@ final class DefaultWebClient implements WebClient {
@SuppressWarnings("deprecation")
@Override
public Mono<ClientResponse> 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);

23
spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientRequestObservationConventionTests.java
View File

@ -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;
}