-
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 load-time weaving and class-level annotations #5060
Conversation
I sent the pointcut syntax separately to be able to merge it ASAP to fix the crashes #5061 |
@marcingrzejszczak I think you have this also covered in your PR draft, feel free to cancel this one if you are adding the tests there. |
The build is failing because of checkstyle, running |
The pointcut syntax used in the aspects is not valid for the AspectJ weaver/compiler. Replaced `and not` with `&& !` to fix it and also had to add a condition to match only methods and not constructors which the implementation assumed it gets. One problem was that the aspects were only tested with Spring AOP proxies, but AspectJ weaver/compiler bugs were going unnoticed. The added LTW test module will run with the `aspectjweaver` java agent to weave the classes at load time, then tests if the aspects are applied and metrics collected as expected.
@jonatan-ivanov @marcingrzejszczak I've updated this PR now with the reverted changes from main and the change from my other LTW PR that I merged into one here. This is now passing every check locally with The only remaining issue is with
|
@@ -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(* *(..))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there && execution(* *(..))
at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the implementation has an unchecked class-cast to MethodSignature that causes runtime crashes if the pointcut matches a constructor, which it does without this addition that restricts it only to method signatures (with a return type).
@within(io.micrometer.core.annotation.Counted)
- will match the methods of class annotated with@Counted
&& !@annotation(io.micrometer.core.annotation.Counted)
- excludes methods that are already annotated separately with@Counted
to avoid double instrumentation&& execution(* *(..))
- matches method signatures only excluding constructors
What happens if you don't include the last part is that if you annotate a class with @Counted
it will crash at runtime when you try to create a new instance of the class because the pointcut would match the constructor which gives a ConstructorSignature and fails to be cast to MethodSignature in the implementation. The tests cover this, so if you remove that last part you'll see the error when running the tests.
Agree with @marcingrzejszczak that we will do this together in his PR over at #5059 (comment) |
The pointcut syntax used in the aspects is not valid for the AspectJ
weaver/compiler. Replaced
and not
with&& !
to fix it and also hadto add a condition to match only methods and not constructors which the
implementation assumed it gets.
One problem was that the aspects were only tested with Spring AOP
proxies, but AspectJ weaver/compiler bugs were going unnoticed. The
added LTW test module will run with the
aspectjweaver
java agent toweave the classes at load time, then tests if the aspects are applied
and metrics collected as expected.