Fix AspectJ LTW and CTW (#5064)

* Revert "Fix AspectJ pointcut syntax (#5058)"

This reverts commit 5e16809.

* Fix AspectJ load-time weaving and class-level annotations

Fix AspectJ pointcut syntax to use `&& !` instead `and not` which is
invalid for the AspectJ compiler/weaver and only works with the Spring
AOP implementation. Also add `&& execution(* *.*(..))` to match only
methods, because the implementation assumes it gets only
MethodSignatures and crashes on ConstructorSignature at runtime.

Fixed the thread-safety and mutability issues with the singleton
Observations class, so changes are propagated to aspects that are
initialized only once.

Added AspectJ load-time weaving tests to make sure that the further
issues with pointcuts and aspects are noticed at build time.

* Add more AspectJ compile-time tests

Added more class-level annotation tests and moved the module to
'micrometer-test-aspectj-ctw' to align with
'micrometer-test-aspectj-ltw'.

* Revert CountedAspect method change with simpler syntax

A fix kindly provided by @kriegaex that avoids changing the method
signature and thus breaking binary compatibility and still fixes the
problem with double counting in AspectJ.

See the explanation in
#1149 (comment)

---------

Co-authored-by: Marcin Grzejszczak <[email protected]>

build.gradle

@@ -337,7 +337,7 @@ subprojects {
 
             check.dependsOn("testModules")
 
-            if (!(project.name in ['micrometer-test-ctw'])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp
+            if (!(project.name in ['micrometer-test-aspectj-ltw', 'micrometer-test-aspectj-ctw'])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp
                 apply plugin: 'me.champeau.gradle.japicmp'
                 apply plugin: 'de.undercouch.download'
 

gradle/libs.versions.toml

@@ -89,6 +89,7 @@ activemqArtemisJunit5 = { module = "org.apache.activemq:artemis-junit-5", versio
 applicationInsights = { module = "com.microsoft.azure:applicationinsights-core", version.ref = "application-insights" }
 archunitJunit5 = { module = "com.tngtech.archunit:archunit-junit5", version.ref = "archunit" }
 asmForPlugins = { module = "org.ow2.asm:asm", version.ref = "asmForPlugins" }
+aspectjrt = { module = "org.aspectj:aspectjrt", version.ref = "aspectjweaver" }
 aspectjweaver = { module = "org.aspectj:aspectjweaver", version.ref = "aspectjweaver" }
 assertj = { module = "org.assertj:assertj-core", version.ref = "assertj" }
 awaitility = { module = "org.awaitility:awaitility", version.ref = "awaitility" }
@@ -232,3 +233,4 @@ plugin-bnd = "biz.aQute.bnd:biz.aQute.bnd.gradle:6.4.0"
 kotlin19 = { id = "org.jetbrains.kotlin.jvm", version = "1.9.23" }
 kotlin17 = { id = "org.jetbrains.kotlin.jvm", version = "1.7.22" }
 jcstress = { id = "io.github.reyerizo.gradle.jcstress", version = "0.8.15" }
+aspectj = { id = 'io.freefair.aspectj.post-compile-weaving', version = '8.6' }

micrometer-core/build.gradle

@@ -1,7 +1,7 @@
 plugins {
     alias(libs.plugins.kotlin19)
+    alias(libs.plugins.aspectj)
     id 'me.champeau.mrjar' version "0.1.1"
-    id 'io.freefair.aspectj.post-compile-weaving' version '8.6'
 }
 
 description 'Core module of Micrometer containing instrumentation API and implementation'
@@ -82,8 +82,8 @@ dependencies {
     }
 
     // Aspects
-    implementation "org.aspectj:aspectjrt:${libs.versions.aspectjweaver.get()}"
-    java11Implementation "org.aspectj:aspectjrt:${libs.versions.aspectjweaver.get()}"
+    implementation libs.aspectjrt
+    java11Implementation libs.aspectjrt
     optionalApi 'org.aspectj:aspectjweaver'
 
     // instrumentation options

micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java

@@ -167,7 +167,7 @@ public CountedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Itera
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.core.annotation.Counted) and not @annotation(io.micrometer.core.annotation.Counted)")
+    @Around("@within(io.micrometer.core.annotation.Counted) && !@annotation(io.micrometer.core.annotation.Counted) && execution(* *(..))")
     @Nullable
     public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {
@@ -202,7 +202,7 @@ public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
      * @return Whatever the intercepted method returns.
      * @throws Throwable When the intercepted method throws one.
      */
-    @Around(value = "@annotation(counted)", argNames = "pjp,counted")
+    @Around(value = "@annotation(counted) && execution(* *(..))", argNames = "pjp,counted")
     @Nullable
     public Object interceptAndRecord(ProceedingJoinPoint pjp, Counted counted) throws Throwable {
         if (shouldSkip.test(pjp)) {

micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java

@@ -161,7 +161,7 @@ public TimedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Iterabl
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.core.annotation.Timed) and not @annotation(io.micrometer.core.annotation.Timed)")
+    @Around("@within(io.micrometer.core.annotation.Timed) && !@annotation(io.micrometer.core.annotation.Timed) && execution(* *(..))")
     @Nullable
     public Object timedClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {

micrometer-observation/src/main/java/io/micrometer/observation/Observations.java

@@ -15,6 +15,11 @@
  */
 package io.micrometer.observation;
 
+import io.micrometer.common.lang.Nullable;
+
+import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
+
 /**
  * Generator of observations bound to a static global registry. For use especially in
  * places where dependency injection of {@link ObservationRegistry} is not possible for an
@@ -27,33 +32,74 @@ public final class Observations {
 
     private static final ObservationRegistry initialRegistry = ObservationRegistry.create();
 
-    private static ObservationRegistry globalRegistry = initialRegistry;
+    private static final DelegatingObservationRegistry globalRegistry =
+        new DelegatingObservationRegistry(initialRegistry);
 
     private Observations() {
         throw new UnsupportedOperationException("You can't instantiate a utility class");
     }
 
     /**
      * Sets a registry as the global registry.
+     *
      * @param registry Registry to set.
      */
     public static void setRegistry(ObservationRegistry registry) {
-        globalRegistry = registry;
+        globalRegistry.setDelegate(registry);
     }
 
     /**
      * Resets registry to the original, empty one.
      */
     public static void resetRegistry() {
-        globalRegistry = initialRegistry;
+        globalRegistry.setDelegate(initialRegistry);
     }
 
     /**
      * Retrieves the current global instance.
+     *
      * @return Global registry.
      */
     public static ObservationRegistry getGlobalRegistry() {
         return globalRegistry;
     }
 
+    private static final class DelegatingObservationRegistry implements ObservationRegistry {
+        private final AtomicReference<ObservationRegistry> delegate = new AtomicReference<>(ObservationRegistry.NOOP);
+
+        DelegatingObservationRegistry(ObservationRegistry delegate) {
+            setDelegate(delegate);
+        }
+
+        void setDelegate(ObservationRegistry delegate) {
+            this.delegate.set(Objects.requireNonNull(delegate, "Delegate must not be null"));
+        }
+
+        @Nullable
+        @Override
+        public Observation getCurrentObservation() {
+            return delegate.get().getCurrentObservation();
+        }
+
+        @Nullable
+        @Override
+        public Observation.Scope getCurrentObservationScope() {
+            return delegate.get().getCurrentObservationScope();
+        }
+
+        @Override
+        public void setCurrentObservationScope(@Nullable Observation.Scope current) {
+            delegate.get().setCurrentObservationScope(current);
+        }
+
+        @Override
+        public ObservationConfig observationConfig() {
+            return delegate.get().observationConfig();
+        }
+
+        @Override
+        public boolean isNoop() {
+            return delegate.get().isNoop();
+        }
+    }
 }

micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java

@@ -111,7 +111,7 @@ public ObservedAspect(ObservationRegistry registry,
         this.shouldSkip = shouldSkip;
     }
 
-    @Around("@within(io.micrometer.observation.annotation.Observed) and not @annotation(io.micrometer.observation.annotation.Observed)")
+    @Around("@within(io.micrometer.observation.annotation.Observed) && !@annotation(io.micrometer.observation.annotation.Observed) && execution(* *.*(..))")
     @Nullable
     public Object observeClass(ProceedingJoinPoint pjp) throws Throwable {
         if (shouldSkip.test(pjp)) {

micrometer-test-ctw/build.gradle → micrometer-test-aspectj-ctw/build.gradle

@@ -1,6 +1,6 @@
 plugins {
     id 'java'
-    id 'io.freefair.aspectj.post-compile-weaving' version '8.6'
+    alias(libs.plugins.aspectj)
 }
 
 description 'AspectJ compile-time weaving test for Micrometer aspects'
@@ -10,8 +10,8 @@ dependencies {
     aspect project(':micrometer-core')
     aspect project(':micrometer-observation')
 
-    testImplementation 'org.junit.jupiter:junit-jupiter'
-    testImplementation 'org.assertj:assertj-core'
+    testImplementation libs.junitJupiter
+    testImplementation libs.assertj
 }
 
 test {

micrometer-test-ctw/src/main/java/io/micrometer/test/ctw/MeasuredClass.java → micrometer-test-aspectj-ctw/src/main/java/io/micrometer/test/ctw/MeasuredClass.java

@@ -19,6 +19,9 @@
 import io.micrometer.core.annotation.Timed;
 import io.micrometer.observation.annotation.Observed;
 
+@Observed
+@Counted
+@Timed
 public class MeasuredClass {
 
     @Timed
@@ -33,4 +36,13 @@ public void countedMethod() {
     public void observedMethod() {
     }
 
+    public void classLevelTimedMethod() {
+    }
+
+    public void classLevelCountedMethod() {
+    }
+
+    public void classLevelObservedMethod() {
+    }
+
 }

micrometer-test-ctw/src/test/java/io/micrometer/test/ctw/MeasuredClassTest.java → micrometer-test-aspectj-ctw/src/test/java/io/micrometer/test/ctw/MeasuredClassTest.java

@@ -111,4 +111,60 @@ void shouldWrapMethodWithObservedAspectThroughCTW() {
         then(timer.count()).isEqualTo(2);
     }
 
+    @Test
+    void shouldWrapMethodWithClassLevelTimedAspectThroughCTW() {
+        // when
+        measured.classLevelTimedMethod();
+        // then
+        Collection<Timer> timers = registry.find(TimedAspect.DEFAULT_METRIC_NAME)
+            .tag("class", MeasuredClass.class.getName())
+            .tag("method", "classLevelTimedMethod")
+            .timers();
+        then(timers).hasSize(1);
+        Timer timer = timers.iterator().next();
+        then(timer.count()).isEqualTo(1);
+
+        // when
+        measured.classLevelTimedMethod();
+        // then
+        then(timer.count()).isEqualTo(2);
+    }
+
+    @Test
+    void shouldWrapMethodWithClassLevelCountedAspectThroughCTW() {
+        // when
+        measured.classLevelCountedMethod();
+        // then
+        Collection<Counter> counters = registry.find("method.counted")
+            .tag("class", MeasuredClass.class.getName())
+            .tag("method", "classLevelCountedMethod")
+            .counters();
+        then(counters).hasSize(1);
+        Counter counter = counters.iterator().next();
+        then(counter.count()).isEqualTo(1);
+
+        // when
+        measured.classLevelCountedMethod();
+        // then
+        then(counter.count()).isEqualTo(2);
+    }
+
+    @Test
+    void shouldWrapMethodWithClassLevelObservedAspectThroughCTW() {
+        // when
+        measured.classLevelObservedMethod();
+        // then
+        Collection<Timer> timers = registry.find("method.observed")
+            .tag("class", MeasuredClass.class.getName())
+            .tag("method", "classLevelObservedMethod")
+            .timers();
+        then(timers).hasSize(1);
+        Timer timer = timers.iterator().next();
+        then(timer.count()).isEqualTo(1);
+
+        // when
+        measured.classLevelObservedMethod();
+        // then
+        then(timer.count()).isEqualTo(2);
+    }
 }

micrometer-test-aspectj-ltw/build.gradle

@@ -0,0 +1,23 @@
+plugins {
+    id 'java'
+}
+
+description 'AspectJ load-time weaving test for Micrometer aspects'
+
+configurations {
+    agents
+}
+
+dependencies {
+    agents libs.aspectjweaver
+    implementation project(':micrometer-core')
+    implementation project(':micrometer-observation')
+
+    testImplementation libs.junitJupiter
+    testImplementation libs.assertj
+}
+
+test {
+    useJUnitPlatform()
+    jvmArgs '-javaagent:' + configurations.agents.files.find { it.name.startsWith('aspectjweaver') }
+}

micrometer-test-aspectj-ltw/src/main/java/io/micrometer/test/ltw/MeasuredClass.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2024 VMware, Inc.
+ *
+ * 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 io.micrometer.test.ltw;
+
+import io.micrometer.core.annotation.Counted;
+import io.micrometer.core.annotation.Timed;
+import io.micrometer.observation.annotation.Observed;
+
+@Observed
+@Counted
+@Timed
+public class MeasuredClass {
+
+    @Timed
+    public void timedMethod() {
+    }
+
+    @Counted
+    public void countedMethod() {
+    }
+
+    @Observed
+    public void observedMethod() {
+    }
+
+    public void classLevelTimedMethod() {
+    }
+
+    public void classLevelCountedMethod() {
+    }
+
+    public void classLevelObservedMethod() {
+    }
+
+}

micrometer-test-aspectj-ltw/src/main/resources/META-INF/aop.xml

@@ -0,0 +1,30 @@
+<!--
+
+    Copyright 2024 VMware, Inc.
+
+    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.
+
+-->
+<!DOCTYPE aspectj PUBLIC "-//AspectJ//DTD//EN" "https://eclipse.dev/aspectj/dtd/aspectj.dtd">
+<aspectj>
+  <weaver options="-verbose -showWeaveInfo">
+    <include within="io.micrometer.core.aop.*"/>
+    <include within="io.micrometer.observation.aop.*"/>
+    <include within="io.micrometer.test.ltw.MeasuredClass"/>
+  </weaver>
+  <aspects>
+    <aspect name="io.micrometer.core.aop.CountedAspect"/>
+    <aspect name="io.micrometer.core.aop.TimedAspect"/>
+    <aspect name="io.micrometer.observation.aop.ObservedAspect"/>
+  </aspects>
+</aspectj>