From ae1a843920434e3c707ffd226a3b900f969bd237 Mon Sep 17 00:00:00 2001 From: Johnny Lim Date: Thu, 12 Dec 2024 11:25:48 +0900 Subject: [PATCH 1/5] Remove outdated warnings in docs for @Counted/@Timed (#5736) --- docs/modules/ROOT/pages/concepts/counters.adoc | 2 -- docs/modules/ROOT/pages/concepts/timers.adoc | 2 -- 2 files changed, 4 deletions(-) diff --git a/docs/modules/ROOT/pages/concepts/counters.adoc b/docs/modules/ROOT/pages/concepts/counters.adoc index c0a165ad9a..ac860d36cf 100644 --- a/docs/modules/ROOT/pages/concepts/counters.adoc +++ b/docs/modules/ROOT/pages/concepts/counters.adoc @@ -50,8 +50,6 @@ Counter counter = Counter The `micrometer-core` module contains a `@Counted` annotation that frameworks can use to add counting support to either specific types of methods such as those serving web request endpoints or, more generally, to all methods. -WARNING: Micrometer's Spring Boot configuration does _not_ recognize `@Counted` on arbitrary methods. - Also, an incubating AspectJ aspect is included in `micrometer-core`. You can use it in your application either through compile/load time AspectJ weaving or through framework facilities that interpret AspectJ aspects and proxy targeted methods in some other way, such as Spring AOP. Here is a sample Spring AOP configuration: [source,java] diff --git a/docs/modules/ROOT/pages/concepts/timers.adoc b/docs/modules/ROOT/pages/concepts/timers.adoc index 660dd60f04..fad4545bda 100644 --- a/docs/modules/ROOT/pages/concepts/timers.adoc +++ b/docs/modules/ROOT/pages/concepts/timers.adoc @@ -73,8 +73,6 @@ Note how we do not decide the timer to which to accumulate the sample until it i The `micrometer-core` module contains a `@Timed` annotation that frameworks can use to add timing support to either specific types of methods such as those serving web request endpoints or, more generally, to all methods. -WARNING: Micrometer's Spring Boot configuration does _not_ recognize `@Timed` on arbitrary methods. - Also, an incubating AspectJ aspect is included in `micrometer-core`. You can use it in your application either through compile/load time AspectJ weaving or through framework facilities that interpret AspectJ aspects and proxy targeted methods in some other way, such as Spring AOP. Here is a sample Spring AOP configuration: [source,java] From 503f2e9390015be91c2e226e2e31981c77501748 Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Thu, 12 Dec 2024 15:58:46 +0900 Subject: [PATCH 2/5] Add jcstress test for get/remove keyvalue This scenario produced a NPE before the change from using a LinkedHashMap to ConcurrentHashMap. See gh-5739 --- .../ObservationContextConcurrencyTest.java | 49 ++++++++++++++++--- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java b/concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java index ae66eb3ec8..c0a3dd638c 100644 --- a/concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java +++ b/concurrency-tests/src/jcstress/java/io/micrometer/concurrencytests/ObservationContextConcurrencyTest.java @@ -35,31 +35,68 @@ public class ObservationContextConcurrencyTest { @State @Outcome(id = "No exception, No exception", expect = ACCEPTABLE) @Outcome(expect = FORBIDDEN) - public static class ConsistentKeyValues { + public static class ConsistentKeyValuesGetAdd { private final Observation.Context context = new TestContext(); private final String uuid = UUID.randomUUID().toString(); @Actor - public void read(LL_Result r) { + public void get(LL_Result r) { try { context.getHighCardinalityKeyValues(); r.r1 = "No exception"; } catch (Exception e) { - r.r1 = e.getClass(); + r.r1 = e.getClass().getSimpleName(); } } @Actor - public void write(LL_Result r) { + public void add(LL_Result r) { try { - context.addHighCardinalityKeyValue(KeyValue.of(uuid, uuid)); + context.addHighCardinalityKeyValue(KeyValue.of("uuid", uuid)); r.r2 = "No exception"; } catch (Exception e) { - r.r2 = e.getClass(); + r.r2 = e.getClass().getSimpleName(); + } + } + + } + + @JCStressTest + @State + @Outcome(id = "No exception, No exception", expect = ACCEPTABLE) + @Outcome(expect = FORBIDDEN) + public static class ConsistentKeyValuesGetRemove { + + private final Observation.Context context = new TestContext(); + + public ConsistentKeyValuesGetRemove() { + context.addLowCardinalityKeyValue(KeyValue.of("keep", "donotremoveme")); + context.addLowCardinalityKeyValue(KeyValue.of("remove", "removeme")); + } + + @Actor + public void get(LL_Result r) { + try { + context.getAllKeyValues(); + r.r1 = "No exception"; + } + catch (Exception e) { + r.r1 = e.getClass().getSimpleName(); + } + } + + @Actor + public void remove(LL_Result r) { + try { + context.removeLowCardinalityKeyValue("remove"); + r.r2 = "No exception"; + } + catch (Exception e) { + r.r2 = e.getClass().getSimpleName(); } } From ba52ad9102e5f33cfbfc0a1d0bf9d5c656b43365 Mon Sep 17 00:00:00 2001 From: Tommy Ludwig <8924140+shakuzen@users.noreply.github.com> Date: Thu, 12 Dec 2024 16:01:34 +0900 Subject: [PATCH 3/5] Jcstress polish Add jcstress related temp/generated files to gitignore. Ensure the upgraded version of jcstress is used by the jcstress Gradle plugin - the way it was declared before did not change the version of jcstress used. --- .gitignore | 5 +++++ concurrency-tests/build.gradle | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index c66f719e15..94db3c90b9 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,8 @@ bin/ .vscode .DS_Store .java-version + +# jcstress +generated/ +results/ +jcstress-results-*.bin.gz diff --git a/concurrency-tests/build.gradle b/concurrency-tests/build.gradle index 067f8c0975..1fe9a5ade1 100644 --- a/concurrency-tests/build.gradle +++ b/concurrency-tests/build.gradle @@ -3,14 +3,17 @@ plugins { } dependencies { + // Comment out the local project references and reference an old version to compare jcstress results across versions + // Make sure to use a consistent version for all micrometer dependencies implementation project(":micrometer-core") -// implementation("io.micrometer:micrometer-core:1.12.4") +// implementation("io.micrometer:micrometer-core:1.14.1") implementation project(":micrometer-registry-prometheus") +// implementation("io.micrometer:micrometer-registry-prometheus:1.14.1") runtimeOnly(libs.logbackLatest) } jcstress { - libs.jcstressCore + jcstressDependency libs.jcstressCore.get().toString() // This affects how long and thorough testing will be // In order of increasing stress: sanity, quick, default, tough, stress From 6be6a7e5c08321aca4c90cddc4eaf7a34f57b165 Mon Sep 17 00:00:00 2001 From: Johnny Lim Date: Mon, 16 Dec 2024 09:37:16 +0900 Subject: [PATCH 4/5] Omit EnumSource.value where possible (#5744) See https://github.com/spring-projects/spring-boot/pull/43214 --- .../micrometer/docs/metrics/TimedAspectTest.java | 6 +++--- .../statsd/StatsdMeterRegistryPublishTest.java | 16 ++++++++-------- .../statsd/StatsdMeterRegistryTest.java | 14 +++++++------- .../io/micrometer/core/aop/TimedAspectTest.java | 6 +++--- .../binder/cache/CaffeineStatsCounterTest.java | 2 +- ...ntTimingInstrumentationVerificationTests.java | 10 +++++----- ...erTimingInstrumentationVerificationTests.java | 10 +++++----- .../ipc/http/HttpSenderCompatibilityKit.java | 8 ++++---- 8 files changed, 36 insertions(+), 36 deletions(-) diff --git a/docs/src/test/java/io/micrometer/docs/metrics/TimedAspectTest.java b/docs/src/test/java/io/micrometer/docs/metrics/TimedAspectTest.java index 59ffe5fae2..0af71e5e75 100644 --- a/docs/src/test/java/io/micrometer/docs/metrics/TimedAspectTest.java +++ b/docs/src/test/java/io/micrometer/docs/metrics/TimedAspectTest.java @@ -42,7 +42,7 @@ static class MeterTagsTests { // end::resolvers[] @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithText(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); @@ -65,7 +65,7 @@ void meterTagsWithText(AnnotatedTestClass annotatedClass) { } @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithResolver(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); @@ -88,7 +88,7 @@ void meterTagsWithResolver(AnnotatedTestClass annotatedClass) { } @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithExpression(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); diff --git a/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryPublishTest.java b/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryPublishTest.java index 36e4e21db7..ae1d89fb6e 100644 --- a/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryPublishTest.java +++ b/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryPublishTest.java @@ -80,7 +80,7 @@ void cleanUp() { } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void receiveAllBufferedMetricsAfterCloseSuccessfully(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); serverLatch = new CountDownLatch(3); @@ -98,7 +98,7 @@ void receiveAllBufferedMetricsAfterCloseSuccessfully(StatsdProtocol protocol) th } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void receiveMetricsSuccessfully(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); serverLatch = new CountDownLatch(3); @@ -116,7 +116,7 @@ void receiveMetricsSuccessfully(StatsdProtocol protocol) throws InterruptedExcep } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void resumeSendingMetrics_whenServerIntermittentlyFails(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); serverLatch = new CountDownLatch(1); @@ -163,7 +163,7 @@ void resumeSendingMetrics_whenServerIntermittentlyFails(StatsdProtocol protocol) } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource @Issue("#1676") void stopAndStartMeterRegistrySendsMetrics(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); @@ -206,7 +206,7 @@ void stopAndStartMeterRegistryWithLineSink() throws InterruptedException { } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void whenBackendInitiallyDown_metricsSentAfterBackendStarts(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); AtomicInteger writeCount = new AtomicInteger(); @@ -245,7 +245,7 @@ void whenBackendInitiallyDown_metricsSentAfterBackendStarts(StatsdProtocol proto } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void whenRegistryStopped_doNotConnectToBackend(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); serverLatch = new CountDownLatch(3); @@ -264,7 +264,7 @@ void whenRegistryStopped_doNotConnectToBackend(StatsdProtocol protocol) throws I } @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource @Issue("#2177") void whenSendError_reconnectsAndWritesNewMetrics(StatsdProtocol protocol) throws InterruptedException { skipUdsTestOnWindows(protocol); @@ -296,7 +296,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) @Issue("#2880") @ParameterizedTest - @EnumSource(StatsdProtocol.class) + @EnumSource void receiveParallelMetricsSuccessfully(StatsdProtocol protocol) throws InterruptedException { final int n = 10; diff --git a/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryTest.java b/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryTest.java index c3fc93166a..582d6cca98 100644 --- a/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryTest.java +++ b/implementations/micrometer-registry-statsd/src/test/java/io/micrometer/statsd/StatsdMeterRegistryTest.java @@ -79,7 +79,7 @@ public StatsdFlavor flavor() { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource void counterLineProtocol(StatsdFlavor flavor) { String line = null; switch (flavor) { @@ -112,7 +112,7 @@ void counterLineProtocol(StatsdFlavor flavor) { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource void gaugeLineProtocol(StatsdFlavor flavor) { final AtomicInteger n = new AtomicInteger(2); final StatsdConfig config = configWithFlavor(flavor); @@ -145,7 +145,7 @@ void gaugeLineProtocol(StatsdFlavor flavor) { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource void timerLineProtocol(StatsdFlavor flavor) { String line = null; switch (flavor) { @@ -178,7 +178,7 @@ void timerLineProtocol(StatsdFlavor flavor) { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource void summaryLineProtocol(StatsdFlavor flavor) { String line = null; switch (flavor) { @@ -211,7 +211,7 @@ void summaryLineProtocol(StatsdFlavor flavor) { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource void longTaskTimerLineProtocol(StatsdFlavor flavor) { final StatsdConfig config = configWithFlavor(flavor); long stepMillis = config.step().toMillis(); @@ -286,7 +286,7 @@ void counterIncrementDoesNotCauseStackOverflow() { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource @Issue("#370") void serviceLevelObjectivesOnlyNoPercentileHistogram(StatsdFlavor flavor) { StatsdConfig config = configWithFlavor(flavor); @@ -390,7 +390,7 @@ void interactWithStoppedRegistry() { } @ParameterizedTest - @EnumSource(StatsdFlavor.class) + @EnumSource @Issue("#600") void memoryPerformanceOfNamingConventionInHotLoops(StatsdFlavor flavor) { AtomicInteger namingConventionUses = new AtomicInteger(); diff --git a/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java b/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java index 40946c00a7..529f11f684 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java @@ -425,7 +425,7 @@ class MeterTagsTests { aClass -> valueExpressionResolver); @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithText(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); @@ -442,7 +442,7 @@ void meterTagsWithText(AnnotatedTestClass annotatedClass) { } @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithResolver(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); @@ -462,7 +462,7 @@ void meterTagsWithResolver(AnnotatedTestClass annotatedClass) { } @ParameterizedTest - @EnumSource(AnnotatedTestClass.class) + @EnumSource void meterTagsWithExpression(AnnotatedTestClass annotatedClass) { MeterRegistry registry = new SimpleMeterRegistry(); TimedAspect timedAspect = new TimedAspect(registry); diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/cache/CaffeineStatsCounterTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/cache/CaffeineStatsCounterTest.java index 358969bd7c..b10fb495a6 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/cache/CaffeineStatsCounterTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/cache/CaffeineStatsCounterTest.java @@ -94,7 +94,7 @@ void loadFailure() { } @ParameterizedTest - @EnumSource(RemovalCause.class) + @EnumSource void evictionWithCause(RemovalCause cause) { stats.recordEviction(3, cause); DistributionSummary summary = fetch("cache.evictions", "cause", cause.name()).summary(); diff --git a/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpClientTimingInstrumentationVerificationTests.java b/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpClientTimingInstrumentationVerificationTests.java index 945c3e18fb..67a0bd0a94 100644 --- a/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpClientTimingInstrumentationVerificationTests.java +++ b/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpClientTimingInstrumentationVerificationTests.java @@ -144,7 +144,7 @@ protected String substitutePathVariables(String templatedPath, String... pathVar } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void getTemplatedPathForUri(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { checkAndSetupTestForTestType(testType); @@ -162,7 +162,7 @@ void getTemplatedPathForUri(TestType testType, WireMockRuntimeInfo wmRuntimeInfo } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource @Disabled("apache/jetty http client instrumentation currently fails this test") void timedWhenServerIsMissing(TestType testType) throws IOException { checkAndSetupTestForTestType(testType); @@ -186,7 +186,7 @@ void timedWhenServerIsMissing(TestType testType) throws IOException { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void serverException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { checkAndSetupTestForTestType(testType); @@ -203,7 +203,7 @@ void serverException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void clientException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { checkAndSetupTestForTestType(testType); @@ -223,7 +223,7 @@ void clientException(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { // TODO this test doesn't need to be parameterized but the custom resolver for // Before/After methods doesn't like when it isn't. @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void headerIsPropagatedFromContext(TestType testType, WireMockRuntimeInfo wmRuntimeInfo) { checkAndSetupTestForTestType(testType); diff --git a/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpServerTimingInstrumentationVerificationTests.java b/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpServerTimingInstrumentationVerificationTests.java index 2b89566f0a..0d86fd29ee 100644 --- a/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpServerTimingInstrumentationVerificationTests.java +++ b/micrometer-test/src/main/java/io/micrometer/core/instrument/HttpServerTimingInstrumentationVerificationTests.java @@ -120,14 +120,14 @@ void afterEach() throws Exception { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void uriIsNotFound_whenRouteIsUnmapped(TestType testType) throws Throwable { sender.get(baseUri + "notFound").send(); checkTimer(rs -> rs.tags("uri", "NOT_FOUND", "status", "404", "method", "GET").timer().count() == 1); } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void uriTemplateIsTagged(TestType testType) throws Throwable { sender.get(baseUri + "hello/world").send(); checkTimer(rs -> rs.tags("uri", InstrumentedRoutes.TEMPLATED_ROUTE, "status", "200", "method", "GET") @@ -136,7 +136,7 @@ void uriTemplateIsTagged(TestType testType) throws Throwable { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void redirect(TestType testType) throws Throwable { sender.get(baseUri + "foundRedirect").send(); checkTimer(rs -> rs.tags("uri", InstrumentedRoutes.REDIRECT, "status", "302", "method", "GET") @@ -145,7 +145,7 @@ void redirect(TestType testType) throws Throwable { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void errorResponse(TestType testType) throws Throwable { sender.post(baseUri + "error").send(); checkTimer( @@ -153,7 +153,7 @@ void errorResponse(TestType testType) throws Throwable { } @ParameterizedTest - @EnumSource(TestType.class) + @EnumSource void canExtractContextFromHeaders(TestType testType) throws Throwable { sender.get(baseUri + "hello/micrometer").withHeader("Test-Propagation", "someValue").send(); diff --git a/micrometer-test/src/main/java/io/micrometer/core/ipc/http/HttpSenderCompatibilityKit.java b/micrometer-test/src/main/java/io/micrometer/core/ipc/http/HttpSenderCompatibilityKit.java index a81ecc24ea..5cfbd5d84a 100644 --- a/micrometer-test/src/main/java/io/micrometer/core/ipc/http/HttpSenderCompatibilityKit.java +++ b/micrometer-test/src/main/java/io/micrometer/core/ipc/http/HttpSenderCompatibilityKit.java @@ -56,7 +56,7 @@ void httpSenderIsNotNull() { @ParameterizedTest @DisplayName("successfully send a request with NO body and receive a response with NO body") - @EnumSource(HttpSender.Method.class) + @EnumSource void successfulRequestSentWithNoBody(HttpSender.Method method, @WiremockResolver.Wiremock WireMockServer server) throws Throwable { server.stubFor(any(urlEqualTo("/metrics"))); @@ -104,7 +104,7 @@ void successfulRequestSentWithBody(HttpSender.Method method, @WiremockResolver.W @ParameterizedTest @DisplayName("receive an error response") - @EnumSource(HttpSender.Method.class) + @EnumSource void errorResponseReceived(HttpSender.Method method, @WiremockResolver.Wiremock WireMockServer server) throws Throwable { server.stubFor(any(urlEqualTo("/metrics")).willReturn(badRequest().withBody("Error processing metrics"))); @@ -121,7 +121,7 @@ void errorResponseReceived(HttpSender.Method method, @WiremockResolver.Wiremock } @ParameterizedTest - @EnumSource(HttpSender.Method.class) + @EnumSource void basicAuth(HttpSender.Method method, @WiremockResolver.Wiremock WireMockServer server) throws Throwable { server.stubFor(any(urlEqualTo("/metrics")).willReturn(unauthorized())); @@ -140,7 +140,7 @@ void basicAuth(HttpSender.Method method, @WiremockResolver.Wiremock WireMockServ } @ParameterizedTest - @EnumSource(HttpSender.Method.class) + @EnumSource void customHeader(HttpSender.Method method, @WiremockResolver.Wiremock WireMockServer server) throws Throwable { server.stubFor(any(urlEqualTo("/metrics")).willReturn(unauthorized())); From 12c1f0822a764ef7163ab3816f52031c0908fa87 Mon Sep 17 00:00:00 2001 From: laststem Date: Mon, 16 Dec 2024 09:43:38 +0900 Subject: [PATCH 5/5] Fix completion stage returns null in aspects (#5742) Handling the case was missing when `null` was returned from an annotated method with return type CompletionStage, resulting in a NullPointerException. Fixes #5741 --- .../io/micrometer/core/aop/CountedAspect.java | 14 ++++- .../io/micrometer/core/aop/TimedAspect.java | 24 ++++++-- .../core/aop/CountedAspectTest.java | 33 +++++++++++ .../micrometer/core/aop/TimedAspectTest.java | 59 +++++++++++++++++++ .../observation/aop/ObservedAspect.java | 12 +++- .../observation/aop/ObservedAspectTests.java | 27 +++++++++ 6 files changed, 161 insertions(+), 8 deletions(-) diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java b/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java index 7038b35fb6..dbadeaffa3 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java +++ b/micrometer-core/src/main/java/io/micrometer/core/aop/CountedAspect.java @@ -69,6 +69,7 @@ * * @author Ali Dehghani * @author Jonatan Ivanov + * @author Jeonggi Kim * @since 1.2.0 * @see Counted */ @@ -212,8 +213,17 @@ public Object interceptAndRecord(ProceedingJoinPoint pjp, Counted counted) throw if (stopWhenCompleted) { try { - return ((CompletionStage) pjp.proceed()) - .whenComplete((result, throwable) -> recordCompletionResult(pjp, counted, throwable)); + Object result = pjp.proceed(); + if (result == null) { + if (!counted.recordFailuresOnly()) { + record(pjp, counted, DEFAULT_EXCEPTION_TAG_VALUE, RESULT_TAG_SUCCESS_VALUE); + } + return result; + } + else { + CompletionStage stage = ((CompletionStage) result); + return stage.whenComplete((res, throwable) -> recordCompletionResult(pjp, counted, throwable)); + } } catch (Throwable e) { record(pjp, counted, e.getClass().getSimpleName(), RESULT_TAG_FAILURE_VALUE); diff --git a/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java b/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java index ab7cc33226..16bed71480 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java +++ b/micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java @@ -77,6 +77,7 @@ * @author Johnny Lim * @author Nejc Korasa * @author Jonatan Ivanov + * @author Jeonggi Kim * @since 1.0.0 */ @Aspect @@ -230,8 +231,16 @@ private Object processWithTimer(ProceedingJoinPoint pjp, Timed timed, String met if (stopWhenCompleted) { try { - return ((CompletionStage) pjp.proceed()).whenComplete( - (result, throwable) -> record(pjp, timed, metricName, sample, getExceptionTag(throwable))); + Object result = pjp.proceed(); + if (result == null) { + record(pjp, timed, metricName, sample, DEFAULT_EXCEPTION_TAG_VALUE); + return result; + } + else { + CompletionStage stage = ((CompletionStage) result); + return stage.whenComplete( + (res, throwable) -> record(pjp, timed, metricName, sample, getExceptionTag(throwable))); + } } catch (Throwable e) { record(pjp, timed, metricName, sample, e.getClass().getSimpleName()); @@ -297,8 +306,15 @@ private Object processWithLongTaskTimer(ProceedingJoinPoint pjp, Timed timed, St if (stopWhenCompleted) { try { - return ((CompletionStage) pjp.proceed()) - .whenComplete((result, throwable) -> sample.ifPresent(this::stopTimer)); + Object result = pjp.proceed(); + if (result == null) { + sample.ifPresent(this::stopTimer); + return result; + } + else { + CompletionStage stage = ((CompletionStage) result); + return stage.whenComplete((res, throwable) -> sample.ifPresent(this::stopTimer)); + } } catch (Throwable e) { sample.ifPresent(this::stopTimer); diff --git a/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java b/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java index 120ae42a47..dddb4195f7 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/aop/CountedAspectTest.java @@ -211,6 +211,29 @@ void pjpFunctionThrows() { assertThat(counter.getId().getDescription()).isNull(); } + @Test + void countedWithSuccessfulMetricsWhenReturnsCompletionStageNull() { + CompletableFuture completableFuture = asyncCountedService.successButNull(); + assertThat(completableFuture).isNull(); + + assertThat(meterRegistry.get("metric.success") + .tag("method", "successButNull") + .tag("class", getClass().getName() + "$AsyncCountedService") + .tag("extra", "tag") + .tag("exception", "none") + .tag("result", "success") + .counter() + .count()).isOne(); + } + + @Test + void countedWithoutSuccessfulMetricsWhenReturnsCompletionStageNull() { + CompletableFuture completableFuture = asyncCountedService.successButNullWithoutMetrics(); + assertThat(completableFuture).isNull(); + + assertThatThrownBy(() -> meterRegistry.get("metric.none").counter()).isInstanceOf(MeterNotFoundException.class); + } + static class CountedService { @Counted(value = "metric.none", recordFailuresOnly = true) @@ -272,6 +295,16 @@ CompletableFuture emptyMetricName(GuardedResult guardedResult) { return supplyAsync(guardedResult::get); } + @Counted(value = "metric.success", extraTags = { "extra", "tag" }) + CompletableFuture successButNull() { + return null; + } + + @Counted(value = "metric.none", recordFailuresOnly = true) + CompletableFuture successButNullWithoutMetrics() { + return null; + } + } static class GuardedResult { diff --git a/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java b/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java index b2cd05773f..4920d429e5 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/aop/TimedAspectTest.java @@ -217,6 +217,29 @@ void timeMethodWhenCompletedExceptionally() { .count()).isEqualTo(1); } + @Test + void timeMethodWhenReturnCompletionStageNull() { + MeterRegistry registry = new SimpleMeterRegistry(); + + AspectJProxyFactory pf = new AspectJProxyFactory(new AsyncTimedService()); + pf.addAspect(new TimedAspect(registry)); + + AsyncTimedService service = pf.getProxy(); + + CompletableFuture completableFuture = service.callNull(); + assertThat(completableFuture).isNull(); + + assertThat(registry.getMeters()).isNotEmpty(); + + assertThat(registry.get("callNull") + .tag("class", getClass().getName() + "$AsyncTimedService") + .tag("method", "callNull") + .tag("extra", "tag") + .tag("exception", "none") + .timer() + .count()).isEqualTo(1); + } + @Test void timeMethodWithLongTaskTimerWhenCompleted() { MeterRegistry registry = new SimpleMeterRegistry(); @@ -277,6 +300,32 @@ void timeMethodWithLongTaskTimerWhenCompletedExceptionally() { .activeTasks()).isEqualTo(0); } + @Test + void timeMethodWithLongTaskTimerWhenReturnCompletionStageNull() { + MeterRegistry registry = new SimpleMeterRegistry(); + + AspectJProxyFactory pf = new AspectJProxyFactory(new AsyncTimedService()); + pf.addAspect(new TimedAspect(registry)); + + AsyncTimedService service = pf.getProxy(); + + CompletableFuture completableFuture = service.longCallNull(); + assertThat(completableFuture).isNull(); + + assertThat(registry.get("longCallNull") + .tag("class", getClass().getName() + "$AsyncTimedService") + .tag("method", "longCallNull") + .tag("extra", "tag") + .longTaskTimers()).hasSize(1); + + assertThat(registry.find("longCallNull") + .tag("class", getClass().getName() + "$AsyncTimedService") + .tag("method", "longCallNull") + .tag("extra", "tag") + .longTaskTimer() + .activeTasks()).isEqualTo(0); + } + @Test void timeMethodFailureWhenCompletedExceptionally() { MeterRegistry failingRegistry = new FailingMeterRegistry(); @@ -641,11 +690,21 @@ CompletableFuture call(GuardedResult guardedResult) { return supplyAsync(guardedResult::get); } + @Timed(value = "callNull", extraTags = { "extra", "tag" }) + CompletableFuture callNull() { + return null; + } + @Timed(value = "longCall", extraTags = { "extra", "tag" }, longTask = true) CompletableFuture longCall(GuardedResult guardedResult) { return supplyAsync(guardedResult::get); } + @Timed(value = "longCallNull", extraTags = { "extra", "tag" }, longTask = true) + CompletableFuture longCallNull() { + return null; + } + } static class GuardedResult { diff --git a/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java b/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java index 7595a51621..dc19aa3a75 100644 --- a/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java +++ b/micrometer-observation/src/main/java/io/micrometer/observation/aop/ObservedAspect.java @@ -68,6 +68,7 @@ * * * @author Jonatan Ivanov + * @author Jeonggi Kim * @since 1.10.0 */ @Aspect @@ -135,8 +136,15 @@ private Object observe(ProceedingJoinPoint pjp, Method method, Observed observed observation.start(); Observation.Scope scope = observation.openScope(); try { - return ((CompletionStage) pjp.proceed()) - .whenComplete((result, error) -> stopObservation(observation, scope, error)); + Object result = pjp.proceed(); + if (result == null) { + stopObservation(observation, scope, null); + return result; + } + else { + CompletionStage stage = (CompletionStage) result; + return stage.whenComplete((res, error) -> stopObservation(observation, scope, error)); + } } catch (Throwable error) { stopObservation(observation, scope, error); diff --git a/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java b/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java index 51834f48d3..6c216effc4 100644 --- a/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java +++ b/micrometer-observation/src/test/java/io/micrometer/observation/aop/ObservedAspectTests.java @@ -360,6 +360,29 @@ void skipPredicateShouldTakeEffectForClass() { TestObservationRegistryAssert.assertThat(registry).doesNotHaveAnyObservation(); } + @Test + void annotatedAsyncClassCallWithNullShouldBeObserved() { + registry.observationConfig().observationHandler(new ObservationTextPublisher()); + + AspectJProxyFactory pf = new AspectJProxyFactory(new ObservedClassLevelAnnotatedService()); + pf.addAspect(new ObservedAspect(registry)); + + ObservedClassLevelAnnotatedService service = pf.getProxy(); + CompletableFuture asyncResult = service.asyncNull(); + assertThat(asyncResult).isNull(); + + TestObservationRegistryAssert.assertThat(registry) + .doesNotHaveAnyRemainingCurrentObservation() + .hasSingleObservationThat() + .hasNameEqualTo("test.class") + .hasContextualNameEqualTo("test.class#call") + .hasLowCardinalityKeyValue("abc", "123") + .hasLowCardinalityKeyValue("test", "42") + .hasLowCardinalityKeyValue("class", ObservedClassLevelAnnotatedService.class.getName()) + .hasLowCardinalityKeyValue("method", "asyncNull") + .doesNotHaveError(); + } + static class ObservedService { @Observed(name = "test.call", contextualName = "test#call", @@ -444,6 +467,10 @@ CompletableFuture async(FakeAsyncTask fakeAsyncTask) { contextSnapshot.wrapExecutor(Executors.newSingleThreadExecutor())); } + CompletableFuture asyncNull() { + return null; + } + } static class FakeAsyncTask implements Supplier {