From 0fedaef9cffb3441f615b6b1bf79980ab66a4c09 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 15 Apr 2022 19:15:49 +0200 Subject: [PATCH] Update the http.route attribute even for not sampled server spans (#5844) --- .../instrumenter/http/HttpRouteHolder.java | 6 +-- .../http/HttpRouteHolderTest.java | 45 +++++++++++++++++++ .../armeria/v1_3/OpenTelemetryClient.java | 10 ++--- .../armeria/v1_3/OpenTelemetryService.java | 10 ++--- 4 files changed, 54 insertions(+), 17 deletions(-) create mode 100644 instrumentation-api-semconv/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java diff --git a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java index 130dfa1e4996..9b25dcddf4c2 100644 --- a/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java +++ b/instrumentation-api-semconv/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java @@ -102,9 +102,9 @@ public static void updateHttpRoute( T arg1, U arg2) { Span serverSpan = ServerSpan.fromContextOrNull(context); - // checking isRecording() is a helpful optimization for more expensive suppliers - // (e.g. Spring MVC instrumentation's HandlerAdapterInstrumentation) - if (serverSpan == null || !serverSpan.isRecording()) { + // even if the server span is not sampled, we have to continue - we need to compute the + // http.route properly so that it can be captured by the server metrics + if (serverSpan == null) { return; } HttpRouteHolder httpRouteHolder = context.get(CONTEXT_KEY); diff --git a/instrumentation-api-semconv/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java b/instrumentation-api-semconv/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java new file mode 100644 index 000000000000..483078385b0b --- /dev/null +++ b/instrumentation-api-semconv/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java @@ -0,0 +1,45 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.http; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.api.trace.TraceFlags; +import io.opentelemetry.api.trace.TraceState; +import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.internal.SpanKey; +import org.junit.jupiter.api.Test; + +class HttpRouteHolderTest { + + @Test + void shouldSetRouteEvenIfSpanIsNotSampled() { + Span span = + Span.wrap( + SpanContext.create( + "00000000000000000000000000000042", + "0000000000000012", + TraceFlags.getDefault(), + TraceState.getDefault())); + + Context context = Context.root(); + context = context.with(span); + context = SpanKey.SERVER.storeInContext(context, span); + context = HttpRouteHolder.get().start(context, null, Attributes.empty()); + + assertNull(HttpRouteHolder.getRoute(context)); + + HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/get/:id"); + + assertEquals("/get/:id", HttpRouteHolder.getRoute(context)); + } + + // TODO(mateusz): add more unit tests for HttpRouteHolder +} diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java index d05b3aa67c64..d62aad83b7cd 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java @@ -11,7 +11,6 @@ import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.logging.RequestLog; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; @@ -36,12 +35,9 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex Context context = instrumenter.start(Context.current(), ctx); - Span span = Span.fromContext(context); - if (span.isRecording()) { - ctx.log() - .whenComplete() - .thenAccept(log -> instrumenter.end(context, ctx, log, log.responseCause())); - } + ctx.log() + .whenComplete() + .thenAccept(log -> instrumenter.end(context, ctx, log, log.responseCause())); try (Scope ignored = context.makeCurrent()) { return unwrap().execute(ctx, req); diff --git a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryService.java b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryService.java index 2ee1f02b2e70..5b53fd872f0b 100644 --- a/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryService.java +++ b/instrumentation/armeria-1.3/library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryService.java @@ -11,7 +11,6 @@ import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.ServiceRequestContext; import com.linecorp.armeria.server.SimpleDecoratingHttpService; -import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; @@ -36,12 +35,9 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc Context context = instrumenter.start(parentContext, ctx); - Span span = Span.fromContext(context); - if (span.isRecording()) { - ctx.log() - .whenComplete() - .thenAccept(log -> instrumenter.end(context, ctx, log, log.responseCause())); - } + ctx.log() + .whenComplete() + .thenAccept(log -> instrumenter.end(context, ctx, log, log.responseCause())); try (Scope ignored = context.makeCurrent()) { return unwrap().serve(ctx, req);