Fix exception thrown when using a custom error decoder with metrics (#…

…1468) * #1466: fixed metered feign client throwing IllegalArgumentException when using custom ErrorDecoder * #1466: add missing license header * Fold MeteredFeignClientTest into AbstractMetricsTestBase Co-authored-by: Marvin Froeder <marvin.froeder@dovetailstudios.com>
OpenFeign · Jul 26, 2021 · 7861fbc · 7861fbc
1 parent 3fa3e1a
commit 7861fbc
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 27 deletions.
diff --git a/dropwizard-metrics4/src/test/java/feign/metrics4/Metrics4CapabilityTest.java b/dropwizard-metrics4/src/test/java/feign/metrics4/Metrics4CapabilityTest.java
@@ -71,7 +71,8 @@ protected Metric getMetric(String suffix, String... tags) {
           }
 
           for (int i = 0; i < tags.length; i += 2) {
-            if (!name.contains(tags[i]) && !name.contains(tags[i] + 1)) {
+            // metrics 4 doesn't support tags, for that reason we don't include tag name
+            if (!name.contains(tags[i + 1])) {
               return false;
             }
           }

diff --git a/micrometer/src/main/java/feign/micrometer/MeteredInvocationHandleFactory.java b/micrometer/src/main/java/feign/micrometer/MeteredInvocationHandleFactory.java
@@ -67,22 +67,15 @@ public InvocationHandler create(Target target, Map<Method, MethodHandler> dispat
       final Timer.Sample sample = Timer.start(meterRegistry);
       Timer timer = null;
       try {
-        try {
-          final Object invoke = invocationHandle.invoke(proxy, method, args);
-          timer = createTimer(target, method, args, null);
-          return invoke;
-        } catch (Exception e) {
-          throw e;
-        } catch (Throwable e) {
-          throw new Exception(e);
-        }
+        final Object invoke = invocationHandle.invoke(proxy, method, args);
+        timer = createTimer(target, method, args, null);
+        return invoke;
       } catch (final FeignException e) {
         timer = createTimer(target, method, args, e);
         createFeignExceptionCounter(target, method, args, e).increment();
         throw e;
       } catch (final Throwable e) {
         timer = createTimer(target, method, args, e);
-        createExceptionCounter(target, method, args, e).increment();
         throw e;
       } finally {
         if (timer == null) {
@@ -99,15 +92,6 @@ protected Timer createTimer(Target target, Method method, Object[] args, Throwab
     return meterRegistry.timer(metricName.name(e), allTags);
   }
 
-  protected Counter createExceptionCounter(Target target,
-                                           Method method,
-                                           Object[] args,
-                                           Throwable e) {
-    final Tag[] extraTags = extraTags(target, method, args, e);
-    final Tags allTags = metricTagResolver.tag(target.type(), method, target.url(), e, extraTags);
-    return meterRegistry.counter(metricName.name(e), allTags);
-  }
-
   protected Counter createFeignExceptionCounter(Target target,
                                                 Method method,
                                                 Object[] args,

diff --git a/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java b/micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java
@@ -15,12 +15,13 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
-import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Test;
 import feign.Capability;
@@ -123,12 +124,30 @@ public void decoderPropagatesUncheckedException() {
         .addCapability(createMetricCapability())
         .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class));
 
-    try {
-      source.get("0x3456789");
-      fail("Should throw NotFound exception");
-    } catch (FeignException.NotFound e) {
-      assertSame(notFound.get(), e);
-    }
+    FeignException.NotFound thrown =
+        assertThrows(FeignException.NotFound.class, () -> source.get("0x3456789"));
+    assertSame(notFound.get(), thrown);
+
   }
 
+
+  @Test
+  public void shouldMetricCollectionWithCustomException() {
+    final SimpleSource source = Feign.builder()
+        .client((request, options) -> {
+          throw new RuntimeException("Test error");
+        })
+        .addCapability(createMetricCapability())
+        .target(new MockTarget<>(MicrometerCapabilityTest.SimpleSource.class));
+
+    RuntimeException thrown = assertThrows(RuntimeException.class, () -> source.get("0x3456789"));
+    assertThat(thrown.getMessage(), equalTo("Test error"));
+
+    assertThat(
+        getMetric("exception", "exception_name", "RuntimeException", "method", "get"),
+        notNullValue());
+  }
+
+
+
 }