Update the http.route attribute even for not sampled server spans (op…

…en-telemetry#5844)
RashmiRam · May 23, 2022 · 0fedaef · 0fedaef
1 parent c522775
commit 0fedaef
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 17 deletions.
diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java
@@ -102,9 +102,9 @@ public static <T, U> 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/...test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java b/...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/...rary/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java b/...rary/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/...ary/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryService.java b/...ary/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);