Skip to content

Commit

Permalink
Compile-time and load-time weaving support for aspects (#5059)
Browse files Browse the repository at this point in the history
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.

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)

Fixes gh-1149
---------

Co-authored-by: Marcin Grzejszczak <[email protected]>
Co-authored-by: Robert Mihaly <[email protected]>
  • Loading branch information
3 people committed Jun 19, 2024
1 parent 51b9b4f commit 763e1f7
Show file tree
Hide file tree
Showing 21 changed files with 750 additions and 12 deletions.
6 changes: 3 additions & 3 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ subprojects {


javadoc {
if (project.name.contains('samples')) {
if (project.name.contains('samples') || project.name.contains("-test-aspectj")) {
enabled = false
} else {
configure(options) {
Expand Down Expand Up @@ -289,7 +289,7 @@ subprojects {
}

// Do not publish some modules
if (!['samples', 'benchmarks', 'micrometer-osgi-test', 'concurrency-tests'].find { project.name.contains(it) }) {
if (!['samples', 'benchmarks', 'micrometer-osgi-test', 'concurrency-tests', 'micrometer-test-aspectj-ctw', 'micrometer-test-aspectj-ltw'].find { project.name.contains(it) }) {
apply plugin: 'com.netflix.nebula.maven-publish'
apply plugin: 'com.netflix.nebula.maven-manifest'
apply plugin: 'com.netflix.nebula.maven-developer'
Expand Down Expand Up @@ -337,7 +337,7 @@ subprojects {

check.dependsOn("testModules")

if (!(project.name in [])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp
if (!(project.name in ['micrometer-test-aspectj-ltw', 'micrometer-test-aspectj-ctw'])) { // 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
4 changes: 3 additions & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ activemq-artemis = "2.34.0"
application-insights = "2.6.4"
archunit = "1.3.0"
asmForPlugins = "7.3.1"
aspectjweaver = "1.9.22.1"
aspectjweaver = "1.9.20.1"
assertj = "3.26.0"
awaitility = "4.2.1"
caffeine = "2.9.3"
Expand Down Expand Up @@ -89,6 +89,7 @@ activemqArtemisJunit5 = { module = "org.apache.activemq:artemis-junit-5", versio
applicationInsights = { module = "com.microsoft.azure:applicationinsights-core", version.ref = "application-insights" }
archunitJunit5 = { module = "com.tngtech.archunit:archunit-junit5", version.ref = "archunit" }
asmForPlugins = { module = "org.ow2.asm:asm", version.ref = "asmForPlugins" }
aspectjrt = { module = "org.aspectj:aspectjrt", version.ref = "aspectjweaver" }
aspectjweaver = { module = "org.aspectj:aspectjweaver", version.ref = "aspectjweaver" }
assertj = { module = "org.assertj:assertj-core", version.ref = "assertj" }
awaitility = { module = "org.awaitility:awaitility", version.ref = "awaitility" }
Expand Down Expand Up @@ -232,3 +233,4 @@ plugin-bnd = "biz.aQute.bnd:biz.aQute.bnd.gradle:6.4.0"
kotlin19 = { id = "org.jetbrains.kotlin.jvm", version = "1.9.23" }
kotlin17 = { id = "org.jetbrains.kotlin.jvm", version = "1.7.22" }
jcstress = { id = "io.github.reyerizo.gradle.jcstress", version = "0.8.15" }
aspectj = { id = 'io.freefair.aspectj.post-compile-weaving', version = '8.6' }
6 changes: 5 additions & 1 deletion micrometer-core/build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
plugins {
alias(libs.plugins.kotlin19)
alias(libs.plugins.aspectj)
id 'me.champeau.mrjar' version "0.1.1"
}

Expand Down Expand Up @@ -81,7 +82,8 @@ dependencies {
}

// Aspects
optionalApi 'org.aspectj:aspectjweaver'
optionalApi libs.aspectjrt
java11RuntimeOnly libs.aspectjrt

// instrumentation options
optionalApi 'io.dropwizard.metrics:metrics-core' // TODO move dropwizard out of core module? DropwizardMeterRegistry for e.g. JMX registry
Expand Down Expand Up @@ -209,6 +211,8 @@ dependencies {
testImplementation 'io.grpc:grpc-inprocess'
testImplementation 'io.grpc:grpc-testing-proto'
testImplementation 'com.squareup.retrofit2:retrofit'

testImplementation 'org.aspectj:aspectjweaver'
}

task shenandoahTest(type: Test) {
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(* *(..))")
@Nullable
public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
if (shouldSkip.test(pjp)) {
Expand Down Expand Up @@ -202,7 +202,7 @@ public Object countedClass(ProceedingJoinPoint pjp) throws Throwable {
* @return Whatever the intercepted method returns.
* @throws Throwable When the intercepted method throws one.
*/
@Around(value = "@annotation(counted)", argNames = "pjp,counted")
@Around(value = "@annotation(counted) && execution(* *(..))", argNames = "pjp,counted")
@Nullable
public Object interceptAndRecord(ProceedingJoinPoint pjp, Counted counted) throws Throwable {
if (shouldSkip.test(pjp)) {
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
7 changes: 6 additions & 1 deletion micrometer-observation/build.gradle
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
plugins {
alias(libs.plugins.aspectj)
}

description 'Module containing Observation related code'

jar {
Expand All @@ -21,7 +25,7 @@ dependencies {
optionalApi 'javax.servlet:javax.servlet-api'

// Aspects
optionalApi 'org.aspectj:aspectjweaver'
optionalApi libs.aspectjrt

// log monitoring
testImplementation 'ch.qos.logback:logback-classic'
Expand All @@ -42,4 +46,5 @@ dependencies {

testImplementation 'org.assertj:assertj-core'
testImplementation 'org.awaitility:awaitility'
testImplementation 'org.aspectj:aspectjweaver'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* Copyright 2024 VMware, Inc.
*
* 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.observation;

import io.micrometer.common.lang.Nullable;

import java.util.Objects;
import java.util.concurrent.atomic.AtomicReference;

/**
* Generator of observations bound to a static global registry. For use especially in
* places where dependency injection of {@link ObservationRegistry} is not possible for an
* instrumented type.
*
* @author Marcin Grzejszczak
* @since 1.14.0
*/
public final class Observations {

private static final ObservationRegistry initialRegistry = ObservationRegistry.create();

private static final DelegatingObservationRegistry globalRegistry = new DelegatingObservationRegistry(
initialRegistry);

private Observations() {
throw new UnsupportedOperationException("You can't instantiate a utility class");
}

/**
* Sets a registry as the global registry.
* @param registry Registry to set.
*/
public static void setRegistry(ObservationRegistry registry) {
globalRegistry.setDelegate(registry);
}

/**
* Resets registry to the original, empty one.
*/
public static void resetRegistry() {
globalRegistry.setDelegate(initialRegistry);
}

/**
* Retrieves the current global instance.
* @return Global registry.
*/
public static ObservationRegistry getGlobalRegistry() {
return globalRegistry;
}

private static final class DelegatingObservationRegistry implements ObservationRegistry {

private final AtomicReference<ObservationRegistry> delegate = new AtomicReference<>(ObservationRegistry.NOOP);

DelegatingObservationRegistry(ObservationRegistry delegate) {
setDelegate(delegate);
}

void setDelegate(ObservationRegistry delegate) {
this.delegate.set(Objects.requireNonNull(delegate, "Delegate must not be null"));
}

@Nullable
@Override
public Observation getCurrentObservation() {
return delegate.get().getCurrentObservation();
}

@Nullable
@Override
public Observation.Scope getCurrentObservationScope() {
return delegate.get().getCurrentObservationScope();
}

@Override
public void setCurrentObservationScope(@Nullable Observation.Scope current) {
delegate.get().setCurrentObservationScope(current);
}

@Override
public ObservationConfig observationConfig() {
return delegate.get().observationConfig();
}

@Override
public boolean isNoop() {
return delegate.get().isNoop();
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.micrometer.common.lang.Nullable;
import io.micrometer.observation.Observation;
import io.micrometer.observation.ObservationRegistry;
import io.micrometer.observation.Observations;
import io.micrometer.observation.annotation.Observed;
import io.micrometer.observation.ObservationConvention;
import org.aspectj.lang.ProceedingJoinPoint;
Expand Down Expand Up @@ -84,6 +85,11 @@ public class ObservedAspect {

private final Predicate<ProceedingJoinPoint> shouldSkip;

// For Compile Time Weaving
public ObservedAspect() {
this(Observations.getGlobalRegistry(), null, DONT_SKIP_ANYTHING);
}

public ObservedAspect(ObservationRegistry registry) {
this(registry, null, DONT_SKIP_ANYTHING);
}
Expand All @@ -105,7 +111,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
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ void skipPredicateShouldTakeEffectForClass() {
void ignoreClassLevelAnnotationIfMethodLevelPresent() {
registry.observationConfig().observationHandler(new ObservationTextPublisher());

AspectJProxyFactory pf = new AspectJProxyFactory(new ObservedClassLevelAnnotatedService());
ObservedClassLevelAnnotatedService annotatedService = new ObservedClassLevelAnnotatedService();
AspectJProxyFactory pf = new AspectJProxyFactory(annotatedService);
pf.addAspect(new ObservedAspect(registry));

ObservedClassLevelAnnotatedService service = pf.getProxy();
Expand Down
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', 'micrometer-test-aspectj-ctw'

['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
19 changes: 19 additions & 0 deletions tests/micrometer-test-aspectj-ctw/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
plugins {
id 'java'
alias(libs.plugins.aspectj)
}

description 'AspectJ compile-time weaving test for Micrometer aspects'

dependencies {
implementation project(':micrometer-core')
aspect project(':micrometer-core')
aspect project(':micrometer-observation')

testImplementation libs.junitJupiter
testImplementation libs.assertj
}

test {
useJUnitPlatform()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright 2024 VMware, Inc.
*
* 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.test.ctw;

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

@Observed
@Counted
@Timed
public class MeasuredClass {

@Timed
public void timedMethod() {
}

@Counted
public void countedMethod() {
}

@Observed
public void observedMethod() {
}

public void classLevelTimedMethod() {
}

public void classLevelCountedMethod() {
}

public void classLevelObservedMethod() {
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright 2024 VMware, Inc.
*
* 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.
*/
@NonNullApi
@NonNullFields
package io.micrometer.test.ctw;

import io.micrometer.common.lang.NonNullApi;
import io.micrometer.common.lang.NonNullFields;
Loading

0 comments on commit 763e1f7

Please sign in to comment.