From b59277f0efb23217a4335e213f1f9ffaf7d96f6e Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 26 Jul 2019 12:12:56 +0200 Subject: [PATCH] Not using httpStatus() method for custom status codes curently whenever we try to retrieve the http status we get an exception for custom codes we've migrated to using the raw status code. Also, due to https://github.com/spring-projects/spring-framework/issues/23366 we had to manually wrap the ClientResponse so as we don't throw an exception on httpStatus() method fixes gh-1393 --- .../TraceWebClientBeanPostProcessor.java | 137 ++++++++++++++++-- .../client/integration/WebClientTests.java | 22 +++ 2 files changed, 148 insertions(+), 11 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java index 7362707f1..74a134925 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java @@ -33,6 +33,7 @@ import org.apache.commons.logging.LogFactory; import org.reactivestreams.Publisher; import org.reactivestreams.Subscription; import reactor.core.CoreSubscriber; +import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.annotation.Nullable; import reactor.util.context.Context; @@ -41,12 +42,20 @@ import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.cloud.sleuth.instrument.reactor.ReactorSleuth; +import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseCookie; +import org.springframework.http.ResponseEntity; +import org.springframework.http.client.reactive.ClientHttpResponse; +import org.springframework.util.MultiValueMap; import org.springframework.web.client.RestClientException; +import org.springframework.web.reactive.function.BodyExtractor; import org.springframework.web.reactive.function.client.ClientRequest; import org.springframework.web.reactive.function.client.ClientResponse; import org.springframework.web.reactive.function.client.ExchangeFilterFunction; import org.springframework.web.reactive.function.client.ExchangeFunction; +import org.springframework.web.reactive.function.client.ExchangeStrategies; import org.springframework.web.reactive.function.client.WebClient; /** @@ -290,17 +299,102 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { this.done = true; try { // decorate response body - this.actual - .onNext(ClientResponse.from(response) - .body(response.bodyToFlux(DataBuffer.class) - .transform(this.scopePassingTransformer)) - .build()); + this.actual.onNext(wrapped(response)); } finally { terminateSpan(response, null); } } + // TODO: Remove once fixed + // https://github.com/spring-projects/spring-framework/issues/23366 + private ClientResponse wrapped(ClientResponse response) { + return new ClientResponse() { + @Override + public HttpStatus statusCode() { + try { + return response.statusCode(); + } + catch (IllegalArgumentException ex) { + return null; + } + } + + @Override + public int rawStatusCode() { + return response.rawStatusCode(); + } + + @Override + public Headers headers() { + return response.headers(); + } + + @Override + public MultiValueMap cookies() { + return response.cookies(); + } + + @Override + public ExchangeStrategies strategies() { + return response.strategies(); + } + + @Override + public T body( + BodyExtractor extractor) { + return response.body(extractor); + } + + @Override + public Mono bodyToMono(Class elementClass) { + return response.bodyToMono(elementClass); + } + + @Override + public Mono bodyToMono( + ParameterizedTypeReference typeReference) { + return response.bodyToMono(typeReference); + } + + @Override + public Flux bodyToFlux(Class elementClass) { + return (Flux) response.bodyToFlux(DataBuffer.class) + .transform(scopePassingTransformer); + } + + @Override + public Flux bodyToFlux( + ParameterizedTypeReference typeReference) { + return (Flux) response.bodyToFlux(DataBuffer.class) + .transform(scopePassingTransformer); + } + + @Override + public Mono> toEntity(Class bodyType) { + return response.toEntity(bodyType); + } + + @Override + public Mono> toEntity( + ParameterizedTypeReference typeReference) { + return response.toEntity(typeReference); + } + + @Override + public Mono>> toEntityList( + Class elementType) { + return response.toEntityList(elementType); + } + + @Override + public Mono>> toEntityList( + ParameterizedTypeReference typeReference) { + return response.toEntityList(typeReference); + } + }; + } + @Override public void onError(Throwable t) { try { @@ -346,7 +440,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { void terminateSpan(@Nullable ClientResponse clientResponse, @Nullable Throwable throwable) { - if (clientResponse == null || clientResponse.statusCode() == null) { + if (clientResponse == null) { if (log.isDebugEnabled()) { log.debug("No response was returned. Will close the span [" + this.span + "]"); @@ -354,8 +448,8 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { handleReceive(this.span, this.ws, clientResponse, throwable); return; } - boolean error = clientResponse.statusCode().is4xxClientError() - || clientResponse.statusCode().is5xxServerError(); + int statusCode = statusCodeAsInt(clientResponse); + boolean error = isError(statusCode); if (error) { if (log.isDebugEnabled()) { log.debug( @@ -363,13 +457,34 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { + this.span + "]"); } throwable = new RestClientException("Status code of the response is [" - + clientResponse.statusCode().value() - + "] and the reason is [" - + clientResponse.statusCode().getReasonPhrase() + "]"); + + statusCode + "] and the reason is [" + + reasonPhrase(clientResponse) + "]"); } handleReceive(this.span, this.ws, clientResponse, throwable); } + private String reasonPhrase(ClientResponse clientResponse) { + try { + return clientResponse.statusCode().getReasonPhrase(); + } + catch (IllegalArgumentException ex) { + return ""; + } + } + + private boolean isError(int code) { + return code >= 400; + } + + private int statusCodeAsInt(ClientResponse response) { + try { + return response.rawStatusCode(); + } + catch (Exception dontCare) { + return 0; + } + } + } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/integration/WebClientTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/integration/WebClientTests.java index bc68b42fc..5c1091c32 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/integration/WebClientTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/integration/WebClientTests.java @@ -377,6 +377,22 @@ public class WebClientTests { .contains("CLIENT"); } + @Test + @SuppressWarnings("unchecked") + public void shouldNotBreakWhenCustomStatusCodeIsSetViaWebClient() { + Span span = this.tracer.nextSpan().name("foo").start(); + + try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span)) { + this.webClient.get() + .uri("http://localhost:" + this.port + "/customstatuscode").exchange() + .block(); + } + finally { + span.finish(); + } + then(this.tracer.currentSpan()).isNull(); + } + @Test @Ignore("Flakey on CI") public void shouldReportTraceForCancelledRequestViaWebClient() { @@ -663,6 +679,12 @@ public class WebClientTests { return traceId; } + @RequestMapping(value = "/customstatuscode", method = RequestMethod.GET) + public ResponseEntity customStatusCode() { + this.span = this.tracer.currentSpan(); + return ResponseEntity.status(499).build(); + } + @RequestMapping("/") public Map home(@RequestHeader HttpHeaders headers) { Map map = new HashMap<>();