diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignBlockingLoadBalancerClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignBlockingLoadBalancerClient.java index cef46883d..fc4faffe8 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignBlockingLoadBalancerClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignBlockingLoadBalancerClient.java @@ -27,6 +27,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; import org.springframework.cloud.sleuth.api.CurrentTraceContext; import org.springframework.cloud.sleuth.api.Span; @@ -55,8 +56,9 @@ class TraceFeignBlockingLoadBalancerClient extends FeignBlockingLoadBalancerClie TracingFeignClient tracingFeignClient; TraceFeignBlockingLoadBalancerClient(Client delegate, LoadBalancerClient loadBalancerClient, - BeanFactory beanFactory, LoadBalancerProperties loadBalancerProperties) { - super(delegate, loadBalancerClient, loadBalancerProperties); + LoadBalancerProperties loadBalancerProperties, LoadBalancerClientFactory loadBalancerClientFactory, + BeanFactory beanFactory) { + super(delegate, loadBalancerClient, loadBalancerProperties, loadBalancerClientFactory); this.beanFactory = beanFactory; } 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 41be08f84..5d8f05b0a 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 @@ -24,9 +24,13 @@ import org.apache.commons.logging.LogFactory; import org.springframework.aop.support.AopUtils; import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; +import org.springframework.cloud.openfeign.loadbalancer.RetryableFeignBlockingLoadBalancerClient; import org.springframework.cloud.util.ProxyUtils; import org.springframework.util.ClassUtils; @@ -51,7 +55,9 @@ final class TraceFeignObjectWrapper { loadBalancerPresent = ClassUtils .isPresent("org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient", null) && ClassUtils.isPresent( - "org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient", null); + "org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient", null) + && ClassUtils.isPresent("org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory", + null); } private final BeanFactory beanFactory; @@ -60,6 +66,10 @@ final class TraceFeignObjectWrapper { private LoadBalancerProperties loadBalancerProperties; + private Object loadBalancerRetryFactory; + + private Object loadBalancerClientFactory; + TraceFeignObjectWrapper(BeanFactory beanFactory) { this.beanFactory = beanFactory; } @@ -70,6 +80,10 @@ final class TraceFeignObjectWrapper { && !(bean instanceof TraceFeignBlockingLoadBalancerClient)) { return instrumentedFeignLoadBalancerClient(bean); } + if (loadBalancerPresent && bean instanceof RetryableFeignBlockingLoadBalancerClient + && !(bean instanceof TraceRetryableFeignBlockingLoadBalancerClient)) { + return instrumentedRetryableFeignLoadBalancerClient(bean); + } return new LazyTracingFeignClient(this.beanFactory, (Client) bean); } return bean; @@ -80,7 +94,8 @@ final class TraceFeignObjectWrapper { FeignBlockingLoadBalancerClient client = ProxyUtils.getTargetObject(bean); return new TraceFeignBlockingLoadBalancerClient( (Client) new TraceFeignObjectWrapper(this.beanFactory).wrap(client.getDelegate()), - (LoadBalancerClient) loadBalancerClient(), this.beanFactory, loadBalancerProperties()); + (LoadBalancerClient) loadBalancerClient(), loadBalancerProperties(), + (LoadBalancerClientFactory) loadBalancerClientFactory(), this.beanFactory); } else { FeignBlockingLoadBalancerClient client = ProxyUtils.getTargetObject(bean); @@ -93,7 +108,34 @@ final class TraceFeignObjectWrapper { log.warn(EXCEPTION_WARNING, e); } return new TraceFeignBlockingLoadBalancerClient(client, (LoadBalancerClient) loadBalancerClient(), - this.beanFactory, loadBalancerProperties()); + loadBalancerProperties(), (LoadBalancerClientFactory) loadBalancerClientFactory(), + this.beanFactory); + } + } + + private Object instrumentedRetryableFeignLoadBalancerClient(Object bean) { + if (AopUtils.getTargetClass(bean).equals(RetryableFeignBlockingLoadBalancerClient.class)) { + RetryableFeignBlockingLoadBalancerClient client = ProxyUtils.getTargetObject(bean); + return new TraceRetryableFeignBlockingLoadBalancerClient( + (Client) new TraceFeignObjectWrapper(beanFactory).wrap(client.getDelegate()), + (BlockingLoadBalancerClient) loadBalancerClient(), + (LoadBalancedRetryFactory) loadBalancerRetryFactory(), loadBalancerProperties(), + (LoadBalancerClientFactory) loadBalancerClientFactory(), beanFactory); + } + else { + RetryableFeignBlockingLoadBalancerClient client = ((RetryableFeignBlockingLoadBalancerClient) bean); + try { + Field delegate = RetryableFeignBlockingLoadBalancerClient.class.getDeclaredField(DELEGATE); + delegate.setAccessible(true); + delegate.set(client, new TraceFeignObjectWrapper(beanFactory).wrap(client.getDelegate())); + } + catch (NoSuchFieldException | IllegalArgumentException | IllegalAccessException | SecurityException e) { + log.warn(EXCEPTION_WARNING, e); + } + return new TraceRetryableFeignBlockingLoadBalancerClient(client, + (BlockingLoadBalancerClient) loadBalancerClient(), + (LoadBalancedRetryFactory) loadBalancerRetryFactory(), loadBalancerProperties(), + (LoadBalancerClientFactory) loadBalancerClientFactory(), beanFactory); } } @@ -111,4 +153,18 @@ final class TraceFeignObjectWrapper { return loadBalancerProperties; } + private Object loadBalancerRetryFactory() { + if (loadBalancerRetryFactory == null) { + loadBalancerRetryFactory = beanFactory.getBean(LoadBalancedRetryFactory.class); + } + return loadBalancerRetryFactory; + } + + private Object loadBalancerClientFactory() { + if (loadBalancerClientFactory == null) { + loadBalancerClientFactory = beanFactory.getBean(LoadBalancerClientFactory.class); + } + return loadBalancerClientFactory; + } + } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceRetryableFeignBlockingLoadBalancerClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceRetryableFeignBlockingLoadBalancerClient.java new file mode 100644 index 000000000..f667f6c22 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceRetryableFeignBlockingLoadBalancerClient.java @@ -0,0 +1,137 @@ +/* + * Copyright 2013-2020 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 + * + * https://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 feign.Client; +import feign.Request; +import feign.Response; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.client.loadbalancer.LoadBalancedRetryFactory; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; +import org.springframework.cloud.openfeign.loadbalancer.RetryableFeignBlockingLoadBalancerClient; +import org.springframework.cloud.sleuth.api.CurrentTraceContext; +import org.springframework.cloud.sleuth.api.Span; +import org.springframework.cloud.sleuth.api.Tracer; +import org.springframework.cloud.sleuth.api.http.HttpClientHandler; + +/** + * A trace representation of {@link RetryableFeignBlockingLoadBalancerClient}. Needed due + * to casts in {@link org.springframework.cloud.openfeign.FeignClientFactoryBean}. + * + * @author Olga Maciaszek-Sharma + * @since 2.2.0 + * @see RetryableFeignBlockingLoadBalancerClient + */ +class TraceRetryableFeignBlockingLoadBalancerClient extends RetryableFeignBlockingLoadBalancerClient { + + private static final Log LOG = LogFactory.getLog(TraceRetryableFeignBlockingLoadBalancerClient.class); + + private final BeanFactory beanFactory; + + Tracer tracer; + + CurrentTraceContext currentTraceContext; + + HttpClientHandler httpClientHandler; + + TracingFeignClient tracingFeignClient; + + TraceRetryableFeignBlockingLoadBalancerClient(Client delegate, BlockingLoadBalancerClient loadBalancerClient, + LoadBalancedRetryFactory retryFactory, LoadBalancerProperties properties, + LoadBalancerClientFactory loadBalancerClientFactory, BeanFactory beanFactory) { + super(delegate, loadBalancerClient, retryFactory, properties, loadBalancerClientFactory); + this.beanFactory = beanFactory; + } + + @Override + public Response execute(Request request, Request.Options options) throws IOException { + if (LOG.isDebugEnabled()) { + LOG.debug("Before send"); + } + Response response = null; + Span fallbackSpan = tracer().nextSpan().start(); + try { + if (delegateIsALoadBalancer()) { + response = getDelegate().execute(request, options); + } + else { + response = super.execute(request, options); + } + if (LOG.isDebugEnabled()) { + LOG.debug("After receive"); + } + return response; + } + catch (Exception e) { + if (LOG.isDebugEnabled()) { + LOG.debug("Exception thrown", e); + } + if (e instanceof IOException) { + if (LOG.isDebugEnabled()) { + LOG.debug( + "IOException was thrown, so most likely the traced client wasn't called. Falling back to a manual span."); + } + tracingFeignClient().handleSendAndReceive(fallbackSpan, request, response, e); + } + throw e; + } + finally { + fallbackSpan.abandon(); + } + } + + private boolean delegateIsALoadBalancer() { + return getDelegate() instanceof RetryableFeignBlockingLoadBalancerClient; + } + + private Tracer tracer() { + if (tracer == null) { + tracer = beanFactory.getBean(Tracer.class); + } + return tracer; + } + + private CurrentTraceContext currentTraceContext() { + if (currentTraceContext == null) { + currentTraceContext = beanFactory.getBean(CurrentTraceContext.class); + } + return currentTraceContext; + } + + private HttpClientHandler httpClientHandler() { + if (httpClientHandler == null) { + httpClientHandler = beanFactory.getBean(HttpClientHandler.class); + } + return httpClientHandler; + } + + private TracingFeignClient tracingFeignClient() { + if (tracingFeignClient == null) { + tracingFeignClient = (TracingFeignClient) TracingFeignClient.create(currentTraceContext(), + httpClientHandler(), getDelegate()); + } + return tracingFeignClient; + } + +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignObjectWrapperTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignObjectWrapperTests.java index 404c6a781..eb55c3960 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignObjectWrapperTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignObjectWrapperTests.java @@ -30,8 +30,11 @@ import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; import org.springframework.cloud.loadbalancer.blocking.client.BlockingLoadBalancerClient; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; +import static org.mockito.Mockito.mock; + /** * @author Marcin Grzejszczak */ @@ -46,7 +49,7 @@ public class TracingFeignObjectWrapperTests { @Test public void should_wrap_a_client_into_lazy_trace_client() { - BDDAssertions.then(this.traceFeignObjectWrapper.wrap(Mockito.mock(Client.class))) + BDDAssertions.then(this.traceFeignObjectWrapper.wrap(mock(Client.class))) .isExactlyInstanceOf(LazyTracingFeignClient.class); } @@ -59,12 +62,13 @@ public class TracingFeignObjectWrapperTests { // gh-1528 @Test public void should_wrap_feign_loadbalancer_client() { - Client delegate = Mockito.mock(Client.class); - BlockingLoadBalancerClient loadBalancerClient = Mockito.mock(BlockingLoadBalancerClient.class); + Client delegate = mock(Client.class); + BlockingLoadBalancerClient loadBalancerClient = mock(BlockingLoadBalancerClient.class); + LoadBalancerClientFactory loadBalancerClientFactory = mock(LoadBalancerClientFactory.class); Mockito.when(beanFactory.getBean(LoadBalancerClient.class)).thenReturn(loadBalancerClient); - Object wrapped = traceFeignObjectWrapper - .wrap(new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient, new LoadBalancerProperties())); + Object wrapped = traceFeignObjectWrapper.wrap(new FeignBlockingLoadBalancerClient(delegate, loadBalancerClient, + new LoadBalancerProperties(), loadBalancerClientFactory)); Assertions.assertThat(wrapped).isInstanceOf(TraceFeignBlockingLoadBalancerClient.class); } @@ -72,21 +76,22 @@ public class TracingFeignObjectWrapperTests { // gh-1528, gh-1125 @Test public void should_wrap_subclass_of_feign_loadbalancer_client() { - Client delegate = Mockito.mock(Client.class); - BlockingLoadBalancerClient loadBalancerClient = Mockito.mock(BlockingLoadBalancerClient.class); + Client delegate = mock(Client.class); + BlockingLoadBalancerClient loadBalancerClient = mock(BlockingLoadBalancerClient.class); + LoadBalancerClientFactory loadBalancerClientFactory = mock(LoadBalancerClientFactory.class); Mockito.when(beanFactory.getBean(LoadBalancerClient.class)).thenReturn(loadBalancerClient); Object wrapped = traceFeignObjectWrapper - .wrap(new TestFeignBlockingLoadBalancerClient(delegate, loadBalancerClient)); + .wrap(new TestFeignBlockingLoadBalancerClient(delegate, loadBalancerClient, loadBalancerClientFactory)); Assertions.assertThat(wrapped).isInstanceOf(TraceFeignBlockingLoadBalancerClient.class); - } static class TestFeignBlockingLoadBalancerClient extends FeignBlockingLoadBalancerClient { - TestFeignBlockingLoadBalancerClient(Client delegate, BlockingLoadBalancerClient loadBalancerClient) { - super(delegate, loadBalancerClient, new LoadBalancerProperties()); + TestFeignBlockingLoadBalancerClient(Client delegate, BlockingLoadBalancerClient loadBalancerClient, + LoadBalancerClientFactory loadBalancerClientFactory) { + super(delegate, loadBalancerClient, new LoadBalancerProperties(), loadBalancerClientFactory); } } diff --git a/tests/brave/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/brave/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java b/tests/brave/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/brave/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java index 030c09b4a..4fb2825d0 100644 --- a/tests/brave/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/brave/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java +++ b/tests/brave/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/brave/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java @@ -37,6 +37,7 @@ import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.cloud.openfeign.EnableFeignClients; import org.springframework.cloud.openfeign.FeignClient; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; @@ -101,8 +102,10 @@ public class ManuallyCreatedLoadBalancerFeignClientTests { class Application { @Bean - public Client client(LoadBalancerClient blockingLoadBalancerClient, LoadBalancerProperties properties) { - return new MyBlockingClient(new MyDelegateClient(), blockingLoadBalancerClient, properties); + public Client client(LoadBalancerClient blockingLoadBalancerClient, LoadBalancerProperties properties, + LoadBalancerClientFactory loadBalancerClientFactory) { + return new MyBlockingClient(new MyDelegateClient(), blockingLoadBalancerClient, properties, + loadBalancerClientFactory); } @Bean @@ -125,8 +128,9 @@ class Application { class MyBlockingClient extends FeignBlockingLoadBalancerClient { - MyBlockingClient(Client delegate, LoadBalancerClient loadBalancerClient, LoadBalancerProperties properties) { - super(delegate, loadBalancerClient, properties); + MyBlockingClient(Client delegate, LoadBalancerClient loadBalancerClient, LoadBalancerProperties properties, + LoadBalancerClientFactory loadBalancerClientFactory) { + super(delegate, loadBalancerClient, properties, loadBalancerClientFactory); } boolean wasCalled;