Skip to content

Commit

Permalink
Record errors thrown by custom handler in RestTemplate observations
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bclozel committed Jan 22, 2024
1 parent c668473 commit b484ab1
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ protected <T> 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);
}
Expand All @@ -869,7 +869,7 @@ protected <T> 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();
}
}

}

0 comments on commit b484ab1

Please sign in to comment.