Spring/spring-framework
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Start RestTemplate observation wtih available request

Browse Source 
Prior to this commit, the `RestTemplate` `ClientHttpObservation` would
be started before the request object is available. While this would also
measure the connection estalishment for some HTTP client libraries, this
arrangement is incompatible with a tracing approach where the request
must be available to propagate information through the request headers.

This commit ensures that the observation only starts when the request is
available.

Fixes gh-29234
This commit is contained in:
Brian Clozel
2022-09-30 23:12:53 +02:00
parent 380795e5b8
commit a18c692b53
4 changed files with 51 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
spring-web/src/test/java/org/springframework/http/client/observation/DefaultClientHttpObservationConventionTests.java
View File

@@ -40,6 +40,8 @@ import static org.mockito.Mockito.mock;
*/
class DefaultClientHttpObservationConventionTests {
private final MockClientHttpRequest request = new MockClientHttpRequest(HttpMethod.GET, "/test");
private final DefaultClientHttpObservationConvention observationConvention = new DefaultClientHttpObservationConvention();
@Test
@@ -49,38 +51,17 @@ class DefaultClientHttpObservationConventionTests {
@Test
void shouldHaveContextualName() {
ClientHttpObservationContext context = new ClientHttpObservationContext();
context.setCarrier(new MockClientHttpRequest(HttpMethod.GET, "/test"));
ClientHttpObservationContext context = new ClientHttpObservationContext(this.request);
assertThat(this.observationConvention.getContextualName(context)).isEqualTo("http get");
}
@Test
void supportsOnlyClientHttpObservationContext() {
assertThat(this.observationConvention.supportsContext(new ClientHttpObservationContext())).isTrue();
ClientHttpObservationContext context = new ClientHttpObservationContext(this.request);
assertThat(this.observationConvention.supportsContext(context)).isTrue();
assertThat(this.observationConvention.supportsContext(new Observation.Context())).isFalse();
}
@Test
void addsKeyValuesForNullExchange() {
ClientHttpObservationContext context = new ClientHttpObservationContext();
assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).hasSize(5)
.contains(KeyValue.of("method", "none"), KeyValue.of("uri", "none"), KeyValue.of("status", "CLIENT_ERROR"),
KeyValue.of("exception", "none"), KeyValue.of("outcome", "UNKNOWN"));
assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).hasSize(2)
.contains(KeyValue.of("client.name", "none"), KeyValue.of("uri.expanded", "none"));
}
@Test
void addsKeyValuesForExchangeWithException() {
ClientHttpObservationContext context = new ClientHttpObservationContext();
context.setError(new IllegalStateException("Could not create client request"));
assertThat(this.observationConvention.getLowCardinalityKeyValues(context)).hasSize(5)
.contains(KeyValue.of("method", "none"), KeyValue.of("uri", "none"), KeyValue.of("status", "CLIENT_ERROR"),
KeyValue.of("exception", "IllegalStateException"), KeyValue.of("outcome", "UNKNOWN"));
assertThat(this.observationConvention.getHighCardinalityKeyValues(context)).hasSize(2)
.contains(KeyValue.of("client.name", "none"), KeyValue.of("uri.expanded", "none"));
}
@Test
void addsKeyValuesForRequestWithUriTemplate() {
ClientHttpObservationContext context = createContext(
@@ -113,7 +94,7 @@ class DefaultClientHttpObservationConventionTests {
@Test
void addsKeyValueForNonResolvableStatus() throws Exception {
ClientHttpObservationContext context = new ClientHttpObservationContext();
ClientHttpObservationContext context = new ClientHttpObservationContext(this.request);
ClientHttpResponse response = mock(ClientHttpResponse.class);
context.setResponse(response);
given(response.getStatusCode()).willThrow(new IOException("test error"));
@@ -121,8 +102,7 @@ class DefaultClientHttpObservationConventionTests {
}
private ClientHttpObservationContext createContext(ClientHttpRequest request, ClientHttpResponse response) {
ClientHttpObservationContext context = new ClientHttpObservationContext();
context.setCarrier(request);
ClientHttpObservationContext context = new ClientHttpObservationContext(request);
context.setResponse(response);
return context;
}

18
spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java
View File

@@ -22,6 +22,8 @@ import java.net.URI;
import java.util.List;
import java.util.Map;
import io.micrometer.observation.Observation;
import io.micrometer.observation.ObservationHandler;
import io.micrometer.observation.tck.TestObservationRegistry;
import io.micrometer.observation.tck.TestObservationRegistryAssert;
import org.junit.jupiter.api.BeforeEach;
@@ -35,8 +37,10 @@ import org.springframework.http.MediaType;
import org.springframework.http.client.ClientHttpRequest;
import org.springframework.http.client.ClientHttpRequestFactory;
import org.springframework.http.client.ClientHttpResponse;
import org.springframework.http.client.observation.ClientHttpObservationContext;
import org.springframework.http.converter.HttpMessageConverter;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
@@ -73,6 +77,7 @@ class RestTemplateObservationTests {
this.template.setRequestFactory(this.requestFactory);
this.template.setErrorHandler(this.errorHandler);
this.template.setObservationRegistry(this.observationRegistry);
this.observationRegistry.observationConfig().observationHandler(new ContextAssertionObservationHandler());
}
@Test
@@ -187,4 +192,17 @@ class RestTemplateObservationTests {
.hasObservationWithNameEqualTo("http.client.requests").that();
}
static class ContextAssertionObservationHandler implements ObservationHandler<ClientHttpObservationContext> {
@Override
public boolean supportsContext(Observation.Context context) {
return context instanceof ClientHttpObservationContext;
}
@Override
public void onStart(ClientHttpObservationContext context) {
assertThat(context.getCarrier()).isNotNull();
}
}
}