Collect http_response_code for successfull and failed requests (#1375)

On previous implementation response code would only be collected for errors. To avoid mixing new and old metrics, gave the one that includes successfull codes a different name
OpenFeign · Mar 13, 2021 · fe901ca · fe901ca
1 parent c4b89c2
commit fe901ca
Show file tree

Hide file tree

Showing 11 changed files with 429 additions and 162 deletions.
diff --git a/dropwizard-metrics4/pom.xml b/dropwizard-metrics4/pom.xml
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
 
-    Copyright 2012-2020 The Feign Authors
+    Copyright 2012-2021 The Feign Authors
 
     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
@@ -50,5 +50,11 @@
       <version>2.0.0.0</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>feign-micrometer</artifactId>
+      <type>test-jar</type>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 </project>
diff --git a/dropwizard-metrics4/src/main/java/feign/metrics4/MeteredClient.java b/dropwizard-metrics4/src/main/java/feign/metrics4/MeteredClient.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2020 The Feign Authors
+ * Copyright 2012-2021 The Feign Authors
  *
  * 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
@@ -45,7 +45,27 @@ public Response execute(Request request, Options options) throws IOException {
         metricRegistry.timer(
             metricName.metricName(template.methodMetadata(), template.feignTarget()),
             metricSuppliers.timers()).time()) {
-      return client.execute(request, options);
+      Response response = client.execute(request, options);
+      metricRegistry.meter(
+          MetricRegistry.name(
+              metricName.metricName(template.methodMetadata(), template.feignTarget(),
+                  "http_response_code"),
+              "status_group", response.status() / 100 + "xx", "http_status",
+              String.valueOf(response.status())),
+          metricSuppliers.meters()).mark();
+      return response;
+    } catch (FeignException e) {
+      metricRegistry.meter(
+          MetricRegistry.name(
+              metricName.metricName(template.methodMetadata(), template.feignTarget(),
+                  "http_response_code"),
+              "status_group", e.status() / 100 + "xx", "http_status", String.valueOf(e.status())),
+          metricSuppliers.meters()).mark();
+      throw e;
+    } catch (IOException | RuntimeException e) {
+      throw e;
+    } catch (Exception e) {
+      throw new IOException(e);
     }
   }
 

diff --git a/dropwizard-metrics4/src/test/java/feign/metrics4/Metrics4CapabilityTest.java b/dropwizard-metrics4/src/test/java/feign/metrics4/Metrics4CapabilityTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2020 The Feign Authors
+ * Copyright 2012-2021 The Feign Authors
  *
  * 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
@@ -13,49 +13,75 @@
  */
 package feign.metrics4;
 
-import static org.hamcrest.Matchers.aMapWithSize;
-import static org.hamcrest.Matchers.containsString;
-import static org.junit.Assert.assertThat;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Map.Entry;
+import com.codahale.metrics.Metric;
 import com.codahale.metrics.MetricRegistry;
-import com.codahale.metrics.SharedMetricRegistries;
-import org.junit.Test;
-import feign.Feign;
-import feign.RequestLine;
-import feign.mock.HttpMethod;
-import feign.mock.MockClient;
-import feign.mock.MockTarget;
+import feign.Capability;
+import feign.Util;
+import feign.micrometer.AbstractMetricsTestBase;
 
-public class Metrics4CapabilityTest {
+public class Metrics4CapabilityTest
+    extends AbstractMetricsTestBase<MetricRegistry, String, Metric> {
 
-  public interface SimpleSource {
+  @Override
+  protected MetricRegistry createMetricsRegistry() {
+    return new MetricRegistry();
+  }
+
+  protected Capability createMetricCapability() {
+    return new Metrics4Capability(metricsRegistry);
+  }
+
+  @Override
+  protected Map<String, Metric> getFeignMetrics() {
+    return metricsRegistry.getMetrics();
+  }
 
-    @RequestLine("GET /get")
-    String get(String body);
+  @Override
+  protected boolean doesMetricIdIncludeClient(String metricId) {
+    return metricId.contains("feign.micrometer.AbstractMetricsTestBase$SimpleSource");
+  }
+
+  @Override
+  protected boolean doesMetricIncludeVerb(String metricId, String verb) {
+    return metricId.contains(verb);
+  }
 
+  @Override
+  protected boolean doesMetricIncludeHost(String metricId) {
+    // since metrics 4 don't have tags, we do not include hostname
+    return true;
   }
 
-  @Test
-  public void addMetricsCapability() {
-    final MetricRegistry registry = SharedMetricRegistries.getOrCreate("unit_test");
 
-    final SimpleSource source = Feign.builder()
-        .client(new MockClient()
-            .ok(HttpMethod.GET, "/get", "1234567890abcde"))
-        .addCapability(new Metrics4Capability(registry))
-        .target(new MockTarget<>(Metrics4CapabilityTest.SimpleSource.class));
+  @Override
+  protected Metric getMetric(String suffix, String... tags) {
+    Util.checkArgument(tags.length % 2 == 0, "tags must contain key-value pairs %s",
+        Arrays.toString(tags));
 
-    source.get("0x3456789");
 
-    assertThat(registry.getMetrics(), aMapWithSize(6));
+    return getFeignMetrics().entrySet()
+        .stream()
+        .filter(entry -> {
+          String name = entry.getKey();
+          if (!name.contains(suffix)) {
+            return false;
+          }
 
-    registry.getMetrics().keySet().forEach(metricName -> assertThat(
-        "Expect all metric names to include client name:" + metricName,
-        metricName,
-        containsString("feign.metrics4.Metrics4CapabilityTest$SimpleSource")));
-    registry.getMetrics().keySet().forEach(metricName -> assertThat(
-        "Expect all metric names to include method name:" + metricName,
-        metricName,
-        containsString("get")));
+          for (int i = 0; i < tags.length; i += 2) {
+            if (!name.contains(tags[i]) && !name.contains(tags[i] + 1)) {
+              return false;
+            }
+          }
+
+          return true;
+        })
+        .findAny()
+        .map(Entry::getValue)
+        .orElse(null);
   }
 
+
 }
diff --git a/dropwizard-metrics5/pom.xml b/dropwizard-metrics5/pom.xml
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
 
-    Copyright 2012-2020 The Feign Authors
+    Copyright 2012-2021 The Feign Authors
 
     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
@@ -50,5 +50,11 @@
       <version>2.0.0.0</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>${project.groupId}</groupId>
+      <artifactId>feign-micrometer</artifactId>
+      <type>test-jar</type>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 </project>
diff --git a/dropwizard-metrics5/src/main/java/feign/metrics5/MeteredClient.java b/dropwizard-metrics5/src/main/java/feign/metrics5/MeteredClient.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2020 The Feign Authors
+ * Copyright 2012-2021 The Feign Authors
  *
  * 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
@@ -45,7 +45,26 @@ public Response execute(Request request, Options options) throws IOException {
         metricRegistry.timer(
             metricName.metricName(template.methodMetadata(), template.feignTarget()),
             metricSuppliers.timers()).time()) {
-      return client.execute(request, options);
+      Response response = client.execute(request, options);
+      metricRegistry.counter(
+          metricName
+              .metricName(template.methodMetadata(), template.feignTarget(), "http_response_code")
+              .tagged("http_status", String.valueOf(response.status()))
+              .tagged("status_group", response.status() / 100 + "xx"))
+          .inc();
+      return response;
+    } catch (FeignException e) {
+      metricRegistry.counter(
+          metricName
+              .metricName(template.methodMetadata(), template.feignTarget(), "http_response_code")
+              .tagged("http_status", String.valueOf(e.status()))
+              .tagged("status_group", e.status() / 100 + "xx"))
+          .inc();
+      throw e;
+    } catch (IOException | RuntimeException e) {
+      throw e;
+    } catch (Exception e) {
+      throw new IOException(e);
     }
   }
 

diff --git a/dropwizard-metrics5/src/test/java/feign/metrics5/Metrics5CapabilityTest.java b/dropwizard-metrics5/src/test/java/feign/metrics5/Metrics5CapabilityTest.java
@@ -1,5 +1,5 @@
 /**
- * Copyright 2012-2020 The Feign Authors
+ * Copyright 2012-2021 The Feign Authors
  *
  * 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
@@ -13,54 +13,79 @@
  */
 package feign.metrics5;
 
-import static org.hamcrest.Matchers.aMapWithSize;
 import static org.hamcrest.Matchers.hasEntry;
-import static org.junit.Assert.assertThat;
-import org.junit.Test;
-import feign.Feign;
-import feign.RequestLine;
-import feign.mock.HttpMethod;
-import feign.mock.MockClient;
-import feign.mock.MockTarget;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Map.Entry;
+import feign.Capability;
+import feign.Util;
+import feign.micrometer.AbstractMetricsTestBase;
+import io.dropwizard.metrics5.Metric;
+import io.dropwizard.metrics5.MetricName;
 import io.dropwizard.metrics5.MetricRegistry;
-import io.dropwizard.metrics5.SharedMetricRegistries;
 
-public class Metrics5CapabilityTest {
+public class Metrics5CapabilityTest
+    extends AbstractMetricsTestBase<MetricRegistry, MetricName, Metric> {
 
-  public interface SimpleSource {
 
-    @RequestLine("GET /get")
-    String get(String body);
+  @Override
+  protected MetricRegistry createMetricsRegistry() {
+    return new MetricRegistry();
+  }
+
+  protected Capability createMetricCapability() {
+    return new Metrics5Capability(metricsRegistry);
+  }
+
+  @Override
+  protected Map<MetricName, Metric> getFeignMetrics() {
+    return metricsRegistry.getMetrics();
+  }
+
+  @Override
+  protected boolean doesMetricIdIncludeClient(MetricName metricId) {
+    return hasEntry("client", "feign.micrometer.AbstractMetricsTestBase$SimpleSource")
+        .matches(metricId.getTags());
+  }
+
+  @Override
+  protected boolean doesMetricIncludeVerb(MetricName metricId, String verb) {
+    return hasEntry("method", verb).matches(metricId.getTags());
+  }
 
+  @Override
+  protected boolean doesMetricIncludeHost(MetricName metricId) {
+    // hostname is null due to feign-mock shortfalls
+    return hasEntry("host", null).matches(metricId.getTags());
   }
 
-  @Test
-  public void addMetricsCapability() {
-    final MetricRegistry registry = SharedMetricRegistries.getOrCreate("unit_test");
+  @Override
+  protected Metric getMetric(String suffix, String... tags) {
+    Util.checkArgument(tags.length % 2 == 0, "tags must contain key-value pairs %s",
+        Arrays.toString(tags));
 
-    final SimpleSource source = Feign.builder()
-        .client(new MockClient()
-            .ok(HttpMethod.GET, "/get", "1234567890abcde"))
-        .addCapability(new Metrics5Capability(registry))
-        .target(new MockTarget<>(Metrics5CapabilityTest.SimpleSource.class));
 
-    source.get("0x3456789");
+    return getFeignMetrics().entrySet()
+        .stream()
+        .filter(entry -> {
+          MetricName name = entry.getKey();
+          if (!name.getKey().endsWith(suffix)) {
+            return false;
+          }
 
-    assertThat(registry.getMetrics(), aMapWithSize(6));
+          for (int i = 0; i < tags.length; i += 2) {
+            if (name.getTags().containsKey(tags[i])) {
+              if (!name.getTags().get(tags[i]).equals(tags[i + 1])) {
+                return false;
+              }
+            }
+          }
 
-    registry.getMetrics().keySet().forEach(metricName -> assertThat(
-        "Expect all metric names to include client name:" + metricName,
-        metricName.getTags(),
-        hasEntry("client", "feign.metrics5.Metrics5CapabilityTest$SimpleSource")));
-    registry.getMetrics().keySet().forEach(metricName -> assertThat(
-        "Expect all metric names to include method name:" + metricName,
-        metricName.getTags(),
-        hasEntry("method", "get")));
-    registry.getMetrics().keySet().forEach(metricName -> assertThat(
-        "Expect all metric names to include host name:" + metricName,
-        metricName.getTags(),
-        // hostname is null due to feign-mock shortfalls
-        hasEntry("host", null)));
+          return true;
+        })
+        .findAny()
+        .map(Entry::getValue)
+        .orElse(null);
   }
 
 }
diff --git a/micrometer/pom.xml b/micrometer/pom.xml
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
 
-    Copyright 2012-2020 The Feign Authors
+    Copyright 2012-2021 The Feign Authors
 
     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
@@ -51,4 +51,20 @@
       <scope>test</scope>
     </dependency>
   </dependencies>
+
+  <build>
+    <plugins>
+      <plugin>
+        <artifactId>maven-jar-plugin</artifactId>
+        <executions>
+          <execution>
+            <goals>
+              <goal>test-jar</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+    </plugins>
+  </build>
+
 </project>