From 0c3cdc856b22589c1cdd3e2ef7f147695e749a0c Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 21 May 2024 23:59:29 +0200 Subject: [PATCH] GH-625 - Make sure we always place the ModuleEntryInterceptor after the AsyncAnnotationAdvisor. To properly create spans for the actual method invocation, not the async dispatch. --- .../ModuleTracingBeanPostProcessor.java | 43 ++++++--- .../observability/ModuleTracingSupport.java | 20 ++++- .../test/java/example/ExampleApplication.java | 2 + .../java/example/sample/SampleComponent.java | 4 +- ...cingBeanPostProcessorIntegrationTests.java | 87 +++++++++++++++++++ .../ObservedModuleTypeUnitTests.java | 2 +- 6 files changed, 140 insertions(+), 18 deletions(-) create mode 100644 spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessorIntegrationTests.java diff --git a/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessor.java b/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessor.java index 5fa06529..66e00619 100644 --- a/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessor.java +++ b/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessor.java @@ -18,11 +18,13 @@ package org.springframework.modulith.observability; import io.micrometer.tracing.Tracer; import java.lang.reflect.Method; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; import java.util.function.Supplier; import org.springframework.aop.Advisor; +import org.springframework.aop.framework.Advised; import org.springframework.aop.support.ComposablePointcut; import org.springframework.aop.support.DefaultPointcutAdvisor; import org.springframework.aop.support.StaticMethodMatcher; @@ -80,19 +82,21 @@ public class ModuleTracingBeanPostProcessor extends ModuleTracingSupport impleme return bean; } + if (alreadyAdvised(bean)) { + return bean; + } + var modules = runtime.get(); - return modules.getModuleByType(type.getName()) - .map(DefaultObservedModule::new) - .map(it -> { + return modules.getModuleByType(type.getName()).map(DefaultObservedModule::new).map(it -> { - var moduleType = it.getObservedModuleType(type, modules); + var moduleType = it.getObservedModuleType(type, modules); - return moduleType != null // - ? addAdvisor(bean, getOrBuildAdvisor(it, moduleType)) // - : bean; + return moduleType != null // + ? addAdvisor(bean, getOrBuildAdvisor(it, moduleType)) // + : bean; - }).orElse(bean); + }).orElse(bean); } private boolean isInfrastructureBean(String beanName) { @@ -104,15 +108,17 @@ public class ModuleTracingBeanPostProcessor extends ModuleTracingSupport impleme private Advisor getOrBuildAdvisor(ObservedModule module, ObservedModuleType type) { return advisors.computeIfAbsent(module.getName(), __ -> { - - var interceptor = ModuleEntryInterceptor.of(module, tracer.get()); - var matcher = new ObservableTypeMethodMatcher(type); - var pointcut = new ComposablePointcut(matcher); - - return new DefaultPointcutAdvisor(pointcut, interceptor); + return new ApplicationModuleObservingAdvisor(type, ModuleEntryInterceptor.of(module, tracer.get())); }); } + private static boolean alreadyAdvised(Object bean) { + + return bean instanceof Advised advised + && Arrays.stream(advised.getAdvisors()) + .anyMatch(ApplicationModuleObservingAdvisor.class::isInstance); + } + private static class ObservableTypeMethodMatcher extends StaticMethodMatcher { private final ObservedModuleType type; @@ -138,4 +144,13 @@ public class ModuleTracingBeanPostProcessor extends ModuleTracingSupport impleme return type.getMethodsToIntercept().test(method); } } + + static class ApplicationModuleObservingAdvisor extends DefaultPointcutAdvisor { + + private static final long serialVersionUID = -391548409986032658L; + + public ApplicationModuleObservingAdvisor(ObservedModuleType type, ModuleEntryInterceptor interceptor) { + super(new ComposablePointcut(new ObservableTypeMethodMatcher(type)), interceptor); + } + } } diff --git a/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingSupport.java b/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingSupport.java index c0504d26..c951d51e 100644 --- a/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingSupport.java +++ b/spring-modulith-observability/src/main/java/org/springframework/modulith/observability/ModuleTracingSupport.java @@ -21,6 +21,7 @@ import org.springframework.aop.Advisor; import org.springframework.aop.framework.Advised; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.factory.BeanClassLoaderAware; +import org.springframework.scheduling.annotation.AsyncAnnotationAdvisor; /** * @author Oliver Drotbohm @@ -44,9 +45,10 @@ class ModuleTracingSupport implements BeanClassLoaderAware { protected final Object addAdvisor(Object bean, Advisor advisor, Consumer customizer) { - if (Advised.class.isInstance(bean)) { + if (bean instanceof Advised advised) { + + advised.addAdvisor(asyncAdvisorIndex(advised) + 1, advisor); - ((Advised) bean).addAdvisor(0, advisor); return bean; } else { @@ -58,4 +60,18 @@ class ModuleTracingSupport implements BeanClassLoaderAware { return factory.getProxy(classLoader); } } + + private static int asyncAdvisorIndex(Advised advised) { + + Advisor[] advisors = advised.getAdvisors(); + + for (int i = 0; i < advised.getAdvisorCount(); i++) { + + if (advisors[i] instanceof AsyncAnnotationAdvisor) { + return i; + } + } + + return -1; + } } diff --git a/spring-modulith-observability/src/test/java/example/ExampleApplication.java b/spring-modulith-observability/src/test/java/example/ExampleApplication.java index 99d7411d..6d1d4c63 100644 --- a/spring-modulith-observability/src/test/java/example/ExampleApplication.java +++ b/spring-modulith-observability/src/test/java/example/ExampleApplication.java @@ -17,10 +17,12 @@ package example; import org.springframework.boot.SpringApplication; import org.springframework.boot.autoconfigure.SpringBootApplication; +import org.springframework.scheduling.annotation.EnableAsync; /** * @author Oliver Drotbohm */ +@EnableAsync @SpringBootApplication public class ExampleApplication { diff --git a/spring-modulith-observability/src/test/java/example/sample/SampleComponent.java b/spring-modulith-observability/src/test/java/example/sample/SampleComponent.java index c249eb40..223c3968 100644 --- a/spring-modulith-observability/src/test/java/example/sample/SampleComponent.java +++ b/spring-modulith-observability/src/test/java/example/sample/SampleComponent.java @@ -15,6 +15,7 @@ */ package example.sample; +import org.springframework.scheduling.annotation.Async; import org.springframework.stereotype.Component; /** @@ -23,5 +24,6 @@ import org.springframework.stereotype.Component; @Component public class SampleComponent { - void someMethod() {} + @Async + public void someMethod() {} } diff --git a/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessorIntegrationTests.java b/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessorIntegrationTests.java new file mode 100644 index 00000000..3e12cd76 --- /dev/null +++ b/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ModuleTracingBeanPostProcessorIntegrationTests.java @@ -0,0 +1,87 @@ +/* + * Copyright 2024 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.modulith.observability; + +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; + +import example.ExampleApplication; +import example.sample.SampleComponent; +import io.micrometer.tracing.Tracer; + +import org.junit.jupiter.api.Test; +import org.springframework.aop.Advisor; +import org.springframework.aop.framework.Advised; +import org.springframework.boot.SpringApplication; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.modulith.observability.ModuleTracingBeanPostProcessor.ApplicationModuleObservingAdvisor; +import org.springframework.modulith.runtime.ApplicationModulesRuntime; +import org.springframework.modulith.runtime.ApplicationRuntime; +import org.springframework.modulith.test.TestApplicationModules; +import org.springframework.scheduling.annotation.AsyncAnnotationAdvisor; + +/** + * Integration tests for {@link ModuleTracingBeanPostProcessor}. + * + * @author Oliver Drotbohm + */ +class ModuleTracingBeanPostProcessorIntegrationTests { + + @Test + void decoratesExposedComponentsWithTracingInterceptor() { + + SampleComponent bean = SpringApplication + .run(new Class[] { ExampleApplication.class, ModuleTracingConfiguration.class }, new String[] {}) + .getBean(SampleComponent.class); + + assertThat(bean).isInstanceOfSatisfying(Advised.class, it -> { + + var advisors = it.getAdvisors(); + + var asyncIndex = advisorIndex(AsyncAnnotationAdvisor.class, advisors); + var tracingIndex = advisorIndex(ApplicationModuleObservingAdvisor.class, advisors); + + assertThat(tracingIndex).isGreaterThan(asyncIndex); + }); + } + + @Configuration + static class ModuleTracingConfiguration { + + @Bean + ModuleTracingBeanPostProcessor foo(ConfigurableApplicationContext context) { + + var runtime = ApplicationRuntime.of(context); + var modulesRuntime = new ApplicationModulesRuntime(() -> TestApplicationModules.of(ExampleApplication.class), + runtime); + + return new ModuleTracingBeanPostProcessor(modulesRuntime, () -> mock(Tracer.class), context.getBeanFactory()); + } + } + + private static int advisorIndex(Class type, Advisor[] advisors) { + + for (int i = 0; i < advisors.length; i++) { + if (type.isInstance(advisors[i])) { + return i; + } + } + + throw new AssertionError("No advisor of type %s found!".formatted(type.getName())); + } +} diff --git a/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ObservedModuleTypeUnitTests.java b/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ObservedModuleTypeUnitTests.java index 08065705..317c0305 100644 --- a/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ObservedModuleTypeUnitTests.java +++ b/spring-modulith-observability/src/test/java/org/springframework/modulith/observability/ObservedModuleTypeUnitTests.java @@ -33,7 +33,7 @@ import org.springframework.util.ReflectionUtils; * * @author Oliver Drotbohm */ -public class ObservedModuleTypeUnitTests { +class ObservedModuleTypeUnitTests { static final ApplicationModules modules = TestApplicationModules.of("example");