Fix AspectJ load-time weaving and class-level annotations #5061
Conversation
|
The load-time weaving test are not part of the PR as they were causing unrelated build failures. They should be added by someone familiar with this project structure, the code can be found in this draft: #5060 |
| @@ -161,7 +161,7 @@ public TimedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Iterabl | |||
| this.shouldSkip = shouldSkip; | |||
| } | |||
|
|
|||
| @Around("@within(io.micrometer.core.annotation.Timed) && !@annotation(io.micrometer.core.annotation.Timed)") | |||
| @Around("@within(io.micrometer.core.annotation.Timed) && !@annotation(io.micrometer.core.annotation.Timed) && execution(* *(..))") | |||
There was a problem hiding this comment.
The added part matches only method signatures (with a return type) so it will not apply to constructors that caused the runtime failures.
| @Around(value = "@annotation(counted)", argNames = "pjp,counted") | ||
| @Around("execution (@io.micrometer.core.annotation.Counted * *.*(..))") | ||
| @Nullable | ||
| public Object interceptAndRecord(ProceedingJoinPoint pjp, Counted counted) throws Throwable { | ||
| public Object interceptAndRecord(ProceedingJoinPoint pjp) throws Throwable { | ||
| if (shouldSkip.test(pjp)) { | ||
| return pjp.proceed(); | ||
| } | ||
|
|
||
| Method method = ((MethodSignature) pjp.getSignature()).getMethod(); | ||
| Counted counted = method.getAnnotation(Counted.class); | ||
| if (counted == null) { | ||
| method = pjp.getTarget().getClass().getMethod(method.getName(), method.getParameterTypes()); | ||
| counted = method.getAnnotation(Counted.class); | ||
| } | ||
|
|
||
| return perform(pjp, counted); | ||
| } |
There was a problem hiding this comment.
I aligned this method with TimedAspect not sure why were there two different approaches for the same, but the previous pointcut declaration wasn't working properly for me and this is also more consistent with ObservedAspect and TimedAspect too.
After fixing the pointcut syntax in the previous commit it revealed that the implementation has further errors. The pointcuts matched also constructors where the code assumed it gets a MethodSignature at runtime it got a ConstructorSignature and failed the unchecked class-cast. Extended the pointucts to only match method signatures. Tested with AspectJ load-time weaving locally and with the existing Spring AOP proxy tests.
|
@marcingrzejszczak @jonatan-ivanov Could you please merge this? My previous changed revealed a more serious issue that is actually causing runtime failures in client-code under certain conditions. There is some japi failure that I have no clue what it is and how to fix it. But the whole build and tests are working othewise. |
|
This is the content of The first message about the removed method is expected since I changed the method to fix the implementation. The second I have no idea what is it about, I haven't touched |
|
japicmp is a tool we use to have binary compatibility reporting for our builds. The first issue in the report is the problem that broke the build, changing this: public Object interceptAndRecord(ProceedingJoinPoint pjp, Counted counted) throws Throwable {to this: public Object interceptAndRecord(ProceedingJoinPoint pjp) throws Throwable {breaks source and binary compatibility so we should not do it in a minor release. On the other hand, the whole aspect class with this method in it should not be used from user code and AspectJ will be able to deal with this change I assume so I think we can add this class to the ignore list (I can help ignoring this). Btw can these methods have The second issue in the report is something we can look into but it does not fail the build, you can handle it as a warning. Locally you can disable this with |
|
Thank you for the details, it makes sense now. I'll take a look if default visibility works, but as you said, it's an aspect and not something a user should depend on directly...hopefully 🙂 Let me rework these PRs a little bit, first let's revert my commit from There is one thing I noticed, Metrics includes a global MeterRegistry, which can be used during weaving since it is done automatically by AspectJ without user involvement. However, for the |
I think we can work on this in my PR. So you can create a PR to my branch and we will work on the weaving feature in one go |
|
Cool, let's do that! Could you please revert my commit from main 5e16809 ? It's not safe without some other changes which should be addressed together and don't want to risk your release. |
|
Done via af7093f |
After fixing the pointcut syntax in the previous commit it revealed that the implementation has further errors. The pointcuts matched also constructors where the code assumed it gets a MethodSignature at runtime it got a ConstructorSignature and failed the unchecked class-cast.
Extended the pointucts to only match method signatures. Tested with AspectJ load-time weaving locally and with the existing Spring AOP proxy tests.