From 15ee807b85f6b9d8493312a30db2c66cb50038f7 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 7 Apr 2020 13:43:37 +0200 Subject: [PATCH 1/3] Refactor tests and make them not rely on `LoadBalancerFeignClient` side effects. --- ...lyCreatedLoadBalancerFeignClientTests.java | 30 +++++++++-------- ...dDelegateLoadBalancerFeignClientTests.java | 32 +++++++++---------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java index 2d60c1d3c..b26ecce6b 100644 --- a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java @@ -59,10 +59,10 @@ import static org.assertj.core.api.BDDAssertions.then; public class ManuallyCreatedLoadBalancerFeignClientTests { @Autowired - MyClient myClient; + MyLoadBalancerClient myLoadBalancerClient; @Autowired - MyNameRemote myNameRemote; + AnnotatedFeignClient annotatedFeignClient; @Autowired ArrayListSpanReporter reporter; @@ -74,29 +74,29 @@ public class ManuallyCreatedLoadBalancerFeignClientTests { @Test public void should_reuse_custom_feign_client() { - String response = this.myNameRemote.get(); + String response = this.annotatedFeignClient.get(); - then(this.myClient.wasCalled()).isTrue(); + then(this.myLoadBalancerClient.wasCalled()).isTrue(); then(response).isEqualTo("foo"); List spans = this.reporter.getSpans(); // retries then(spans).hasSize(1); - then(spans.get(0).tags().get("http.path")).isEqualTo("/"); + then(spans.get(0).tags().get("http.path")).isEqualTo("/test"); } @Test public void my_client_called() { - this.myNameRemote.get(); - then(this.myClient.wasCalled()).isTrue(); + this.annotatedFeignClient.get(); + then(this.myLoadBalancerClient.wasCalled()).isTrue(); } @Test public void span_captured() { - this.myNameRemote.get(); + this.annotatedFeignClient.get(); List spans = this.reporter.getSpans(); // retries then(spans).hasSize(1); - then(spans.get(0).tags().get("http.path")).isEqualTo("/"); + then(spans.get(0).tags().get("http.path")).isEqualTo("/test"); } } @@ -109,7 +109,8 @@ class Application { @Bean public Client client(CachingSpringLoadBalancerFactory cachingFactory, SpringClientFactory clientFactory) { - return new MyClient(new MyDelegateClient(), cachingFactory, clientFactory); + return new MyLoadBalancerClient(new MyDelegateClient(), cachingFactory, + clientFactory); } @Bean @@ -124,9 +125,10 @@ class Application { } -class MyClient extends LoadBalancerFeignClient { +class MyLoadBalancerClient extends LoadBalancerFeignClient { - MyClient(Client delegate, CachingSpringLoadBalancerFactory lbClientFactory, + MyLoadBalancerClient(Client delegate, + CachingSpringLoadBalancerFactory lbClientFactory, SpringClientFactory clientFactory) { super(delegate, lbClientFactory, clientFactory); } @@ -161,9 +163,9 @@ class MyDelegateClient implements Client { } @FeignClient(name = "foo", url = "http://foo") -interface MyNameRemote { +interface AnnotatedFeignClient { - @RequestMapping(value = "/", method = RequestMethod.GET) + @RequestMapping(value = "/test", method = RequestMethod.GET) String get(); } diff --git a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java index b3a0f8a61..d93971b8d 100644 --- a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java @@ -65,13 +65,13 @@ import static org.assertj.core.api.BDDAssertions.then; public class ManuallyCreatedDelegateLoadBalancerFeignClientTests { @Autowired - MyClient myClient; + MyLoadBalancerClient myLoadBalancerClient; @Autowired MyDelegateClient myDelegateClient; @Autowired - MyNameRemote myNameRemote; + AnnotatedFeignClient annotatedFeignClient; @Autowired ArrayListSpanReporter reporter; @@ -83,31 +83,31 @@ public class ManuallyCreatedDelegateLoadBalancerFeignClientTests { @Test public void should_reuse_custom_feign_client() { - String response = this.myNameRemote.get(); + String response = this.annotatedFeignClient.get(); - then(this.myClient.wasCalled()).isTrue(); + then(this.myLoadBalancerClient.wasCalled()).isTrue(); then(this.myDelegateClient.wasCalled()).isTrue(); then(response).isEqualTo("foo"); List spans = this.reporter.getSpans(); // retries then(spans).hasSize(1); - then(spans.get(0).tags().get("http.path")).isEqualTo("/"); + then(spans.get(0).tags().get("http.path")).isEqualTo("/test"); } @Test public void my_client_called() { - this.myNameRemote.get(); - then(this.myClient.wasCalled()).isTrue(); + this.annotatedFeignClient.get(); + then(this.myLoadBalancerClient.wasCalled()).isTrue(); then(this.myDelegateClient.wasCalled()).isTrue(); } @Test public void span_captured() { - this.myNameRemote.get(); + this.annotatedFeignClient.get(); List spans = this.reporter.getSpans(); // retries then(spans).hasSize(1); - then(spans.get(0).tags().get("http.path")).isEqualTo("/"); + then(spans.get(0).tags().get("http.path")).isEqualTo("/test"); } } @@ -126,15 +126,15 @@ class Application { public Client client(MyDelegateClient myDelegateClient, CachingSpringLoadBalancerFactory cachingFactory, SpringClientFactory clientFactory) { - return new MyClient(myDelegateClient, cachingFactory, clientFactory); + return new MyLoadBalancerClient(myDelegateClient, cachingFactory, clientFactory); } @Bean - public MyNameRemote myNameRemote(Client client, Decoder decoder, Encoder encoder, + public AnnotatedFeignClient myNameRemote(Client client, Decoder decoder, Encoder encoder, Contract contract) { return Feign.builder().client(client).encoder(encoder).decoder(decoder) .contract(contract) - .target(new HardCodedTarget<>(MyNameRemote.class, "foo", "http://foo")); + .target(new HardCodedTarget<>(AnnotatedFeignClient.class, "foo", "http://foo")); } @Bean @@ -149,9 +149,9 @@ class Application { } -class MyClient extends LoadBalancerFeignClient { +class MyLoadBalancerClient extends LoadBalancerFeignClient { - MyClient(Client delegate, CachingSpringLoadBalancerFactory lbClientFactory, + MyLoadBalancerClient(Client delegate, CachingSpringLoadBalancerFactory lbClientFactory, SpringClientFactory clientFactory) { super(delegate, lbClientFactory, clientFactory); } @@ -190,9 +190,9 @@ class MyDelegateClient implements Client { } @FeignClient(name = "foo", url = "http://foo") -interface MyNameRemote { +interface AnnotatedFeignClient { - @RequestMapping(value = "/", method = RequestMethod.GET) + @RequestMapping(value = "/test", method = RequestMethod.GET) String get(); } From 47c2523edeccce0bee74ad6378a32e9b0e471021 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 7 Apr 2020 13:44:23 +0200 Subject: [PATCH 2/3] Avoid double-load-balancing. Fixes gh-1610. --- .../client/feign/TraceLoadBalancerFeignClient.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) 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 index d27656fb3..8644610e7 100644 --- 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 @@ -30,6 +30,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.netflix.ribbon.SpringClientFactory; +import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; import org.springframework.cloud.openfeign.ribbon.CachingSpringLoadBalancerFactory; import org.springframework.cloud.openfeign.ribbon.LoadBalancerFeignClient; @@ -67,7 +68,12 @@ public class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { Response response = null; Span fallbackSpan = tracer().nextSpan().start(); try { - response = super.execute(request, options); + if (delegateIsALoadBalancer()) { + response = getDelegate().execute(request, options); + } + else { + response = super.execute(request, options); + } if (log.isDebugEnabled()) { log.debug("After receive"); } @@ -95,6 +101,11 @@ public class TraceLoadBalancerFeignClient extends LoadBalancerFeignClient { } } + private boolean delegateIsALoadBalancer() { + return getDelegate() instanceof LoadBalancerFeignClient + || getDelegate() instanceof FeignBlockingLoadBalancerClient; + } + private Tracer tracer() { if (this.tracer == null) { this.tracer = this.beanFactory.getBean(Tracer.class); From 20e29f2d805f8d90091039c900ffa11eb17cc6d4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Tue, 7 Apr 2020 13:46:37 +0200 Subject: [PATCH 3/3] Adjust method name to previous changes. --- .../ManuallyCreatedDelegateLoadBalancerFeignClientTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java index d93971b8d..d28b39527 100644 --- a/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java @@ -130,7 +130,7 @@ class Application { } @Bean - public AnnotatedFeignClient myNameRemote(Client client, Decoder decoder, Encoder encoder, + public AnnotatedFeignClient annotatedFeignClient(Client client, Decoder decoder, Encoder encoder, Contract contract) { return Feign.builder().client(client).encoder(encoder).decoder(decoder) .contract(contract)