diff --git a/.circleci/config.yml b/.circleci/config.yml index bbc27ed71..a9d1dd966 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,7 +10,6 @@ jobs: branches: ignore: - gh-pages # list of branches to ignore - resource_class: large steps: - checkout - restore_cache: diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java index d23a6c155..c19a5572e 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilter.java @@ -32,6 +32,7 @@ import org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter; import org.springframework.http.HttpHeaders; import org.springframework.http.server.reactive.ServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.lang.NonNull; import org.springframework.web.server.ServerWebExchange; final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { @@ -51,21 +52,20 @@ final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { if (log.isDebugEnabled()) { log.debug("Will instrument the HTTP request headers"); } - ServerHttpRequest.Builder builder = exchange.getRequest().mutate(); - Span span = this.handler.handleSend(this.injector, builder); + TraceCarrier carrier = new TraceCarrier(exchange.getRequest(), input); + Span span = this.handler.handleSend(this.injector, carrier); if (log.isDebugEnabled()) { - log.debug( - "Client span " + span + " created for the request. New headers are " - + builder.build().getHeaders().toSingleValueMap()); + log.debug("Client span " + span + " created for the request. New headers are " + + carrier.filteredHeaders.toSingleValueMap()); } exchange.getAttributes().put(SPAN_ATTRIBUTE, span); HttpHeaders headersWithInput = new HttpHeaders(); headersWithInput.addAll(input); - addHeadersWithInput(builder, headersWithInput); + addHeadersWithInput(carrier.filteredHeaders, headersWithInput); return headersWithInput; } - private void addHeadersWithInput(ServerHttpRequest.Builder builder, + private void addHeadersWithInput(HttpHeaders filteredHeaders, HttpHeaders headersWithInput) { for (Map.Entry> entry : builder.build().getHeaders() .entrySet()) { @@ -82,10 +82,22 @@ final class TraceRequestHttpHeadersFilter extends AbstractHttpHeadersFilter { } +class TraceCarrier { + + final ServerHttpRequest originalRequest; + + final HttpHeaders filteredHeaders; + + TraceCarrier(@NonNull ServerHttpRequest originalRequest, @NonNull HttpHeaders filteredHeaders) { + this.originalRequest = originalRequest; + this.filteredHeaders = filteredHeaders; + } + +} + final class TraceResponseHttpHeadersFilter extends AbstractHttpHeadersFilter { - private static final Log log = LogFactory - .getLog(TraceResponseHttpHeadersFilter.class); + private static final Log log = LogFactory.getLog(TraceResponseHttpHeadersFilter.class); private TraceResponseHttpHeadersFilter(HttpTracing httpTracing) { super(httpTracing); @@ -122,23 +134,23 @@ abstract class AbstractHttpHeadersFilter implements HttpHeadersFilter { static final String SPAN_ATTRIBUTE = Span.class.getName(); - private static final Propagation.Setter SETTER = new Propagation.Setter() { + private static final Propagation.Setter SETTER = new Propagation.Setter() { @Override - public void put(ServerHttpRequest.Builder carrier, String key, String value) { - carrier.headers(httpHeaders -> httpHeaders.set(key, value)); + public void put(TraceCarrier carrier, String key, String value) { + carrier.filteredHeaders.set(key, value); } @Override public String toString() { - return "ServerHttpRequest.Builder::header"; + return "TraceCarrier::httpHeaders::set"; } }; final Tracer tracer; - final HttpClientHandler handler; + final HttpClientHandler handler; - final TraceContext.Injector injector; + final TraceContext.Injector injector; final HttpTracing httpTracing; @@ -149,29 +161,27 @@ abstract class AbstractHttpHeadersFilter implements HttpHeadersFilter { this.httpTracing = httpTracing; } - private static class ServerHttpAdapter extends - brave.http.HttpClientAdapter { + private static class ServerHttpAdapter extends brave.http.HttpClientAdapter { @Override - public String method(ServerHttpRequest.Builder request) { - return request.build().getMethodValue(); + public String method(TraceCarrier request) { + return request.originalRequest.getMethodValue(); } @Override - public String url(ServerHttpRequest.Builder request) { - return request.build().getURI().toString(); + public String url(TraceCarrier request) { + return request.originalRequest.getURI().toString(); } @Override - public String requestHeader(ServerHttpRequest.Builder request, String name) { - Object result = request.build().getHeaders().get(name); + public String requestHeader(TraceCarrier request, String name) { + Object result = request.filteredHeaders.get(name); return result != null ? result.toString() : ""; } @Override public Integer statusCode(ServerHttpResponse response) { - return response.getStatusCode() != null ? response.getStatusCode().value() - : null; + return response.getStatusCode() != null ? response.getStatusCode().value() : null; } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java index 1711f7bc9..20b527b9d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/ExecutorBeanPostProcessorTests.java @@ -33,6 +33,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import brave.Tracing; import org.aopalliance.aop.Advice; +import org.assertj.core.api.BDDAssertions; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -72,8 +73,7 @@ public class ExecutorBeanPostProcessorTests { @Before public void setup() { this.sleuthAsyncProperties = new SleuthAsyncProperties(); - Mockito.when(this.beanFactory.getBean(SleuthAsyncProperties.class)) - .thenReturn(this.sleuthAsyncProperties); + Mockito.when(this.beanFactory.getBean(SleuthAsyncProperties.class)).thenReturn(this.sleuthAsyncProperties); } @After @@ -83,8 +83,7 @@ public class ExecutorBeanPostProcessorTests { @Test public void should_create_a_cglib_proxy_by_default() throws Exception { - Object o = new ExecutorBeanPostProcessor(this.beanFactory) - .postProcessAfterInitialization(new Foo(), "foo"); + Object o = new ExecutorBeanPostProcessor(this.beanFactory).postProcessAfterInitialization(new Foo(), "foo"); then(o).isInstanceOf(Foo.class); then(AopUtils.isCglibProxy(o)).isTrue(); @@ -107,8 +106,7 @@ public class ExecutorBeanPostProcessorTests { throws Exception { ScheduledExecutorService service = Executors.newSingleThreadScheduledExecutor(); - Object o = new ExecutorBeanPostProcessor(this.beanFactory) - .postProcessAfterInitialization(service, "foo"); + Object o = new ExecutorBeanPostProcessor(this.beanFactory).postProcessAfterInitialization(service, "foo"); then(o).isInstanceOf(TraceableScheduledExecutorService.class); service.shutdown(); @@ -174,8 +172,7 @@ public class ExecutorBeanPostProcessorTests { } @Test - public void should_fallback_to_default_implementation_when_exception_thrown() - throws Exception { + public void should_fallback_to_default_implementation_when_exception_thrown() throws Exception { ScheduledExecutorService service = Executors.newSingleThreadScheduledExecutor(); ExecutorBeanPostProcessor bpp = new ExecutorBeanPostProcessor(this.beanFactory) { @@ -193,8 +190,7 @@ public class ExecutorBeanPostProcessorTests { } @Test - public void should_create_a_cglib_proxy_by_default_for_ThreadPoolTaskExecutor() - throws Exception { + public void should_create_a_cglib_proxy_by_default_for_ThreadPoolTaskExecutor() throws Exception { Object o = new ExecutorBeanPostProcessor(this.beanFactory) .postProcessAfterInitialization(new FooThreadPoolTaskExecutor(), "foo"); @@ -208,8 +204,7 @@ public class ExecutorBeanPostProcessorTests { ThreadPoolTaskExecutor taskExecutor = new ThreadPoolTaskExecutor(); ExecutorBeanPostProcessor bpp = new ExecutorBeanPostProcessor(this.beanFactory) { @Override - Object createThreadPoolTaskExecutorProxy(Object bean, boolean cglibProxy, - ThreadPoolTaskExecutor executor) { + Object createThreadPoolTaskExecutorProxy(Object bean, boolean cglibProxy, ThreadPoolTaskExecutor executor) { throw new AopConfigException("foo"); } }; @@ -239,8 +234,7 @@ public class ExecutorBeanPostProcessorTests { ExecutorService service = exceptionThrowingExecutorService(); ExecutorBeanPostProcessor bpp = new ExecutorBeanPostProcessor(this.beanFactory); - ExecutorService o = (ExecutorService) bpp.postProcessAfterInitialization(service, - "foo"); + ExecutorService o = (ExecutorService) bpp.postProcessAfterInitialization(service, "foo"); thenThrownBy(() -> o.submit((Callable) () -> "hello")).hasMessage("foo") .isInstanceOf(IllegalStateException.class); @@ -274,8 +268,7 @@ public class ExecutorBeanPostProcessorTests { } @Override - public boolean awaitTermination(long timeout, TimeUnit unit) - throws InterruptedException { + public boolean awaitTermination(long timeout, TimeUnit unit) throws InterruptedException { return false; } @@ -295,14 +288,13 @@ public class ExecutorBeanPostProcessorTests { } @Override - public List> invokeAll(Collection> tasks) - throws InterruptedException { + public List> invokeAll(Collection> tasks) throws InterruptedException { return null; } @Override - public List> invokeAll(Collection> tasks, - long timeout, TimeUnit unit) throws InterruptedException { + public List> invokeAll(Collection> tasks, long timeout, TimeUnit unit) + throws InterruptedException { return null; } @@ -313,8 +305,7 @@ public class ExecutorBeanPostProcessorTests { } @Override - public T invokeAny(Collection> tasks, long timeout, - TimeUnit unit) + public T invokeAny(Collection> tasks, long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { return null; } @@ -323,11 +314,9 @@ public class ExecutorBeanPostProcessorTests { @Test public void should_use_jdk_proxy_when_executor_has_final_methods() { - ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( - this.beanFactory); + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor(this.beanFactory); Executor executor = Runnable::run; - Executor wrappedExecutor = (Executor) beanPostProcessor - .postProcessAfterInitialization(executor, "executor"); + Executor wrappedExecutor = (Executor) beanPostProcessor.postProcessAfterInitialization(executor, "executor"); then(AopUtils.isJdkDynamicProxy(wrappedExecutor)).isTrue(); then(AopUtils.isCglibProxy(wrappedExecutor)).isFalse(); @@ -340,10 +329,8 @@ public class ExecutorBeanPostProcessorTests { } @Test - public void should_use_jdk_proxy_when_executor_service_has_final_methods() - throws Exception { - ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( - this.beanFactory); + public void should_use_jdk_proxy_when_executor_service_has_final_methods() throws Exception { + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor(this.beanFactory); ExecutorService executorService = new DelegatingSecurityContextExecutorService( Executors.newSingleThreadExecutor()); ExecutorService wrappedExecutor = (ExecutorService) beanPostProcessor @@ -356,10 +343,8 @@ public class ExecutorBeanPostProcessorTests { } @Test - public void should_use_jdk_proxy_when_async_task_executor_has_final_methods() - throws Exception { - ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor( - this.beanFactory); + public void should_use_jdk_proxy_when_async_task_executor_has_final_methods() throws Exception { + ExecutorBeanPostProcessor beanPostProcessor = new ExecutorBeanPostProcessor(this.beanFactory); AsyncTaskExecutor wrappedExecutor = (AsyncTaskExecutor) beanPostProcessor .postProcessAfterInitialization(new DirectTaskExecutor(), "taskExecutor"); @@ -371,13 +356,11 @@ public class ExecutorBeanPostProcessorTests { @Test public void should_fallback_to_sleuth_impl_when_thread_pool_task_executor_has_final_methods() { - ExecutorBeanPostProcessor postProcessor = new ExecutorBeanPostProcessor( - this.beanFactory); + ExecutorBeanPostProcessor postProcessor = new ExecutorBeanPostProcessor(this.beanFactory); ThreadPoolTaskExecutor threadPoolTaskExecutor = new PoolTaskExecutor(); ThreadPoolTaskExecutor wrappedTaskExecutor = (ThreadPoolTaskExecutor) postProcessor - .postProcessAfterInitialization(threadPoolTaskExecutor, - "threadPoolTaskExecutor"); + .postProcessAfterInitialization(threadPoolTaskExecutor, "threadPoolTaskExecutor"); then(wrappedTaskExecutor).isInstanceOf(LazyTraceThreadPoolTaskExecutor.class); then(AopUtils.isCglibProxy(wrappedTaskExecutor)).isFalse(); @@ -387,31 +370,26 @@ public class ExecutorBeanPostProcessorTests { @Test public void proxy_is_not_needed() throws Exception { - this.sleuthAsyncProperties - .setIgnoredBeans(Collections.singletonList("fooExecutor")); + this.sleuthAsyncProperties.setIgnoredBeans(Collections.singletonList("fooExecutor")); - boolean isProxyNeeded = new ExecutorBeanPostProcessor(this.beanFactory) - .isProxyNeeded("fooExecutor"); + boolean isProxyNeeded = new ExecutorBeanPostProcessor(this.beanFactory).isProxyNeeded("fooExecutor"); then(isProxyNeeded).isFalse(); } @Test public void proxy_is_needed() throws Exception { - boolean isProxyNeeded = new ExecutorBeanPostProcessor(this.beanFactory) - .isProxyNeeded("fooExecutor"); + boolean isProxyNeeded = new ExecutorBeanPostProcessor(this.beanFactory).isProxyNeeded("fooExecutor"); then(isProxyNeeded).isTrue(); } @Test public void should_not_create_proxy() throws Exception { - this.sleuthAsyncProperties - .setIgnoredBeans(Collections.singletonList("fooExecutor")); + this.sleuthAsyncProperties.setIgnoredBeans(Collections.singletonList("fooExecutor")); Object o = new ExecutorBeanPostProcessor(this.beanFactory) - .postProcessAfterInitialization(new ThreadPoolTaskExecutor(), - "fooExecutor"); + .postProcessAfterInitialization(new ThreadPoolTaskExecutor(), "fooExecutor"); then(o).isInstanceOf(ThreadPoolTaskExecutor.class); then(AopUtils.isCglibProxy(o)).isFalse(); @@ -420,8 +398,7 @@ public class ExecutorBeanPostProcessorTests { @Test public void should_throw_real_exception_when_using_proxy() throws Exception { Object o = new ExecutorBeanPostProcessor(this.beanFactory) - .postProcessAfterInitialization(new RejectedExecutionExecutor(), - "fooExecutor"); + .postProcessAfterInitialization(new RejectedExecutionExecutor(), "fooExecutor"); then(o).isInstanceOf(RejectedExecutionExecutor.class); then(AopUtils.isCglibProxy(o)).isTrue(); @@ -429,6 +406,32 @@ public class ExecutorBeanPostProcessorTests { })).isInstanceOf(RejectedExecutionException.class).hasMessage("rejected"); } + // #1463 + @Test + public void should_not_double_instrument_traced_executors() throws Exception { + LazyTraceThreadPoolTaskExecutor lazyTraceThreadPoolTaskExecutor = BDDMockito + .mock(LazyTraceThreadPoolTaskExecutor.class); + Object o = new ExecutorBeanPostProcessor(this.beanFactory) + .postProcessAfterInitialization(lazyTraceThreadPoolTaskExecutor, "executor"); + BDDAssertions.then(o).isSameAs(lazyTraceThreadPoolTaskExecutor); + + TraceableExecutorService traceableExecutorService = BDDMockito.mock(TraceableExecutorService.class); + o = new ExecutorBeanPostProcessor(this.beanFactory).postProcessAfterInitialization(traceableExecutorService, + "executor"); + BDDAssertions.then(o).isSameAs(traceableExecutorService); + + LazyTraceAsyncTaskExecutor lazyTraceAsyncTaskExecutor = BDDMockito.mock(LazyTraceAsyncTaskExecutor.class); + o = new ExecutorBeanPostProcessor(this.beanFactory).postProcessAfterInitialization(lazyTraceAsyncTaskExecutor, + "executor"); + BDDAssertions.then(o).isSameAs(lazyTraceAsyncTaskExecutor); + + LazyTraceExecutor lazyTraceExecutor = BDDMockito.mock(LazyTraceExecutor.class); + o = new ExecutorBeanPostProcessor(this.beanFactory).postProcessAfterInitialization(lazyTraceExecutor, + "executor"); + BDDAssertions.then(o).isSameAs(lazyTraceExecutor); + + } + class Foo implements Executor { @Override diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java index 4917b4f4a..35ebce273 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceRequestHttpHeadersFilterTests.java @@ -48,14 +48,13 @@ public class TraceRequestHttpHeadersFilterTests { HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.set("X-Hello", "World"); - httpHeaders.set("X-Auth-User", "aaaa"); httpHeaders.set("X-B3-TraceId", "52f112af7472aff0"); httpHeaders.set("X-B3-SpanId", "53e6ab6fc5dfee58"); MockServerHttpRequest request = MockServerHttpRequest.post("foo/bar") .headers(httpHeaders).build(); MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); - HttpHeaders filteredHeaders = filter.filter(requestHeaders(), exchange); + HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")) .isNotEqualTo(httpHeaders.get("X-B3-TraceId")); @@ -81,7 +80,7 @@ public class TraceRequestHttpHeadersFilterTests { .headers(httpHeaders).build(); MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); - HttpHeaders filteredHeaders = filter.filter(requestHeaders(), exchange); + HttpHeaders filteredHeaders = filter.filter(requestHeaders(httpHeaders), exchange); BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); @@ -95,18 +94,40 @@ public class TraceRequestHttpHeadersFilterTests { .isNotNull(); } - // #1352 + // #1469 @Test - public void should_set_tracing_headers_with_multiple_values() { + public void should_reuse_headers_only_from_input_since_exchange_may_contain_already_ignored_headers() { HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); HttpHeaders httpHeaders = new HttpHeaders(); - httpHeaders.addAll("X-Hello", Arrays.asList("World1", "World2")); + httpHeaders.set("X-Hello", "World"); MockServerHttpRequest request = MockServerHttpRequest.post("foo/bar") .headers(httpHeaders).build(); MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); HttpHeaders filteredHeaders = filter.filter(requestHeaders(), exchange); + BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")).isNotEmpty(); + BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); + BDDAssertions.then(filteredHeaders.get("X-Hello")).isNullOrEmpty(); + BDDAssertions + .then((Object) exchange + .getAttribute(TraceRequestHttpHeadersFilter.SPAN_ATTRIBUTE)) + .isNotNull(); + } + + // #1352 + @Test + public void should_set_tracing_headers_with_multiple_values() { + HttpHeadersFilter filter = TraceRequestHttpHeadersFilter.create(this.httpTracing); + HttpHeaders httpHeaders = new HttpHeaders(); + httpHeaders.add("X-Hello-Request", "Request World"); + httpHeaders.addAll("X-Hello", Arrays.asList("World1", "World2")); + MockServerHttpRequest request = MockServerHttpRequest.post("foo/bar") + .headers(httpHeaders).build(); + MockServerWebExchange exchange = MockServerWebExchange.builder(request).build(); + + HttpHeaders filteredHeaders = filter.filter(httpHeaders, exchange); + BDDAssertions.then(filteredHeaders.get("X-B3-TraceId")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-B3-SpanId")).isNotEmpty(); BDDAssertions.then(filteredHeaders.get("X-Hello")) @@ -126,4 +147,12 @@ public class TraceRequestHttpHeadersFilterTests { return headers; } + private HttpHeaders requestHeaders(HttpHeaders originalHeaders) { + HttpHeaders headers = new HttpHeaders(); + headers.putAll(originalHeaders); + headers.add("X-Hello-Request", "Request World"); + headers.add("X-Auth-User", "aaaa"); + return headers; + } + }