From 5a410f32cd3efe8ce5d9695da8cdb94254147eef Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Mon, 1 Nov 2021 08:55:28 -0400 Subject: [PATCH 1/8] Update HTTP metrics 'view' code to match the specification, including optional attribute rules. --- .../instrumenter/http/HttpClientMetrics.java | 4 +- .../instrumenter/http/HttpServerMetrics.java | 4 +- .../http/TemporaryMetricsView.java | 81 +++++-- .../http/TemporaryMetricsViewTest.java | 211 +++++++++++++++++- 4 files changed, 276 insertions(+), 24 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 9454ab3549f0..0f32fc1519d1 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 void end(Context context, Attributes endAttributes, long endNanos) { } 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 78518b61214e..426728a0957b 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 void end(Context context, Attributes endAttributes, long endNanos) { 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 4ceea27bc33b..84593eddc10a 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,18 @@ @SuppressWarnings("rawtypes") final class TemporaryMetricsView { - private static final Set durationView = buildDurationView(); + private static final Set durationAlwaysInclude = buildDurationAlwaysInclude(); 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_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_STATUS_CODE); // Optional + view.add(SemanticAttributes.HTTP_FLAVOR); // Optional return view; } @@ -52,10 +45,68 @@ private static Set buildActiveRequestsView() { return view; } - static Attributes applyDurationView(Attributes startAttributes, Attributes endAttributes) { + static Attributes applyClientDurationView(Attributes startAttributes, Attributes endAttributes) { + Set fullSet = new HashSet<>(durationAlwaysInclude); + // We pull identifying attributes according to: + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives + if (containsAttribute(SemanticAttributes.HTTP_URL, startAttributes, endAttributes)) { + // Use http.url alone + fullSet.add(SemanticAttributes.HTTP_URL); + } else if (containsAttribute(SemanticAttributes.HTTP_HOST, startAttributes, endAttributes)) { + // Use http.scheme, http.host and http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.HTTP_HOST); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } else if (containsAttribute(SemanticAttributes.NET_PEER_NAME, startAttributes, endAttributes)) { + // Use http.scheme, net.peer.name, net.peer.port and http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.NET_PEER_NAME); + fullSet.add(SemanticAttributes.NET_PEER_PORT); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } else { + // Use http.scheme, net.peer.ip, net.peer.port and http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.NET_PEER_IP); + fullSet.add(SemanticAttributes.NET_PEER_PORT); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } + AttributesBuilder filtered = Attributes.builder(); + applyView(filtered, startAttributes, fullSet); + applyView(filtered, endAttributes, fullSet); + 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 = new HashSet<>(durationAlwaysInclude); + // We pull identifying attributes according to: + // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives + if (containsAttribute(SemanticAttributes.HTTP_HOST, startAttributes, endAttributes)) { + // Use http.scheme, http.host and http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.HTTP_HOST); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } else if (containsAttribute(SemanticAttributes.HTTP_SERVER_NAME, startAttributes, endAttributes)) { + // Use http.scheme, http.server_name, net.host.port, http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.HTTP_SERVER_NAME); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } else if (containsAttribute(SemanticAttributes.NET_HOST_NAME, startAttributes, endAttributes)) { + // Use http.scheme, net.host.name, net.host.port, http.target + fullSet.add(SemanticAttributes.HTTP_SCHEME); + fullSet.add(SemanticAttributes.NET_HOST_NAME); + fullSet.add(SemanticAttributes.NET_HOST_PORT); + fullSet.add(SemanticAttributes.HTTP_TARGET); + } else { + // Use http.url + fullSet.add(SemanticAttributes.HTTP_URL); + } AttributesBuilder filtered = Attributes.builder(); - applyView(filtered, startAttributes, durationView); - applyView(filtered, endAttributes, durationView); + applyView(filtered, startAttributes, fullSet); + applyView(filtered, endAttributes, fullSet); return filtered.build(); } 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 51e5367bdcda..67d853166ef4 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,224 @@ 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.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_httpHostIdentity() { + Attributes startAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_METHOD, "GET") + .put(SemanticAttributes.HTTP_SCHEME, "https") + .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_SCHEME.getKey(), "https"), + attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"), + attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), + attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), + attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); + } + + @Test + public void shouldApplyClientDurationView_netPeerNameIdentity() { + Attributes startAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_METHOD, "GET") + .put(SemanticAttributes.HTTP_SCHEME, "https") + .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345") + .build(); + + Attributes endAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_STATUS_CODE, 500) .put(SemanticAttributes.NET_PEER_NAME, "somehost") + .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_SCHEME.getKey(), "https"), + attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost"), + attributeEntry(SemanticAttributes.NET_PEER_PORT.getKey(), 443), + attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), + attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), + attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); + } + + @Test + public void shouldApplyClientDurationView_netPeerIpIdentity() { + Attributes startAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_METHOD, "GET") + .put(SemanticAttributes.HTTP_SCHEME, "https") + .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345") + .build(); + + Attributes endAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_STATUS_CODE, 500) + .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_SCHEME.getKey(), "https"), + attributeEntry(SemanticAttributes.NET_PEER_IP.getKey(), "127.0.0.1"), + attributeEntry(SemanticAttributes.NET_PEER_PORT.getKey(), 443), + attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), + attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), + attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); + } + + + @Test + public void shouldApplyServerDurationView_hostIdentity() { + 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") + .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.HTTP_STATUS_CODE.getKey(), 500)); + } + + @Test + public void shouldApplyServerDurationView_serverNameIdentity() { + 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_SERVER_NAME, "somehost") + .put(SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345") + .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_SERVER_NAME.getKey(), "somehost"), + attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), + attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), + attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); + } + + @Test + public void shouldApplyServerDurationView_netHostIdentity() { + 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_TARGET, "/somehost/high/cardinality/12345") + .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_IP, "127.0.0.1") + .put(SemanticAttributes.NET_PEER_PORT, 14204) + .build(); + + OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes)) + .containsOnly( + attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), + attributeEntry(SemanticAttributes.NET_HOST_NAME.getKey(), "somehost"), + attributeEntry(SemanticAttributes.NET_HOST_PORT.getKey(), 443), + attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), + attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), + attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); + } + + @Test + public void shouldApplyServerDurationView_urlIdentity() { + 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_TARGET, "/somehost/high/cardinality/12345") + .put(SemanticAttributes.NET_HOST_PORT, 443) + .build(); + + Attributes endAttributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_STATUS_CODE, 500) + .put(SemanticAttributes.NET_PEER_IP, "127.0.0.1") + .put(SemanticAttributes.NET_PEER_PORT, 443) .build(); - OpenTelemetryAssertions.assertThat(applyDurationView(startAttributes, endAttributes)) + OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes)) .containsOnly( + attributeEntry(SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost2"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } From 1ba87444dd02ad3096ac138f77eef07cef8b39ed Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Mon, 1 Nov 2021 09:10:23 -0400 Subject: [PATCH 2/8] Update server metrics test for new logic. --- .../api/instrumenter/http/HttpServerMetricsTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) 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 82aa3d9b9cd7..c189c6ba23c2 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 @@ void collectsMetrics() { 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 @@ void collectsMetrics() { .hasSum(150 /* millis */) .attributes() .containsOnly( + attributeEntry("http.scheme", "https"), attributeEntry("http.host", "host"), + attributeEntry("http.target", "/"), 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.flavor", "2.0")))); }); listener.end(context2, responseAttributes, nanos(300)); From a8756a6d666f6485dddceaeee3cd359bd856211a Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Mon, 1 Nov 2021 09:12:49 -0400 Subject: [PATCH 3/8] Fix client metrics test. --- .../api/instrumenter/http/HttpClientMetricsTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) 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 ed932248c5a4..d8821a0485f8 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 @@ -33,6 +33,7 @@ void collectsMetrics() { 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) @@ -85,14 +86,12 @@ void collectsMetrics() { .hasSum(150 /* millis */) .attributes() .containsOnly( + attributeEntry("http.scheme", "https"), attributeEntry("http.host", "host"), + attributeEntry("http.target", "/"), 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)); From a09108e493b3fac35c21b91ba89b1af15a2aea60 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Mon, 1 Nov 2021 10:47:15 -0400 Subject: [PATCH 4/8] Spotless fix. --- .../instrumenter/http/TemporaryMetricsView.java | 13 ++++++++----- .../http/TemporaryMetricsViewTest.java | 16 ++++++++++------ 2 files changed, 18 insertions(+), 11 deletions(-) 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 84593eddc10a..885d897f79f5 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 @@ -22,7 +22,6 @@ final class TemporaryMetricsView { private static final Set activeRequestsView = buildActiveRequestsView(); - 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 @@ -57,7 +56,8 @@ static Attributes applyClientDurationView(Attributes startAttributes, Attributes fullSet.add(SemanticAttributes.HTTP_SCHEME); fullSet.add(SemanticAttributes.HTTP_HOST); fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute(SemanticAttributes.NET_PEER_NAME, startAttributes, endAttributes)) { + } else if (containsAttribute( + SemanticAttributes.NET_PEER_NAME, startAttributes, endAttributes)) { // Use http.scheme, net.peer.name, net.peer.port and http.target fullSet.add(SemanticAttributes.HTTP_SCHEME); fullSet.add(SemanticAttributes.NET_PEER_NAME); @@ -76,7 +76,8 @@ static Attributes applyClientDurationView(Attributes startAttributes, Attributes return filtered.build(); } - private static boolean containsAttribute(AttributeKey key, Attributes startAttributes, Attributes endAttributes) { + private static boolean containsAttribute( + AttributeKey key, Attributes startAttributes, Attributes endAttributes) { return startAttributes.get(key) != null || endAttributes.get(key) != null; } @@ -89,12 +90,14 @@ static Attributes applyServerDurationView(Attributes startAttributes, Attributes fullSet.add(SemanticAttributes.HTTP_SCHEME); fullSet.add(SemanticAttributes.HTTP_HOST); fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute(SemanticAttributes.HTTP_SERVER_NAME, startAttributes, endAttributes)) { + } else if (containsAttribute( + SemanticAttributes.HTTP_SERVER_NAME, startAttributes, endAttributes)) { // Use http.scheme, http.server_name, net.host.port, http.target fullSet.add(SemanticAttributes.HTTP_SCHEME); fullSet.add(SemanticAttributes.HTTP_SERVER_NAME); fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute(SemanticAttributes.NET_HOST_NAME, startAttributes, endAttributes)) { + } else if (containsAttribute( + SemanticAttributes.NET_HOST_NAME, startAttributes, endAttributes)) { // Use http.scheme, net.host.name, net.host.port, http.target fullSet.add(SemanticAttributes.HTTP_SCHEME); fullSet.add(SemanticAttributes.NET_HOST_NAME); 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 67d853166ef4..197ff9a884e8 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 @@ -38,7 +38,8 @@ public void shouldApplyClientDurationView_urlIdentity() { OpenTelemetryAssertions.assertThat(applyClientDurationView(startAttributes, endAttributes)) .containsOnly( - attributeEntry(SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"), + attributeEntry( + SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } @@ -123,7 +124,6 @@ public void shouldApplyClientDurationView_netPeerIpIdentity() { attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } - @Test public void shouldApplyServerDurationView_hostIdentity() { Attributes startAttributes = @@ -150,7 +150,8 @@ public void shouldApplyServerDurationView_hostIdentity() { .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_TARGET.getKey(), "/somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } @@ -180,7 +181,8 @@ public void shouldApplyServerDurationView_serverNameIdentity() { .containsOnly( attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), attributeEntry(SemanticAttributes.HTTP_SERVER_NAME.getKey(), "somehost"), - attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), + attributeEntry( + SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } @@ -209,7 +211,8 @@ public void shouldApplyServerDurationView_netHostIdentity() { attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), attributeEntry(SemanticAttributes.NET_HOST_NAME.getKey(), "somehost"), attributeEntry(SemanticAttributes.NET_HOST_PORT.getKey(), 443), - attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), + attributeEntry( + SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } @@ -234,7 +237,8 @@ public void shouldApplyServerDurationView_urlIdentity() { OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes)) .containsOnly( - attributeEntry(SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"), + attributeEntry( + SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } From 0310004e5506a1fd1c90a06d07a8ac3863369471 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Thu, 4 Nov 2021 13:50:55 -0400 Subject: [PATCH 5/8] Updates from Java SiG discussion. --- .../http/TemporaryMetricsView.java | 95 ++++++------- .../http/HttpClientMetricsTest.java | 5 +- .../http/TemporaryMetricsViewTest.java | 130 ++---------------- 3 files changed, 57 insertions(+), 173 deletions(-) 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 885d897f79f5..12f53f54b6f7 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 @@ -19,7 +19,8 @@ final class TemporaryMetricsView { private static final Set durationAlwaysInclude = buildDurationAlwaysInclude(); - + private static final Set durationClientView = buildDurationClientView(); + private static final Set durationServerView = buildDurationServerView(); private static final Set activeRequestsView = buildActiveRequestsView(); private static Set buildDurationAlwaysInclude() { @@ -32,6 +33,27 @@ private static Set buildDurationAlwaysInclude() { 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_HOST); + view.add(SemanticAttributes.HTTP_ROUTE); + return view; + } + private static Set 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 @@ -45,34 +67,10 @@ private static Set buildActiveRequestsView() { } static Attributes applyClientDurationView(Attributes startAttributes, Attributes endAttributes) { - Set fullSet = new HashSet<>(durationAlwaysInclude); - // We pull identifying attributes according to: - // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives - if (containsAttribute(SemanticAttributes.HTTP_URL, startAttributes, endAttributes)) { - // Use http.url alone - fullSet.add(SemanticAttributes.HTTP_URL); - } else if (containsAttribute(SemanticAttributes.HTTP_HOST, startAttributes, endAttributes)) { - // Use http.scheme, http.host and http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.HTTP_HOST); - fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute( - SemanticAttributes.NET_PEER_NAME, startAttributes, endAttributes)) { - // Use http.scheme, net.peer.name, net.peer.port and http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.NET_PEER_NAME); - fullSet.add(SemanticAttributes.NET_PEER_PORT); - fullSet.add(SemanticAttributes.HTTP_TARGET); - } else { - // Use http.scheme, net.peer.ip, net.peer.port and http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.NET_PEER_IP); - fullSet.add(SemanticAttributes.NET_PEER_PORT); - fullSet.add(SemanticAttributes.HTTP_TARGET); - } AttributesBuilder filtered = Attributes.builder(); - applyView(filtered, startAttributes, fullSet); - applyView(filtered, endAttributes, fullSet); + // TODO - Remove query parameters from URL. + applyView(filtered, startAttributes, durationClientView); + applyView(filtered, endAttributes, durationClientView); return filtered.build(); } @@ -82,30 +80,11 @@ private static boolean containsAttribute( } static Attributes applyServerDurationView(Attributes startAttributes, Attributes endAttributes) { - Set fullSet = new HashSet<>(durationAlwaysInclude); - // We pull identifying attributes according to: - // https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives - if (containsAttribute(SemanticAttributes.HTTP_HOST, startAttributes, endAttributes)) { - // Use http.scheme, http.host and http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.HTTP_HOST); - fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute( - SemanticAttributes.HTTP_SERVER_NAME, startAttributes, endAttributes)) { - // Use http.scheme, http.server_name, net.host.port, http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.HTTP_SERVER_NAME); + Set fullSet = durationServerView; + // Use http.scheme, http.host and http.target when http.route is not available. + if (!containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) { + fullSet = new HashSet<>(durationServerView); fullSet.add(SemanticAttributes.HTTP_TARGET); - } else if (containsAttribute( - SemanticAttributes.NET_HOST_NAME, startAttributes, endAttributes)) { - // Use http.scheme, net.host.name, net.host.port, http.target - fullSet.add(SemanticAttributes.HTTP_SCHEME); - fullSet.add(SemanticAttributes.NET_HOST_NAME); - fullSet.add(SemanticAttributes.NET_HOST_PORT); - fullSet.add(SemanticAttributes.HTTP_TARGET); - } else { - // Use http.url - fullSet.add(SemanticAttributes.HTTP_URL); } AttributesBuilder filtered = Attributes.builder(); applyView(filtered, startAttributes, fullSet); @@ -126,10 +105,22 @@ private static void applyView( (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)) { + filtered.put( + SemanticAttributes.HTTP_URL, removeQueryParamFromUrl(value.toString())); + } else { + filtered.put(key, value); + } } }); } + private static String removeQueryParamFromUrl(String url) { + // Note: Maybe not the most robust, but purely to limit cardinality. + int idx = url.lastIndexOf('?'); + return idx == -1 ? url : url.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 d8821a0485f8..11d616c47a78 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,6 +32,7 @@ void collectsMetrics() { 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") @@ -86,9 +87,7 @@ void collectsMetrics() { .hasSum(150 /* millis */) .attributes() .containsOnly( - attributeEntry("http.scheme", "https"), - attributeEntry("http.host", "host"), - attributeEntry("http.target", "/"), + attributeEntry("http.url", "https://localhost:1234/"), attributeEntry("http.method", "GET"), attributeEntry("http.flavor", "2.0"), attributeEntry("http.status_code", 200)))); 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 197ff9a884e8..75e53f4001a5 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 @@ -45,9 +45,12 @@ public void shouldApplyClientDurationView_urlIdentity() { } @Test - public void shouldApplyClientDurationView_httpHostIdentity() { + 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") @@ -64,68 +67,14 @@ public void shouldApplyClientDurationView_httpHostIdentity() { OpenTelemetryAssertions.assertThat(applyClientDurationView(startAttributes, endAttributes)) .containsOnly( - attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), - attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"), - attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), - attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); - } - - @Test - public void shouldApplyClientDurationView_netPeerNameIdentity() { - Attributes startAttributes = - Attributes.builder() - .put(SemanticAttributes.HTTP_METHOD, "GET") - .put(SemanticAttributes.HTTP_SCHEME, "https") - .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345") - .build(); - - Attributes endAttributes = - Attributes.builder() - .put(SemanticAttributes.HTTP_STATUS_CODE, 500) - .put(SemanticAttributes.NET_PEER_NAME, "somehost") - .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_SCHEME.getKey(), "https"), - attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost"), - attributeEntry(SemanticAttributes.NET_PEER_PORT.getKey(), 443), - attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), - attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); - } - - @Test - public void shouldApplyClientDurationView_netPeerIpIdentity() { - Attributes startAttributes = - Attributes.builder() - .put(SemanticAttributes.HTTP_METHOD, "GET") - .put(SemanticAttributes.HTTP_SCHEME, "https") - .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345") - .build(); - - Attributes endAttributes = - Attributes.builder() - .put(SemanticAttributes.HTTP_STATUS_CODE, 500) - .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_SCHEME.getKey(), "https"), - attributeEntry(SemanticAttributes.NET_PEER_IP.getKey(), "127.0.0.1"), - attributeEntry(SemanticAttributes.NET_PEER_PORT.getKey(), 443), - attributeEntry(SemanticAttributes.HTTP_TARGET.getKey(), "/high/cardinality/12345"), + 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_hostIdentity() { + public void shouldApplyServerDurationView_withRoute() { Attributes startAttributes = Attributes.builder() .put(SemanticAttributes.HTTP_METHOD, "GET") @@ -134,6 +83,7 @@ public void shouldApplyServerDurationView_hostIdentity() { .put(SemanticAttributes.HTTP_HOST, "somehost") .put(SemanticAttributes.HTTP_SERVER_NAME, "somehost") .put(SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345") + .put(SemanticAttributes.HTTP_ROUTE, "/somehost/high/{name}/{id}") .put(SemanticAttributes.NET_HOST_NAME, "somehost") .put(SemanticAttributes.NET_HOST_PORT, 443) .build(); @@ -150,19 +100,19 @@ public void shouldApplyServerDurationView_hostIdentity() { .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_ROUTE.getKey(), "/somehost/high/{name}/{id}"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } @Test - public void shouldApplyServerDurationView_serverNameIdentity() { + 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") .put(SemanticAttributes.NET_HOST_NAME, "somehost") @@ -180,69 +130,13 @@ public void shouldApplyServerDurationView_serverNameIdentity() { OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes)) .containsOnly( attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), - attributeEntry(SemanticAttributes.HTTP_SERVER_NAME.getKey(), "somehost"), - attributeEntry( - SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), - attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); - } - - @Test - public void shouldApplyServerDurationView_netHostIdentity() { - 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_TARGET, "/somehost/high/cardinality/12345") - .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_IP, "127.0.0.1") - .put(SemanticAttributes.NET_PEER_PORT, 14204) - .build(); - - OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes)) - .containsOnly( - attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"), - attributeEntry(SemanticAttributes.NET_HOST_NAME.getKey(), "somehost"), - attributeEntry(SemanticAttributes.NET_HOST_PORT.getKey(), 443), + attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"), attributeEntry( SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"), attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); } - @Test - public void shouldApplyServerDurationView_urlIdentity() { - 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_TARGET, "/somehost/high/cardinality/12345") - .put(SemanticAttributes.NET_HOST_PORT, 443) - .build(); - - Attributes endAttributes = - Attributes.builder() - .put(SemanticAttributes.HTTP_STATUS_CODE, 500) - .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_URL.getKey(), "https://somehost/high/cardinality/12345"), - attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); - } - @Test public void shouldApplyActiveRequestsView() { Attributes attributes = From facdbeaf4bccf5f9e074f2820b4f0f55286130ad Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 5 Nov 2021 09:57:41 -0400 Subject: [PATCH 6/8] Fixes from review. --- .../http/TemporaryMetricsView.java | 42 ++++++++++++++----- .../http/TemporaryMetricsViewTest.java | 3 +- 2 files changed, 34 insertions(+), 11 deletions(-) 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 12f53f54b6f7..7bf7a875ce77 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 @@ -21,6 +21,8 @@ final class TemporaryMetricsView { 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 buildDurationAlwaysInclude() { @@ -54,6 +56,19 @@ private static Set buildDurationServerView() { 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; + } + private static Set 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 @@ -68,7 +83,6 @@ private static Set buildActiveRequestsView() { static Attributes applyClientDurationView(Attributes startAttributes, Attributes endAttributes) { AttributesBuilder filtered = Attributes.builder(); - // TODO - Remove query parameters from URL. applyView(filtered, startAttributes, durationClientView); applyView(filtered, endAttributes, durationClientView); return filtered.build(); @@ -81,10 +95,9 @@ private static boolean containsAttribute( static Attributes applyServerDurationView(Attributes startAttributes, Attributes endAttributes) { Set fullSet = durationServerView; - // Use http.scheme, http.host and http.target when http.route is not available. + // Use http.target when http.route is not available. if (!containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) { - fullSet = new HashSet<>(durationServerView); - fullSet.add(SemanticAttributes.HTTP_TARGET); + fullSet = durationServerFallbackView; } AttributesBuilder filtered = Attributes.builder(); applyView(filtered, startAttributes, fullSet); @@ -106,9 +119,9 @@ private static void applyView( (key, value) -> { if (view.contains(key)) { // For now, we filter query parameters out of URLs in metrics. - if (SemanticAttributes.HTTP_URL.equals(key)) { - filtered.put( - SemanticAttributes.HTTP_URL, removeQueryParamFromUrl(value.toString())); + if (SemanticAttributes.HTTP_URL.equals(key) + || SemanticAttributes.HTTP_TARGET.equals(key)) { + filtered.put(key, removeQueryParamFromUrlOrTarget(value.toString())); } else { filtered.put(key, value); } @@ -116,10 +129,19 @@ private static void applyView( }); } - private static String removeQueryParamFromUrl(String url) { + // 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 = url.lastIndexOf('?'); - return idx == -1 ? url : url.substring(0, idx); + 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/TemporaryMetricsViewTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java index 75e53f4001a5..9292b0c381b1 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 @@ -114,7 +114,8 @@ public void shouldApplyServerDurationView_withTarget() { .put(SemanticAttributes.HTTP_SCHEME, "https") .put(SemanticAttributes.HTTP_HOST, "somehost") .put(SemanticAttributes.HTTP_SERVER_NAME, "somehost") - .put(SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345") + .put( + SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345;jsessionId=12145") .put(SemanticAttributes.NET_HOST_NAME, "somehost") .put(SemanticAttributes.NET_HOST_PORT, 443) .build(); From ec3eabc83e212a6ab6fd6b207d4dfe7e674a2d67 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 5 Nov 2021 10:01:15 -0400 Subject: [PATCH 7/8] Apply suggestions from code review Co-authored-by: Trask Stalnaker --- .../api/instrumenter/http/TemporaryMetricsViewTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 9292b0c381b1..379ff51fba30 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 @@ -54,7 +54,7 @@ public void shouldApplyClientDurationView_urlWithQuery() { .put(SemanticAttributes.HTTP_METHOD, "GET") .put(SemanticAttributes.HTTP_SCHEME, "https") .put(SemanticAttributes.HTTP_HOST, "somehost") - .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345") + .put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345?jsessionId=121454") .build(); Attributes endAttributes = @@ -78,11 +78,11 @@ public void shouldApplyServerDurationView_withRoute() { Attributes startAttributes = Attributes.builder() .put(SemanticAttributes.HTTP_METHOD, "GET") - .put(SemanticAttributes.HTTP_URL, "https://somehost/high/cardinality/12345") + .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") + .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) From 7daf7a62e78b8575ad0b3d52fa7ab6dfcbf7b765 Mon Sep 17 00:00:00 2001 From: Josh Suereth Date: Fri, 5 Nov 2021 12:16:45 -0400 Subject: [PATCH 8/8] Update spotless from code reivew merge. --- .../api/instrumenter/http/TemporaryMetricsViewTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 379ff51fba30..fac42cb3195b 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 @@ -78,11 +78,15 @@ 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_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_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)