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 load-time weaving and class-level annotations #5060

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ subprojects {
ext.year = Calendar.getInstance().get(Calendar.YEAR)
skipExistingHeaders = true
exclude '**/*.json' // comments not supported
exclude '**/aop.xml'
}

spotless {
Expand Down Expand Up @@ -337,7 +338,7 @@ subprojects {

check.dependsOn("testModules")

if (!(project.name in ['micrometer-registry-prometheus', 'micrometer-registry-prometheus-simpleclient', 'micrometer-jakarta9', 'micrometer-java11', 'micrometer-jetty12'])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp
if (!(project.name in ['micrometer-registry-prometheus', 'micrometer-registry-prometheus-simpleclient', 'micrometer-jakarta9', 'micrometer-java11', 'micrometer-jetty12', 'micrometer-test-aspectj-ltw'])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp
apply plugin: 'me.champeau.gradle.japicmp'
apply plugin: 'de.undercouch.download'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(* *(..))")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@Nullable
public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
if (shouldSkip.test(pjp)) {
Expand Down Expand Up @@ -198,17 +198,23 @@ public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
* {@link Counted#recordFailuresOnly()} is set to {@code false}, a success is
* recorded.
* @param pjp Encapsulates some information about the intercepted area.
* @param counted The annotation.
* @return Whatever the intercepted method returns.
* @throws Throwable When the intercepted method throws one.
*/
@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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public TimedAspect(MeterRegistry registry, Function<ProceedingJoinPoint, Iterabl
this.shouldSkip = shouldSkip;
}

@Around("@within(io.micrometer.core.annotation.Timed) and not @annotation(io.micrometer.core.annotation.Timed)")
@Around("@within(io.micrometer.core.annotation.Timed) && !@annotation(io.micrometer.core.annotation.Timed) && execution(* *(..))")
@Nullable
public Object timedClass(ProceedingJoinPoint pjp) throws Throwable {
if (shouldSkip.test(pjp)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public ObservedAspect(ObservationRegistry registry,
this.shouldSkip = shouldSkip;
}

@Around("@within(io.micrometer.observation.annotation.Observed) and not @annotation(io.micrometer.observation.annotation.Observed)")
@Around("@within(io.micrometer.observation.annotation.Observed) && !@annotation(io.micrometer.observation.annotation.Observed) && execution(* *.*(..))")
@Nullable
public Object observeClass(ProceedingJoinPoint pjp) throws Throwable {
if (shouldSkip.test(pjp)) {
Expand Down
25 changes: 25 additions & 0 deletions micrometer-test-aspectj-ltw/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
plugins {
id 'java'
}

group = 'io.micrometer'

repositories {
mavenCentral()
}

configurations {
agents
}

dependencies {
agents libs.aspectjweaver
implementation project(':micrometer-core')
testImplementation libs.junitJupiter
testImplementation libs.assertj
}

test {
useJUnitPlatform()
jvmArgs '-javaagent:' + configurations.agents.files.find { it.name.startsWith('aspectjweaver') }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2024
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micrometer;

import io.micrometer.core.annotation.Counted;
import io.micrometer.core.annotation.Timed;

@Counted
@Timed
public class MeasuredClass {

@Timed
public void timedMethod() {
}

@Counted
public void countedMethod() {
}

public void classLevelTimedMethod() {
}

public void classLevelCountedMethod() {
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright 2024
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* Support for testing.
*/
package io.micrometer;
11 changes: 11 additions & 0 deletions micrometer-test-aspectj-ltw/src/main/resources/META-INF/aop.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!DOCTYPE aspectj PUBLIC "-//AspectJ//DTD//EN" "https://eclipse.dev/aspectj/dtd/aspectj.dtd">
<aspectj>
<weaver options="-verbose -showWeaveInfo">
<include within="io.micrometer.core.aop.*"/>
<include within="io.micrometer.MeasuredClass"/>
</weaver>
<aspects>
<aspect name="io.micrometer.core.aop.CountedAspect"/>
<aspect name="io.micrometer.core.aop.TimedAspect"/>
</aspects>
</aspectj>
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
* Copyright 2024
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micrometer;

import static org.assertj.core.api.Assertions.assertThat;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Metrics;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import org.junit.jupiter.api.*;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
class MeasuredClassTest {

static MeterRegistry meterRegistry;

@BeforeAll
static void setUpAll() {
meterRegistry = new SimpleMeterRegistry();
Metrics.addRegistry(meterRegistry);
}

MeasuredClass measuredClass;

@BeforeEach
void setUp() {
measuredClass = new MeasuredClass();
}

@Test
void it_measures_timed_method() {
measuredClass.timedMethod();

var timer = meterRegistry.get("method.timed")
.tag("class", MeasuredClass.class.getName())
.tag("method", "timedMethod")
.timer();

assertThat(timer.count()).isOne();

measuredClass.timedMethod();
assertThat(timer.count()).isEqualTo(2);
}

@Test
void it_measures_counted_method() {
measuredClass.countedMethod();

var counter = meterRegistry.get("method.counted")
.tag("class", MeasuredClass.class.getName())
.tag("method", "countedMethod")
.counter();

assertThat(counter.count()).isOne();

measuredClass.countedMethod();
assertThat(counter.count()).isEqualTo(2);
}

@Test
void it_measures_class_level_timed_method() {
measuredClass.classLevelTimedMethod();

var timer = meterRegistry.get("method.timed")
.tag("class", MeasuredClass.class.getName())
.tag("method", "classLevelTimedMethod")
.timer();

assertThat(timer.count()).isOne();

measuredClass.classLevelTimedMethod();
assertThat(timer.count()).isEqualTo(2);
}

@Test
void it_measures_class_level_counted_method() {
measuredClass.classLevelCountedMethod();

var counter = meterRegistry.get("method.counted")
.tag("class", MeasuredClass.class.getName())
.tag("method", "classLevelCountedMethod")
.counter();

assertThat(counter.count()).isOne();

measuredClass.classLevelCountedMethod();
assertThat(counter.count()).isEqualTo(2);
}

}
2 changes: 1 addition & 1 deletion settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ include 'micrometer-commons', 'micrometer-core', 'micrometer-observation'
project(":micrometer-samples-$sample").projectDir = new File(rootProject.projectDir, "samples/micrometer-samples-$sample")
}

include 'micrometer-test', 'micrometer-observation-test'
include 'micrometer-test', 'micrometer-observation-test', 'micrometer-test-aspectj-ltw'

['atlas', 'prometheus', 'prometheus-simpleclient', 'datadog', 'elastic', 'ganglia', 'graphite', 'health', 'jmx', 'influx', 'otlp', 'statsd', 'new-relic', 'cloudwatch2', 'signalfx', 'wavefront', 'dynatrace', 'azure-monitor', 'humio', 'appoptics', 'kairos', 'stackdriver', 'opentsdb'].each { sys ->
include "micrometer-registry-$sys"
Expand Down