From ea05343137eba4463146609ff6a877636eef1a55 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 8 Sep 2016 13:03:40 +0200 Subject: [PATCH] Fixed wrapping the wrapper --- spring-cloud-sleuth-core/pom.xml | 5 + .../client/feign/TraceFeignObjectWrapper.java | 30 ++++ .../feign/TraceLoadBalancerFeignClient.java | 28 ++++ .../feign/issues/issue393/Issue393Tests.java | 130 ++++++++++++++++++ .../src/test/resources/application.yml | 2 + 5 files changed, 195 insertions(+) create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue393/Issue393Tests.java diff --git a/spring-cloud-sleuth-core/pom.xml b/spring-cloud-sleuth-core/pom.xml index 2dcecf6d9..7d5e89f4c 100644 --- a/spring-cloud-sleuth-core/pom.xml +++ b/spring-cloud-sleuth-core/pom.xml @@ -151,6 +151,11 @@ h2 test + + org.springframework.cloud + spring-cloud-starter-eureka + test + 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 index dae2e6b06..5319f8514 100644 --- 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 @@ -1,6 +1,9 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.netflix.feign.ribbon.CachingSpringLoadBalancerFactory; +import org.springframework.cloud.netflix.feign.ribbon.LoadBalancerFeignClient; +import org.springframework.cloud.netflix.ribbon.SpringClientFactory; import feign.Client; @@ -14,14 +17,41 @@ final class TraceFeignObjectWrapper { private final BeanFactory beanFactory; + private CachingSpringLoadBalancerFactory cachingSpringLoadBalancerFactory; + private SpringClientFactory springClientFactory; + TraceFeignObjectWrapper(BeanFactory beanFactory) { this.beanFactory = beanFactory; } Object wrap(Object bean) { if (bean instanceof Client && !(bean instanceof TraceFeignClient)) { + if (bean instanceof LoadBalancerFeignClient && !(bean instanceof TraceLoadBalancerFeignClient)) { + LoadBalancerFeignClient client = ((LoadBalancerFeignClient) bean); + return new TraceLoadBalancerFeignClient( + client.getDelegate(), factory(), + clientFactory(), this.beanFactory); + } else if (bean instanceof TraceLoadBalancerFeignClient) { + return bean; + } return new TraceFeignClient(this.beanFactory, (Client) bean); } return bean; } + + private CachingSpringLoadBalancerFactory factory() { + if (this.cachingSpringLoadBalancerFactory == null) { + this.cachingSpringLoadBalancerFactory = this.beanFactory + .getBean(CachingSpringLoadBalancerFactory.class); + } + return this.cachingSpringLoadBalancerFactory; + } + + private SpringClientFactory clientFactory() { + if (this.springClientFactory == null) { + this.springClientFactory = this.beanFactory + .getBean(SpringClientFactory.class); + } + return this.springClientFactory; + } } 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 new file mode 100644 index 000000000..ec4ab9129 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceLoadBalancerFeignClient.java @@ -0,0 +1,28 @@ +package org.springframework.cloud.sleuth.instrument.web.client.feign; + +import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.netflix.feign.ribbon.CachingSpringLoadBalancerFactory; +import org.springframework.cloud.netflix.feign.ribbon.LoadBalancerFeignClient; +import org.springframework.cloud.netflix.ribbon.SpringClientFactory; + +import feign.Client; + +/** + * We need to wrap the {@link LoadBalancerFeignClient} into a trace representation + * due to casts in {@link org.springframework.cloud.netflix.feign.FeignClientFactoryBean}. + * + * @author Marcin Grzejszczak + * @since 1.0.7 + */ +class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { + + public TraceLoadBalancerFeignClient(Client delegate, + CachingSpringLoadBalancerFactory lbClientFactory, + SpringClientFactory clientFactory, BeanFactory beanFactory) { + super(wrap(delegate, beanFactory), lbClientFactory, clientFactory); + } + + private static Client wrap(Client delegate, BeanFactory beanFactory) { + return (Client) new TraceFeignObjectWrapper(beanFactory).wrap(delegate); + } +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue393/Issue393Tests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue393/Issue393Tests.java new file mode 100644 index 000000000..2f513f4dc --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/issue393/Issue393Tests.java @@ -0,0 +1,130 @@ +/* + * 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.issues.issue393; + +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.context.SpringBootTest; +import org.springframework.cloud.client.discovery.EnableDiscoveryClient; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.cloud.sleuth.Tracer; +import org.springframework.cloud.sleuth.sampler.AlwaysSampler; +import org.springframework.cloud.sleuth.trace.TestSpanContextHolder; +import org.springframework.cloud.sleuth.util.ExceptionUtils; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.http.ResponseEntity; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.web.bind.annotation.PathVariable; +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 static org.assertj.core.api.BDDAssertions.then; + +/** + * @author Marcin Grzejszczak + */ +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = Application.class, webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) +@TestPropertySource(properties = {"spring.application.name=demo-feign-uri", + "server.port=9978", "eureka.client.enabled=true"}) +public class Issue393Tests { + + RestTemplate template = new RestTemplate(); + @Autowired Tracer tracer; + + @Before + public void open() { + TestSpanContextHolder.removeCurrentSpan(); + ExceptionUtils.setFail(true); + } + + @After + public void cleanup() { + TestSpanContextHolder.removeCurrentSpan(); + } + + @Test + public void should_successfully_work_when_service_discovery_is_on_classpath_and_feign_uses_url() { + String url = "http://localhost:9978/hello/mikesarver"; + + ResponseEntity response = this.template.getForEntity(url, String.class); + + then(response.getBody()).isEqualTo("mikesarver foo"); + then(ExceptionUtils.getLastException()).isNull(); + then(this.tracer.getCurrentSpan()).isNull(); + } +} + +@Configuration +@EnableAutoConfiguration +@EnableFeignClients +@EnableDiscoveryClient +class Application { + + @Bean + public DemoController demoController(MyNameRemote myNameRemote) { + return new DemoController(myNameRemote); + } + + @Bean + public feign.Logger.Level feignLoggerLevel() { + return feign.Logger.Level.BASIC; + } + + @Bean + public AlwaysSampler defaultSampler() { + return new AlwaysSampler(); + } + +} + +@FeignClient(name="no-name", + url="http://localhost:9978") +interface MyNameRemote { + + @RequestMapping(value = "/name/{id}", method = RequestMethod.GET) + String getName(@PathVariable("id") String id); +} + +@RestController +class DemoController { + + private final MyNameRemote myNameRemote; + + public DemoController(MyNameRemote myNameRemote) { + this.myNameRemote = myNameRemote; + } + + @RequestMapping(value = "/hello/{name}") + public String getHello(@PathVariable("name") String name) { + return myNameRemote.getName(name) + " foo"; + } + + @RequestMapping(value = "/name/{name}") + public String getName(@PathVariable("name") String name) { + return name; + } +} diff --git a/spring-cloud-sleuth-core/src/test/resources/application.yml b/spring-cloud-sleuth-core/src/test/resources/application.yml index d081e7e83..503ad9c80 100644 --- a/spring-cloud-sleuth-core/src/test/resources/application.yml +++ b/spring-cloud-sleuth-core/src/test/resources/application.yml @@ -9,6 +9,8 @@ exceptionService.ribbon: ConnectTimeout: 1 ReadTimeout: 1 +eureka.client.enabled: false + spring.sleuth.scheduled.skipPattern: "^org.*TestBeanWithScheduledMethodToBeIgnored$" # comma separated list of matchers spring.sleuth.rxjava.schedulers.ignoredthreads: HystixMetricPoller,^MyCustomThread.*$,^RxComputation.*$ \ No newline at end of file