From 8994c8fbb74162162eeea8832baa5edac0ea5b79 Mon Sep 17 00:00:00 2001 From: Marina <66474474+MarinaMoiseenko@users.noreply.github.com> Date: Thu, 19 Sep 2024 20:58:45 +0100 Subject: [PATCH] @Timed should record Throwables not only Exceptions (#5479) --------- Co-authored-by: Jonatan Ivanov --- .../io/micrometer/core/aop/TimedAspect.java | 16 +-- .../micrometer/core/aop/TimedAspectTest.java | 126 ++++++++++-------- 2 files changed, 79 insertions(+), 63 deletions(-) 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 95f657c494..987c959f86 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 @@ -210,9 +210,9 @@ private Object processWithTimer(ProceedingJoinPoint pjp, Timed timed, String met return ((CompletionStage) pjp.proceed()).whenComplete( (result, throwable) -> record(pjp, timed, metricName, sample, getExceptionTag(throwable))); } - catch (Exception ex) { - record(pjp, timed, metricName, sample, ex.getClass().getSimpleName()); - throw ex; + catch (Throwable e) { + record(pjp, timed, metricName, sample, e.getClass().getSimpleName()); + throw e; } } @@ -220,9 +220,9 @@ private Object processWithTimer(ProceedingJoinPoint pjp, Timed timed, String met try { return pjp.proceed(); } - catch (Exception ex) { - exceptionClass = ex.getClass().getSimpleName(); - throw ex; + catch (Throwable e) { + exceptionClass = e.getClass().getSimpleName(); + throw e; } finally { record(pjp, timed, metricName, sample, exceptionClass); @@ -269,9 +269,9 @@ private Object processWithLongTaskTimer(ProceedingJoinPoint pjp, Timed timed, St return ((CompletionStage) pjp.proceed()) .whenComplete((result, throwable) -> sample.ifPresent(this::stopTimer)); } - catch (Exception ex) { + catch (Throwable e) { sample.ifPresent(this::stopTimer); - throw ex; + throw e; } } 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 293d444024..668095f63f 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 @@ -22,7 +22,6 @@ import io.micrometer.core.instrument.Timer; import io.micrometer.core.instrument.distribution.DistributionStatisticConfig; import io.micrometer.core.instrument.distribution.pause.PauseDetector; -import io.micrometer.core.instrument.search.MeterNotFoundException; import io.micrometer.core.instrument.simple.SimpleMeterRegistry; import io.micrometer.core.lang.NonNull; import org.aspectj.lang.ProceedingJoinPoint; @@ -69,7 +68,7 @@ void timeMethodWithSkipPredicate() { service.call(); - assertThat(registry.find("call").timer()).isNull(); + assertThat(registry.getMeters()).isEmpty(); } @Test @@ -87,8 +86,7 @@ void timeMethodWithLongTaskTimer() { .tag("class", getClass().getName() + "$TimedService") .tag("method", "longCall") .tag("extra", "tag") - .longTaskTimers() - .size()).isEqualTo(1); + .longTaskTimers()).hasSize(1); } @Test @@ -102,13 +100,7 @@ void timeMethodFailure() { service.call(); - assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> { - failingRegistry.get("call") - .tag("class", getClass().getName() + "$TimedService") - .tag("method", "call") - .tag("extra", "tag") - .timer(); - }); + assertThat(failingRegistry.getMeters()).isEmpty(); } @Test @@ -122,13 +114,50 @@ void timeMethodFailureWithLongTaskTimer() { service.longCall(); - assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> { - failingRegistry.get("longCall") - .tag("class", getClass().getName() + "$TimedService") - .tag("method", "longCall") - .tag("extra", "tag") - .longTaskTimer(); - }); + assertThat(failingRegistry.getMeters()).isEmpty(); + } + + @Test + void timeMethodWithError() { + MeterRegistry registry = new SimpleMeterRegistry(); + + AspectJProxyFactory pf = new AspectJProxyFactory(new TimedService()); + pf.addAspect(new TimedAspect(registry)); + + TimedService service = pf.getProxy(); + + assertThat(registry.getMeters()).isEmpty(); + + assertThatThrownBy(service::callRaisingError).isInstanceOf(TestError.class); + + assertThat(registry.get("callRaisingError") + .tag("class", getClass().getName() + "$TimedService") + .tag("method", "callRaisingError") + .tag("extra", "tag") + .tag("exception", "TestError") + .timer() + .count()).isEqualTo(1); + } + + @Test + void timeMethodWithErrorAndLongTaskTimer() { + MeterRegistry registry = new SimpleMeterRegistry(); + + AspectJProxyFactory pf = new AspectJProxyFactory(new TimedService()); + pf.addAspect(new TimedAspect(registry)); + + TimedService service = pf.getProxy(); + + assertThat(registry.getMeters()).isEmpty(); + + assertThatThrownBy(service::longCallRaisingError).isInstanceOf(TestError.class); + + assertThat(registry.get("longCallRaisingError") + .tag("class", getClass().getName() + "$TimedService") + .tag("method", "longCallRaisingError") + .tag("extra", "tag") + .longTaskTimer() + .activeTasks()).isEqualTo(0); } @Test @@ -143,12 +172,7 @@ void timeMethodWhenCompleted() { GuardedResult guardedResult = new GuardedResult(); CompletableFuture completableFuture = service.call(guardedResult); - assertThat(registry.find("call") - .tag("class", getClass().getName() + "$AsyncTimedService") - .tag("method", "call") - .tag("extra", "tag") - .tag("exception", "none") - .timer()).isNull(); + assertThat(registry.getMeters()).isEmpty(); guardedResult.complete(); completableFuture.join(); @@ -174,21 +198,16 @@ void timeMethodWhenCompletedExceptionally() { GuardedResult guardedResult = new GuardedResult(); CompletableFuture completableFuture = service.call(guardedResult); - assertThat(registry.find("call") - .tag("class", getClass().getName() + "$AsyncTimedService") - .tag("method", "call") - .tag("extra", "tag") - .tag("exception", "NullPointerException") - .timer()).isNull(); + assertThat(registry.getMeters()).isEmpty(); - guardedResult.complete(new NullPointerException()); + guardedResult.complete(new IllegalStateException("simulated")); catchThrowableOfType(completableFuture::join, CompletionException.class); assertThat(registry.get("call") .tag("class", getClass().getName() + "$AsyncTimedService") .tag("method", "call") .tag("extra", "tag") - .tag("exception", "NullPointerException") + .tag("exception", "IllegalStateException") .timer() .count()).isEqualTo(1); } @@ -267,12 +286,7 @@ void timeMethodFailureWhenCompletedExceptionally() { guardedResult.complete(); completableFuture.join(); - assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> failingRegistry.get("call") - .tag("class", getClass().getName() + "$AsyncTimedService") - .tag("method", "call") - .tag("extra", "tag") - .tag("exception", "none") - .timer()); + assertThat(failingRegistry.getMeters()).isEmpty(); } @Test @@ -289,13 +303,7 @@ void timeMethodFailureWithLongTaskTimerWhenCompleted() { guardedResult.complete(); completableFuture.join(); - assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> { - failingRegistry.get("longCall") - .tag("class", getClass().getName() + "$AsyncTimedService") - .tag("method", "longCall") - .tag("extra", "tag") - .longTaskTimer(); - }); + assertThat(failingRegistry.getMeters()).isEmpty(); } @Test @@ -361,16 +369,10 @@ void timeClassFailure() { service.call(); - assertThatExceptionOfType(MeterNotFoundException.class).isThrownBy(() -> { - failingRegistry.get("call") - .tag("class", "io.micrometer.core.aop.TimedAspectTest$TimedClass") - .tag("method", "call") - .tag("extra", "tag") - .timer(); - }); + assertThat(failingRegistry.getMeters()).isEmpty(); } - private final class FailingMeterRegistry extends SimpleMeterRegistry { + private static final class FailingMeterRegistry extends SimpleMeterRegistry { private FailingMeterRegistry() { super(); @@ -380,14 +382,14 @@ private FailingMeterRegistry() { @Override protected Timer newTimer(@NonNull Id id, @NonNull DistributionStatisticConfig distributionStatisticConfig, @NonNull PauseDetector pauseDetector) { - throw new RuntimeException(); + throw new RuntimeException("FailingMeterRegistry"); } @NonNull @Override protected LongTaskTimer newLongTaskTimer(@Nonnull Id id, @Nonnull DistributionStatisticConfig distributionStatisticConfig) { - throw new RuntimeException(); + throw new RuntimeException("FailingMeterRegistry"); } } @@ -402,6 +404,16 @@ void call() { void longCall() { } + @Timed(value = "callRaisingError", extraTags = { "extra", "tag" }) + void callRaisingError() { + throw new TestError(); + } + + @Timed(value = "longCallRaisingError", extraTags = { "extra", "tag" }, longTask = true) + void longCallRaisingError() { + throw new TestError(); + } + } static class AsyncTimedService { @@ -476,4 +488,8 @@ public void call() { } + static class TestError extends Error { + + } + }