diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/LazyClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/LazyClient.java index e589d346c..480354d0a 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/LazyClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/LazyClient.java @@ -28,6 +28,8 @@ class LazyClient implements Client { private final BeanFactory beanFactory; private final Client delegate; + private TraceFeignObjectWrapper wrapper; + LazyClient(BeanFactory beanFactory, Client delegate) { this.beanFactory = beanFactory; this.delegate = delegate; @@ -39,6 +41,9 @@ class LazyClient implements Client { } private TraceFeignObjectWrapper wrapper() { - return new TraceFeignObjectWrapper(this.beanFactory); + if (this.wrapper == null) { + this.wrapper = new TraceFeignObjectWrapper(this.beanFactory); + } + return this.wrapper; } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java index 3c0f38f3e..1d9905414 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java @@ -17,14 +17,21 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import java.io.IOException; +import java.util.HashMap; +import brave.Span; +import brave.Tracer; +import brave.http.HttpTracing; +import com.netflix.client.ClientException; import feign.Client; import feign.Request; import feign.Response; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.netflix.ribbon.SpringClientFactory; import org.springframework.cloud.openfeign.ribbon.CachingSpringLoadBalancerFactory; import org.springframework.cloud.openfeign.ribbon.LoadBalancerFeignClient; -import org.springframework.cloud.netflix.ribbon.SpringClientFactory; /** * We need to wrap the {@link LoadBalancerFeignClient} into a trace representation @@ -35,7 +42,12 @@ import org.springframework.cloud.netflix.ribbon.SpringClientFactory; */ class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { + private static final Log log = LogFactory.getLog(TraceLoadBalancerFeignClient.class); + private final BeanFactory beanFactory; + Tracer tracer; + HttpTracing httpTracing; + TracingFeignClient tracingFeignClient; TraceLoadBalancerFeignClient(Client delegate, CachingSpringLoadBalancerFactory lbClientFactory, @@ -46,8 +58,54 @@ class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { @Override public Response execute(Request request, Request.Options options) throws IOException { - return ((Client) new TraceFeignObjectWrapper(this.beanFactory).wrap( - (Client) TraceLoadBalancerFeignClient.super::execute)).execute(request, options); + if (log.isDebugEnabled()) { + log.debug("Before send"); + } + Response response = null; + Span fallbackSpan = tracer().nextSpan().start(); + try { + response = super.execute(request, options); + if (log.isDebugEnabled()) { + log.debug("After receive"); + } + return response; + } catch (Exception e){ + if (log.isDebugEnabled()) { + log.debug("Exception thrown", e); + } + if (e instanceof IOException || e.getCause() != null && + e.getCause() instanceof ClientException && + ((ClientException) e.getCause()).getErrorType() == ClientException.ErrorType.GENERAL ) { + if (log.isDebugEnabled()) { + log.debug("General exception was thrown, so most likely the traced client wasn't called. Falling back to a manual span"); + } + fallbackSpan = tracingFeignClient().handleSend(new HashMap<>(request.headers()), request, fallbackSpan); + tracingFeignClient().handleReceive(fallbackSpan, response, e); + } + throw e; + } + } + + private Tracer tracer() { + if (this.tracer == null) { + this.tracer = this.beanFactory.getBean(Tracer.class); + } + return this.tracer; + } + + private HttpTracing httpTracing() { + if (this.httpTracing == null) { + this.httpTracing = this.beanFactory.getBean(HttpTracing.class); + } + return this.httpTracing; + } + + private TracingFeignClient tracingFeignClient() { + if (this.tracingFeignClient == null) { + this.tracingFeignClient = + (TracingFeignClient) TracingFeignClient.create(httpTracing(), getDelegate()); + } + return this.tracingFeignClient; } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java index fffcf4749..f21553525 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java @@ -26,7 +26,6 @@ import java.util.Map; import brave.Span; import brave.Tracer; -import brave.Tracing; import brave.http.HttpClientHandler; import brave.http.HttpTracing; import brave.propagation.Propagation; @@ -63,11 +62,7 @@ final class TracingFeignClient implements Client { } }; - public static Client create(Tracing tracing, Client delegate) { - return create(HttpTracing.create(tracing), delegate); - } - - public static Client create(HttpTracing httpTracing, Client delegate) { + static Client create(HttpTracing httpTracing, Client delegate) { return new TracingFeignClient(httpTracing, delegate); } @@ -86,7 +81,7 @@ final class TracingFeignClient implements Client { @Override public Response execute(Request request, Request.Options options) throws IOException { Map> headers = new HashMap<>(request.headers()); - Span span = this.handler.handleSend(this.injector, headers, request); + Span span = handleSend(headers, request, null); if (log.isDebugEnabled()) { log.debug("Handled send of " + span); } @@ -100,13 +95,24 @@ final class TracingFeignClient implements Client { throw e; } finally { - this.handler.handleReceive(response, error, span); + handleReceive(span, response, error); if (log.isDebugEnabled()) { log.debug("Handled receive of " + span); } } } + Span handleSend(Map> headers, Request request, Span clientSpan) { + if (clientSpan != null) { + return this.handler.handleSend(this.injector, headers, request, clientSpan); + } + return this.handler.handleSend(this.injector, headers, request); + } + + void handleReceive(Span span, Response response, Throwable error) { + this.handler.handleReceive(response, error, span); + } + private Request modifiedRequest(Request request, Map> headers) { String method = request.method(); String url = request.url(); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/discoveryexception/WebClientDiscoveryExceptionTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/discoveryexception/WebClientDiscoveryExceptionTests.java index dfa2d8e88..008902ef8 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/discoveryexception/WebClientDiscoveryExceptionTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/discoveryexception/WebClientDiscoveryExceptionTests.java @@ -89,7 +89,6 @@ public class WebClientDiscoveryExceptionTests { // hystrix commands should finish at this point Thread.sleep(200); List spans = this.reporter.getSpans(); - then(spans).hasSize(2); then(spans.stream() .filter(span1 -> span1.kind() == zipkin2.Span.Kind.CLIENT) .findFirst() diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue350/Issue350Tests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue350/Issue350Tests.java index 0ba89b862..d2f643051 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue350/Issue350Tests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue350/Issue350Tests.java @@ -22,6 +22,7 @@ import java.util.concurrent.ExecutionException; import brave.Tracing; import brave.sampler.Sampler; import feign.Logger; +import org.junit.Before; import zipkin2.Span; import zipkin2.reporter.Reporter; import org.junit.Test; @@ -59,6 +60,11 @@ public class Issue350Tests { @Autowired Tracing tracer; @Autowired ArrayListSpanReporter reporter; + @Before + public void setup() { + this.reporter.clear(); + } + @Test public void should_successfully_work_without_hystrix() { this.template.getForEntity("http://localhost:9988/sleuth/test-not-ok", String.class); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/servererrors/FeignClientServerErrorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/servererrors/FeignClientServerErrorTests.java index d4f61bd68..c4fcafd63 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/servererrors/FeignClientServerErrorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/servererrors/FeignClientServerErrorTests.java @@ -149,12 +149,13 @@ public class FeignClientServerErrorTests { Awaitility.await().untilAsserted(() -> { List spans = this.reporter.getSpans(); log.info("Spans " + spans); - then(spans.size()).isGreaterThanOrEqualTo(2); + then(spans.size()).isEqualTo(1); Optional httpSpan = spans.stream() .filter(span -> span.tags().containsKey("http.method")).findFirst(); then(httpSpan.isPresent()).isTrue(); then(httpSpan.get().tags()) - .containsEntry("http.method", "GET"); + .containsEntry("http.method", "GET") + .doesNotContainEntry("http.url", "http://fooservice/ok"); }); } @@ -171,7 +172,7 @@ public class FeignClientServerErrorTests { Awaitility.await().untilAsserted(() -> { List spans = this.reporter.getSpans(); log.info("Spans " + spans); - then(spans.size()).isGreaterThanOrEqualTo(2); + then(spans.size()).isGreaterThanOrEqualTo(1); Optional httpSpan = spans.stream() .filter(span -> span.tags().containsKey("http.method")).findFirst(); then(httpSpan.isPresent()).isTrue();