From 35fc2df94860e0d59c4399d8b6f01c7a57fe9c5d Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 25 Aug 2023 16:23:04 +0200 Subject: [PATCH] Instrument RestClient for Observability This commit instruments the new `RestClient` HTTP client for observability. Since this client is sharing its HTTP infrastructure with `RestTemplate` and operates on the same request/response types, this instrumentation reuses the Observation convention and context. This choice makes sense since one can build a new `RestClient` instance using a `RestTemplate` instance, effectively reusing the underlying configuration. Closes gh-31114 --- .../ROOT/pages/integration/observability.adoc | 26 +++ .../web/client/DefaultRestClient.java | 33 ++- .../web/client/DefaultRestClientBuilder.java | 14 ++ .../web/client/RestClient.java | 10 + .../web/client/RestTemplate.java | 8 + .../client/RestClientObservationTests.java | 208 ++++++++++++++++++ 6 files changed, 298 insertions(+), 1 deletion(-) create mode 100644 spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java diff --git a/framework-docs/modules/ROOT/pages/integration/observability.adoc b/framework-docs/modules/ROOT/pages/integration/observability.adoc index 98a4ab9649d..b6bd43da042 100644 --- a/framework-docs/modules/ROOT/pages/integration/observability.adoc +++ b/framework-docs/modules/ROOT/pages/integration/observability.adoc @@ -294,6 +294,32 @@ Instrumentation uses the `org.springframework.http.client.observation.ClientRequ |=== +[[observability.http-client.restclient]] +=== RestClient + +Applications must configure an `ObservationRegistry` on the `RestClient.Builder` to enable the instrumentation; without that, observations are "no-ops". + +Instrumentation uses the `org.springframework.http.client.observation.ClientRequestObservationConvention` by default, backed by the `ClientRequestObservationContext`. + +.Low cardinality Keys +[cols="a,a"] +|=== +|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. 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. +|`exception` _(required)_|Name of the exception thrown during the exchange, or `"none"` if no exception happened. +|=== + +.High cardinality Keys +[cols="a,a"] +|=== +|Name | Description +|`http.url` _(required)_|HTTP request URI. +|=== + [[observability.http-client.webclient]] === WebClient diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java index 00a7a1632dd..c6de84a2222 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java @@ -31,6 +31,8 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -49,6 +51,10 @@ import org.springframework.http.client.ClientHttpRequestInitializer; import org.springframework.http.client.ClientHttpRequestInterceptor; import org.springframework.http.client.ClientHttpResponse; import org.springframework.http.client.InterceptingClientHttpRequestFactory; +import org.springframework.http.client.observation.ClientHttpObservationDocumentation; +import org.springframework.http.client.observation.ClientRequestObservationContext; +import org.springframework.http.client.observation.ClientRequestObservationConvention; +import org.springframework.http.client.observation.DefaultClientRequestObservationConvention; import org.springframework.http.converter.GenericHttpMessageConverter; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.converter.HttpMessageNotReadableException; @@ -72,6 +78,8 @@ final class DefaultRestClient implements RestClient { private static final Log logger = LogFactory.getLog(DefaultRestClient.class); + private static final ClientRequestObservationConvention DEFAULT_OBSERVATION_CONVENTION = new DefaultClientRequestObservationConvention(); + private static final String URI_TEMPLATE_ATTRIBUTE = RestClient.class.getName() + ".uriTemplate"; @@ -97,6 +105,8 @@ final class DefaultRestClient implements RestClient { private final List> messageConverters; + private final ObservationRegistry observationRegistry; + DefaultRestClient(ClientHttpRequestFactory clientRequestFactory, @Nullable List interceptors, @@ -105,6 +115,7 @@ final class DefaultRestClient implements RestClient { @Nullable HttpHeaders defaultHeaders, @Nullable List statusHandlers, List> messageConverters, + ObservationRegistry observationRegistry, DefaultRestClientBuilder builder) { this.clientRequestFactory = clientRequestFactory; @@ -114,6 +125,7 @@ final class DefaultRestClient implements RestClient { this.defaultHeaders = defaultHeaders; this.defaultStatusHandlers = (statusHandlers != null ? new ArrayList<>(statusHandlers) : new ArrayList<>()); this.messageConverters = messageConverters; + this.observationRegistry = observationRegistry; this.builder = builder; } @@ -372,12 +384,17 @@ final class DefaultRestClient implements RestClient { Assert.notNull(exchangeFunction, "ExchangeFunction must not be null"); ClientHttpResponse clientResponse = null; + Observation observation = null; URI uri = null; try { uri = initUri(); HttpHeaders headers = initHeaders(); ClientHttpRequest clientRequest = createRequest(uri); clientRequest.getHeaders().addAll(headers); + ClientRequestObservationContext observationContext = new ClientRequestObservationContext(clientRequest); + observationContext.setUriTemplate((String) this.attributes.get(URI_TEMPLATE_ATTRIBUTE)); + observation = ClientHttpObservationDocumentation.HTTP_CLIENT_EXCHANGES.observation(null, + DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, observationRegistry).start(); if (this.body != null) { this.body.writeTo(clientRequest); } @@ -385,15 +402,29 @@ final class DefaultRestClient implements RestClient { this.httpRequestConsumer.accept(clientRequest); } clientResponse = clientRequest.execute(); + observationContext.setResponse(clientResponse); return exchangeFunction.exchange(clientRequest, clientResponse); } catch (IOException ex) { - throw createResourceAccessException(uri, this.httpMethod, ex); + ResourceAccessException resourceAccessException = createResourceAccessException(uri, this.httpMethod, ex); + if (observation != null) { + observation.error(resourceAccessException); + } + throw resourceAccessException; + } + catch (Throwable error) { + if (observation != null) { + observation.error(error); + } + throw error; } finally { if (close && clientResponse != null) { clientResponse.close(); } + if (observation != null) { + observation.stop(); + } } } diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java index 7bca9d25208..1d2f2aa70a5 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java @@ -24,6 +24,8 @@ import java.util.Map; import java.util.function.Consumer; import java.util.function.Predicate; +import io.micrometer.observation.ObservationRegistry; + import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatusCode; import org.springframework.http.client.ClientHttpRequestFactory; @@ -128,6 +130,8 @@ final class DefaultRestClientBuilder implements RestClient.Builder { @Nullable private List initializers; + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + public DefaultRestClientBuilder() { } @@ -156,6 +160,7 @@ final class DefaultRestClientBuilder implements RestClient.Builder { this.interceptors = (other.interceptors != null) ? new ArrayList<>(other.interceptors) : null; this.initializers = (other.initializers != null) ? new ArrayList<>(other.initializers) : null; + this.observationRegistry = other.observationRegistry; } public DefaultRestClientBuilder(RestTemplate restTemplate) { @@ -176,6 +181,7 @@ final class DefaultRestClientBuilder implements RestClient.Builder { if (!CollectionUtils.isEmpty(restTemplate.getClientHttpRequestInitializers())) { this.initializers = new ArrayList<>(restTemplate.getClientHttpRequestInitializers()); } + this.observationRegistry = restTemplate.getObservationRegistry(); } @@ -294,6 +300,13 @@ final class DefaultRestClientBuilder implements RestClient.Builder { return this; } + @Override + public RestClient.Builder observationRegistry(ObservationRegistry observationRegistry) { + Assert.notNull(observationRegistry, "observationRegistry must not be null"); + this.observationRegistry = observationRegistry; + return this; + } + @Override public RestClient.Builder apply(Consumer builderConsumer) { builderConsumer.accept(this); @@ -348,6 +361,7 @@ final class DefaultRestClientBuilder implements RestClient.Builder { defaultHeaders, this.statusHandlers, messageConverters, + this.observationRegistry, new DefaultRestClientBuilder(this) ); } diff --git a/spring-web/src/main/java/org/springframework/web/client/RestClient.java b/spring-web/src/main/java/org/springframework/web/client/RestClient.java index 930a2fd799d..3628035efc7 100644 --- a/spring-web/src/main/java/org/springframework/web/client/RestClient.java +++ b/spring-web/src/main/java/org/springframework/web/client/RestClient.java @@ -27,6 +27,8 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; +import io.micrometer.observation.ObservationRegistry; + import org.springframework.core.ParameterizedTypeReference; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; @@ -364,6 +366,14 @@ public interface RestClient { */ Builder messageConverters(Consumer>> configurer); + /** + * Configure the {@link io.micrometer.observation.ObservationRegistry} to use + * for recording HTTP client observations. + * @param observationRegistry the observation registry to use + * @return this builder + */ + Builder observationRegistry(ObservationRegistry observationRegistry); + /** * Apply the given {@code Consumer} to this builder instance. *

This can be useful for applying pre-packaged customizations. diff --git a/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java b/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java index ae42808722c..9377c9fcf6c 100644 --- a/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java +++ b/spring-web/src/main/java/org/springframework/web/client/RestTemplate.java @@ -353,6 +353,14 @@ public class RestTemplate extends InterceptingHttpAccessor implements RestOperat this.observationRegistry = observationRegistry; } + /** + * Return the configured {@link ObservationRegistry}. + * @since 6.1 + */ + public ObservationRegistry getObservationRegistry() { + return this.observationRegistry; + } + /** * Configure an {@link ObservationConvention} that sets the name of the * {@link Observation observation} as well as its {@link io.micrometer.common.KeyValues} diff --git a/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java new file mode 100644 index 00000000000..67b112bee17 --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java @@ -0,0 +1,208 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.client; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.net.URI; +import java.util.Map; +import java.util.UUID; + +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; +import org.junit.jupiter.api.Test; + +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +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.ClientRequestObservationContext; +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.BDDMockito.given; +import static org.mockito.BDDMockito.willThrow; +import static org.mockito.Mockito.mock; +import static org.springframework.http.HttpMethod.GET; +import static org.springframework.http.HttpMethod.POST; + +/** + * Tests for the client HTTP observations with {@link RestClient}. + * @author Brian Clozel + */ +class RestClientObservationTests { + + + private final TestObservationRegistry observationRegistry = TestObservationRegistry.create(); + + private final ClientHttpRequestFactory requestFactory = mock(); + + private final ClientHttpRequest request = mock(); + + private final ClientHttpResponse response = mock(); + + private final ResponseErrorHandler errorHandler = mock(); + + @SuppressWarnings("unchecked") + private final HttpMessageConverter converter = mock(); + + private RestClient client; + + + @BeforeEach + void setupEach() { + + this.client = RestClient.builder() + .messageConverters(converters -> converters.add(0, this.converter)) + .requestFactory(this.requestFactory) + .defaultStatusHandler(this.errorHandler) + .observationRegistry(this.observationRegistry) + .build(); + this.observationRegistry.observationConfig().observationHandler(new ContextAssertionObservationHandler()); + } + + @Test + void executeVarArgsAddsUriTemplateAsKeyValue() throws Exception { + mockSentRequest(GET, "https://example.com/hotels/42/bookings/21"); + mockResponseStatus(HttpStatus.OK); + + client.get().uri("https://example.com/hotels/{hotel}/bookings/{booking}", "42", "21") + .retrieve().toBodilessEntity(); + + assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "/hotels/{hotel}/bookings/{booking}"); + } + + @Test + void executeArgsMapAddsUriTemplateAsKeyValue() throws Exception { + mockSentRequest(GET, "https://example.com/hotels/42/bookings/21"); + mockResponseStatus(HttpStatus.OK); + + Map vars = Map.of("hotel", "42", "booking", "21"); + + client.get().uri("https://example.com/hotels/{hotel}/bookings/{booking}", vars) + .retrieve().toBodilessEntity(); + + assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "/hotels/{hotel}/bookings/{booking}"); + } + + + @Test + void executeAddsSuccessAsOutcome() throws Exception { + mockSentRequest(GET, "https://example.org"); + mockResponseStatus(HttpStatus.OK); + mockResponseBody("Hello World", MediaType.TEXT_PLAIN); + + client.get().uri("https://example.org").retrieve().toBodilessEntity(); + + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS"); + } + + @Test + void executeAddsServerErrorAsOutcome() throws Exception { + String url = "https://example.org"; + mockSentRequest(GET, url); + mockResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR); + willThrow(new HttpServerErrorException(HttpStatus.INTERNAL_SERVER_ERROR)) + .given(errorHandler).handleError(URI.create(url), GET, response); + + assertThatExceptionOfType(HttpServerErrorException.class).isThrownBy(() -> + client.get().uri(url).retrieve().toBodilessEntity()); + + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SERVER_ERROR"); + } + + @Test + void executeAddsExceptionAsKeyValue() throws Exception { + mockSentRequest(POST, "https://example.org/resource"); + mockResponseStatus(HttpStatus.OK); + + MediaType other = new MediaType("test", "other"); + mockResponseBody("Test Body", other); + + assertThatExceptionOfType(RestClientException.class).isThrownBy(() -> + client.post().uri("https://example.org/{p}", "resource") + .contentType(other) + .body(UUID.randomUUID()) + .retrieve().toBodilessEntity()); + assertThatHttpObservation().hasLowCardinalityKeyValue("exception", "RestClientException"); + } + + @Test + void executeWithIoExceptionAddsUnknownOutcome() throws Exception { + String url = "https://example.org/resource"; + mockSentRequest(GET, url); + given(request.execute()).willThrow(new IOException("Socket failure")); + + assertThatExceptionOfType(ResourceAccessException.class).isThrownBy(() -> + client.get().uri(url).retrieve().body(String.class)); + assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "UNKNOWN"); + } + + + private void mockSentRequest(HttpMethod method, String uri) throws Exception { + mockSentRequest(method, uri, new HttpHeaders()); + } + + private void mockSentRequest(HttpMethod method, String uri, HttpHeaders requestHeaders) throws Exception { + given(requestFactory.createRequest(URI.create(uri), method)).willReturn(request); + given(request.getHeaders()).willReturn(requestHeaders); + given(request.getMethod()).willReturn(method); + given(request.getURI()).willReturn(URI.create(uri)); + } + + private void mockResponseStatus(HttpStatus responseStatus) throws Exception { + given(request.execute()).willReturn(response); + given(errorHandler.hasError(response)).willReturn(responseStatus.isError()); + given(response.getStatusCode()).willReturn(responseStatus); + given(response.getStatusText()).willReturn(responseStatus.getReasonPhrase()); + } + + private void mockResponseBody(String expectedBody, MediaType mediaType) throws Exception { + HttpHeaders responseHeaders = new HttpHeaders(); + responseHeaders.setContentType(mediaType); + responseHeaders.setContentLength(expectedBody.length()); + given(response.getHeaders()).willReturn(responseHeaders); + given(response.getBody()).willReturn(new ByteArrayInputStream(expectedBody.getBytes())); + } + + + private TestObservationRegistryAssert.TestObservationRegistryAssertReturningObservationContextAssert assertThatHttpObservation() { + return TestObservationRegistryAssert.assertThat(this.observationRegistry) + .hasObservationWithNameEqualTo("http.client.requests").that(); + } + + static class ContextAssertionObservationHandler implements ObservationHandler { + + @Override + public boolean supportsContext(Observation.Context context) { + return context instanceof ClientRequestObservationContext; + } + + @Override + public void onStart(ClientRequestObservationContext context) { + assertThat(context.getCarrier()).isNotNull(); + } + } + +}