diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java index deece503a..36b09f289 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignBeanPostProcessor.java @@ -17,18 +17,10 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import org.springframework.beans.BeansException; -import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.config.BeanPostProcessor; -import org.springframework.cloud.sleuth.Tracer; - -import feign.Client; -import feign.Retryer; -import feign.codec.Decoder; -import feign.codec.ErrorDecoder; /** - * Post processor that wraps Feign related classes {@link Decoder}, - * {@link Retryer} + * Post processor that wraps Feign related classes in their tracing representations. * * @author Marcin Grzejszczak * @@ -36,33 +28,16 @@ import feign.codec.ErrorDecoder; */ final class FeignBeanPostProcessor implements BeanPostProcessor { - private Tracer tracer; - private final BeanFactory beanFactory; + private final TraceFeignObjectWrapper traceFeignObjectWrapper; - FeignBeanPostProcessor(BeanFactory beanFactory) { - this.beanFactory = beanFactory; + FeignBeanPostProcessor(TraceFeignObjectWrapper traceFeignObjectWrapper) { + this.traceFeignObjectWrapper = traceFeignObjectWrapper; } @Override public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException { - if (bean instanceof Decoder && !(bean instanceof TraceFeignDecoder)) { - return new TraceFeignDecoder(getTracer(), (Decoder) bean); - } else if (bean instanceof Retryer && !(bean instanceof TraceFeignRetryer)) { - return new TraceFeignRetryer(getTracer(), (Retryer) bean); - } else if (bean instanceof Client && !(bean instanceof TraceFeignClient)) { - return new TraceFeignClient(getTracer(), (Client) bean); - } else if (bean instanceof ErrorDecoder && !(bean instanceof TraceFeignErrorDecoder)) { - return new TraceFeignErrorDecoder(getTracer(), (ErrorDecoder) bean); - } - return bean; - } - - private Tracer getTracer() { - if (this.tracer==null) { - this.tracer = this.beanFactory.getBean(Tracer.class); - } - return this.tracer; + return this.traceFeignObjectWrapper.wrap(bean); } @Override 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 8ad85a3aa..0512303a7 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,9 +16,14 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; -import java.io.IOException; -import java.lang.reflect.Type; - +import com.netflix.hystrix.HystrixCommand; +import feign.Client; +import feign.Feign; +import feign.FeignException; +import feign.RequestInterceptor; +import feign.RequestTemplate; +import feign.Response; +import feign.codec.Decoder; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.ObjectFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -29,6 +34,8 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.web.HttpMessageConverters; import org.springframework.cloud.netflix.feign.FeignAutoConfiguration; +import org.springframework.cloud.netflix.feign.FeignClientSpecification; +import org.springframework.cloud.netflix.feign.FeignContext; import org.springframework.cloud.netflix.feign.support.ResponseEntityDecoder; import org.springframework.cloud.netflix.feign.support.SpringDecoder; import org.springframework.cloud.sleuth.SpanInjector; @@ -40,15 +47,10 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Primary; import org.springframework.context.annotation.Scope; -import com.netflix.hystrix.HystrixCommand; - -import feign.Client; -import feign.Feign; -import feign.FeignException; -import feign.RequestInterceptor; -import feign.RequestTemplate; -import feign.Response; -import feign.codec.Decoder; +import java.io.IOException; +import java.lang.reflect.Type; +import java.util.ArrayList; +import java.util.List; /** * {@link org.springframework.boot.autoconfigure.EnableAutoConfiguration @@ -81,12 +83,17 @@ public class TraceFeignClientAutoConfiguration { @Bean @ConditionalOnProperty(name = "spring.sleuth.feign.processor.enabled", matchIfMissing = true) - FeignBeanPostProcessor feignBeanPostProcessor(BeanFactory beanFactory) { - return new FeignBeanPostProcessor(beanFactory); + FeignBeanPostProcessor feignBeanPostProcessor(TraceFeignObjectWrapper traceFeignObjectWrapper) { + return new FeignBeanPostProcessor(traceFeignObjectWrapper); } } + @Bean + TraceFeignObjectWrapper traceFeignObjectWrapper(BeanFactory beanFactory) { + return new TraceFeignObjectWrapper(beanFactory); + } + @Bean @Primary Decoder feignDecoder(final Tracer tracer) { @@ -120,6 +127,17 @@ public class TraceFeignClientAutoConfiguration { return new TraceFeignRequestInterceptor(tracer, feignRequestTemplateInjector()); } + @Autowired(required = false) + private List configurations = new ArrayList<>(); + + @Bean + @Primary + FeignContext sleuthFeignContext(TraceFeignObjectWrapper traceFeignObjectWrapper) { + FeignContext feignContext = new TraceFeignContext(traceFeignObjectWrapper); + feignContext.setConfigurations(this.configurations); + return feignContext; + } + private SpanInjector feignRequestTemplateInjector() { return new FeignRequestTemplateInjector(); } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignContext.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignContext.java new file mode 100644 index 000000000..61a6d8d1a --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignContext.java @@ -0,0 +1,39 @@ +package org.springframework.cloud.sleuth.instrument.web.client.feign; + +import org.springframework.cloud.netflix.feign.FeignContext; + +import java.util.HashMap; +import java.util.Map; + +/** + * Custom FeignContext that wraps beans in custom Feign configurations in their + * tracing representations. + * + * @author Marcin Grzejszczak + * @since 1.0.1 + */ +public class TraceFeignContext extends FeignContext { + + private final TraceFeignObjectWrapper traceFeignObjectWrapper; + + public TraceFeignContext(TraceFeignObjectWrapper traceFeignObjectWrapper) { + this.traceFeignObjectWrapper = traceFeignObjectWrapper; + } + + @Override + public T getInstance(String name, Class type) { + T object = super.getInstance(name, type); + return (T) this.traceFeignObjectWrapper.wrap(object); + } + + @Override + public Map getInstances(String name, Class type) { + Map instances = super.getInstances(name, type); + Map convertedInstances = new HashMap<>(); + for (Map.Entry entry : instances.entrySet()) { + convertedInstances.put(entry.getKey(), (T) this.traceFeignObjectWrapper.wrap(entry.getValue())); + } + return convertedInstances; + } + +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java new file mode 100644 index 000000000..873af78af --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java @@ -0,0 +1,44 @@ +package org.springframework.cloud.sleuth.instrument.web.client.feign; + +import feign.Client; +import feign.Retryer; +import feign.codec.Decoder; +import feign.codec.ErrorDecoder; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.sleuth.Tracer; + +/** + * Class that wraps Feign related classes into their Trace representative + * + * @author Marcin Grzejszczak + * @since 1.0.1 + */ +final class TraceFeignObjectWrapper { + + private final BeanFactory beanFactory; + private Tracer tracer; + + TraceFeignObjectWrapper(BeanFactory beanFactory) { + this.beanFactory = beanFactory; + } + + Object wrap(Object bean) { + if (bean instanceof Decoder && !(bean instanceof TraceFeignDecoder)) { + return new TraceFeignDecoder(getTracer(), (Decoder) bean); + } else if (bean instanceof Retryer && !(bean instanceof TraceFeignRetryer)) { + return new TraceFeignRetryer(getTracer(), (Retryer) bean); + } else if (bean instanceof Client && !(bean instanceof TraceFeignClient)) { + return new TraceFeignClient(getTracer(), (Client) bean); + } else if (bean instanceof ErrorDecoder && !(bean instanceof TraceFeignErrorDecoder)) { + return new TraceFeignErrorDecoder(getTracer(), (ErrorDecoder) bean); + } + return bean; + } + + private Tracer getTracer() { + if (this.tracer==null) { + this.tracer = this.beanFactory.getBean(Tracer.class); + } + return this.tracer; + } +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java index 0ebb47a6d..8fd9da7d5 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java @@ -16,10 +16,12 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; - +import com.netflix.hystrix.exception.HystrixRuntimeException; +import com.netflix.loadbalancer.BaseLoadBalancer; +import com.netflix.loadbalancer.ILoadBalancer; +import com.netflix.loadbalancer.Server; +import feign.codec.Decoder; +import feign.codec.ErrorDecoder; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -34,6 +36,7 @@ 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.SpanReporter; import org.springframework.cloud.sleuth.Tracer; @@ -50,10 +53,9 @@ import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.RestTemplate; -import com.netflix.hystrix.exception.HystrixRuntimeException; -import com.netflix.loadbalancer.BaseLoadBalancer; -import com.netflix.loadbalancer.ILoadBalancer; -import com.netflix.loadbalancer.Server; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import static org.assertj.core.api.BDDAssertions.then; @@ -62,23 +64,26 @@ import static org.assertj.core.api.BDDAssertions.then; * * @author ryarabori */ -@RunWith(SpringJUnit4ClassRunner.class) @SpringApplicationConfiguration( - classes = { FeignClientServerErrorTests.TestConfiguration.class }) -@WebIntegrationTest(value = { "spring.application.name=fooservice" }, randomPort = true) +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration( + classes = {FeignClientServerErrorTests.TestConfiguration.class}) +@WebIntegrationTest(value = {"spring.application.name=fooservice"}, randomPort = true) public class FeignClientServerErrorTests { @Autowired TestFeignInterface feignInterface; + @Autowired TestFeignWithCustomConfInterface customConfFeignInterface; @Rule public OutputCapture capture = new OutputCapture(); - @Before public void setup() { + @Before + public void setup() { ExceptionUtils.setFail(true); } - @Test public void shouldCloseSpanOnInternalServerError() throws InterruptedException { + @Test + public void shouldCloseSpanOnInternalServerError() throws InterruptedException { try { this.feignInterface.internalError(); - } - catch (HystrixRuntimeException e) { + } catch (HystrixRuntimeException e) { } // ugly :/ waiting for rx thread to complete @@ -87,11 +92,11 @@ public class FeignClientServerErrorTests { .doesNotContain("Tried to close span but it is not the current span"); } - @Test public void shouldCloseSpanOnNotFound() throws InterruptedException { + @Test + public void shouldCloseSpanOnNotFound() throws InterruptedException { try { this.feignInterface.notFound(); - } - catch (HystrixRuntimeException e) { + } catch (HystrixRuntimeException e) { } // ugly :/ waiting for rx thread to complete @@ -100,26 +105,74 @@ public class FeignClientServerErrorTests { .doesNotContain("Tried to close span but it is not the current span"); } - @Configuration @EnableAutoConfiguration @EnableFeignClients - @RibbonClient(value = "fooservice", - configuration = SimpleRibbonClientConfiguration.class) + @Test + public void shouldCloseSpanOnOk() throws InterruptedException { + try { + this.feignInterface.ok(); + } catch (HystrixRuntimeException e) { + } + + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()) + .doesNotContain("Tried to close span but it is not the current span"); + } + + @Test + public void shouldCloseSpanOnOkWithCustomFeignConfiguration() throws InterruptedException { + try { + this.customConfFeignInterface.ok(); + } catch (HystrixRuntimeException e) { + } + + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()) + .doesNotContain("Tried to close span but it is not the current span"); + } + + @Test + public void shouldCloseSpanOnNotFoundWithCustomFeignConfiguration() throws InterruptedException { + try { + this.customConfFeignInterface.notFound(); + } catch (HystrixRuntimeException e) { + } + + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()) + .doesNotContain("Tried to close span but it is not the current span"); + } + + @Configuration + @EnableAutoConfiguration + @EnableFeignClients + @RibbonClients({@RibbonClient(value = "fooservice", + configuration = SimpleRibbonClientConfiguration.class), + @RibbonClient(value = "customConfFooService", + configuration = SimpleRibbonClientConfiguration.class)}) public static class TestConfiguration { - @Bean FooController fooController() { + @Bean + FooController fooController() { return new FooController(); } - @Bean Listener listener() { + @Bean + Listener listener() { return new Listener(); } - @LoadBalanced @Bean public RestTemplate restTemplate() { + @LoadBalanced + @Bean + public RestTemplate restTemplate() { return new RestTemplate(); } } - @FeignClient(value = "fooservice") public interface TestFeignInterface { + @FeignClient(value = "fooservice") + public interface TestFeignInterface { @RequestMapping(method = RequestMethod.GET, value = "/internalerror") ResponseEntity internalError(); @@ -127,25 +180,56 @@ public class FeignClientServerErrorTests { @RequestMapping(method = RequestMethod.GET, value = "/notfound") ResponseEntity notFound(); + @RequestMapping(method = RequestMethod.GET, value = "/ok") + ResponseEntity ok(); } - @Component public static class Listener implements SpanReporter { + @FeignClient(value = "customConfFooService", configuration = CustomFeignClientConfiguration.class) + public interface TestFeignWithCustomConfInterface { + + @RequestMapping(method = RequestMethod.GET, value = "/notfound") + ResponseEntity notFound(); + + @RequestMapping(method = RequestMethod.GET, value = "/ok") + ResponseEntity ok(); + } + + + @Configuration + public static class CustomFeignClientConfiguration { + @Bean + Decoder decoder() { + return new Decoder.Default(); + } + + @Bean + ErrorDecoder errorDecoder() { + return new ErrorDecoder.Default(); + } + } + + @Component + public static class Listener implements SpanReporter { private List events = new ArrayList<>(); public List getEvents() { return this.events; } - @Override public void report(Span span) { + @Override + public void report(Span span) { this.events.add(span); } } - @RestController public static class FooController { + @RestController + public static class FooController { - @Autowired Tracer tracer; + @Autowired + Tracer tracer; - @RequestMapping("/internalerror") public ResponseEntity internalError( + @RequestMapping("/internalerror") + public ResponseEntity internalError( @RequestHeader(Span.TRACE_ID_NAME) String traceId, @RequestHeader(Span.SPAN_ID_NAME) String spanId, @RequestHeader(Span.PARENT_ID_NAME) String parentId) { @@ -153,19 +237,31 @@ public class FeignClientServerErrorTests { HttpStatus.INTERNAL_SERVER_ERROR); } - @RequestMapping("/notfound") public ResponseEntity notFound( + @RequestMapping("/notfound") + public ResponseEntity notFound( @RequestHeader(Span.TRACE_ID_NAME) String traceId, @RequestHeader(Span.SPAN_ID_NAME) String spanId, @RequestHeader(Span.PARENT_ID_NAME) String parentId) { return new ResponseEntity<>("not found", HttpStatus.NOT_FOUND); } + + @RequestMapping("/ok") + public ResponseEntity ok( + @RequestHeader(Span.TRACE_ID_NAME) String traceId, + @RequestHeader(Span.SPAN_ID_NAME) String spanId, + @RequestHeader(Span.PARENT_ID_NAME) String parentId) { + return new ResponseEntity<>("ok", HttpStatus.OK); + } } - @Configuration public static class SimpleRibbonClientConfiguration { + @Configuration + public static class SimpleRibbonClientConfiguration { - @Value("${local.server.port}") private int port = 0; + @Value("${local.server.port}") + private int port = 0; - @Bean public ILoadBalancer ribbonLoadBalancer() { + @Bean + public ILoadBalancer ribbonLoadBalancer() { BaseLoadBalancer balancer = new BaseLoadBalancer(); balancer.setServersList( Collections.singletonList(new Server("localhost", this.port))); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapperTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapperTests.java new file mode 100644 index 000000000..383bf71f3 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapperTests.java @@ -0,0 +1,60 @@ +package org.springframework.cloud.sleuth.instrument.web.client.feign; + +import feign.Client; +import feign.Retryer; +import feign.codec.Decoder; +import feign.codec.ErrorDecoder; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.sleuth.Tracer; + +import static org.assertj.core.api.BDDAssertions.then; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; + +/** + * @author Marcin Grzejszczak + */ +@RunWith(MockitoJUnitRunner.class) +public class TraceFeignObjectWrapperTests { + + @Mock Tracer tracer; + @Mock BeanFactory beanFactory; + @InjectMocks TraceFeignObjectWrapper traceFeignObjectWrapper; + + @Before + public void setup() { + given(this.beanFactory.getBean(Tracer.class)).willReturn(this.tracer); + } + + @Test + public void should_wrap_a_decoder_into_trace_decoder() throws Exception { + then(this.traceFeignObjectWrapper.wrap(mock(Decoder.class))).isExactlyInstanceOf(TraceFeignDecoder.class); + } + + @Test + public void should_wrap_a_retryer_into_trace_retryer() throws Exception { + then(this.traceFeignObjectWrapper.wrap(mock(Retryer.class))).isExactlyInstanceOf(TraceFeignRetryer.class); + } + + @Test + public void should_wrap_a_client_into_trace_client() throws Exception { + then(this.traceFeignObjectWrapper.wrap(mock(Client.class))).isExactlyInstanceOf(TraceFeignClient.class); + } + + @Test + public void should_wrap_a_error_decoder_into_trace_error_decoder() throws Exception { + then(this.traceFeignObjectWrapper.wrap(mock(ErrorDecoder.class))).isExactlyInstanceOf(TraceFeignErrorDecoder.class); + } + + @Test + public void should_not_wrap_a_bean_that_is_not_feign_related() throws Exception { + String notFeignRelatedObject = "object"; + then(this.traceFeignObjectWrapper.wrap(notFeignRelatedObject)).isSameAs(notFeignRelatedObject); + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/src/test/resources/logback.xml b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/src/test/resources/logback.xml index 3cee99001..3154e6af4 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/src/test/resources/logback.xml +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin-stream/src/test/resources/logback.xml @@ -1,9 +1,9 @@ - - - - - - + + + + + + \ No newline at end of file