From b484ab116f818129471d3bf19feea2a9df1f2c92 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 22 Jan 2024 11:03:57 +0100 Subject: [PATCH] Record errors thrown by custom handler in RestTemplate observations Prior to this commit, the `RestTemplate` observation instrumentation would only record `RestClientException` and `IOException` as errors in the observation. Other types of errors can be thrown by custom components, such as `ResponseErrorHandler` and in this case they aren't recorded with the observation. Also, the current instrumentation does not create any observation scope around the execution. While this would have a limited benefit as no application code is executed there, developers could set up custom components (such as, again, `ResponseErrorHandler`) that could use contextual logging with trace ids. This commit ensures that all `Throwable` are recorded as errors with the observations and that an observation `Scope` is created around the execution of the client exchange. Fixes gh-32063 --- .../web/client/RestTemplate.java | 4 +- .../client/RestTemplateObservationTests.java | 49 +++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) 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 19e42a303979..f88c41b7c163 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 @@ -855,7 +855,7 @@ protected T doExecute(URI url, @Nullable String uriTemplate, @Nullable HttpM Observation observation = ClientHttpObservationDocumentation.HTTP_CLIENT_EXCHANGES.observation(this.observationConvention, DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, this.observationRegistry).start(); ClientHttpResponse response = null; - try { + try (Observation.Scope scope = observation.openScope()){ if (requestCallback != null) { requestCallback.doWithRequest(request); } @@ -869,7 +869,7 @@ protected T doExecute(URI url, @Nullable String uriTemplate, @Nullable HttpM observation.error(exception); throw exception; } - catch (RestClientException exc) { + catch (Throwable exc) { observation.error(exc); throw exc; } diff --git a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java index 97eb7fc5fef9..2080b265ad0e 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java @@ -39,9 +39,11 @@ import org.springframework.http.client.ClientHttpResponse; import org.springframework.http.client.observation.ClientRequestObservationContext; import org.springframework.http.converter.HttpMessageConverter; +import org.springframework.lang.Nullable; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; @@ -158,6 +160,31 @@ void executeWithIoExceptionAddsUnknownOutcome() throws Exception { assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "UNKNOWN"); } + @Test // gh-32060 + void executeShouldRecordErrorsThrownByErrorHandler() throws Exception { + mockSentRequest(GET, "https://example.org"); + mockResponseStatus(HttpStatus.OK); + mockResponseBody("Hello World", MediaType.TEXT_PLAIN); + given(errorHandler.hasError(any())).willThrow(new IllegalStateException("error handler")); + + assertThatIllegalStateException().isThrownBy(() -> + template.execute("https://example.org", GET, null, null)); + + assertThatHttpObservation().hasLowCardinalityKeyValue("exception", "IllegalStateException"); + } + + @Test // gh-32060 + void executeShouldCreateObservationScope() throws Exception { + mockSentRequest(GET, "https://example.org"); + mockResponseStatus(HttpStatus.OK); + mockResponseBody("Hello World", MediaType.TEXT_PLAIN); + ObservationErrorHandler observationErrorHandler = new ObservationErrorHandler(observationRegistry); + template.setErrorHandler(observationErrorHandler); + + template.execute("https://example.org", GET, null, null); + assertThat(observationErrorHandler.currentObservation).isNotNull(); + } + private void mockSentRequest(HttpMethod method, String uri) throws Exception { mockSentRequest(method, uri, new HttpHeaders()); @@ -205,4 +232,26 @@ public void onStart(ClientRequestObservationContext context) { } } + static class ObservationErrorHandler implements ResponseErrorHandler { + + final TestObservationRegistry observationRegistry; + + @Nullable + Observation currentObservation; + + public ObservationErrorHandler(TestObservationRegistry observationRegistry) { + this.observationRegistry = observationRegistry; + } + + @Override + public boolean hasError(ClientHttpResponse response) throws IOException { + return true; + } + + @Override + public void handleError(ClientHttpResponse response) throws IOException { + currentObservation = this.observationRegistry.getCurrentObservation(); + } + } + }