Compile-time and load-time weaving support for aspects #5059

marcingrzejszczak · 2024-05-09T12:55:55Z

Adds support for CTW (compile time weaving) and LTW (load time weaving) support

mihalyr · 2024-05-10T11:28:29Z

What would be the best way to merge this with my changes from #5060 and https://github.com/mihalyr/micrometer/tree/fix-aspectj-ctw ?

There are a few things I noticed missing from this PR:

there are no load-time weaving tests with AspectJ
there are no class-level annotation tests
aspects will not work properly without the fixes in Fix AspectJ load-time weaving and class-level annotations #5060

I have all this addressed in #5060 and added also CTW in https://github.com/mihalyr/micrometer/tree/fix-aspectj-ctw. I named the module slightly differently micrometer-test-aspectj-ltw and micrometer-test-aspectj-ctw, but that's just a small detail. We could either merge #5060 to main and I will send another PR for your branch with the missing tests for CTW. Or I can change #5060 to be merged into your branch. But I would prefer merging #5060 to main first since that way your change would be more focused and limited to one issue at a time without the pointcut problems fixed in my PR.

But I didn't address the @Observed with the global ObservationRegistry, so that would need to be added for the LTW part too, which I can address in a PR also for your branch.

There is a thread-safety issue with the new Observations class that should be addressed too.

marcingrzejszczak · 2024-05-10T13:45:38Z

I think we can do everything in this PR. So please create a PR to the issues_#1149_ctw branch so that I can merge your changes.

mihalyr · 2024-05-10T14:02:23Z

Ok, no problem, just wanted to point out that my change in #5060 is a bugfix for the broken pointcut with AspectJ load-time weaving rather an enhancement as the compile-time weaving is.

marcingrzejszczak · 2024-05-10T14:17:36Z

I understand. Regardless, let's go with everything in that PR. I'll merge it to my branch and then will refine the branch.

mihalyr · 2024-05-10T15:20:40Z

@marcingrzejszczak here we go #5064

This works for me locally the only issue is that Spring aspect check that we know about - it should work with JDK17 and Spring 6.1.7-SNAPSHOT. ~~And of course the japicmp that was mentioned in my other PR.~~ UPDATE: The japicmp issue was fixed in the latest version.

marcingrzejszczak · 2024-05-15T11:53:23Z

build.gradle

@@ -408,6 +408,11 @@ subprojects {
    description = 'Application monitoring instrumentation facade'

    repositories {
+        // mavenLocal()


TODO: To be removed when 5.3.35 is out

marcingrzejszczak · 2024-05-15T11:54:03Z

gradle/libs.versions.toml

@@ -3,7 +3,7 @@ activemq-artemis = "2.33.0"
 application-insights = "2.6.4"
 archunit = "1.3.0"
 asmForPlugins = "7.3.1"
-aspectjweaver = "1.9.22.1"
+aspectjweaver = "1.9.20.1"


1.9.20.1 is the last release that accepts jdk 11 for building https://github.com/eclipse-aspectj/aspectj/blob/master/docs/release/JavaVersionCompatibility.adoc

marcingrzejszczak · 2024-05-15T11:54:28Z

gradle/libs.versions.toml

@@ -75,7 +75,7 @@ rest-assured = "5.4.0"
 signalfx = "1.0.41"
 slf4j = "1.7.36"
 spectator-atlas = "1.7.12"
-spring = "5.3.34"
+spring = "5.3.35-SNAPSHOT"


Change to 5.3.35 when released

mihalyr

Thank you! Looks good, just minor comments.

micrometer-observation/build.gradle

mihalyr · 2024-05-15T22:20:01Z

micrometer-observation/src/test/resources/META-INF/aop.xml

+  <aspects>
+    <!-- These are the two aspects we want to switch on for now. -->
+    <aspect name="io.micrometer.observation.aop.ObservedAspect"/>
+  </aspects>


I don't think this aop.xml file is being used, it should be only needed for LTW with AspectJ which is tested in the micrometer-test-aspectj-ltw module.

@kriegaex

* 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 <marcin@grzejszczak.pl>

Co-authored-by: Robert Mihaly <rob@mihalyr.com>

shakuzen · 2024-05-21T02:04:27Z

micrometer-core/build.gradle

@@ -81,6 +82,8 @@ dependencies {
    }

    // Aspects
+    implementation libs.aspectjrt


I don't think we want to add a transitive dependency for micrometer-core users (same for micrometer-observation). When/how is this dependency used?

Oh good catch, @mihalyr @kriegaex can you try to shed more light on this?

kriegaex · 2024-05-21T11:35:59Z

This is a known fact, and of course it is a dependency. I already explained long ago in #1149 (comment). Using annotations like @Aspect, @Before or classes like JoinPoint are from aspectjrt, so of course it is a dependency. So far, so obvious. Instead, you can remove the aspectjweaver dependency, because the latter is a significantly bigger superset of AspectJ runtime + LTW classes.

Why would you want to avoid the dependency? It is like e.g. using Log4J and trying to avoid declaring a dependency on the library. IMO, it does make a lot of sense to have this dependency. You always had it, both for Spring AOP and AspectJ. If you did not advertise it to users before, it was wrong to begin with and a bug.

Edit: In Maven, there is the concept of an optional dependency. If not all Micrometer users need Spring AOP or AspectJ aspects - I have no idea how to use Micrometer, not being an active user - maybe you can declare it as optional, if Gradle supports that. But I guess anyone using the aspect-driven Micrometer annotations absolutely needs the dependency. How else are the aspects supposed to work?

marcingrzejszczak · 2024-05-21T11:47:55Z

Yeah so that being optional would be the way to go. E.g. in Spring users need to explicitly say that they want aspect support by adding spring-boot-starter-aop dependency

kriegaex · 2024-05-21T11:51:35Z

Maybe in the future, you can split the library and create something like a micrometer-aspects library which has an explicit aspectjrt dependency. I believe that the whole concept of optional dependencies is error-prone, burdening users with adding information to the build configuration which the library should take care of. Then, you have a clear opt-in situation, which is also easier to document and clearly expressed in users' build configurations.

marcingrzejszczak · 2024-05-21T12:06:27Z

This commit makes the aspectj deps optional

mihalyr · 2024-05-21T13:16:55Z

I mentioned this also previously in #1149 (comment)

I also had to include aspectjrt as an implementation (compile-time) dependency, I think previously aspectjweaver was an optional dep (because you decided if you want to weave or not during load time, but it seems to be needed for CTW at build-time, I wonder if we could get away with compileOnly dep for this and users can include the runtime if they use weaving).

But for some reason my build failed when I only used it as an optional as it seemed CTW required the dependency during the build, not sure how does it work with the latest change, but I'm happy with it 😄

mihalyr · 2024-05-21T13:34:19Z

micrometer-core/build.gradle

-    java11Implementation libs.aspectjrt
-    optionalApi 'org.aspectj:aspectjweaver'
+    optionalApi libs.aspectjrt
+    java11RuntimeOnly libs.aspectjrt


this still adds this as a transient dependency for the Java 11 build, is that OK?

That was the closest I could get to optional for java11. I'm not an expert with Gradle 😢 cc @jonatan-ivanov

I am also not an expert with Gradle and don't know much about the project setup, just caught my 👁️ on the "runtime", but I made a little test with Java 11 locally and I don't see aspecjrt included, so it might be OK

runtimeClasspath - Runtime classpath of source set 'main'. +--- io.micrometer:micrometer-java11:1.14.0-SNAPSHOT | \--- io.micrometer:micrometer-core:1.14.0-SNAPSHOT | +--- io.micrometer:micrometer-commons:1.14.0-SNAPSHOT | +--- io.micrometer:micrometer-observation:1.14.0-SNAPSHOT | | \--- io.micrometer:micrometer-commons:1.14.0-SNAPSHOT | +--- org.hdrhistogram:HdrHistogram:2.2.1 | \--- org.latencyutils:LatencyUtils:2.0.3

marcingrzejszczak mentioned this pull request May 9, 2024

Compile-time weaving support for TimedAspect #1149

Open

marcingrzejszczak changed the title ~~Compile-time weaving support for aspects~~ Compile-time and load-time weaving support for aspects May 10, 2024

mihalyr mentioned this pull request May 10, 2024

Fix AspectJ load-time weaving and class-level annotations #5060

Closed

marcingrzejszczak force-pushed the issues_#1149_ctw branch from 65a1dca to b77a51d Compare May 15, 2024 11:33

marcingrzejszczak marked this pull request as ready for review May 15, 2024 11:51

marcingrzejszczak requested a review from shakuzen May 15, 2024 11:51

marcingrzejszczak commented May 15, 2024

View reviewed changes

mihalyr approved these changes May 15, 2024

View reviewed changes

marcingrzejszczak mentioned this pull request May 20, 2024

Fix AspectJ LTW and CTW #5064

Merged

marcingrzejszczak and others added 10 commits May 20, 2024 11:44

WIP

2a72ea9

Fixed checkstyle issues

dee1591

WIP

5dae0fc

Moved to spring snapshots, moved modules around

d1aac52

Downgrades aspectjweaver to the last requiring jdk11 to build

5470a2f

Removed snapshot references

365f45a

Update micrometer-observation/build.gradle

bbb5d11

Co-authored-by: Robert Mihaly <rob@mihalyr.com>

Apply suggestions from code review

546a174

Co-authored-by: Robert Mihaly <rob@mihalyr.com>

Removed aop.xml from micrometer obs test resources

6dd40dc

marcingrzejszczak force-pushed the issues_#1149_ctw branch from 87851de to 6dd40dc Compare May 20, 2024 09:44

shakuzen reviewed May 21, 2024

View reviewed changes

Made aspectj deps optional

1d06cc4

mihalyr reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile-time and load-time weaving support for aspects #5059

Compile-time and load-time weaving support for aspects #5059

marcingrzejszczak commented May 9, 2024 •

edited

mihalyr commented May 10, 2024

marcingrzejszczak commented May 10, 2024

mihalyr commented May 10, 2024

marcingrzejszczak commented May 10, 2024

mihalyr commented May 10, 2024 •

edited

marcingrzejszczak May 15, 2024

marcingrzejszczak May 15, 2024

marcingrzejszczak May 15, 2024

mihalyr left a comment

mihalyr May 15, 2024

shakuzen May 21, 2024

marcingrzejszczak May 21, 2024

kriegaex commented May 21, 2024 •

edited

marcingrzejszczak commented May 21, 2024

kriegaex commented May 21, 2024 •

edited

marcingrzejszczak commented May 21, 2024

mihalyr commented May 21, 2024 •

edited

mihalyr May 21, 2024

marcingrzejszczak May 21, 2024

mihalyr May 21, 2024 •

edited

Compile-time and load-time weaving support for aspects #5059

Are you sure you want to change the base?

Compile-time and load-time weaving support for aspects #5059

Conversation

marcingrzejszczak commented May 9, 2024 • edited

mihalyr commented May 10, 2024

marcingrzejszczak commented May 10, 2024

mihalyr commented May 10, 2024

marcingrzejszczak commented May 10, 2024

mihalyr commented May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriegaex commented May 21, 2024 • edited

marcingrzejszczak commented May 21, 2024

kriegaex commented May 21, 2024 • edited

marcingrzejszczak commented May 21, 2024

mihalyr commented May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalyr May 21, 2024 • edited

Choose a reason for hiding this comment

marcingrzejszczak commented May 9, 2024 •

edited

mihalyr commented May 10, 2024 •

edited

kriegaex commented May 21, 2024 •

edited

kriegaex commented May 21, 2024 •

edited

mihalyr commented May 21, 2024 •

edited

mihalyr May 21, 2024 •

edited