diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/AbstractTraceHttpRequestInterceptor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/AbstractTraceHttpRequestInterceptor.java index a75259149..8d9ac8c49 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/AbstractTraceHttpRequestInterceptor.java +++ b/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()) { diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceAsyncClientHttpRequestFactoryWrapper.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceAsyncClientHttpRequestFactoryWrapper.java index c78fa8838..9366d8942 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceAsyncClientHttpRequestFactoryWrapper.java +++ b/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; } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptor.java index 3ab5e54f1..7d38edbd3 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptor.java +++ b/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); } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java index 760e5bd26..1aa92c341 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java +++ b/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> headers, String name, String value) { if (StringUtils.hasText(value) && !headers.containsKey(name) diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptorTests.java index e45649572..ee1e6b202 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRestTemplateInterceptorTests.java +++ b/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 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 headers = this.template.getForEntity("/", Map.class) - .getBody(); - assertFalse("Wrong headers: " + headers, headers.containsKey(Span.SPAN_ID_NAME)); - } - // issue #198 @Test public void spanRemovedFromThreadUponException() { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java similarity index 61% rename from spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceTests.java rename to spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java index a46d72b70..0d01994d5 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceTests.java +++ b/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 response = this.testFeignInterface.getNoTrace(); + @Parameters + @SuppressWarnings("unchecked") + public void shouldCreateANewSpanWhenNoPreviousTracingWasPresent(ResponseEntityProvider provider) { + ResponseEntity 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> response = this.testFeignInterface.headers(); + ResponseEntity> 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 response = this.testFeignInterface.getTraceId(); + ResponseEntity 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> headers(); @RequestMapping(method = RequestMethod.GET, value = "/noresponse") - void noResponseBody(); + ResponseEntity noResponseBody(); + } + + @FeignClient(name = "exceptionService", url = "http://invalid.host.to.break.tests") + public interface TestFeignInterfaceWithException { + @RequestMapping(method = RequestMethod.GET, value = "/") + ResponseEntity 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); + } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceExceptionTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceExceptionTests.java deleted file mode 100644 index e1f4b85fe..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignTraceExceptionTests.java +++ /dev/null @@ -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 { - - } -} diff --git a/spring-cloud-sleuth-core/src/test/resources/application.yml b/spring-cloud-sleuth-core/src/test/resources/application.yml index 56c7b8d01..d1791c3df 100644 --- a/spring-cloud-sleuth-core/src/test/resources/application.yml +++ b/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 \ No newline at end of file + ReadTimeout: 5000 + +exceptionService.ribbon: + MaxAutoRetries: 3 + OkToRetryOnAllOperations: true + ConnectTimeout: 1 + ReadTimeout: 1 \ No newline at end of file