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.

build.gradle

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

If we want to avoid bumping this version in the future, we should leave a comment here so we know why. I'll add it as a polish commit.

gradle/libs.versions.toml

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.

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

I don't think we need the dependency at all for the java11 sources in micrometer-core. There is no aspect-related code there, and the code that is there is deprecated in favor of the separate module micrometer-java11.

If I do that then we get 😞

* What went wrong: Execution failed for task ':micrometer-core:compileJava11Java'. > Cannot infer AspectJ class path because no AspectJ Jar was found on class path: configuration ':micrometer-core:java11RuntimeClasspath'

shakuzen

The two new test modules should be configured to not be published. Other than that, and my comment about the java11 dependency, the changes look good to me.

kriegaex · 2024-06-07T15:34:41Z

There is TimedAspect in Core and ObservedAspect in Observation. Why are you tring to avoid a dependency that clearly exists? Why are you trying to eat the cake and keep it at the same time? Chances are, there is something I do not understand, but to me this does not make much sense. Forgive me, if I am too stupid to understand this.

marcingrzejszczak · 2024-06-10T15:00:18Z

I mean we do have those classes but you don't need to use them. E.g. in Spring Boot not a single aspect will be registered if the aop dependencies are not on the classpath.

marcingrzejszczak · 2024-06-10T15:49:27Z

I've added a commit about not deploying the aspectj test modules. Hopefully now it's ok?

@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 <[email protected]>

Co-authored-by: Robert Mihaly <[email protected]>

shakuzen · 2024-06-19T07:12:12Z

I rebased and resolved the conflicts. I think this is ready to be merged.

Why are you tring to avoid a dependency that clearly exists? Why are you trying to eat the cake and keep it at the same time? Chances are, there is something I do not understand, but to me this does not make much sense.

I think your points are reasonable. Historically, Micrometer has not taken such an approach except where we have no choice due to a conflict (see micrometer-jakarta9, micrometer-jetty12, etc). Almost all of the optional dependencies in micrometer-core are libraries that Micrometer maintains instrumentation for in micrometer-core. The aspects are a bit different in nature, and perhaps a different approach would have been better for them, but TimedAspect at least has been there since 1.0.0. We could consider making a separate aspect module and migrating users to there from the aspects in micrometer-core and micrometer-observation now, but that will be a long process and it should be considered separate of this work since this can still provide benefit without otherwise changing the current arrangement.

…spectj-ltw This commit also polishes the changes made in micrometer-metricsgh-5059.

…spectj-ltw (#5401) This commit also polishes the changes made in gh-5059. Excludes the modules from the BOM so they don't end up in there despite not being published. * Disable MeasuredClassTest on JDK 17+ temporarily See #5403

marcingrzejszczak mentioned this pull request May 9, 2024

Compile-time weaving support for aspects #1149

Closed

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

build.gradle Outdated Show resolved Hide resolved

marcingrzejszczak commented May 15, 2024

View reviewed changes

gradle/libs.versions.toml Outdated Show resolved Hide resolved

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 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

mihalyr reviewed May 21, 2024

View reviewed changes

shakuzen reviewed Jun 7, 2024

View reviewed changes

marcingrzejszczak added 2 commits June 19, 2024 15:44

WIP

1a4dd16

Fixed checkstyle issues

29b003f

mihalyr and others added 10 commits June 19, 2024 15:45

WIP

34b3bcf

Moved to spring snapshots, moved modules around

037305d

Downgrades aspectjweaver to the last requiring jdk11 to build

14a08ed

Removed snapshot references

9e78261

Update micrometer-observation/build.gradle

440f998

Co-authored-by: Robert Mihaly <[email protected]>

Apply suggestions from code review

42c9868

Co-authored-by: Robert Mihaly <[email protected]>

Removed aop.xml from micrometer obs test resources

1d0c2c9

Made aspectj deps optional

706ef1a

Not deploying aspectj tests

a1c9d3f

shakuzen force-pushed the issues_#1149_ctw branch from 52f2f80 to a1c9d3f Compare June 19, 2024 06:48

shakuzen merged commit 763e1f7 into main Jun 19, 2024
8 checks passed

shakuzen deleted the issues_#1149_ctw branch June 19, 2024 07:24

izeye added a commit to izeye/micrometer that referenced this pull request Aug 24, 2024

Fix directories for micrometer-test-aspectj-ctw and micrometer-test-a…

f37f487

…spectj-ltw This commit also polishes the changes made in micrometer-metricsgh-5059.

izeye mentioned this pull request Aug 24, 2024

Fix directories for micrometer-test-aspectj-ctw and micrometer-test-aspectj-ltw #5401

Merged

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

Loading

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

Loading

marcingrzejszczak May 15, 2024

shakuzen Jun 19, 2024

mihalyr left a comment

mihalyr May 15, 2024

shakuzen May 21, 2024

marcingrzejszczak May 21, 2024

kriegaex commented May 21, 2024 •

edited

Loading

marcingrzejszczak commented May 21, 2024

kriegaex commented May 21, 2024 •

edited

Loading

marcingrzejszczak commented May 21, 2024

mihalyr commented May 21, 2024 •

edited

Loading

mihalyr May 21, 2024

marcingrzejszczak May 21, 2024

mihalyr May 21, 2024 •

edited

Loading

shakuzen Jun 7, 2024

marcingrzejszczak Jun 7, 2024

shakuzen left a comment

kriegaex commented Jun 7, 2024 •

edited

Loading

marcingrzejszczak commented Jun 10, 2024

marcingrzejszczak commented Jun 10, 2024

shakuzen commented Jun 19, 2024

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

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

Conversation

marcingrzejszczak commented May 9, 2024 • edited Loading

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 Loading

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 Loading

marcingrzejszczak commented May 21, 2024

kriegaex commented May 21, 2024 • edited Loading

marcingrzejszczak commented May 21, 2024

mihalyr commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalyr May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shakuzen left a comment

Choose a reason for hiding this comment

kriegaex commented Jun 7, 2024 • edited Loading

marcingrzejszczak commented Jun 10, 2024

marcingrzejszczak commented Jun 10, 2024

shakuzen commented Jun 19, 2024

marcingrzejszczak commented May 9, 2024 •

edited

Loading

mihalyr commented May 10, 2024 •

edited

Loading

kriegaex commented May 21, 2024 •

edited

Loading

kriegaex commented May 21, 2024 •

edited

Loading

mihalyr commented May 21, 2024 •

edited

Loading

mihalyr May 21, 2024 •

edited

Loading

kriegaex commented Jun 7, 2024 •

edited

Loading