Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AspectJ LTW and CTW #5064

Merged
merged 4 commits into from
May 13, 2024
Merged

Fix AspectJ LTW and CTW #5064

merged 4 commits into from
May 13, 2024

Conversation

mihalyr
Copy link
Contributor

@mihalyr mihalyr commented May 10, 2024

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.

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

marcingrzejszczak and others added 3 commits May 10, 2024 16:13
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.
Added more class-level annotation tests and moved the module to
'micrometer-test-aspectj-ctw' to align with
'micrometer-test-aspectj-ltw'.
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)
@marcingrzejszczak marcingrzejszczak merged commit 65a1dca into micrometer-metrics:issues_#1149_ctw May 13, 2024
3 of 6 checks passed
marcingrzejszczak added a commit that referenced this pull request May 15, 2024
* 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>
@cigaly
Copy link

cigaly commented May 19, 2024

What is the reason why this commit has been reverted? Now CTW is failing due to bad expressions in Around annotation

@marcingrzejszczak
Copy link
Contributor

We are working on proper CTW support in this PR #5059

marcingrzejszczak added a commit that referenced this pull request May 20, 2024
* 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>
@cigaly
Copy link

cigaly commented May 20, 2024

OK, but those expressions were fixed, then reverted to invalid and published as 1.13.0. That is confusing to me :-(

@marcingrzejszczak
Copy link
Contributor

We never said that we supported LTW or CTW in the first place. So adding LTW and CTW support is a new feature which we wanted to add for 1.14.0. Those changes were made just before a GA so we didn't want to take chances

@mihalyr
Copy link
Contributor Author

mihalyr commented May 20, 2024

@cigaly sorry that I left this here without proper explanation, this PR was meant for Marcin to be merged into his branch. Let me add a bit of clarification here.

First, the commits in this PR (#5064) were not reverted, they were merged into Marcin's new PR which will add complete AspectJ load-time and compile-time weaving support: #5059

I think what you may be referring to as reverted was the commit from my earlier PR #5058 which fixed some of the pointcuts for use with AspectJ. However, this change was incomplete as it revealed some other issues in the implementation that would have caused more harm if released unfixed and due to the timing of the new release it was considered too risky, so we rather reverted to preserve the status quo and prepare a complete change for the next release (see link to Marcin's PR above).

You can read the comments on my earlier PR about the issues with the initial pointcut fix e.g. #5058 (comment) or #5058 (comment) while they did fix the annotations on the method level, they led to runtime errors when the annotations were used on class level.

shakuzen pushed a commit that referenced this pull request Jun 19, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants