Use http.route in HttpServerMetrics (#5266)

* Use http.route in HttpServerMetrics

* remove http.target fallback

Author: Mateusz Rzeszutek

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java

@@ -146,7 +146,6 @@ private boolean isBetterRoute(String name) {
     return name.length() > routeLength;
   }
 
-  // TODO: use that in HttpServerMetrics
   /**
    * Returns the {@code http.route} attribute value that's stored in the passed {@code context}, or
    * null if it was not set before.

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java

@@ -18,7 +18,9 @@
 import io.opentelemetry.instrumentation.api.annotations.UnstableApi;
 import io.opentelemetry.instrumentation.api.instrumenter.RequestListener;
 import io.opentelemetry.instrumentation.api.instrumenter.RequestMetrics;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -49,8 +51,14 @@ public static RequestMetrics get() {
 
   private final LongUpDownCounter activeRequests;
   private final DoubleHistogram duration;
+  private final Function<Context, String> httpRouteHolderGetter;
 
   private HttpServerMetrics(Meter meter) {
+    this(meter, HttpRouteHolder::getRoute);
+  }
+
+  // visible for tests
+  HttpServerMetrics(Meter meter, Function<Context, String> httpRouteHolderGetter) {
     activeRequests =
         meter
             .upDownCounterBuilder("http.server.active_requests")
@@ -64,6 +72,8 @@ private HttpServerMetrics(Meter meter) {
             .setUnit("ms")
             .setDescription("The duration of the inbound HTTP request")
             .build();
+
+    this.httpRouteHolderGetter = httpRouteHolderGetter;
   }
 
   @Override
@@ -86,10 +96,19 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
     activeRequests.add(-1, applyActiveRequestsView(state.startAttributes()));
     duration.record(
         (endNanos - state.startTimeNanos()) / NANOS_PER_MS,
-        applyServerDurationView(state.startAttributes(), endAttributes),
+        applyServerDurationView(state.startAttributes(), addHttpRoute(context, endAttributes)),
         context);
   }
 
+  // TODO: the http.route should be extracted by the AttributesExtractor, HttpServerMetrics should
+  // not access the context to get it
+  private Attributes addHttpRoute(Context context, Attributes endAttributes) {
+    String route = httpRouteHolderGetter.apply(context);
+    return route == null
+        ? endAttributes
+        : endAttributes.toBuilder().put(SemanticAttributes.HTTP_ROUTE, route).build();
+  }
+
   @AutoValue
   abstract static class State {
 

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java

@@ -8,7 +8,6 @@
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
-import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashSet;
 import java.util.Set;
@@ -19,19 +18,9 @@
 @SuppressWarnings("rawtypes")
 final class TemporaryMetricsView {
 
-  // TODO (trask) remove this once http.route is captured consistently
-  //
-  // this is not enabled by default because it falls back to http.target (which can be high
-  // cardinality) when http.route is not available
-  private static final boolean USE_HTTP_TARGET_FALLBACK =
-      Config.get()
-          .getBoolean("otel.instrumentation.metrics.experimental.use-http-target-fallback", false);
-
   private static final Set<AttributeKey> durationAlwaysInclude = buildDurationAlwaysInclude();
   private static final Set<AttributeKey> durationClientView = buildDurationClientView();
   private static final Set<AttributeKey> durationServerView = buildDurationServerView();
-  private static final Set<AttributeKey> durationServerFallbackView =
-      buildDurationServerFallbackView();
   private static final Set<AttributeKey> activeRequestsView = buildActiveRequestsView();
 
   private static Set<AttributeKey> buildDurationAlwaysInclude() {
@@ -65,19 +54,6 @@ private static Set<AttributeKey> buildDurationServerView() {
     return view;
   }
 
-  private static Set<AttributeKey> buildDurationServerFallbackView() {
-    // We pull identifying attributes according to:
-    // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives
-    // With the following caveat:
-    // - we always rely on http.route + http.host in this repository.
-    // - we prefer http.route (which is scrubbed) over http.target (which is not scrubbed).
-    Set<AttributeKey> view = new HashSet<>(durationAlwaysInclude);
-    view.add(SemanticAttributes.HTTP_SCHEME);
-    view.add(SemanticAttributes.HTTP_HOST);
-    view.add(SemanticAttributes.HTTP_TARGET);
-    return view;
-  }
-
   private static Set<AttributeKey> buildActiveRequestsView() {
     // the list of included metrics is from
     // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attributes
@@ -97,21 +73,10 @@ static Attributes applyClientDurationView(Attributes startAttributes, Attributes
     return filtered.build();
   }
 
-  private static <T> boolean containsAttribute(
-      AttributeKey<T> key, Attributes startAttributes, Attributes endAttributes) {
-    return startAttributes.get(key) != null || endAttributes.get(key) != null;
-  }
-
   static Attributes applyServerDurationView(Attributes startAttributes, Attributes endAttributes) {
-    Set<AttributeKey> fullSet = durationServerView;
-    // Use http.target when http.route is not available.
-    if (USE_HTTP_TARGET_FALLBACK
-        && !containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) {
-      fullSet = durationServerFallbackView;
-    }
     AttributesBuilder filtered = Attributes.builder();
-    applyView(filtered, startAttributes, fullSet);
-    applyView(filtered, endAttributes, fullSet);
+    applyView(filtered, startAttributes, durationServerView);
+    applyView(filtered, endAttributes, durationServerView);
     return filtered.build();
   }
 

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java

@@ -156,6 +156,49 @@ void collectsMetrics() {
                     .satisfiesExactly(point -> assertThat(point).hasSum(300 /* millis */)));
   }
 
+  @Test
+  void collectsHttpRouteFromContext() {
+    // given
+    InMemoryMetricReader metricReader = InMemoryMetricReader.create();
+    SdkMeterProvider meterProvider =
+        SdkMeterProvider.builder()
+            .registerMetricReader(metricReader)
+            .setMinimumCollectionInterval(Duration.ZERO)
+            .build();
+
+    RequestListener listener = new HttpServerMetrics(meterProvider.get("test"), c -> "/test/{id}");
+
+    Attributes requestAttributes =
+        Attributes.builder().put("http.host", "host").put("http.scheme", "https").build();
+
+    Attributes responseAttributes = Attributes.empty();
+
+    Context parentContext = Context.root();
+
+    // when
+    Context context = listener.start(parentContext, requestAttributes, nanos(100));
+    listener.end(context, responseAttributes, nanos(200));
+
+    // then
+    assertThat(metricReader.collectAllMetrics())
+        .anySatisfy(
+            metric ->
+                assertThat(metric)
+                    .hasName("http.server.duration")
+                    .hasUnit("ms")
+                    .hasDoubleHistogram()
+                    .points()
+                    .satisfiesExactly(
+                        point ->
+                            assertThat(point)
+                                .hasSum(100 /* millis */)
+                                .attributes()
+                                .containsOnly(
+                                    attributeEntry("http.scheme", "https"),
+                                    attributeEntry("http.host", "host"),
+                                    attributeEntry("http.route", "/test/{id}"))));
+  }
+
   private static long nanos(int millis) {
     return TimeUnit.MILLISECONDS.toNanos(millis);
   }

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java

@@ -109,37 +109,6 @@ public void shouldApplyServerDurationView_withRoute() {
             attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
   }
 
-  @Test
-  public void shouldApplyServerDurationView_withTarget() {
-    Attributes startAttributes =
-        Attributes.builder()
-            .put(SemanticAttributes.HTTP_METHOD, "GET")
-            .put(SemanticAttributes.HTTP_URL, "https://somehost/high/cardinality/12345")
-            .put(SemanticAttributes.HTTP_SCHEME, "https")
-            .put(SemanticAttributes.HTTP_HOST, "somehost")
-            .put(SemanticAttributes.HTTP_SERVER_NAME, "somehost")
-            .put(
-                SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345;jsessionId=12145")
-            .put(SemanticAttributes.NET_HOST_NAME, "somehost")
-            .put(SemanticAttributes.NET_HOST_PORT, 443)
-            .build();
-
-    Attributes endAttributes =
-        Attributes.builder()
-            .put(SemanticAttributes.HTTP_STATUS_CODE, 500)
-            .put(SemanticAttributes.NET_PEER_NAME, "somehost2")
-            .put(SemanticAttributes.NET_PEER_IP, "127.0.0.1")
-            .put(SemanticAttributes.NET_PEER_PORT, 443)
-            .build();
-
-    OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes))
-        .containsOnly(
-            attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"),
-            attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"),
-            attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
-            attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
-  }
-
   @Test
   public void shouldApplyActiveRequestsView() {
     Attributes attributes =