@timed should record Throwables not only Exceptions (#5479)

---------

Co-authored-by: Jonatan Ivanov <[email protected]>

Author: MarinaMoiseenko

micrometer-core/src/main/java/io/micrometer/core/aop/TimedAspect.java

@@ -210,19 +210,19 @@ 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;
             }
         }
 
         String exceptionClass = DEFAULT_EXCEPTION_TAG_VALUE;
         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;
             }
         }
 

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 {
+
+    }
+
 }