From d3f8ab6c8232881018a38dba7aa075cc8ec1fb65 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Sat, 6 Nov 2021 12:09:01 -0400 Subject: [PATCH] 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 * Update spotless from code reivew merge. Co-authored-by: Trask Stalnaker --- .../instrumenter/http/HttpClientMetrics.java | 4 +- .../instrumenter/http/HttpServerMetrics.java | 4 +- .../http/TemporaryMetricsView.java | 99 ++++++++++++--- .../http/HttpClientMetricsTest.java | 10 +- .../http/HttpServerMetricsTest.java | 11 +- .../http/TemporaryMetricsViewTest.java | 116 +++++++++++++++++- 6 files changed, 206 insertions(+), 38 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetrics.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetrics.java index 9454ab3549..0f32fc1519 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetrics.java +++ b/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 diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java index 78518b6121..426728a095 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java +++ b/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 diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java index 4ceea27bc3..7bf7a875ce 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java +++ b/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 durationView = buildDurationView(); - + private static final Set durationAlwaysInclude = buildDurationAlwaysInclude(); + private static final Set durationClientView = buildDurationClientView(); + private static final Set durationServerView = buildDurationServerView(); + private static final Set durationServerFallbackView = + buildDurationServerFallbackView(); private static final Set activeRequestsView = buildActiveRequestsView(); - private static Set buildDurationView() { + private static Set 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 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 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 view = new HashSet<>(durationAlwaysInclude); + view.add(SemanticAttributes.HTTP_URL); + return view; + } + + private static Set 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 view = new HashSet<>(durationAlwaysInclude); view.add(SemanticAttributes.HTTP_SCHEME); - view.add(SemanticAttributes.HTTP_STATUS_CODE); - view.add(SemanticAttributes.HTTP_FLAVOR); - view.add(SemanticAttributes.NET_PEER_NAME); - view.add(SemanticAttributes.NET_PEER_PORT); - view.add(SemanticAttributes.NET_PEER_IP); - view.add(SemanticAttributes.HTTP_SERVER_NAME); - view.add(SemanticAttributes.NET_HOST_NAME); - view.add(SemanticAttributes.NET_HOST_PORT); + view.add(SemanticAttributes.HTTP_HOST); + view.add(SemanticAttributes.HTTP_ROUTE); + return view; + } + + private static Set 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 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, endAttributes, durationView); + applyView(filtered, startAttributes, durationClientView); + applyView(filtered, endAttributes, durationClientView); + return filtered.build(); + } + + private static boolean containsAttribute( + AttributeKey key, Attributes startAttributes, Attributes endAttributes) { + return startAttributes.get(key) != null || endAttributes.get(key) != null; + } + + static Attributes applyServerDurationView(Attributes startAttributes, Attributes endAttributes) { + Set 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) (key, value) -> { if (view.contains(key)) { - filtered.put(key, value); + // 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() {} } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java index ed932248c5..11d616c47a 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java +++ b/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("net.host.name", "localhost"), - attributeEntry("net.host.port", 1234L)))); + attributeEntry("http.status_code", 200)))); }); listener.end(context2, responseAttributes, nanos(300)); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java index 82aa3d9b9c..c189c6ba23 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java +++ b/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.server_name", "server"), + attributeEntry("http.host", "host"), + attributeEntry("http.target", "/"), + attributeEntry("http.method", "GET"), attributeEntry("http.status_code", 200), - attributeEntry("net.host.name", "localhost"), - attributeEntry("net.host.port", 1234L)))); + attributeEntry("http.flavor", "2.0")))); }); listener.end(context2, responseAttributes, nanos(300)); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java index 51e5367bdc..fac42cb319 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java +++ b/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.NET_PEER_NAME, "somehost") + .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(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)); }