Skip to content

Commit da4547a

Browse files
bclozelbclozel
committed
Extend observations to RestClient ResponseSpec
Prior to this commit, the `RestClient` observations would be stopped as soon as the exchange function was called. This means that all errors related to response decoding or mapping would not be recorded by the obsevations. This commit extends the observation recording to the `ResponseSpec` DSL calls as well as custom exchange functions. Fixes gh-32575
1 parent 97eddb7 commit da4547a

File tree

2 files changed

+102
-19
lines changed

2 files changed

+102
-19
lines changed

‎spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java

+39-11
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ public Builder mutate() {
192192
@Nullable
193193
@SuppressWarnings({"rawtypes", "unchecked"})
194194
private <T> T readWithMessageConverters(ClientHttpResponse clientResponse, Runnable callback, Type bodyType,
195-
Class<T> bodyClass) {
195+
Class<T> bodyClass, @Nullable Observation observation) {
196196

197197
MediaType contentType = getContentType(clientResponse);
198198

@@ -220,9 +220,13 @@ private <T> T readWithMessageConverters(ClientHttpResponse clientResponse, Runna
220220
return (T) messageConverter.read((Class)bodyClass, responseWrapper);
221221
}
222222
}
223-
throw new UnknownContentTypeException(bodyType, contentType,
223+
UnknownContentTypeException unknownContentTypeException = new UnknownContentTypeException(bodyType, contentType,
224224
responseWrapper.getStatusCode(), responseWrapper.getStatusText(),
225225
responseWrapper.getHeaders(), RestClientUtils.getBody(responseWrapper));
226+
if (observation != null) {
227+
observation.error(unknownContentTypeException);
228+
}
229+
throw unknownContentTypeException;
226230
}
227231
catch (UncheckedIOException | IOException | HttpMessageNotReadableException ex) {
228232
Throwable cause;
@@ -232,8 +236,17 @@ private <T> T readWithMessageConverters(ClientHttpResponse clientResponse, Runna
232236
else {
233237
cause = ex;
234238
}
235-
throw new RestClientException("Error while extracting response for type [" +
239+
RestClientException restClientException = new RestClientException("Error while extracting response for type [" +
236240
ResolvableType.forType(bodyType) + "] and content type [" + contentType + "]", cause);
241+
if (observation != null) {
242+
observation.error(restClientException);
243+
}
244+
throw restClientException;
245+
}
246+
finally {
247+
if (observation != null) {
248+
observation.stop();
249+
}
237250
}
238251
}
239252

@@ -475,28 +488,30 @@ private <T> T exchangeInternal(ExchangeFunction<T> exchangeFunction, boolean clo
475488
}
476489
clientResponse = clientRequest.execute();
477490
observationContext.setResponse(clientResponse);
478-
ConvertibleClientHttpResponse convertibleWrapper = new DefaultConvertibleClientHttpResponse(clientResponse);
491+
ConvertibleClientHttpResponse convertibleWrapper = new DefaultConvertibleClientHttpResponse(clientResponse, observation);
479492
return exchangeFunction.exchange(clientRequest, convertibleWrapper);
480493
}
481494
catch (IOException ex) {
482495
ResourceAccessException resourceAccessException = createResourceAccessException(uri, this.httpMethod, ex);
483496
if (observation != null) {
484497
observation.error(resourceAccessException);
498+
observation.stop();
485499
}
486500
throw resourceAccessException;
487501
}
488502
catch (Throwable error) {
489503
if (observation != null) {
490504
observation.error(error);
505+
observation.stop();
491506
}
492507
throw error;
493508
}
494509
finally {
495510
if (close && clientResponse != null) {
496511
clientResponse.close();
497-
}
498-
if (observation != null) {
499-
observation.stop();
512+
if (observation != null) {
513+
observation.stop();
514+
}
500515
}
501516
}
502517
}
@@ -665,7 +680,7 @@ public ResponseEntity<Void> toBodilessEntity() {
665680
@Nullable
666681
private <T> T readBody(Type bodyType, Class<T> bodyClass) {
667682
return DefaultRestClient.this.readWithMessageConverters(this.clientResponse, this::applyStatusHandlers,
668-
bodyType, bodyClass);
683+
bodyType, bodyClass, getCurrentObservation());
669684

670685
}
671686

@@ -686,31 +701,43 @@ private void applyStatusHandlers() {
686701
throw new UncheckedIOException(ex);
687702
}
688703
}
704+
705+
@Nullable
706+
private Observation getCurrentObservation() {
707+
if (this.clientResponse instanceof DefaultConvertibleClientHttpResponse convertibleResponse) {
708+
return convertibleResponse.observation;
709+
}
710+
return null;
711+
}
712+
689713
}
690714

691715

692716
private class DefaultConvertibleClientHttpResponse implements RequestHeadersSpec.ConvertibleClientHttpResponse {
693717

694718
private final ClientHttpResponse delegate;
695719

720+
private final Observation observation;
721+
696722

697-
public DefaultConvertibleClientHttpResponse(ClientHttpResponse delegate) {
723+
public DefaultConvertibleClientHttpResponse(ClientHttpResponse delegate, Observation observation) {
698724
this.delegate = delegate;
725+
this.observation = observation;
699726
}
700727

701728

702729
@Nullable
703730
@Override
704731
public <T> T bodyTo(Class<T> bodyType) {
705-
return readWithMessageConverters(this.delegate, () -> {} , bodyType, bodyType);
732+
return readWithMessageConverters(this.delegate, () -> {} , bodyType, bodyType, this.observation);
706733
}
707734

708735
@Nullable
709736
@Override
710737
public <T> T bodyTo(ParameterizedTypeReference<T> bodyType) {
711738
Type type = bodyType.getType();
712739
Class<T> bodyClass = bodyClass(type);
713-
return readWithMessageConverters(this.delegate, () -> {} , type, bodyClass);
740+
return readWithMessageConverters(this.delegate, () -> {}, type, bodyClass, this.observation);
714741
}
715742

716743
@Override
@@ -736,6 +763,7 @@ public String getStatusText() throws IOException {
736763
@Override
737764
public void close() {
738765
this.delegate.close();
766+
this.observation.stop();
739767
}
740768

741769
}

‎spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java

+63-8
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.io.ByteArrayInputStream;
2020
import java.io.IOException;
2121
import java.net.URI;
22+
import java.nio.charset.StandardCharsets;
2223
import java.util.Map;
2324
import java.util.UUID;
2425

@@ -40,6 +41,7 @@
4041
import org.springframework.http.client.observation.ClientRequestObservationConvention;
4142
import org.springframework.http.client.observation.DefaultClientRequestObservationConvention;
4243
import org.springframework.http.converter.HttpMessageConverter;
44+
import org.springframework.util.StreamUtils;
4345

4446
import static org.assertj.core.api.Assertions.assertThat;
4547
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -84,7 +86,7 @@ void setupEach() {
8486
}
8587

8688
@Test
87-
void executeVarArgsAddsUriTemplateAsKeyValue() throws Exception {
89+
void shouldContributeTemplateWhenUriVariables() throws Exception {
8890
mockSentRequest(GET, "https://example.com/hotels/42/bookings/21");
8991
mockResponseStatus(HttpStatus.OK);
9092

@@ -95,7 +97,7 @@ void executeVarArgsAddsUriTemplateAsKeyValue() throws Exception {
9597
}
9698

9799
@Test
98-
void executeArgsMapAddsUriTemplateAsKeyValue() throws Exception {
100+
void shouldContributeTemplateWhenMap() throws Exception {
99101
mockSentRequest(GET, "https://example.com/hotels/42/bookings/21");
100102
mockResponseStatus(HttpStatus.OK);
101103

@@ -107,9 +109,8 @@ void executeArgsMapAddsUriTemplateAsKeyValue() throws Exception {
107109
assertThatHttpObservation().hasLowCardinalityKeyValue("uri", "/hotels/{hotel}/bookings/{booking}");
108110
}
109111

110-
111112
@Test
112-
void executeAddsSuccessAsOutcome() throws Exception {
113+
void shouldContributeSuccessOutcome() throws Exception {
113114
mockSentRequest(GET, "https://example.org");
114115
mockResponseStatus(HttpStatus.OK);
115116
mockResponseBody("Hello World", MediaType.TEXT_PLAIN);
@@ -120,7 +121,7 @@ void executeAddsSuccessAsOutcome() throws Exception {
120121
}
121122

122123
@Test
123-
void executeAddsServerErrorAsOutcome() throws Exception {
124+
void shouldContributeServerErrorOutcome() throws Exception {
124125
String url = "https://example.org";
125126
mockSentRequest(GET, url);
126127
mockResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR);
@@ -134,7 +135,7 @@ void executeAddsServerErrorAsOutcome() throws Exception {
134135
}
135136

136137
@Test
137-
void executeAddsExceptionAsKeyValue() throws Exception {
138+
void shouldContributeDecodingError() throws Exception {
138139
mockSentRequest(POST, "https://example.org/resource");
139140
mockResponseStatus(HttpStatus.OK);
140141

@@ -150,7 +151,7 @@ void executeAddsExceptionAsKeyValue() throws Exception {
150151
}
151152

152153
@Test
153-
void executeWithIoExceptionAddsUnknownOutcome() throws Exception {
154+
void shouldContributeIOError() throws Exception {
154155
String url = "https://example.org/resource";
155156
mockSentRequest(GET, url);
156157
given(request.execute()).willThrow(new IOException("Socket failure"));
@@ -161,7 +162,7 @@ void executeWithIoExceptionAddsUnknownOutcome() throws Exception {
161162
}
162163

163164
@Test
164-
void executeWithCustomConventionUsesCustomObservationName() throws Exception {
165+
void shouldUseCustomConvention() throws Exception {
165166
ClientRequestObservationConvention observationConvention =
166167
new DefaultClientRequestObservationConvention("custom.requests");
167168
RestClient restClient = this.client.mutate().observationConvention(observationConvention).build();
@@ -174,6 +175,56 @@ void executeWithCustomConventionUsesCustomObservationName() throws Exception {
174175
.hasObservationWithNameEqualTo("custom.requests");
175176
}
176177

178+
@Test
179+
void shouldAddClientDecodingErrorAsException() throws Exception {
180+
String url = "https://example.org";
181+
mockSentRequest(GET, url);
182+
mockResponseStatus(HttpStatus.OK);
183+
mockResponseBody("INVALID", MediaType.APPLICATION_JSON);
184+
185+
assertThatExceptionOfType(RestClientException.class).isThrownBy(() ->
186+
client.get().uri(url).retrieve().body(User.class));
187+
188+
assertThatHttpObservation().hasLowCardinalityKeyValue("exception", "RestClientException");
189+
}
190+
191+
@Test
192+
void shouldAddUnknownContentTypeErrorAsException() throws Exception {
193+
String url = "https://example.org";
194+
mockSentRequest(GET, url);
195+
mockResponseStatus(HttpStatus.OK);
196+
mockResponseBody("Not Found", MediaType.TEXT_HTML);
197+
198+
assertThatExceptionOfType(RestClientException.class).isThrownBy(() ->
199+
client.get().uri(url).retrieve().body(User.class));
200+
201+
assertThatHttpObservation().hasLowCardinalityKeyValue("exception", "UnknownContentTypeException");
202+
}
203+
204+
@Test
205+
void registerObservationWhenReadingBody() throws Exception {
206+
mockSentRequest(GET, "https://example.org");
207+
mockResponseStatus(HttpStatus.OK);
208+
mockResponseBody("Hello World", MediaType.TEXT_PLAIN);
209+
210+
client.get().uri("https://example.org").exchange((request, response) -> response.bodyTo(String.class));
211+
assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS");
212+
}
213+
214+
@Test
215+
void registerObservationWhenReadingStream() throws Exception {
216+
mockSentRequest(GET, "https://example.org");
217+
mockResponseStatus(HttpStatus.OK);
218+
mockResponseBody("Hello World", MediaType.TEXT_PLAIN);
219+
220+
client.get().uri("https://example.org").exchange((request, response) -> {
221+
String result = StreamUtils.copyToString(response.getBody(), StandardCharsets.UTF_8);
222+
response.close();
223+
return result;
224+
}, false);
225+
assertThatHttpObservation().hasLowCardinalityKeyValue("outcome", "SUCCESS");
226+
}
227+
177228

178229
private void mockSentRequest(HttpMethod method, String uri) throws Exception {
179230
mockSentRequest(method, uri, new HttpHeaders());
@@ -220,4 +271,8 @@ public void onStart(ClientRequestObservationContext context) {
220271
}
221272
}
222273

274+
record User(String name) {
275+
276+
}
277+
223278
}

0 commit comments

Comments
 (0)