Make HttpClientMetrics report low cardinality metrics (#5319)

open-telemetry · Feb 8, 2022 · ea31ca8 · ea31ca8
1 parent edfd842
commit ea31ca8
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 59 deletions.
diff --git a/...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java b/...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java
@@ -36,8 +36,10 @@ private static Set<AttributeKey> buildDurationAlwaysInclude() {
   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
+    // We only pull net.peer.name and net.peer.port because http.url has too high cardinality
     Set<AttributeKey> view = new HashSet<>(durationAlwaysInclude);
-    view.add(SemanticAttributes.HTTP_URL);
+    view.add(SemanticAttributes.NET_PEER_NAME);
+    view.add(SemanticAttributes.NET_PEER_PORT);
     return view;
   }
 
@@ -93,31 +95,10 @@ private static void applyView(
         (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);
-                }
+                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() {}
 }
diff --git a/...st/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java b/...st/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java
@@ -41,8 +41,9 @@ void collectsMetrics() {
             .put("http.host", "host")
             .put("http.target", "/")
             .put("http.scheme", "https")
-            .put("net.host.name", "localhost")
-            .put("net.host.port", 1234)
+            .put("net.peer.name", "localhost")
+            .put("net.peer.ip", "0.0.0.0")
+            .put("net.peer.port", 1234)
             .build();
 
     Attributes responseAttributes =
@@ -87,7 +88,8 @@ void collectsMetrics() {
                               .hasSum(150 /* millis */)
                               .attributes()
                               .containsOnly(
-                                  attributeEntry("http.url", "https://localhost:1234/"),
+                                  attributeEntry("net.peer.name", "localhost"),
+                                  attributeEntry("net.peer.port", 1234),
                                   attributeEntry("http.method", "GET"),
                                   attributeEntry("http.flavor", "2.0"),
                                   attributeEntry("http.status_code", 200));

diff --git a/...java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java b/...java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java
@@ -15,37 +15,10 @@
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import org.junit.jupiter.api.Test;
 
-public class TemporaryMetricsViewTest {
+class TemporaryMetricsViewTest {
 
   @Test
-  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_HOST, "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(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() {
+  void shouldApplyClientDurationView() {
     Attributes startAttributes =
         Attributes.builder()
             .put(
@@ -67,14 +40,14 @@ public void shouldApplyClientDurationView_urlWithQuery() {
 
     OpenTelemetryAssertions.assertThat(applyClientDurationView(startAttributes, endAttributes))
         .containsOnly(
-            attributeEntry(
-                SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"),
+            attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost2"),
+            attributeEntry(SemanticAttributes.NET_PEER_PORT.getKey(), 443),
             attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
             attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
   }
 
   @Test
-  public void shouldApplyServerDurationView_withRoute() {
+  void shouldApplyServerDurationView() {
     Attributes startAttributes =
         Attributes.builder()
             .put(SemanticAttributes.HTTP_METHOD, "GET")
@@ -110,7 +83,7 @@ public void shouldApplyServerDurationView_withRoute() {
   }
 
   @Test
-  public void shouldApplyActiveRequestsView() {
+  void shouldApplyActiveRequestsView() {
     Attributes attributes =
         Attributes.builder()
             .put(SemanticAttributes.HTTP_METHOD, "GET")