-
Notifications
You must be signed in to change notification settings - Fork 990
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 pointcut syntax #5058
Fix AspectJ pointcut syntax #5058
Conversation
AspectJ pointcuts use the `&&` and `!` operators and the alternative word form syntax with `and` and `not` operators is only allowed for load-time weaving with `aop.xml` which cannot use the symbols in the XML. This fixes compile-time weaving support which failed on the `and not` syntax.
@mihalyr Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
The previous syntax was introduced recently in #3811 Related discussion about the correct syntax in #1149 (comment) |
@mihalyr Thank you for signing the Contributor License Agreement! |
Thank you for fixing this! Do you think we should open an issue for this in AspectJ? |
Hi @jonatan-ivanov there isn't really a problem with AspectJ, more like Micrometer is missing tests for this functionality. There are two ways basically how you can use the aspects with AspectJ: Compile-time weaving If your library was compiled (or post-compiled) with the AspectJ compiler (
However, at the moment you don't have an AspectJ compilation step in your build process (see #1149) so you don't get any error at build time. Load-time weaving People can also use your aspects with AspectJ at load time. In this case the error happens at runtime when the class is loaded and presented to the AspectJ weaver at execution time. You should probably have tests for this to catch these errors. Otherwise your aspects would throw exceptions for users at runtime like this:
And some tests should fail as the aspect is not run. Now that I tried to repro it not sure how well these class annotations work, I got some runtime failures when I tried to use them. The method annotations should be OK, but some tests should be perhaps included to make sure users get correct metrics. |
And I forgot the third approach that you use in your tests actually, using Spring AspectJ proxy at runtime, which perhaps is able to deal with the alternative expression syntax. But would be good to add some AspectJ LTW tests to make sure things work as intended also if you don't use Spring proxies. I've seen some aop.xml added in https://github.com/micrometer-metrics/micrometer/pull/5059/files probably @marcingrzejszczak was adding also some LTW tests to the project there. |
@marcingrzejszczak @jonatan-ivanov it seems that this change revealed some other issues with these aspects. Before this PR my local tests with load-time weaving succeeded for
Here is my test code https://github.com/mihalyr/micrometer/commit/427fd4f579ff1c58794045347c6266467aae4610 |
@marcingrzejszczak @jonatan-ivanov so there is a more serious bug that was just revealed by this PR. Before this PR the class-level annotation pointcut was just ignored due to the syntax issue with AspectJ LTW. After this PR however, the pointcut started to work and matches also constructors that cause runtime failures of the user programs when calling a constructor on such annotated classes, because the Timed/Counted aspect implementations assume a MethodSignature but they get a ConstructorSignature instance. I have a fix in this draft PR including AspectJ LTW tests that will help you find these issues next time. #5060 However, I don't really have time to help you clean this up and adjust for your project, especially the tests are very problematic there, because they need AspectJ and the current version will not work with JDK11 that you also build for. |
I think, I'll just extract the pointcut fixes from #5060 and send it separately, that should work and then you can later add some tests according to your priorities and in a way that suits the project. |
Sent a fix in #5061 |
From my perspective (user of AspectJ), there is: if AspectJ knows (I assume) that I'm doing something that I should not, it could warn me but I would not call this a "problem" but it would be a nice gesture towards the users. Based on the other things you wrote and since we are post-RC and our GA release is on Monday, I think we should revert your fix in gh-5058 and apply it with your other fixes after the release. Please don't get me wrong, I think we should fix these things and I really appreciate your help on this but I think we should do this in the next release so that we have more time to find issues and write tests. /cc @marcingrzejszczak @shakuzen |
Hi @jonatan-ivanov the AspectJ compiler/weaver really tells you what is wrong, it is showing you errors and the code doesn't work. But you don't have tests for that, because you use Spring AOP proxies and not AspectJ CTW/LTW in your tests, and the proxies seem to work differently from the actual compiler/weaver. That's why I suggested that you should add also AspectJ tests to have these things covered and then you would see the errors and get test failures too. Regarding the revert, yes, that's the safe decision. I didn't know what stage you are in right now and I wanted to suggest that if you can't merge the other fixes, then please revert my change from Thank you for your quick response and really appreciate taking the time to help out with this. |
This reverts commit 5e16809.
The changes in this PR were reverted in af7093f we are planning to apply AspectJ fixes after our release (2024-05-13). |
@jonatan-ivanov I have all the fixes now in #5064 that goes into Marcin's branch with all LTW/CTW stuff together. The binary compatibility issue is also fixed in that PR, so once we are able to get the Spring fix from 6.1.7-SNAPSHOT for the build, it should all be good for the next release hopefully. |
* 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>
* 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>
* 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>
* 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>
Hi Folks - the above has me a little confused - after upgrading from Spring boot
Right now is there anything I can do to work-around this? |
@frankjkelly the changes from this PR were originally reverted from the 1.13.1 release, but a more complete implementation including this patch and proper AspectJ CTW/LTW support has been already pushed to Not sure why do you see this error only now, it was there all along if you used AspectJ LTW, it was showing the error at startup also on Spring Boot 3.2.x. But if you don't use |
@mihalyr Thanks for getting back to me - maybe my problem is similar but not this one as I am getting a compilation at the compile step with the following. So it does appear to be AspectJ post compile weaving complaining about something.
|
@frankjkelly yes, that's the same issue, Micrometer doesn't work with compile-time weaving, the fix is in the commit waiting to be released I mentioned before. With load-time weaving you would get the error during runtime and could ignore, but CTW will fail the build on the syntax error. I had the same issue when I first switched to Micrometer from Dropwizard which led to this patch. See also #1149 which was the main issue tracking AspectJ compile-time weaving support. |
@frankjkelly So you can revert to a Micrometer version before that commit until the fix from 1.14.0-SNAPSHOT is released. |
@frankjkelly now I realized why you saw this with the Spring upgrade. That bad commit was added in Micrometer 1.13.0 which is what came with Spring Boot 3.3. Now, it makes sense. So, yeah, your choice for now is to stay with Micrometer 1.12.x, not sure how it works with SB 3.3, though. Alternatively, you can switch to load-time weaving to ignore the error, that's what I did as a workaround until waiting for the fix to be released. |
fyi: I'm backporting that change to 1.13.x: #5285 |
Backport of: "Fix AspectJ pointcut syntax (#5058)" AspectJ pointcuts use the `&&` and `!` operators and the alternative word form syntax with `and` and `not` operators is only allowed for load-time weaving with `aop.xml` which cannot use the symbols in the XML. This fixes compile-time weaving support which failed on the `and not` syntax. Closes gh-5285 Backport-Issue: gh-5058 Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
@jonatan-ivanov please do not backport this, it was reverted for a reason: #5058 (comment) My complete fix is in #5064 that was merged to main in 763e1f7 Please use the changes from 763e1f7 instead for the Aspect classes. And consider the LTW tests at least. |
I reverted the change, sorry for jumping too quickly: #5285 (comment) |
AspectJ pointcuts use the
&&
and!
operators and the alternative word form syntax withand
andnot
operators is only allowed for load-time weaving withaop.xml
which cannot use the symbols in the XML. This fixes compile-time weaving support which failed on theand not
syntax.