From 2df78bc7f1625974587463a64c7b98ab68e59afc Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 14 Jun 2018 15:32:12 +0200 Subject: [PATCH] Wrapping the Feign client only when using the LoadBalancerFeignClient without this change we wrapped in the handleSend and handleReceive both the method wrapping the execution of the TraceLoadBalancerFeignClient and its delegate too. That way we had 2 pairs of CS & CR. with this change we disable that functionality by wrapping only the delegate. The problem with such an approach was such that there were exceptions thrown before the delegate was executed. That's why we added a fallback mechanism in that case. fixes gh-1007 --- .../web/client/feign/LazyClient.java | 7 +- .../feign/TraceLoadBalancerFeignClient.java | 64 ++++++++++++++++++- .../web/client/feign/TracingFeignClient.java | 22 ++++--- .../WebClientDiscoveryExceptionTests.java | 1 - .../feign/issues/issue350/Issue350Tests.java | 6 ++ .../FeignClientServerErrorTests.java | 7 +- 6 files changed, 91 insertions(+), 16 deletions(-) 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();