From 3acf8db61c247f77cf10550fa9f872522ecdef03 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 4 Jul 2019 08:07:18 +0800 Subject: [PATCH 1/4] Latest brave --- benchmarks/pom.xml | 2 +- pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index a8f48c92c..8040cc56d 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -34,7 +34,7 @@ 1.8 1.8 2.1.6.RELEASE - 5.6.6 + 5.6.7 diff --git a/pom.xml b/pom.xml index fda1cfac9..593b3b3f3 100644 --- a/pom.xml +++ b/pom.xml @@ -265,7 +265,7 @@ 2.1.3.BUILD-SNAPSHOT 2.1.3.BUILD-SNAPSHOT - 5.6.6 + 5.6.7 2.1.2.RELEASE From b96392c0f30e09376cc23493f05017356c13a7f6 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 26 Jul 2019 10:13:06 +0200 Subject: [PATCH 2/4] Removed the cycle by not fetching http tracing currently when an exception is thrown upon trying to fetch a Feign client bean, we're trying to fetch the httptracing bean to create a default client. The thing is that httptracing might be in creation this fix removes the fetching of the httptracing bean since we can use the lazy client fixes gh-1334 --- .../instrument/web/client/feign/SleuthFeignBuilder.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java index b2a718fc5..696a3dc41 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java @@ -16,7 +16,6 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; -import brave.http.HttpTracing; import feign.Client; import feign.Feign; import feign.Retryer; @@ -46,8 +45,7 @@ final class SleuthFeignBuilder { return new LazyClient(beanFactory, client); } catch (BeansException ex) { - return TracingFeignClient.create(beanFactory.getBean(HttpTracing.class), - new Client.Default(null, null)); + return new LazyClient(beanFactory, new Client.Default(null, null)); } } From d642ffd09a0153ed53658b45f3f3706fc8024e98 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 26 Jul 2019 10:36:24 +0200 Subject: [PATCH 3/4] Removed the eager injection of a bean in the configuration class at the point of creating the configuration class, that bean can use httptracing that can be in progress of being created. with this fix we're making the call lazy fixes gh-1354 --- .../web/client/TraceWebClientAutoConfiguration.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientAutoConfiguration.java index 533658beb..cfae6daa6 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientAutoConfiguration.java @@ -84,7 +84,7 @@ public class TraceWebClientAutoConfiguration { protected static class TraceInterceptorConfiguration { @Autowired - private TracingClientHttpRequestInterceptor clientInterceptor; + private BeanFactory beanFactory; @Bean static TraceRestTemplateBeanPostProcessor traceRestTemplateBeanPostProcessor( @@ -95,7 +95,8 @@ public class TraceWebClientAutoConfiguration { @Bean @Order RestTemplateCustomizer traceRestTemplateCustomizer() { - return new TraceRestTemplateCustomizer(this.clientInterceptor); + return new TraceRestTemplateCustomizer( + new LazyTracingClientHttpRequestInterceptor(this.beanFactory)); } } @@ -247,9 +248,9 @@ class RestTemplateInterceptorInjector { class TraceRestTemplateCustomizer implements RestTemplateCustomizer { - private final TracingClientHttpRequestInterceptor interceptor; + private final ClientHttpRequestInterceptor interceptor; - TraceRestTemplateCustomizer(TracingClientHttpRequestInterceptor interceptor) { + TraceRestTemplateCustomizer(ClientHttpRequestInterceptor interceptor) { this.interceptor = interceptor; } From b59277f0efb23217a4335e213f1f9ffaf7d96f6e Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 26 Jul 2019 12:12:56 +0200 Subject: [PATCH 4/4] 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<>();