From a3c0f816713f2606acaaa39a51b3ec444c73ceb0 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 21 Nov 2016 16:41:20 +0100 Subject: [PATCH] Fixed wrong proxying of executors without this change tracing worked fine but the custom types were not registered as beans. Thus autowiring of them was not possible with this change the bean post processor is removed and an aspect is used - that way tracing is still working fine but we don't interfere in bean registration fixes #445 --- .../async/AsyncDefaultAutoConfiguration.java | 9 +-- .../LazyTraceThreadPoolTaskExecutor.java | 6 ++ .../instrument/async/TraceAsyncAspect.java | 42 ++++++++++++- .../async/TraceExecutorBeanPostProcessor.java | 60 ------------------- .../async/issues/issue410/Issue410Tests.java | 25 ++++---- 5 files changed, 60 insertions(+), 82 deletions(-) delete mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceExecutorBeanPostProcessor.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/AsyncDefaultAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/AsyncDefaultAutoConfiguration.java index 38b5d9b65..fc4c7ac26 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/AsyncDefaultAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/AsyncDefaultAutoConfiguration.java @@ -51,6 +51,8 @@ import org.springframework.scheduling.annotation.EnableAsync; @AutoConfigureAfter(AsyncCustomAutoConfiguration.class) public class AsyncDefaultAutoConfiguration { + @Autowired private BeanFactory beanFactory; + @Configuration @ConditionalOnMissingBean(AsyncConfigurer.class) @ConditionalOnProperty(value = "spring.sleuth.async.configurer.enabled", matchIfMissing = true) @@ -66,12 +68,7 @@ public class AsyncDefaultAutoConfiguration { @Bean public TraceAsyncAspect traceAsyncAspect(Tracer tracer, TraceKeys traceKeys) { - return new TraceAsyncAspect(tracer, traceKeys); - } - - @Bean - public TraceExecutorBeanPostProcessor traceExecutorBeanPostProcessor(BeanFactory beanFactory) { - return new TraceExecutorBeanPostProcessor(beanFactory); + return new TraceAsyncAspect(tracer, traceKeys, this.beanFactory); } } \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/LazyTraceThreadPoolTaskExecutor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/LazyTraceThreadPoolTaskExecutor.java index b09925bab..30d83f720 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/LazyTraceThreadPoolTaskExecutor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/LazyTraceThreadPoolTaskExecutor.java @@ -19,6 +19,7 @@ package org.springframework.cloud.sleuth.instrument.async; import java.lang.invoke.MethodHandles; import java.util.concurrent.Callable; import java.util.concurrent.Future; +import java.util.concurrent.ThreadPoolExecutor; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -84,6 +85,11 @@ public class LazyTraceThreadPoolTaskExecutor extends ThreadPoolTaskExecutor { return this.delegate.submitListenable(new SpanContinuingTraceCallable<>(tracer(), traceKeys(), spanNamer(), task)); } + @Override + public ThreadPoolExecutor getThreadPoolExecutor() throws IllegalStateException { + return this.delegate.getThreadPoolExecutor(); + } + public void destroy() { this.delegate.destroy(); super.destroy(); diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceAsyncAspect.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceAsyncAspect.java index 361a5c1f3..76aa0bf4c 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceAsyncAspect.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceAsyncAspect.java @@ -16,13 +16,20 @@ package org.springframework.cloud.sleuth.instrument.async; +import java.lang.reflect.Method; +import java.util.concurrent.Executor; + import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; +import org.aspectj.lang.reflect.MethodSignature; +import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.Span; -import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.TraceKeys; +import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.util.SpanNameUtil; +import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; +import org.springframework.util.ReflectionUtils; /** * Aspect that creates a new Span for running threads executing methods annotated with @@ -40,10 +47,12 @@ public class TraceAsyncAspect { private final Tracer tracer; private final TraceKeys traceKeys; + private final BeanFactory beanFactory; - public TraceAsyncAspect(Tracer tracer, TraceKeys traceKeys) { + public TraceAsyncAspect(Tracer tracer, TraceKeys traceKeys, BeanFactory beanFactory) { this.tracer = tracer; this.traceKeys = traceKeys; + this.beanFactory = beanFactory; } @Around("execution (@org.springframework.scheduling.annotation.Async * *.*(..))") @@ -62,4 +71,33 @@ public class TraceAsyncAspect { } } + @Around("execution (* org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor.*(..))") + public Object traceThreadPoolTaskExecutor(final ProceedingJoinPoint pjp) throws Throwable { + LazyTraceThreadPoolTaskExecutor executor = new LazyTraceThreadPoolTaskExecutor(this.beanFactory, + (ThreadPoolTaskExecutor) pjp.getTarget()); + Method methodOnTracedBean = getMethod(pjp, executor); + if (methodOnTracedBean != null) { + return methodOnTracedBean.invoke(executor, pjp.getArgs()); + } + return pjp.proceed(); + } + + @Around("execution (* java.util.concurrent.Executor.*(..))") + public Object traceExecutor(final ProceedingJoinPoint pjp) throws Throwable { + LazyTraceExecutor executor = new LazyTraceExecutor(this.beanFactory, + (Executor) pjp.getTarget()); + Method methodOnTracedBean = getMethod(pjp, executor); + if (methodOnTracedBean != null) { + return methodOnTracedBean.invoke(executor, pjp.getArgs()); + } + return pjp.proceed(); + } + + private Method getMethod(ProceedingJoinPoint pjp, Object object) { + MethodSignature signature = (MethodSignature) pjp.getSignature(); + Method method = signature.getMethod(); + return ReflectionUtils + .findMethod(object.getClass(), method.getName(), method.getParameterTypes()); + } + } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceExecutorBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceExecutorBeanPostProcessor.java deleted file mode 100644 index 3d59e9e48..000000000 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/async/TraceExecutorBeanPostProcessor.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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.async; - -import java.util.concurrent.Executor; -import java.util.concurrent.ExecutorService; - -import org.springframework.beans.BeansException; -import org.springframework.beans.factory.BeanFactory; -import org.springframework.beans.factory.config.BeanPostProcessor; -import org.springframework.scheduling.TaskScheduler; -import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor; - -/** - * Bean post processor that wraps {@link Executor} in its Trace representation - * - * @author Marcin Grzejszczak - * @since 1.0.10 - */ -class TraceExecutorBeanPostProcessor implements BeanPostProcessor { - - private final BeanFactory beanFactory; - - public TraceExecutorBeanPostProcessor(BeanFactory beanFactory) { - this.beanFactory = beanFactory; - } - - @Override public Object postProcessBeforeInitialization(Object bean, String beanName) - throws BeansException { - if (bean instanceof ThreadPoolTaskExecutor && !(bean instanceof TaskScheduler) && - !(bean instanceof LazyTraceThreadPoolTaskExecutor)) { - return new LazyTraceThreadPoolTaskExecutor(this.beanFactory, (ThreadPoolTaskExecutor) bean); - } else if (bean instanceof Executor && !(bean instanceof ExecutorService) && - !(bean instanceof TaskScheduler) && !(bean instanceof LazyTraceExecutor)) { - return new LazyTraceExecutor(this.beanFactory, (Executor) bean); - } else if (bean instanceof ExecutorService) { - return new TraceableExecutorService(this.beanFactory, (ExecutorService) bean); - } - return bean; - } - - @Override public Object postProcessAfterInitialization(Object bean, String beanName) - throws BeansException { - return bean; - } -} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java index 272230de7..c7572f627 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/async/issues/issue410/Issue410Tests.java @@ -19,8 +19,6 @@ package org.springframework.cloud.sleuth.instrument.async.issues.issue410; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.logging.Log; @@ -38,7 +36,6 @@ import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.instrument.async.LazyTraceExecutor; -import org.springframework.cloud.sleuth.instrument.async.TraceableExecutorService; import org.springframework.cloud.sleuth.sampler.AlwaysSampler; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -73,7 +70,7 @@ public class Issue410Tests { /** * Related to issue #445 */ - @Autowired ExecutorService executorService; + @Autowired Application.MyService executorService; @Test public void should_pass_tracing_info_for_tasks_running_without_a_pool() { @@ -145,14 +142,6 @@ public class Issue410Tests { } } - /** - * Related to issue #445 - */ - @Test - public void should_wrap_executor_service_in_trace_representation() { - then(this.executorService).isInstanceOf(TraceableExecutorService.class); - } - private int port() { return this.environment.getProperty("local.server.port", Integer.class); } @@ -299,8 +288,16 @@ class Application { /** * Related to issue #445 */ - @Bean public ExecutorService executorService() { - return Executors.newSingleThreadExecutor(); + @Bean public MyService executorService() { + return new MyService() { + @Override public void execute(Runnable command) { + + } + }; + } + + interface MyService extends Executor { + } }