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

Compile-time weaving support for TimedAspect #1149

Open
jefftap opened this issue Jan 11, 2019 · 63 comments · May be fixed by #5059
Open

Compile-time weaving support for TimedAspect #1149

jefftap opened this issue Jan 11, 2019 · 63 comments · May be fixed by #5059
Labels
Projects

Comments

@jefftap
Copy link

jefftap commented Jan 11, 2019

Hi, I am attempting to make use of TimedAspect via AspectJ compile-time weaving using the aspectj-maven-plugin. When the aspect code is executed, it throws one of the two errors, depending on what is being executed:

java.lang.NoSuchMethodError: io.micrometer.core.aop.TimedAspect.aspectOf()Lio/micrometer/core/aop/TimedAspect
java.lang.NoSuchMethodException: io.micrometer.core.aop.TimedAspect.aspectOf()

This usually indicates that the AspectJ weaver has not properly processed the aspect code. I am not sure if this is a problem with my own code/configuration, or if it's an issue in Micrometer's jar. The aspect works perfectly fine using Spring AOP, but I would prefer to use AspectJ weaving with @Timed to allow it access to private/protected methods and the other various benefits one gets from AspectJ integration.

Relevant information:

  • micrometer-core version: 1.1.1
  • Spring Boot version: 2.1.1.RELEASE
  • AspectJ verison: 1.9.2
  • Java version: 1.8
  • aspectj-maven-plugin version: 1.11

aspectj-maven-plugin configuration:

            <plugin>
                <groupId>org.codehaus.mojo</groupId>
                <artifactId>aspectj-maven-plugin</artifactId>
                <version>1.11</version>
                <configuration>
                    <complianceLevel>1.8</complianceLevel>
                    <source>1.8</source>
                    <target>1.8</target>
                    <showWeaveInfo>true</showWeaveInfo>
                    <Xlint>ignore</Xlint>
                    <aspectLibraries>
                        <aspectLibrary>
                            <groupId>org.springframework</groupId>
                            <artifactId>spring-aspects</artifactId>
                        </aspectLibrary>
                        <aspectLibrary>
                            <groupId>io.micrometer</groupId>
                            <artifactId>micrometer-core</artifactId>
                        </aspectLibrary>
                    </aspectLibraries>
                </configuration>
                <executions>
                    <execution>
                        <goals>
                            <goal>compile</goal>
                            <goal>test-compile</goal>
                        </goals>
                    </execution>
                </executions>
                <dependencies>
                    <dependency>
                        <groupId>org.aspectj</groupId>
                        <artifactId>aspectjtools</artifactId>
                        <version>${aspectj.version}</version>
                    </dependency>
                </dependencies>
            </plugin>
@jkschneider jkschneider added the question A user question, probably better suited for StackOverflow label Jan 12, 2019
@jkschneider
Copy link
Contributor

Have you added aspectj weaving to the build section of your POM?

<build>
<plugins>
  <plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>aspectj-maven-plugin</artifactId>
    <executions>
      <execution>
        <goals>
          <goal>compile</goal> 
        </goals>
      </execution>
    </executions>
  </plugin>
</plugins>

@jefftap
Copy link
Author

jefftap commented Jan 14, 2019

Yes. The aspectj-maven-plugin configuration I posted in my initial post is under <build>/<plugins>.

@jefftap
Copy link
Author

jefftap commented Jan 17, 2019

My current hypothesis is that the build.gradle file needs the aspectj plugin applied to it. This should cause the Micrometer build to use ajc, which should process the aspect.

@jefftap
Copy link
Author

jefftap commented Jan 17, 2019

Also, one way to work around this is to write your own aspect that weaves around an annotation and use the Timer methods in there. It still needs to be coupled with Spring, though. Inject the MeterRegistry as a dependency. The bean should be declared in the Spring configuration using the Aspects.aspectOf method to pull in the AspectJ instance, and Spring handles the injection from there.

@izeye
Copy link
Contributor

izeye commented Feb 12, 2019

I don't have much expertise on AspectJ but after some quick research, it seems to require a similar configuration to spring-aspects to support compile-time weaving as already pointed out by @jefftap although I'm not sure there's any plan for compile-time weaving support.

@jefftap
Copy link
Author

jefftap commented Feb 12, 2019

Looks like spring-aspects.jar compiles with ajc, then. I think the solution to this ticket is to either compile Micrometer with ajc, or improve the documentation on how to get Micrometer to work properly with AspectJ. I can write a small addition to the documentation explaining this, if desired.

@ewencluley
Copy link

I am running into the exact same issue as @jefftap. While the compile time weaving seems to succeed, at runtime the aspectOf methods are not there on the TimedAspect classes. The documentation for micrometer suggests the @Timed annotation should be supported using compile time aspectj weaving. (https://micrometer.io/docs/concepts#_the_code_timed_code_annotation)

use in your application either through compile/load time AspectJ weaving or through framework facilities that interpret AspectJ aspects

Would be great to have some documentation on how to get this working.

thm-deploy pushed a commit to particify/arsnova-server that referenced this issue Aug 4, 2019
Methods annotated with @timed now use Micrometers annotation. The @Gauge
annotation has been remove without replacement for now.

While configuration for the use of Micrometer's TimedAspect has been
prepared, it is not yet active because of compatibility issues with
compile-time weaving.
See micrometer-metrics/micrometer#1149.
@Phil-Ba
Copy link

Phil-Ba commented Nov 14, 2019

I didn't manage to get this working with CTW. As far as I understood it, it seems that micrometer-core is not compiled with the ajc and thus the aspects are missing the required factory methods.
The only solution I found, was to do LTW and have the micrometer-core classes also be woven during load time.
Another solution would be the recompile the micrometer-core jar with the aspectj maven plugin, but this also requires all dependencies...

@Exidex
Copy link

Exidex commented Jun 18, 2020

The issue still persist. As noted higher it could be resolved by using LTW, but in my case it is not an option because I do not have access to the deployment setup, and the only option is CTW which doesn't work with this library

@innovationhub-asia
Copy link

Same issue here, CTW does not work. Is there any plan to support it properly, so CTW will work ?

@venushka
Copy link

venushka commented Sep 13, 2020

I've had the same issue with CTW, although I got around this by wrapping the aspect like this for now.

@Aspect
public class MetricsAspect {
  @Around("execution (@io.micrometer.core.annotation.Timed * *.*(..))")
  public Object timedMethod(ProceedingJoinPoint pjp) throws Throwable {
    final TimedAspect timeAspect = new TimedAspect(MetricsServlet.getRegistry());
    return timeAspect.timedMethod(pjp);
  }
}

and adding the following to the aop.xml

<aspectj>
  <aspects>
    <aspect name="com.venushka.metrics.MetricsAspect"/>
  </aspects>
</aspectj>

It would be great if this works out of the box for CTW.

@shakuzen shakuzen added enhancement A general enhancement and removed question A user question, probably better suited for StackOverflow labels Sep 24, 2020
@shakuzen shakuzen added this to the 1.x milestone Sep 24, 2020
@shakuzen shakuzen changed the title java.lang.NoSuchMethodException: io.micrometer.core.aop.TimedAspect.aspectOf() Compile-time weaving support for TimedAspect Sep 24, 2020
@shakuzen shakuzen added this to To do in Aspect epic Mar 24, 2021
@dalbani
Copy link

dalbani commented May 15, 2021

Hi, for those on the interweb who came here for load time weaving (LTW), I confirm that it works.
That's my META-INF/aop.xml file:

<aspectj>
    <weaver>
        <include within="io.micrometer.core..*"/>
        <include within="my.app..*"/>
    </weaver>

    <aspects>
        <aspect name="io.micrometer.core.aop.TimedAspect"/>
    </aspects>
</aspectj>

To be used in conjunction with a class like:

package my.app;

public class App {
  @Timed("xyz")
  public void fun1() { }
}

In a Spring Boot application with the @EnableLoadTimeWeaving annotation.

I'm a newbie when it comes to aspects, but I'm wondering what the advantages of CTW over LTW are.
@Exidex: when you said that "you do not have access to the deployment setup", did you mean that you couldn't make sure that the JVM was configured with the appropriate agent?
I don't know if that would solve your problem, but I personally use https://github.com/invesdwin/invesdwin-instrument to have the application code register the agent by itself.

@kriegaex
Copy link

kriegaex commented Jun 5, 2021

Because I often answer questions on StackOverflow, I found this issue, not for the first time in the last few years. Today I took some time to look into it, producing a little example project, showing how to apply TimedAspect via @Timed, using compile-time weaving with AspectJ Maven plugin. The same basic approach should also work with Gradlle, but I am a Maven guy.

https://github.com/kriegaex/SO_AJ_MicrometerTimed_67803726

The project simply consists of one class and a POM + an explanatory read-me file. The gist of it is: You must make sure to apply binary weaving to the Micrometer aspect in order to transform it into a real AspectJ aspect, and then apply that aspect to your own project.

@marcingrzejszczak
Copy link
Contributor

Since there are ways of how to achieve this is there anything else we should do here, other than maybe document this?

@marcingrzejszczak marcingrzejszczak removed this from the 1.x milestone Dec 21, 2023
@marcingrzejszczak marcingrzejszczak added the waiting for feedback We need additional information before we can continue label Dec 21, 2023
@kriegaex
Copy link

kriegaex commented Dec 22, 2023

Since there are ways of how to achieve this

My workaround is very contrived. IMO, it is unreasonable to assume that any normal user who does not happen to be an AspectJ expert can find out how to do this. Even if it was documented, users would be doing the Micrometer team's work there.

is there anything else we should do here, other than maybe document this?

Yes, add an AspectJ compilation step to the Gradle build (e.g. using Freefair plugin) and make sure that what is advertised as an AspectJ aspect, ...

* AspectJ aspect for intercepting types or methods annotated with
* {@link Timed @Timed}.<br>

... actually really does deserve its name, and avoid that incomplete non-AspectJ compilation with Javac needs to be finished by the user. That this happens to work with LTW, because the AspectJ weaver is smart enough to notice and finish the job, does not mean that CTW is some kind of second-class citizen. Make your product easy to use. The correctly compiled aspect would, of course, still work with LTW. With CTW, the library could just be put on the aspectpath, as is customary. No extra build steps or extra modules, creating a refined version of this library as an input for later steps.

Of course, aspectjrt would become a dependency of this library, but in fact it already is anyway, because as-is, you need LTW and aspectjweaver,

// Aspects
optionalApi 'org.aspectj:aspectjweaver'

which is a superset of aspectjrt and actually more than the library really needs. The dependency should be downgraded to aspectjrt.

@shakuzen shakuzen added feedback-provided and removed waiting for feedback We need additional information before we can continue labels Dec 22, 2023
@mihalyr-prospect
Copy link

mihalyr-prospect commented May 6, 2024

I am trying to migrate my project from Dropwizard Metrics to Micrometer and I'm blocked on this issue as I am trying to use compile-time weaving just like with Dropwizard annotations before and it doesn't seem to work with Micrometer despite the documentation saying it can be used also with compile time weaving.

I have a Gradle project and was trying to convert the above mentioned workaround from Maven to Gradle, but the plugins are different and am running into various build failures. I tried to revert to Spring AOP proxies, but that doesn't work on my project, CGLIB breaking some of the classes.

Are there any updates on the fix? Or did anyone make it work with Gradle (Freefair plugin)?

(Update: Fixed it by configuring AspectJ load-time weaving, seems to be working alongside CTW for other classes)

@kriegaex
Copy link

kriegaex commented May 7, 2024

@marcingrzejszczak, see, this is what I mean. It is possible to finish the job unfinished by Micrometer manually as a user, but it is not trivial. Instead of burdening users, trying to figure out how to use your aspect, because you think it is unnecessary to compile it with the AspectJ compiler, you should finally get this issue off the table. Or are you expecting me to convert my Maven solution to Gradle on behalf of @mihalyr-prospect?

@marcingrzejszczak
Copy link
Contributor

@kriegaex I don't think that your answer is nice or brings any value to this discussion. We will take care of this issue whenever we have bandwith to do so.

@kriegaex
Copy link

kriegaex commented May 7, 2024

@marcingrzejszczak: I wanted to be clear rather than nice. I was not super nice, maybe, but also not non-nice. I did not insult anyone. I simply want to push this forward. Honestly, this issue has been open for more than 5 years, and I do not really buy the bandwidth argument, because

  • your product is an aspect library, but you do not compile the aspect as such,
  • which makes this issue a high priority one, IMO,
  • it is clear what needs to be done, no further debugging or even any code changes necessary,
  • changing the build configuration should be straightforward and in no way rocket science.

The product I maintain, AspectJ, is a cornerstone of Micrometer, so please understand that I would rather like to see it fixed than feeling compelled to devise custom workarounds to various users for a few more years.

@kriegaex
Copy link

@jhoeller, sorry for asking instead of digging into the source code, but I am neither involved in Spring nor Micrometer and a bit under time pressure today: Is that relaxed check something that could be backported to older Spring versions which still receive regular or occasional updates?

@mihalyr
Copy link
Contributor

mihalyr commented May 10, 2024

That's a good questions, because the Micrometer build seems to be using Spring 5.x too for some JDK11 compatibility perhaps.

      > No matching variant of org.springframework:spring-context:6.1.7-SNAPSHOT:20240508.160934-49 was found. The consumer was configured to find a library for use during compile-time, compatible with Java 11, preferably in the form of class files, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm' but:
          - Variant 'apiElements' capability org.springframework:spring-context:6.1.7-SNAPSHOT declares a library for use during compile-time, packaged as a jar, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm':
              - Incompatible because this component declares a component, compatible with Java 17 and the consumer needed a component, compatible with Java 11
          - Variant 'runtimeElements' capability org.springframework:spring-context:6.1.7-SNAPSHOT declares a library for use during runtime, packaged as a jar, preferably optimized for standard JVMs, and its dependencies declared externally, as well as attribute 'org.jetbrains.kotlin.platform.type' with value 'jvm':
              - Incompatible because this component declares a component, compatible with Java 17 and the consumer needed a component, compatible with Java 11

@jhoeller
Copy link

jhoeller commented May 10, 2024

If there is a concrete need for upcoming Micrometer releases, we can consider backporting this to 6.0.20 and 5.3.35. It is not entirely trivial though since it involves some potential issues for mixed AspectJ usage (using different @Aspect aspects for proxying and weaving in the same application, with both kinds of aspects defined as Spring beans) which we need to leniently handle; that came in a follow-up commit for 6.1.7. The latest snapshot should address all known concerns now.

@marcingrzejszczak please advise how concrete the need for a backport is here.

@mihalyr
Copy link
Contributor

mihalyr commented May 10, 2024

I quickly checked the new Spring 6.1.7 snapshot on my branch, bumped compatibility to Java 17 and tests worked for Spring AOP, AspectJ LTW and CTW too. This looks like a good direction, just need to solve the compatibility issue in some way.

@jhoeller
Copy link

Thanks for the immediate check, @mihalyr! The present arrangement in 6.1.7 is good to go, there's just the question of the backports... mostly a concern with unexpected side effects in niche scenarios where we currently limit the problem to 6.1.x, unless we have a scenario where this has to work in older branches as well.

@marcingrzejszczak
Copy link
Contributor

We still maintain micrometer 1.11.x and 1.12.x which both are using Spring 5.3.x. If we backported Spring Framework for this version then all of the OSS supported versions would be fixed

@jhoeller
Copy link

jhoeller commented May 10, 2024

So you intend to backport the ajc compilation step to Micrometer 1.11.x and 1.12.x as well, @marcingrzejszczak? I originally assumed that would only apply to the latest Micrometer feature branch... I guess it depends on whether you see this as an enhancement or a bug fix.

@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented May 10, 2024

Oh yeah, that's a good question. I wouldn't say it's a bug, I'd say that's a new feature. So we could stay with the current version and release this for Micrometer 1.14.0. I'll ping @shakuzen and @jonatan-ivanov to know what they think.

@kriegaex
Copy link

I also think it is an improvement rather than a bug fix, but a very useful one. Is there a strict bugfix-only policy in Micrometer?

@marcingrzejszczak
Copy link
Contributor

Yeah we try to follow that rule as much as possible - features in new minors, bug fixes in patches

@kriegaex
Copy link

kriegaex commented May 10, 2024

OK, i.e. users who want to use Micrometer with AspectJ in a CTW scenario need to use the latest version and also upgrade to Spring 6 - or use my workaround. (No need to reply, unless I am wrong.)

@marcingrzejszczak
Copy link
Contributor

We can document the workaround for older versions and add this as a feature in 1.14

@kriegaex
Copy link

We can document the workaround for older versions and add this as a feature in 1.14

If somebody knows how to do in Gradle what I suggested for Maven, yeah.

@mihalyr
Copy link
Contributor

mihalyr commented May 10, 2024

@kriegaex to make LTW/CTW work I had to make some changes to pointcuts - but I'm not that good with aspects in general, would it be possible for you to take a quick look?

My changes are now in #5064 to be merged to Marcin's PR in #5059

One change I had to make was to make the pointcuts match only methods and not constructors because the implementation casted the signature to MethodSignature and getting a ConstructorSignature crashed it.

This is the one for class-level annotations which should match all methods within an annotated class that are otherwise not annotated already and exclude constructors.

-    @Around("@within(io.micrometer.core.annotation.Counted) && !@annotation(io.micrometer.core.annotation.Counted)")
+    @Around("@within(io.micrometer.core.annotation.Counted) && !@annotation(io.micrometer.core.annotation.Counted) && execution(* *(..))")

The other issue was with a method pointcut for which I changed the aspect method signature to make it work, but this breaks some binary compatibility checks in the project, so would be perhaps easier if we could find a fix without the method signature change if possible. Here is my change currently which works with LTW/CTW and Spring AOP too:

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

Please let me know if there is a better way to do this.

@kriegaex
Copy link

@mihalyr, maybe we should talk (not write) one of these days, because the back-and-forth might get tedious. I need to learn more about your usage scenarios before being able to answer. For instance, I do not understand why you would need to change anything in your pointcuts for CTW in the first place. The only reason would be that that it was broken before for LTW, too, which is unrelated to this issue here. But of course, it should be fixed anyway if it is broken.

@kriegaex

This comment was marked as resolved.

@mihalyr
Copy link
Contributor

mihalyr commented May 10, 2024

For instance, I do not understand why you would need to change anything in your pointcuts for CTW in the first place. The only reason would be that that it was broken before for LTW, too, which is unrelated to this issue here.

Exactly, it was broken for LTW too.

The annotations could be added for a whole class and for separate methods. If it is on a class, it would instrument all methods (that are otherwise not annotated with the same annotation). But this class level annotation was broken because of the and not syntax so the pointcut didn't work (only worked for methods). When I fixed this by changing and not to && ! the pointcut started to work but also matched constructors which the code didn't expect and crashed - both CTW and LTW. My fix for this was to add && execution(* *(..)) to filter out constructors, which works now.

After all this I noticed that I get double counts using @Counted and the only difference between that and the other aspects was the pointcut.

Counted used: @Around(value = "@annotation(counted)", argNames = "pjp,counted")

While Timed used: @Around("execution (@io.micrometer.core.annotation.Timed * *.*(..))")

So I changed Counted also to @Around("execution (@io.micrometer.core.annotation.Counted * *.*(..))") and now it works fine.

So right now with these pointcut changes everything seems to be working, AspectJ with CTW or LTW and also Spring AOP (with 6.1.7-SNAPSHOT and JDK17).

I'm not really involved with Micrometer project though, I just wanted to use this library the first time ever and naturally went for CTW which didn't work and ended up here, found a little free time to help out, but don't know much about why things were written in a certain way or what other goals are here. For that I think Marcin is the person to ask.

@kriegaex
Copy link

kriegaex commented May 10, 2024

Thanks for the clarification. I recommend a more readable change like this instead as a minimal and clean change compared to the main branch:

@Around(value = "@annotation(counted) && execution(* *(..))", argNames = "pjp,counted")

I find that more readable, and it is a simple and mechanical thing to do to just add && execution(* *(..)) when converting a Spring AOP aspect to native AspectJ without breaking compatibility. The reason why you want to add that is to exclude AspectJ's call() joinpoints, which cause the double counts in your case. Spring AOP only knows execution(), i.e. there it is unnecessary. But AspectJ knows both pointcut types, which are semantically different - one is in the calling class/method, the other one in the called one. I.e., && execution(* *(..)) makes explicit what Spring AOP assumes implicitly. I find that more readable than mixing everything into a single pointcut.

@kriegaex
Copy link

BTW, I tested a manually Ajc-compiled aspect in my old Spring AOP playground project with Spring 5.3.16 in the debugger, setting a breakpoint inside AbstractAspectJAdvisorFactory::compiledByAjc and each time forcing a return value of false, which made the native aspect work with Spring AOP. I guess, that is similar to your change for Spring 6.x., isn't it? I just wanted to make sure I understand this a bit better.

@kriegaex
Copy link

kriegaex commented May 10, 2024

Another hint, just in case you happen to be using or be planning to use pointcuts with something like within(my.package..*). Be careful there in native AspectJ, if the aspect is in the same package. In that case, add && !within(MyAspect) to the pointcut to avoid the aspect from weaving itself or one aspect weaving the other one, which sometimes makes sense in AspectJ, but usually is not what you want. In Spring AOP, aspects cannot weave each other or themselves, i.e. it is implicitly excluded by the proxy-based approach, similar to how call() is not a thing due to the proxies.

Probably, this is not a problem at the moment, otherwise I guess you would be running into test problems already. Just a caveat for the future...

@mihalyr
Copy link
Contributor

mihalyr commented May 10, 2024

Thank you Alexander, super useful information and the simpler syntax would solve binary compatibility issue I have also.

BTW, I tested a manually Ajc-compiled aspect in my old Spring AOP playground project with Spring 5.3.16 in the debugger, setting a breakpoint inside AbstractAspectJAdvisorFactory::compiledByAjc and each time forcing a return value of false, which made the native aspect work with Spring AOP. I guess, that is similar to your change for Spring 6.x., isn't it? I just wanted to make sure I understand this a bit better.

I can't comment on this part as it was @jhoeller who made that change in Spring, but I also assume it was something like that.

mihalyr added a commit to mihalyr/micrometer that referenced this issue May 10, 2024
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
micrometer-metrics#1149 (comment)
marcingrzejszczak added a commit that referenced this issue May 13, 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>
@marcingrzejszczak
Copy link
Contributor

@kriegaex we're building micrometer with jdk11. Can we fix the AspectJ compiler to 1.9.20.1?

@kriegaex
Copy link

@marcingrzejszczak, yes, if you do not mind missing compiler enhancements and bugfixes. Would it be a big deal to build on JDK 17+ instead? JDKs 17 and 21 are LTS.

@marcingrzejszczak
Copy link
Contributor

We would have to discuss this internally @shakuzen @jonatan-ivanov

marcingrzejszczak added a commit that referenced this issue 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>
@marcingrzejszczak
Copy link
Contributor

Ok, #5059 is ready for review. Let's try to close this 5 year old issue ;)

marcingrzejszczak added a commit that referenced this issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.