Unified the way RestTemplate and Feign work

* both always start a new span (RT started a span only if there was previous tracing)
* both DO NOT return in the response PARENT-ID (Fiegn didn't do that, neither does TraceFilter)

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/AbstractTraceHttpRequestInterceptor.java

@@ -66,10 +66,6 @@ abstract class AbstractTraceHttpRequestInterceptor
 		return !span.getParents().isEmpty() ? span.getParents().get(0) : null;
 	}
 
-	protected void doNotSampleThisSpan(HttpRequest request) {
-		setHeader(request, Span.NOT_SAMPLED_NAME, "true");
-	}
-
 	private void setHeader(HttpRequest request, String name, String value) {
 		if (StringUtils.hasText(value) && !request.getHeaders().containsKey(name) &&
 				this.tracer.isTracing()) {

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceAsyncClientHttpRequestFactoryWrapper.java

@@ -96,10 +96,6 @@ public class TraceAsyncClientHttpRequestFactoryWrapper extends AbstractTraceHttp
 			throws IOException {
 		AsyncClientHttpRequest request = this.asyncDelegate
 				.createAsyncRequest(uri, httpMethod);
-		if (!isTracing()) {
-			doNotSampleThisSpan(request);
-			return request;
-		}
 		publishStartEvent(request);
 		return request;
 	}
@@ -108,10 +104,6 @@ public class TraceAsyncClientHttpRequestFactoryWrapper extends AbstractTraceHttp
 	public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod)
 			throws IOException {
 		ClientHttpRequest request = this.syncDelegate.createRequest(uri, httpMethod);
-		if (!isTracing()) {
-			doNotSampleThisSpan(request);
-			return request;
-		}
 		publishStartEvent(request);
 		return request;
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptor.java

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.springframework.cloud.sleuth.instrument.web.client;
 
 import java.io.IOException;
@@ -44,10 +45,6 @@ public class TraceRestTemplateInterceptor extends AbstractTraceHttpRequestInterc
 	@Override
 	public ClientHttpResponse intercept(HttpRequest request, byte[] body,
 			ClientHttpRequestExecution execution) throws IOException {
-		if (!isTracing()) {
-			doNotSampleThisSpan(request);
-			return execution.execute(request, body);
-		}
 		publishStartEvent(request);
 		return response(request, body, execution);
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java

@@ -16,8 +16,6 @@
 
 package org.springframework.cloud.sleuth.instrument.web.client.feign;
 
-import static java.util.Collections.singletonList;
-
 import java.io.IOException;
 import java.lang.reflect.Type;
 import java.util.Collection;
@@ -35,8 +33,8 @@ import org.springframework.cloud.netflix.feign.FeignAutoConfiguration;
 import org.springframework.cloud.netflix.feign.support.ResponseEntityDecoder;
 import org.springframework.cloud.netflix.feign.support.SpringDecoder;
 import org.springframework.cloud.sleuth.Span;
-import org.springframework.cloud.sleuth.Tracer;
 import org.springframework.cloud.sleuth.TraceKeys;
+import org.springframework.cloud.sleuth.Tracer;
 import org.springframework.cloud.sleuth.instrument.hystrix.SleuthHystrixAutoConfiguration;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.context.annotation.Bean;
@@ -54,6 +52,8 @@ import feign.RequestInterceptor;
 import feign.Response;
 import feign.codec.Decoder;
 
+import static java.util.Collections.singletonList;
+
 /**
  * {@link org.springframework.boot.autoconfigure.EnableAutoConfiguration Auto-configuration}
  * enables span information propagation when using Feign.
@@ -128,14 +128,9 @@ public class TraceFeignClientAutoConfiguration {
 		}
 		setHeader(newHeaders, Span.TRACE_ID_NAME, span.getTraceId());
 		setHeader(newHeaders, Span.SPAN_ID_NAME, span.getSpanId());
-		setHeader(newHeaders, Span.PARENT_ID_NAME, getParentId(span));
 		return newHeaders;
 	}
 
-	private Long getParentId(Span span) {
-		return !span.getParents().isEmpty() ? span.getParents().get(0) : null;
-	}
-
 	public void setHeader(Map<String, Collection<String>> headers, String name,
 			String value) {
 		if (StringUtils.hasText(value) && !headers.containsKey(name)

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2013-2016 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -41,7 +41,6 @@ import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RestController;
 import org.springframework.web.client.RestTemplate;
 
-import static org.junit.Assert.assertFalse;
 import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
 
 /**
@@ -77,6 +76,16 @@ public class TraceRestTemplateInterceptorTests {
 		TestSpanContextHolder.removeCurrentSpan();
 	}
 
+	@Test
+	public void headersAddedWhenNoTracingWasPresent() {
+		@SuppressWarnings("unchecked")
+		Map<String, String> headers = this.template.getForEntity("/", Map.class)
+				.getBody();
+
+		then(Span.hexToId(headers.get(Span.TRACE_ID_NAME))).isNotNull();
+		then(Span.hexToId(headers.get(Span.SPAN_ID_NAME))).isNotNull();
+	}
+
 	@Test
 	public void headersAddedWhenTracing() {
 		this.tracer.continueSpan(Span.builder().traceId(1L).spanId(2L).parent(3L).build());
@@ -99,14 +108,6 @@ public class TraceRestTemplateInterceptorTests {
 		then(headers.get(Span.NOT_SAMPLED_NAME)).isEqualTo("true");
 	}
 
-	@Test
-	public void headersNotAddedWhenNotTracing() {
-		@SuppressWarnings("unchecked")
-		Map<String, String> headers = this.template.getForEntity("/", Map.class)
-				.getBody();
-		assertFalse("Wrong headers: " + headers, headers.containsKey(Span.SPAN_ID_NAME));
-	}
-
 	// issue #198
 	@Test
 	public void spanRemovedFromThreadUponException() {

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java

@@ -14,16 +14,20 @@
  * limitations under the License.
  */
 
-package org.springframework.cloud.sleuth.instrument.web.client.feign;
+package org.springframework.cloud.sleuth.instrument.web.client;
 
+import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
 
 import org.junit.After;
+import org.junit.Assert;
+import org.junit.ClassRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -31,11 +35,13 @@ import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
 import org.springframework.boot.test.SpringApplicationConfiguration;
 import org.springframework.boot.test.WebIntegrationTest;
+import org.springframework.cloud.client.loadbalancer.LoadBalanced;
 import org.springframework.cloud.netflix.feign.EnableFeignClients;
 import org.springframework.cloud.netflix.feign.FeignClient;
-import org.springframework.cloud.netflix.ribbon.RibbonClient;
+import org.springframework.cloud.netflix.ribbon.RibbonClients;
 import org.springframework.cloud.sleuth.Span;
 import org.springframework.cloud.sleuth.Tracer;
+import org.springframework.cloud.sleuth.assertions.SleuthAssertions;
 import org.springframework.cloud.sleuth.event.ClientReceivedEvent;
 import org.springframework.cloud.sleuth.event.ClientSentEvent;
 import org.springframework.cloud.sleuth.trace.TestSpanContextHolder;
@@ -46,33 +52,37 @@ import org.springframework.context.event.EventListener;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Component;
-import org.springframework.test.annotation.DirtiesContext;
-import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+import org.springframework.test.context.junit4.rules.SpringClassRule;
+import org.springframework.test.context.junit4.rules.SpringMethodRule;
 import org.springframework.web.bind.annotation.RequestHeader;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestMethod;
 import org.springframework.web.bind.annotation.RestController;
+import org.springframework.web.client.RestTemplate;
 
 import com.netflix.loadbalancer.BaseLoadBalancer;
 import com.netflix.loadbalancer.ILoadBalancer;
 import com.netflix.loadbalancer.Server;
 
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+
+import static junitparams.JUnitParamsRunner.$;
 import static org.assertj.core.api.BDDAssertions.then;
 
-@RunWith(SpringJUnit4ClassRunner.class)
-@SpringApplicationConfiguration(classes = { FeignTraceTests.TestConfiguration.class })
+@RunWith(JUnitParamsRunner.class)
+@SpringApplicationConfiguration(classes = { WebClientTests.TestConfiguration.class })
 @WebIntegrationTest(value = { "spring.application.name=fooservice" }, randomPort = true)
-@DirtiesContext
-public class FeignTraceTests {
+public class WebClientTests {
 
-	@Autowired
-	TestFeignInterface testFeignInterface;
-
-	@Autowired
-	Listener listener;
-
-	@Autowired
-	Tracer tracer;
+	@ClassRule public static final SpringClassRule SCR = new SpringClassRule();
+	@Rule public final SpringMethodRule springMethodRule = new SpringMethodRule();
+	
+	@Autowired TestFeignInterface testFeignInterface;
+	@Autowired TestFeignInterfaceWithException testFeignInterfaceWithException;
+	@Autowired @LoadBalanced RestTemplate template;
+	@Autowired Listener listener;
+	@Autowired Tracer tracer;
 
 	@After
 	public void close() {
@@ -81,46 +91,68 @@ public class FeignTraceTests {
 	}
 
 	@Test
-	public void shouldCreateANewSpanWhenNoPreviousTracingWasPresent() {
-		ResponseEntity<String> response = this.testFeignInterface.getNoTrace();
+	@Parameters
+	@SuppressWarnings("unchecked")
+	public void shouldCreateANewSpanWhenNoPreviousTracingWasPresent(ResponseEntityProvider provider) {
+		ResponseEntity<String> response = provider.get(this);
 
 		then(getHeader(response, Span.TRACE_ID_NAME)).isNotNull();
+		then(getHeader(response, Span.SPAN_ID_NAME)).isNotNull();
 		then(this.listener.getEvents()).isNotEmpty();
 	}
 
+	private Object[] parametersForShouldCreateANewSpanWhenNoPreviousTracingWasPresent() {
+		return $((ResponseEntityProvider) (tests) -> tests.testFeignInterface.getNoTrace(),
+				(ResponseEntityProvider) (tests) -> tests.template.getForEntity("http://fooservice/notrace", String.class));
+	}
+
 	@Test
-	public void shouldPropagateNotSamplingHeader() {
+	@Parameters
+	@SuppressWarnings("unchecked")
+	public void shouldPropagateNotSamplingHeader(ResponseEntityProvider provider) {
 		Long currentTraceId = 1L;
 		Long currentParentId = 2L;
 		this.tracer.continueSpan(Span.builder().traceId(currentTraceId)
 				.spanId(generatedId()).exportable(false).parent(currentParentId).build());
 
-		ResponseEntity<Map<String, String>> response = this.testFeignInterface.headers();
+		ResponseEntity<Map<String, String>> response = provider.get(this);
 
 		then(response.getBody().get(Span.TRACE_ID_NAME)).isNotNull();
 		then(response.getBody().get(Span.NOT_SAMPLED_NAME)).isNotNull();
 		then(this.listener.getEvents()).isNotEmpty();
 	}
 
+	private Object[] parametersForShouldPropagateNotSamplingHeader() {
+		return $((ResponseEntityProvider) (tests) -> tests.testFeignInterface.headers(),
+				(ResponseEntityProvider) (tests) -> tests.template.getForEntity("http://fooservice/", Map.class));
+	}
+
 	@Test
-	public void shouldAttachTraceIdWhenUsingFeignClient() {
+	@Parameters
+	@SuppressWarnings("unchecked")
+	public void shouldAttachTraceIdWhenCallingAnotherService(ResponseEntityProvider provider) {
 		Long currentTraceId = 1L;
 		Long currentParentId = 2L;
 		Long currentSpanId = 100L;
 		this.tracer.continueSpan(Span.builder().traceId(currentTraceId)
 				.spanId(currentSpanId).parent(currentParentId).build());
 
-		ResponseEntity<String> response = this.testFeignInterface.getTraceId();
+		ResponseEntity<String> response = provider.get(this);
 
 		then(Span.hexToId(getHeader(response, Span.TRACE_ID_NAME)))
 				.isEqualTo(currentTraceId);
-		then(Span.hexToId(getHeader(response, Span.PARENT_ID_NAME)))
-				.isEqualTo(currentSpanId);
 		thenRegisteredClientSentAndReceivedEvents();
 	}
 
+	private Object[] parametersForShouldAttachTraceIdWhenCallingAnotherService() {
+		return $((ResponseEntityProvider) (tests) -> tests.testFeignInterface.headers(),
+				(ResponseEntityProvider) (tests) -> tests.template.getForEntity("http://fooservice/traceid", String.class));
+	}
+
 	@Test
-	public void shouldAttachTraceIdWhenUsingFeignClientWithoutResponseBody() {
+	@Parameters
+	@SuppressWarnings("unchecked")
+	public void shouldAttachTraceIdWhenUsingFeignClientWithoutResponseBody(ResponseEntityProvider provider) {
 		Long currentTraceId = 1L;
 		Long currentParentId = 2L;
 		Long currentSpanId = generatedId();
@@ -128,12 +160,40 @@ public class FeignTraceTests {
 				.spanId(currentSpanId).parent(currentParentId).build();
 		this.tracer.continueSpan(span);
 
-		this.testFeignInterface.noResponseBody();
+		provider.get(this);
 
 		thenRegisteredClientSentAndReceivedEvents();
 		then(this.tracer.getCurrentSpan()).isEqualTo(span);
 	}
 
+	private Object[] parametersForShouldAttachTraceIdWhenUsingFeignClientWithoutResponseBody() {
+		return $((ResponseEntityProvider) (tests) -> tests.testFeignInterface.noResponseBody(),
+				(ResponseEntityProvider) (tests) -> tests.template.getForEntity("http://fooservice/noresponse", String.class));
+	}
+
+	// issue #198
+	@Test
+	@Parameters
+	@SuppressWarnings("unchecked")
+	public void shouldCloseSpanUponException(ResponseEntityProvider provider) throws IOException {
+		Span span = this.tracer.createSpan("new trace");
+
+		try {
+			provider.get(this);
+			Assert.fail("should throw an exception");
+		} catch (RuntimeException e) {
+			SleuthAssertions.then(e).hasRootCauseInstanceOf(IOException.class);
+		}
+
+		SleuthAssertions.then(this.tracer.getCurrentSpan()).isEqualTo(span);
+		this.tracer.close(span);
+	}
+
+	private Object[] parametersForShouldCloseSpanUponException() {
+		return $((ResponseEntityProvider) (tests) -> tests.testFeignInterfaceWithException.shouldFailToConnect(),
+				(ResponseEntityProvider) (tests) -> tests.template.getForEntity("http://exceptionService/", Map.class));
+	}
+
 	private void thenRegisteredClientSentAndReceivedEvents() {
 		then(this.listener.getEvents().size()).isEqualTo(2);
 		then(this.listener.getEvents().get(0)).isExactlyInstanceOf(ClientSentEvent.class);
@@ -161,13 +221,19 @@ public class FeignTraceTests {
 		ResponseEntity<Map<String, String>> headers();
 
 		@RequestMapping(method = RequestMethod.GET, value = "/noresponse")
-		void noResponseBody();
+		ResponseEntity<Void> noResponseBody();
+	}
+
+	@FeignClient(name = "exceptionService", url = "http://invalid.host.to.break.tests")
+	public interface TestFeignInterfaceWithException {
+		@RequestMapping(method = RequestMethod.GET, value = "/")
+		ResponseEntity<String> shouldFailToConnect();
 	}
 
 	@Configuration
 	@EnableAutoConfiguration
 	@EnableFeignClients
-	@RibbonClient(name = "fooservice", configuration = SimpleRibbonClientConfiguration.class)
+	@RibbonClients(defaultConfiguration = SimpleRibbonClientConfiguration.class)
 	public static class TestConfiguration {
 
 		@Bean
@@ -249,9 +315,15 @@ public class FeignTraceTests {
 		@Bean
 		public ILoadBalancer ribbonLoadBalancer() {
 			BaseLoadBalancer balancer = new BaseLoadBalancer();
-			balancer.setServersList(Arrays.asList(new Server("localhost", this.port)));
+			balancer.setServersList(
+					Collections.singletonList(new Server("localhost", this.port)));
 			return balancer;
 		}
 
 	}
+
+	@FunctionalInterface
+	interface ResponseEntityProvider {
+		ResponseEntity get(WebClientTests webClientTests);
+	}
 }

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceExceptionTests.java

@@ -1,98 +0,0 @@
-/*
- * Copyright 2013-2016 the original author or authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.cloud.sleuth.instrument.web.client.feign;
-
-import java.io.IOException;
-import java.net.UnknownHostException;
-
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
-import org.springframework.boot.test.SpringApplicationConfiguration;
-import org.springframework.cloud.netflix.feign.EnableFeignClients;
-import org.springframework.cloud.netflix.feign.FeignClient;
-import org.springframework.cloud.sleuth.Span;
-import org.springframework.cloud.sleuth.Tracer;
-import org.springframework.cloud.sleuth.trace.TestSpanContextHolder;
-import org.springframework.cloud.sleuth.util.ExceptionUtils;
-import org.springframework.context.annotation.Configuration;
-import org.springframework.test.annotation.DirtiesContext;
-import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
-import org.springframework.web.bind.annotation.RequestMapping;
-import org.springframework.web.bind.annotation.RequestMethod;
-
-import com.netflix.config.ConfigurationManager;
-import com.netflix.hystrix.HystrixCommandProperties;
-
-import static org.assertj.core.api.BDDAssertions.then;
-import static org.junit.Assert.fail;
-
-@RunWith(SpringJUnit4ClassRunner.class)
-@SpringApplicationConfiguration(classes = { FeignTraceExceptionTests.TestConfiguration.class })
-@DirtiesContext
-public class FeignTraceExceptionTests {
-
-	@Autowired
-	TestFeignInterfaceWithException testFeignInterfaceWithException;
-
-	@Autowired
-	Tracer tracer;
-
-	@Before
-	public void before() {
-		ExceptionUtils.setFail(true);
-	}
-
-	@After
-	public void close() {
-		TestSpanContextHolder.removeCurrentSpan();
-	}
-
-	@Test
-	public void shouldRemoveSpanFromThreadUponConnectionException() throws IOException {
-		Span span = this.tracer.createSpan("new trace");
-		ConfigurationManager
-				.getConfigInstance().setProperty("hystrix.command.shouldFailToConnect.execution.isolation.strategy",
-				HystrixCommandProperties.ExecutionIsolationStrategy.SEMAPHORE);
-
-		try {
-			this.testFeignInterfaceWithException.shouldFailToConnect();
-			fail("should throw an exception");
-		} catch (Exception e) {
-			then(e).hasRootCauseInstanceOf(UnknownHostException.class);
-		}
-
-		then(this.tracer.getCurrentSpan()).isEqualTo(span);
-		this.tracer.close(span);
-	}
-
-	@FeignClient(name = "exceptionService", url = "http://invalid.host.to.break.tests")
-	public interface TestFeignInterfaceWithException {
-		@RequestMapping(method = RequestMethod.GET, value = "/")
-		String shouldFailToConnect();
-	}
-
-	@Configuration
-	@EnableAutoConfiguration
-	@EnableFeignClients
-	public static class TestConfiguration {
-
-	}
-}

spring-cloud-sleuth-core/src/test/resources/application.yml

@@ -1,4 +1,10 @@
 hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds: 5000
 ribbon:
   ConnectTimeout: 3000
-  ReadTimeout: 5000
+  ReadTimeout: 5000
+
+exceptionService.ribbon:
+  MaxAutoRetries: 3
+  OkToRetryOnAllOperations: true
+  ConnectTimeout: 1
+  ReadTimeout: 1