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

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;
 	}
 }

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;
 	}
 
 }

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<String, Collection<String>> 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<String, Collection<String>> 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<String, Collection<String>> headers) {
 		String method = request.method();
 		String url = request.url();

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<zipkin2.Span> spans = this.reporter.getSpans();
-		then(spans).hasSize(2);
 		then(spans.stream()
 				.filter(span1 -> span1.kind() == zipkin2.Span.Kind.CLIENT)
 				.findFirst()

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);

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<Span> spans = this.reporter.getSpans();
 			log.info("Spans " + spans);
-			then(spans.size()).isGreaterThanOrEqualTo(2);
+			then(spans.size()).isEqualTo(1);
 			Optional<Span> 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<Span> spans = this.reporter.getSpans();
 			log.info("Spans " + spans);
-			then(spans.size()).isGreaterThanOrEqualTo(2);
+			then(spans.size()).isGreaterThanOrEqualTo(1);
 			Optional<Span> httpSpan = spans.stream()
 					.filter(span -> span.tags().containsKey("http.method")).findFirst();
 			then(httpSpan.isPresent()).isTrue();