diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java index f26a3f3f452b..6840c86de65d 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java @@ -146,7 +146,6 @@ private boolean isBetterRoute(String name) { return name.length() > routeLength; } - // TODO: use that in HttpServerMetrics /** * Returns the {@code http.route} attribute value that's stored in the passed {@code context}, or * null if it was not set before. 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 22413c7321c4..dc085de60708 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 @@ -18,7 +18,9 @@ import io.opentelemetry.instrumentation.api.annotations.UnstableApi; import io.opentelemetry.instrumentation.api.instrumenter.RequestListener; import io.opentelemetry.instrumentation.api.instrumenter.RequestMetrics; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,8 +51,14 @@ public static RequestMetrics get() { private final LongUpDownCounter activeRequests; private final DoubleHistogram duration; + private final Function httpRouteHolderGetter; private HttpServerMetrics(Meter meter) { + this(meter, HttpRouteHolder::getRoute); + } + + // visible for tests + HttpServerMetrics(Meter meter, Function httpRouteHolderGetter) { activeRequests = meter .upDownCounterBuilder("http.server.active_requests") @@ -64,6 +72,8 @@ private HttpServerMetrics(Meter meter) { .setUnit("ms") .setDescription("The duration of the inbound HTTP request") .build(); + + this.httpRouteHolderGetter = httpRouteHolderGetter; } @Override @@ -86,10 +96,19 @@ public void end(Context context, Attributes endAttributes, long endNanos) { activeRequests.add(-1, applyActiveRequestsView(state.startAttributes())); duration.record( (endNanos - state.startTimeNanos()) / NANOS_PER_MS, - applyServerDurationView(state.startAttributes(), endAttributes), + applyServerDurationView(state.startAttributes(), addHttpRoute(context, endAttributes)), context); } + // TODO: the http.route should be extracted by the AttributesExtractor, HttpServerMetrics should + // not access the context to get it + private Attributes addHttpRoute(Context context, Attributes endAttributes) { + String route = httpRouteHolderGetter.apply(context); + return route == null + ? endAttributes + : endAttributes.toBuilder().put(SemanticAttributes.HTTP_ROUTE, route).build(); + } + @AutoValue abstract static class State { 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 b2df3a6e84c0..2189135ac2ab 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 @@ -8,7 +8,6 @@ import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; -import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.util.HashSet; import java.util.Set; @@ -19,19 +18,9 @@ @SuppressWarnings("rawtypes") final class TemporaryMetricsView { - // TODO (trask) remove this once http.route is captured consistently - // - // this is not enabled by default because it falls back to http.target (which can be high - // cardinality) when http.route is not available - private static final boolean USE_HTTP_TARGET_FALLBACK = - Config.get() - .getBoolean("otel.instrumentation.metrics.experimental.use-http-target-fallback", false); - 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() { @@ -65,19 +54,6 @@ 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 @@ -97,21 +73,10 @@ static Attributes applyClientDurationView(Attributes startAttributes, Attributes 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 (USE_HTTP_TARGET_FALLBACK - && !containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) { - fullSet = durationServerFallbackView; - } AttributesBuilder filtered = Attributes.builder(); - applyView(filtered, startAttributes, fullSet); - applyView(filtered, endAttributes, fullSet); + applyView(filtered, startAttributes, durationServerView); + applyView(filtered, endAttributes, durationServerView); return filtered.build(); } 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 f90c70931bda..3ade6bd6b811 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 @@ -156,6 +156,49 @@ void collectsMetrics() { .satisfiesExactly(point -> assertThat(point).hasSum(300 /* millis */))); } + @Test + void collectsHttpRouteFromContext() { + // given + InMemoryMetricReader metricReader = InMemoryMetricReader.create(); + SdkMeterProvider meterProvider = + SdkMeterProvider.builder() + .registerMetricReader(metricReader) + .setMinimumCollectionInterval(Duration.ZERO) + .build(); + + RequestListener listener = new HttpServerMetrics(meterProvider.get("test"), c -> "/test/{id}"); + + Attributes requestAttributes = + Attributes.builder().put("http.host", "host").put("http.scheme", "https").build(); + + Attributes responseAttributes = Attributes.empty(); + + Context parentContext = Context.root(); + + // when + Context context = listener.start(parentContext, requestAttributes, nanos(100)); + listener.end(context, responseAttributes, nanos(200)); + + // then + assertThat(metricReader.collectAllMetrics()) + .anySatisfy( + metric -> + assertThat(metric) + .hasName("http.server.duration") + .hasUnit("ms") + .hasDoubleHistogram() + .points() + .satisfiesExactly( + point -> + assertThat(point) + .hasSum(100 /* millis */) + .attributes() + .containsOnly( + attributeEntry("http.scheme", "https"), + attributeEntry("http.host", "host"), + attributeEntry("http.route", "/test/{id}")))); + } + private static long nanos(int millis) { return TimeUnit.MILLISECONDS.toNanos(millis); } 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 92b70ebe82c6..132ae8cdde37 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 @@ -109,37 +109,6 @@ public void shouldApplyServerDurationView_withRoute() { 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_METHOD.getKey(), "GET"), - attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500)); - } - @Test public void shouldApplyActiveRequestsView() { Attributes attributes =