Aggregate Micrometer "uri" label values under uri template (#1493)

* Use path expression rather than explicit value for Micrometer "uri" labels

* e.g. "/get/{id}" rather than "/get/123"
* this provides better aggregation, protecting the time series database

* Make assertions more self explanatory

* Test Micrometer Decoder uri label

* Move tests to AbstractMetricsTestBase, add uri label to dropwizard-metrics4&5

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>

dropwizard-metrics4/src/main/java/feign/metrics4/MeteredClient.java

@@ -43,23 +43,28 @@ public Response execute(Request request, Options options) throws IOException {
     final RequestTemplate template = request.requestTemplate();
     try (final Timer.Context classTimer =
         metricRegistry.timer(
-            metricName.metricName(template.methodMetadata(), template.feignTarget()),
+            MetricRegistry.name(
+                metricName.metricName(template.methodMetadata(), template.feignTarget()),
+                "uri", template.methodMetadata().template().path()),
             metricSuppliers.timers()).time()) {
       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())),
+              "status_group", response.status() / 100 + "xx",
+              "http_status", String.valueOf(response.status()),
+              "uri", template.methodMetadata().template().path()),
           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())),
+              "status_group", e.status() / 100 + "xx",
+              "http_status", String.valueOf(e.status()),
+              "uri", template.methodMetadata().template().path()),
           metricSuppliers.meters()).mark();
       throw e;
     } catch (IOException | RuntimeException e) {

dropwizard-metrics4/src/main/java/feign/metrics4/MeteredDecoder.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
@@ -55,7 +55,10 @@ public Object decode(Response response, Type type)
     final Object decoded;
     try (final Timer.Context classTimer =
         metricRegistry
-            .timer(metricName.metricName(template.methodMetadata(), template.feignTarget()),
+            .timer(
+                MetricRegistry.name(
+                    metricName.metricName(template.methodMetadata(), template.feignTarget()),
+                    "uri", template.methodMetadata().template().path()),
                 metricSuppliers.timers())
             .time()) {
       decoded = decoder.decode(response, type);

dropwizard-metrics4/src/test/java/feign/metrics4/Metrics4CapabilityTest.java

@@ -13,14 +13,14 @@
  */
 package feign.metrics4;
 
-import java.util.Arrays;
-import java.util.Map;
-import java.util.Map.Entry;
 import com.codahale.metrics.Metric;
 import com.codahale.metrics.MetricRegistry;
 import feign.Capability;
 import feign.Util;
 import feign.micrometer.AbstractMetricsTestBase;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Map.Entry;
 
 public class Metrics4CapabilityTest
     extends AbstractMetricsTestBase<MetricRegistry, String, Metric> {
@@ -84,5 +84,19 @@ protected Metric getMetric(String suffix, String... tags) {
         .orElse(null);
   }
 
+  @Override
+  protected boolean isClientMetric(String metricId) {
+    return metricId.startsWith("feign.Client");
+  }
+
+  @Override
+  protected boolean isDecoderMetric(String metricId) {
+    return metricId.startsWith("feign.codec.Decoder");
+  }
+
+  @Override
+  protected boolean doesMetricIncludeUri(String metricId, String uri) {
+    return metricId.contains(uri);
+  }
 
 }

dropwizard-metrics5/src/main/java/feign/metrics5/MeteredClient.java

@@ -43,22 +43,26 @@ public Response execute(Request request, Options options) throws IOException {
     final RequestTemplate template = request.requestTemplate();
     try (final Context classTimer =
         metricRegistry.timer(
-            metricName.metricName(template.methodMetadata(), template.feignTarget()),
+            metricName.metricName(template.methodMetadata(),
+                template.feignTarget())
+                .tagged("uri", template.methodMetadata().template().path()),
             metricSuppliers.timers()).time()) {
       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"))
+              .tagged("status_group", response.status() / 100 + "xx")
+              .tagged("uri", template.methodMetadata().template().path()))
           .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"))
+              .tagged("status_group", e.status() / 100 + "xx")
+              .tagged("uri", template.methodMetadata().template().path()))
           .inc();
       throw e;
     } catch (IOException | RuntimeException e) {

dropwizard-metrics5/src/main/java/feign/metrics5/MeteredDecoder.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
@@ -55,28 +55,32 @@ public Object decode(Response response, Type type)
     final Object decoded;
     try (final Context classTimer =
         metricRegistry
-            .timer(metricName.metricName(template.methodMetadata(), template.feignTarget()),
+            .timer(metricName.metricName(template.methodMetadata(), template.feignTarget())
+                .tagged("uri", template.methodMetadata().template().path()),
                 metricSuppliers.timers())
             .time()) {
       decoded = decoder.decode(response, type);
     } catch (IOException | RuntimeException e) {
       metricRegistry.meter(
           metricName.metricName(template.methodMetadata(), template.feignTarget(), "error_count")
-              .tagged("exception_name", e.getClass().getSimpleName()),
+              .tagged("exception_name", e.getClass().getSimpleName())
+              .tagged("uri", template.methodMetadata().template().path()),
           metricSuppliers.meters()).mark();
       throw e;
     } catch (Exception e) {
       metricRegistry.meter(
           metricName.metricName(template.methodMetadata(), template.feignTarget(), "error_count")
-              .tagged("exception_name", e.getClass().getSimpleName()),
+              .tagged("exception_name", e.getClass().getSimpleName())
+              .tagged("uri", template.methodMetadata().template().path()),
           metricSuppliers.meters()).mark();
       throw new IOException(e);
     }
 
     if (body != null) {
       metricRegistry.histogram(
           metricName.metricName(template.methodMetadata(), template.feignTarget(),
-              "response_size"),
+              "response_size")
+              .tagged("uri", template.methodMetadata().template().path()),
           metricSuppliers.histograms()).update(body.count());
     }
 

dropwizard-metrics5/src/test/java/feign/metrics5/Metrics5CapabilityTest.java

@@ -88,4 +88,19 @@ protected Metric getMetric(String suffix, String... tags) {
         .orElse(null);
   }
 
+  @Override
+  protected boolean isClientMetric(MetricName metricId) {
+    return metricId.getKey().startsWith("feign.Client");
+  }
+
+  @Override
+  protected boolean isDecoderMetric(MetricName metricId) {
+    return metricId.getKey().startsWith("feign.codec.Decoder");
+  }
+
+  @Override
+  protected boolean doesMetricIncludeUri(MetricName metricId, String uri) {
+    return uri.equals(metricId.getTags().get("uri"));
+  }
+
 }

micrometer/src/main/java/feign/micrometer/MeteredClient.java

@@ -83,9 +83,9 @@ protected void countResponseCode(Request request,
     final RequestTemplate template = request.requestTemplate();
     final Tags allTags = metricTagResolver
         .tag(template.methodMetadata(), template.feignTarget(), e,
-            new Tag[] {Tag.of("http_status", String.valueOf(responseStatus)),
-                Tag.of("status_group", responseStatus / 100 + "xx"),
-                Tag.of("uri", template.path())})
+            Tag.of("http_status", String.valueOf(responseStatus)),
+            Tag.of("status_group", responseStatus / 100 + "xx"),
+            Tag.of("uri", template.methodMetadata().template().path()))
         .and(extraTags);
     meterRegistry.counter(
         metricName.name("http_response_code"),
@@ -99,7 +99,8 @@ protected Timer createTimer(Request request,
                               Exception e) {
     final RequestTemplate template = request.requestTemplate();
     final Tags allTags = metricTagResolver
-        .tag(template.methodMetadata(), template.feignTarget(), e, Tag.of("uri", template.path()))
+        .tag(template.methodMetadata(), template.feignTarget(), e,
+            Tag.of("uri", template.methodMetadata().template().path()))
         .and(extraTags(request, response, options, e));
     return meterRegistry.timer(metricName.name(e), allTags);
   }

micrometer/src/main/java/feign/micrometer/MeteredDecoder.java

@@ -95,7 +95,7 @@ protected Counter createExceptionCounter(Response response, Type type, Exception
     final Tag[] extraTags = extraTags(response, type, e);
     final RequestTemplate template = response.request().requestTemplate();
     final Tags allTags = metricTagResolver.tag(template.methodMetadata(), template.feignTarget(),
-        Tag.of("uri", template.path()),
+        Tag.of("uri", template.methodMetadata().template().path()),
         Tag.of("exception_name", e.getClass().getSimpleName()))
         .and(extraTags);
     return meterRegistry.counter(metricName.name("error_count"), allTags);
@@ -110,6 +110,7 @@ protected DistributionSummary createSummary(Response response, Type type) {
   }
 
   protected Tag[] extraTags(Response response, Type type, Exception e) {
-    return EMPTY_TAGS_ARRAY;
+    RequestTemplate template = response.request().requestTemplate();
+    return new Tag[] {Tag.of("uri", template.methodMetadata().template().path())};
   }
 }

micrometer/src/test/java/feign/micrometer/AbstractMetricsTestBase.java

@@ -13,24 +13,18 @@
  */
 package feign.micrometer;
 
-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.junit.Before;
-import org.junit.Test;
-import feign.Capability;
-import feign.Feign;
-import feign.FeignException;
-import feign.RequestLine;
+import feign.*;
 import feign.mock.HttpMethod;
 import feign.mock.MockClient;
 import feign.mock.MockTarget;
+import org.junit.Before;
+import org.junit.Test;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
 
 public abstract class AbstractMetricsTestBase<MR, METRIC_ID, METRIC> {
 
@@ -149,5 +143,82 @@ public void shouldMetricCollectionWithCustomException() {
   }
 
 
+  @Test
+  public void clientMetricsHaveUriLabel() {
+    final SimpleSource source = Feign.builder()
+        .client(new MockClient()
+            .ok(HttpMethod.GET, "/get", "1234567890abcde"))
+        .addCapability(createMetricCapability())
+        .target(new MockTarget<>(SimpleSource.class));
+
+    source.get("0x3456789");
+
+    final Map<METRIC_ID, METRIC> clientMetrics = getFeignMetrics().entrySet().stream()
+        .filter(entry -> isClientMetric(entry.getKey()))
+        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+    clientMetrics.keySet().forEach(metricId -> assertThat(
+        "Expect all Client metric names to include uri:" + metricId,
+        doesMetricIncludeUri(metricId, "/get")));
+  }
+
+  public interface SourceWithPathExpressions {
+
+    @RequestLine("GET /get/{id}")
+    String get(@Param("id") String id, String body);
+
+  }
+
+  @Test
+  public void clientMetricsHaveUriLabelWithPathExpression() {
+    final SourceWithPathExpressions source = Feign.builder()
+        .client(new MockClient()
+            .ok(HttpMethod.GET, "/get/123", "1234567890abcde"))
+        .addCapability(createMetricCapability())
+        .target(new MockTarget<>(SourceWithPathExpressions.class));
+
+    source.get("123", "0x3456789");
+
+    final Map<METRIC_ID, METRIC> clientMetrics = getFeignMetrics().entrySet().stream()
+        .filter(entry -> isClientMetric(entry.getKey()))
+        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+    clientMetrics.keySet().forEach(metricId -> assertThat(
+        "Expect all Client metric names to include uri as aggregated path expression:" + metricId,
+        doesMetricIncludeUri(metricId, "/get/{id}")));
+  }
+
+  @Test
+  public void decoderExceptionCounterHasUriLabelWithPathExpression() {
+    final AtomicReference<FeignException.NotFound> notFound = new AtomicReference<>();
+
+    final SourceWithPathExpressions source = Feign.builder()
+        .client(new MockClient()
+            .ok(HttpMethod.GET, "/get/123", "1234567890abcde"))
+        .decoder((response, type) -> {
+          notFound.set(new FeignException.NotFound("test", response.request(), null, null));
+          throw notFound.get();
+        })
+        .addCapability(createMetricCapability())
+        .target(new MockTarget<>(MicrometerCapabilityTest.SourceWithPathExpressions.class));
+
+    FeignException.NotFound thrown =
+        assertThrows(FeignException.NotFound.class, () -> source.get("123", "0x3456789"));
+    assertSame(notFound.get(), thrown);
+
+    final Map<METRIC_ID, METRIC> decoderMetrics = getFeignMetrics().entrySet().stream()
+        .filter(entry -> isDecoderMetric(entry.getKey()))
+        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+
+    decoderMetrics.keySet().forEach(metricId -> assertThat(
+        "Expect all Decoder metric names to include uri as aggregated path expression:" + metricId,
+        doesMetricIncludeUri(metricId, "/get/{id}")));
+  }
+
+  protected abstract boolean isClientMetric(METRIC_ID metricId);
+
+  protected abstract boolean isDecoderMetric(METRIC_ID metricId);
+
+  protected abstract boolean doesMetricIncludeUri(METRIC_ID metricId, String uri);
 
 }

micrometer/src/test/java/feign/micrometer/MicrometerCapabilityTest.java

@@ -13,24 +13,25 @@
  */
 package feign.micrometer;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.function.Function;
-import java.util.stream.Collectors;
 import feign.Capability;
 import feign.Util;
 import io.micrometer.core.instrument.Meter;
 import io.micrometer.core.instrument.Meter.Id;
 import io.micrometer.core.instrument.MockClock;
 import io.micrometer.core.instrument.simple.SimpleConfig;
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 
 public class MicrometerCapabilityTest
     extends AbstractMetricsTestBase<SimpleMeterRegistry, Id, Meter> {
 
+
   @Override
   protected SimpleMeterRegistry createMetricsRegistry() {
     return new SimpleMeterRegistry(SimpleConfig.DEFAULT, new MockClock());
@@ -97,5 +98,19 @@ protected Meter getMetric(String suffix, String... tags) {
         .orElse(null);
   }
 
+  @Override
+  protected boolean isClientMetric(Id metricId) {
+    return metricId.getName().startsWith("feign.Client");
+  }
+
+  @Override
+  protected boolean isDecoderMetric(Id metricId) {
+    return metricId.getName().startsWith("feign.codec.Decoder");
+  }
+
+  @Override
+  protected boolean doesMetricIncludeUri(Id metricId, String uri) {
+    return uri.equals(metricId.getTag("uri"));
+  }
 
 }