Update HTTP metrics 'view' code to match the specification (#4556)

* Update HTTP metrics 'view' code to match the specification, including optional attribute rules.

* Update server metrics test for new logic.

* Fix client metrics test.

* Spotless fix.

* Updates from Java SiG discussion.

* Fixes from review.

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* Update spotless from code reivew merge.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

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

@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
+import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyClientDurationView;
 
 import com.google.auto.value.AutoValue;
 import io.opentelemetry.api.common.Attributes;
@@ -76,7 +76,7 @@ public final class HttpClientMetrics implements RequestListener {
     }
     duration.record(
         (endNanos - state.startTimeNanos()) / NANOS_PER_MS,
-        applyDurationView(state.startAttributes(), endAttributes));
+        applyClientDurationView(state.startAttributes(), endAttributes));
   }
 
   @AutoValue

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

@@ -6,7 +6,7 @@
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
 import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyActiveRequestsView;
-import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
+import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyServerDurationView;
 
 import com.google.auto.value.AutoValue;
 import io.opentelemetry.api.common.Attributes;
@@ -89,7 +89,7 @@ public final class HttpServerMetrics implements RequestListener {
     activeRequests.add(-1, applyActiveRequestsView(state.startAttributes()));
     duration.record(
         (endNanos - state.startTimeNanos()) / NANOS_PER_MS,
-        applyDurationView(state.startAttributes(), endAttributes));
+        applyServerDurationView(state.startAttributes(), endAttributes));
   }
 
   @AutoValue

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

@@ -18,25 +18,54 @@ import java.util.function.BiConsumer;
 @SuppressWarnings("rawtypes")
 final class TemporaryMetricsView {
 
-  private static final Set<AttributeKey> durationView = buildDurationView();
+  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> buildDurationView() {
+  private static Set<AttributeKey> buildDurationAlwaysInclude() {
     // the list of included metrics is from
     // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attributes
     Set<AttributeKey> view = new HashSet<>();
     view.add(SemanticAttributes.HTTP_METHOD);
-    view.add(SemanticAttributes.HTTP_HOST);
+    view.add(SemanticAttributes.HTTP_STATUS_CODE); // Optional
+    view.add(SemanticAttributes.HTTP_FLAVOR); // Optional
+    return view;
+  }
+
+  private static Set<AttributeKey> buildDurationClientView() {
+    // We pull identifying attributes according to:
+    // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives
+    Set<AttributeKey> view = new HashSet<>(durationAlwaysInclude);
+    view.add(SemanticAttributes.HTTP_URL);
+    return view;
+  }
+
+  private static Set<AttributeKey> buildDurationServerView() {
+    // 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_STATUS_CODE);
+    view.add(SemanticAttributes.HTTP_HOST);
-    view.add(SemanticAttributes.HTTP_FLAVOR);
+    view.add(SemanticAttributes.HTTP_ROUTE);
-    view.add(SemanticAttributes.NET_PEER_NAME);
+    return view;
-    view.add(SemanticAttributes.NET_PEER_PORT);
+  }
-    view.add(SemanticAttributes.NET_PEER_IP);
+
-    view.add(SemanticAttributes.HTTP_SERVER_NAME);
+  private static Set<AttributeKey> buildDurationServerFallbackView() {
-    view.add(SemanticAttributes.NET_HOST_NAME);
+    // We pull identifying attributes according to:
-    view.add(SemanticAttributes.NET_HOST_PORT);
+    // 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;
   }
 
@@ -52,10 +81,27 @@ final class TemporaryMetricsView {
     return view;
   }
 
-  static Attributes applyDurationView(Attributes startAttributes, Attributes endAttributes) {
+  static Attributes applyClientDurationView(Attributes startAttributes, Attributes endAttributes) {
     AttributesBuilder filtered = Attributes.builder();
-    applyView(filtered, startAttributes, durationView);
+    applyView(filtered, startAttributes, durationClientView);
-    applyView(filtered, endAttributes, durationView);
+    applyView(filtered, endAttributes, durationClientView);
+    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 (!containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) {
+      fullSet = durationServerFallbackView;
+    }
+    AttributesBuilder filtered = Attributes.builder();
+    applyView(filtered, startAttributes, fullSet);
+    applyView(filtered, endAttributes, fullSet);
     return filtered.build();
   }
 
@@ -72,10 +118,31 @@ final class TemporaryMetricsView {
         (BiConsumer<AttributeKey, Object>)
             (key, value) -> {
               if (view.contains(key)) {
+                // For now, we filter query parameters out of URLs in metrics.
+                if (SemanticAttributes.HTTP_URL.equals(key)
+                    || SemanticAttributes.HTTP_TARGET.equals(key)) {
+                  filtered.put(key, removeQueryParamFromUrlOrTarget(value.toString()));
+                } else {
                   filtered.put(key, value);
                 }
+              }
             });
   }
 
+  // Attempt to handle cleaning URLs like http://myServer;jsessionId=1 or targets like
+  // /my/path?queryParam=2
+  private static String removeQueryParamFromUrlOrTarget(String urlOrTarget) {
+    // Note: Maybe not the most robust, but purely to limit cardinality.
+    int idx = -1;
+    for (int i = 0; i < urlOrTarget.length(); ++i) {
+      char ch = urlOrTarget.charAt(i);
+      if (ch == '?' || ch == ';') {
+        idx = i;
+        break;
+      }
+    }
+    return idx == -1 ? urlOrTarget : urlOrTarget.substring(0, idx);
+  }
+
   private TemporaryMetricsView() {}
 }

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

@@ -32,7 +32,9 @@ class HttpClientMetricsTest {
     Attributes requestAttributes =
         Attributes.builder()
             .put("http.method", "GET")
+            .put("http.url", "https://localhost:1234/")
             .put("http.host", "host")
+            .put("http.target", "/")
             .put("http.scheme", "https")
             .put("net.host.name", "localhost")
             .put("net.host.port", 1234)
@@ -85,14 +87,10 @@ class HttpClientMetricsTest {
                                           .hasSum(150 /* millis */)
                                           .attributes()
                                           .containsOnly(
-                                              attributeEntry("http.host", "host"),
+                                              attributeEntry("http.url", "https://localhost:1234/"),
                                               attributeEntry("http.method", "GET"),
-                                              attributeEntry("http.scheme", "https"),
                                               attributeEntry("http.flavor", "2.0"),
-                                              attributeEntry("http.server_name", "server"),
+                                              attributeEntry("http.status_code", 200))));
-                                              attributeEntry("http.status_code", 200),
-                                              attributeEntry("net.host.name", "localhost"),
-                                              attributeEntry("net.host.port", 1234L))));
             });
 
     listener.end(context2, responseAttributes, nanos(300));

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

@@ -33,6 +33,7 @@ class HttpServerMetricsTest {
         Attributes.builder()
             .put("http.method", "GET")
             .put("http.host", "host")
+            .put("http.target", "/")
             .put("http.scheme", "https")
             .put("net.host.name", "localhost")
             .put("net.host.port", 1234)
@@ -120,14 +121,12 @@ class HttpServerMetricsTest {
                                           .hasSum(150 /* millis */)
                                           .attributes()
                                           .containsOnly(
-                                              attributeEntry("http.host", "host"),
-                                              attributeEntry("http.method", "GET"),
                                               attributeEntry("http.scheme", "https"),
-                                              attributeEntry("http.flavor", "2.0"),
+                                              attributeEntry("http.host", "host"),
-                                              attributeEntry("http.server_name", "server"),
+                                              attributeEntry("http.target", "/"),
+                                              attributeEntry("http.method", "GET"),
                                               attributeEntry("http.status_code", 200),
-                                              attributeEntry("net.host.name", "localhost"),
+                                              attributeEntry("http.flavor", "2.0"))));
-                                              attributeEntry("net.host.port", 1234L))));
             });
 
     listener.end(context2, responseAttributes, nanos(300));

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

@@ -6,7 +6,8 @@
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
 import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyActiveRequestsView;
-import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
+import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyClientDurationView;
+import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyServerDurationView;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;
 
 import io.opentelemetry.api.common.Attributes;
@@ -17,24 +18,127 @@ import org.junit.jupiter.api.Test;
 public class TemporaryMetricsViewTest {
 
   @Test
-  public void shouldApplyDurationView() {
+  public void shouldApplyClientDurationView_urlIdentity() {
     Attributes startAttributes =
         Attributes.builder()
+            .put(SemanticAttributes.HTTP_URL, "https://somehost/high/cardinality/12345")
+            .put(SemanticAttributes.HTTP_SCHEME, "https")
             .put(SemanticAttributes.HTTP_METHOD, "GET")
-            .put(SemanticAttributes.HTTP_URL, "http://somehost/high/cardinality/12345")
+            .put(SemanticAttributes.HTTP_HOST, "somehost")
-            .put(SemanticAttributes.NET_PEER_NAME, "somehost")
+            .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345")
             .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(applyDurationView(startAttributes, endAttributes))
+    OpenTelemetryAssertions.assertThat(applyClientDurationView(startAttributes, endAttributes))
         .containsOnly(
+            attributeEntry(
+                SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"),
+            attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
+            attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
+  }
+
+  @Test
+  public void shouldApplyClientDurationView_urlWithQuery() {
+    Attributes startAttributes =
+        Attributes.builder()
+            .put(
+                SemanticAttributes.HTTP_URL,
+                "https://somehost/high/cardinality/12345?jsessionId=121454")
+            .put(SemanticAttributes.HTTP_METHOD, "GET")
+            .put(SemanticAttributes.HTTP_SCHEME, "https")
+            .put(SemanticAttributes.HTTP_HOST, "somehost")
+            .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345?jsessionId=121454")
+            .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(applyClientDurationView(startAttributes, endAttributes))
+        .containsOnly(
+            attributeEntry(
+                SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"),
+            attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
+            attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
+  }
+
+  @Test
+  public void shouldApplyServerDurationView_withRoute() {
+    Attributes startAttributes =
+        Attributes.builder()
+            .put(SemanticAttributes.HTTP_METHOD, "GET")
+            .put(
+                SemanticAttributes.HTTP_URL,
+                "https://somehost/high/cardinality/12345?jsessionId=121454")
+            .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=121454")
+            .put(SemanticAttributes.HTTP_ROUTE, "/somehost/high/{name}/{id}")
+            .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_ROUTE.getKey(), "/somehost/high/{name}/{id}"),
+            attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
+            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_TARGET.getKey(), "/somehost/high/cardinality/12345"),
             attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
-            attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost2"),
             attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
   }