Ignore class level AOP annotation if method level present (#3811)

Previously, the method would be instrumented twice if there was an annotation at the class and method level. This is generally not what users want, and as a matter of best practice, we do not want to instrument the same thing multiple times. With this change, the class level annotation will be ignored if there is a method level annotation, resulting in instrumenting only once with the configuration on the method level annotation.
micrometer-metrics · Mar 1, 2024 · a7a0274 · a7a0274
1 parent 8604662
commit a7a0274
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 5 deletions.
diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java b/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java
@@ -69,6 +69,7 @@
  * @author Ali Dehghani
  * @author Jonatan Ivanov
  * @author Johnny Lim
+ * @author Yanming Zhou
  * @since 1.2.0
  * @see Counted
  */
@@ -166,7 +167,7 @@ public CountedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Itera
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.core.annotation.Counted)")
+    @Around("@within(io.micrometer.core.annotation.Counted) and not @annotation(io.micrometer.core.annotation.Counted)")
     @Nullable
     public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {

diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java b/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java
@@ -76,6 +76,7 @@
  * @author Johnny Lim
  * @author Nejc Korasa
  * @author Jonatan Ivanov
+ * @author Yanming Zhou
  * @since 1.0.0
  */
 @Aspect
@@ -160,7 +161,7 @@ public TimedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Iterabl
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.core.annotation.Timed)")
+    @Around("@within(io.micrometer.core.annotation.Timed) and not @annotation(io.micrometer.core.annotation.Timed)")
     @Nullable
     public Object timedClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {

diff --git a/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java b/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java
@@ -28,15 +28,15 @@
 import java.util.function.Predicate;
 
 import static java.util.concurrent.CompletableFuture.supplyAsync;
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.*;
 
 /**
  * Unit tests for the {@link CountedAspect} aspect.
  *
  * @author Ali Dehghani
  * @author Tommy Ludwig
  * @author Johnny Lim
+ * @author Yanming Zhou
  */
 class CountedAspectTest {
 
@@ -327,6 +327,24 @@ void countClassWithFailure() {
             .count()).isEqualTo(1);
     }
 
+    @Test
+    void ignoreClassLevelAnnotationIfMethodLevelPresent() {
+        CountedClassService service = getAdvisedService(new CountedClassService());
+
+        service.greet();
+
+        assertThatExceptionOfType(MeterNotFoundException.class)
+            .isThrownBy(() -> meterRegistry.get("class.counted").counter());
+
+        assertThat(meterRegistry.get("method.counted")
+            .tag("class", "io.micrometer.core.aop.CountedAspectTest$CountedClassService")
+            .tag("method", "greet")
+            .tag("result", "success")
+            .tag("exception", "none")
+            .counter()
+            .count()).isEqualTo(1);
+    }
+
     @Counted("class.counted")
     static class CountedClassService {
 
@@ -338,6 +356,11 @@ void fail() {
             throw new RuntimeException("Oops");
         }
 
+        @Counted("method.counted")
+        String greet() {
+            return "hello";
+        }
+
     }
 
 }
diff --git a/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java b/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java
@@ -374,6 +374,26 @@ void timeClassFailure() {
         });
     }
 
+    @Test
+    void ignoreClassLevelAnnotationIfMethodLevelPresent() {
+        MeterRegistry registry = new SimpleMeterRegistry();
+
+        AspectJProxyFactory pf = new AspectJProxyFactory(new TimedClass());
+        pf.addAspect(new TimedAspect(registry));
+
+        TimedClass service = pf.getProxy();
+
+        service.annotatedOnMethod();
+
+        assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> registry.get("call").timer());
+        assertThat(registry.get("annotatedOnMethod")
+            .tag("class", "io.micrometer.core.aop.TimedAspectTest$TimedClass")
+            .tag("method", "annotatedOnMethod")
+            .tag("extra", "tag2")
+            .timer()
+            .count()).isEqualTo(1);
+    }
+
     static class MeterTagsTests {
 
         ValueResolver valueResolver = parameter -> "Value from myCustomTagValueResolver [" + parameter + "]";
@@ -625,6 +645,10 @@ static class TimedClass {
         void call() {
         }
 
+        @Timed(value = "annotatedOnMethod", extraTags = { "extra", "tag2" })
+        void annotatedOnMethod() {
+        }
+
     }
 
     interface TimedInterface {

diff --git a/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java b/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java
@@ -68,6 +68,7 @@
  * </pre>
  *
  * @author Jonatan Ivanov
+ * @author Yanming Zhou
  * @since 1.10.0
  */
 @Aspect
@@ -104,7 +105,7 @@ public ObservedAspect(ObservationRegistry registry,
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.observation.annotation.Observed)")
+    @Around("@within(io.micrometer.observation.annotation.Observed) and not @annotation(io.micrometer.observation.annotation.Observed)")
     @Nullable
     public Object observeClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {

diff --git a/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java b/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java
@@ -335,6 +335,23 @@ void skipPredicateShouldTakeEffectForClass() {
         TestObservationRegistryAssert.assertThat(registry).doesNotHaveAnyObservation();
     }
 
+    @Test
+    void ignoreClassLevelAnnotationIfMethodLevelPresent() {
+        registry.observationConfig().observationHandler(new ObservationTextPublisher());
+
+        AspectJProxyFactory pf = new AspectJProxyFactory(new ObservedClassLevelAnnotatedService());
+        pf.addAspect(new ObservedAspect(registry));
+
+        ObservedClassLevelAnnotatedService service = pf.getProxy();
+        service.annotatedOnMethod();
+        TestObservationRegistryAssert.assertThat(registry)
+            .doesNotHaveAnyRemainingCurrentObservation()
+            .hasSingleObservationThat()
+            .hasBeenStopped()
+            .hasNameEqualTo("test.class")
+            .hasContextualNameEqualTo("test.class#annotatedOnMethod");
+    }
+
     static class ObservedService {
 
         @Observed(name = "test.call", contextualName = "test#call",
@@ -387,6 +404,10 @@ CompletableFuture<String> async(FakeAsyncTask fakeAsyncTask) {
                     contextSnapshot.wrapExecutor(Executors.newSingleThreadExecutor()));
         }
 
+        @Observed(name = "test.class", contextualName = "test.class#annotatedOnMethod")
+        void annotatedOnMethod() {
+        }
+
     }
 
     static class FakeAsyncTask implements Supplier<String> {