From 9a315599968af8f8511f16a0fd4c00dda3e94217 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 26 Aug 2021 18:45:19 -0700 Subject: [PATCH] Fix metrics cardinality (#3972) * Fix metrics cardinality * Add test --- .../instrumenter/http/HttpClientMetrics.java | 5 +- .../instrumenter/http/HttpServerMetrics.java | 10 ++- .../http/TemporaryMetricsView.java | 77 +++++++++++++++++++ .../http/HttpServerMetricsTest.java | 4 +- .../http/TemporaryMetricsViewTest.java | 45 +++++++++++ 5 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java 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 b203a6064e99..bef2d7aa10f4 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,6 +5,8 @@ package io.opentelemetry.instrumentation.api.instrumenter.http; +import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView; + import com.google.auto.value.AutoValue; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.metrics.DoubleHistogram; @@ -75,7 +77,8 @@ public void end(Context context, Attributes responseAttributes) { return; } duration.record( - (System.nanoTime() - state.startTimeNanos()) / NANOS_PER_MS, state.startAttributes()); + (System.nanoTime() - state.startTimeNanos()) / NANOS_PER_MS, + applyDurationView(state.startAttributes())); } @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 0b68eff6ca05..63891ffc7a60 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 @@ -5,6 +5,9 @@ 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 com.google.auto.value.AutoValue; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.metrics.DoubleHistogram; @@ -69,7 +72,7 @@ private HttpServerMetrics(Meter meter) { @Override public Context start(Context context, Attributes requestAttributes) { long startTimeNanos = System.nanoTime(); - activeRequests.add(1, requestAttributes); + activeRequests.add(1, applyActiveRequestsView(requestAttributes)); return context.with( HTTP_SERVER_REQUEST_METRICS_STATE, @@ -84,9 +87,10 @@ public void end(Context context, Attributes responseAttributes) { "No state present when ending context {}. Cannot reset HTTP request metrics.", context); return; } - activeRequests.add(-1, state.startAttributes()); + activeRequests.add(-1, applyActiveRequestsView(state.startAttributes())); duration.record( - (System.nanoTime() - state.startTimeNanos()) / NANOS_PER_MS, state.startAttributes()); + (System.nanoTime() - state.startTimeNanos()) / NANOS_PER_MS, + applyDurationView(state.startAttributes())); } @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 new file mode 100644 index 000000000000..8c2aa1f79e34 --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java @@ -0,0 +1,77 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.http; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import java.util.HashSet; +import java.util.Set; +import java.util.function.BiConsumer; + +// this is temporary, see +// https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/3962#issuecomment-906606325 +@SuppressWarnings("rawtypes") +final class TemporaryMetricsView { + + private static final Set durationView = buildDurationView(); + + private static final Set activeRequestsView = buildActiveRequestsView(); + + private static Set buildDurationView() { + // 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); + 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 + Set view = new HashSet<>(); + view.add(SemanticAttributes.HTTP_METHOD); + view.add(SemanticAttributes.HTTP_HOST); + view.add(SemanticAttributes.HTTP_SCHEME); + view.add(SemanticAttributes.HTTP_FLAVOR); + view.add(SemanticAttributes.HTTP_SERVER_NAME); + return view; + } + + static Attributes applyDurationView(Attributes attributes) { + return applyView(attributes, durationView); + } + + static Attributes applyActiveRequestsView(Attributes attributes) { + return applyView(attributes, activeRequestsView); + } + + @SuppressWarnings("unchecked") + private static Attributes applyView(Attributes attributes, Set view) { + AttributesBuilder filtered = Attributes.builder(); + attributes.forEach( + (BiConsumer) + (key, value) -> { + if (view.contains(key)) { + filtered.put(key, value); + } + }); + return filtered.build(); + } + + private TemporaryMetricsView() {} +} 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 df64e83f3605..a54eaadca5da 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 @@ -63,9 +63,7 @@ void collectsMetrics() { .containsOnly( attributeEntry("http.host", "host"), attributeEntry("http.method", "GET"), - attributeEntry("http.scheme", "https"), - attributeEntry("net.host.name", "localhost"), - attributeEntry("net.host.port", 1234L)))); + attributeEntry("http.scheme", "https")))); Context context2 = listener.start(Context.current(), requestAttributes); 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 new file mode 100644 index 000000000000..0250096cf4ac --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java @@ -0,0 +1,45 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +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.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry; + +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import org.junit.jupiter.api.Test; + +public class TemporaryMetricsViewTest { + + @Test + public void shouldApplyDurationView() { + Attributes attributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_METHOD, "GET") + .put(SemanticAttributes.HTTP_URL, "http://somehost/high/cardinality/12345") + .put(SemanticAttributes.NET_PEER_NAME, "somehost") + .build(); + + OpenTelemetryAssertions.assertThat(applyDurationView(attributes)) + .containsOnly( + attributeEntry("http.method", "GET"), attributeEntry("net.peer.name", "somehost")); + } + + @Test + public void shouldApplyActiveRequestsView() { + Attributes attributes = + Attributes.builder() + .put(SemanticAttributes.HTTP_METHOD, "GET") + .put(SemanticAttributes.HTTP_URL, "/high/cardinality/12345") + .put(SemanticAttributes.NET_PEER_NAME, "somehost") + .build(); + + OpenTelemetryAssertions.assertThat(applyActiveRequestsView(attributes)) + .containsOnly(attributeEntry("http.method", "GET")); + } +}