From 2dee3b4a325af50f19b37337e461f4d687c7940a Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 24 Oct 2024 10:51:49 +0300 Subject: [PATCH 1/3] Fix SpanKey bridging for unbridgeable span --- .../SpanKeyInstrumentation.java | 62 ++++++++++++------- .../instrumentationapi/ContextBridgeTest.java | 14 +++++ 2 files changed, 52 insertions(+), 24 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java index 2439bfb7cc82..dca71a1b81a8 100644 --- a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java +++ b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java @@ -41,22 +41,16 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class StoreInContextAdvice { - @Advice.OnMethodEnter(skipOn = Advice.OnDefaultValue.class) - public static Object onEnter() { - return null; - } - - @Advice.OnMethodExit(suppress = Throwable.class) - public static void onExit( + @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) + public static Context onEnter( @Advice.This SpanKey applicationSpanKey, @Advice.Argument(0) Context applicationContext, - @Advice.Argument(1) Span applicationSpan, - @Advice.Return(readOnly = false) Context newApplicationContext) { + @Advice.Argument(1) Span applicationSpan) { io.opentelemetry.instrumentation.api.internal.SpanKey agentSpanKey = SpanKeyBridging.toAgentOrNull(applicationSpanKey); if (agentSpanKey == null) { - return; + return null; } io.opentelemetry.context.Context agentContext = @@ -64,41 +58,61 @@ public static void onExit( io.opentelemetry.api.trace.Span agentSpan = Bridging.toAgentOrNull(applicationSpan); if (agentSpan == null) { - return; + // if application span can not be bridged to agent span, this could happen when it is not + // created through bridged GlobalOpenTelemetry, we'll let the original method run and + // store the span in context without bridging + return null; } io.opentelemetry.context.Context newAgentContext = agentSpanKey.storeInContext(agentContext, agentSpan); - newApplicationContext = AgentContextStorage.toApplicationContext(newAgentContext); + return AgentContextStorage.toApplicationContext(newAgentContext); + } + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onExit( + @Advice.Enter Context newApplicationContext, + @Advice.Return(readOnly = false) Context result) { + + if (newApplicationContext != null) { + result = newApplicationContext; + } } } @SuppressWarnings("unused") public static class FromContextOrNullAdvice { - @Advice.OnMethodEnter(skipOn = Advice.OnDefaultValue.class) - public static Object onEnter() { - return null; - } - - @Advice.OnMethodExit(suppress = Throwable.class) - public static void onExit( - @Advice.This SpanKey applicationSpanKey, - @Advice.Argument(0) Context applicationContext, - @Advice.Return(readOnly = false) Span applicationSpan) { + @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) + public static Span onEnter( + @Advice.This SpanKey applicationSpanKey, @Advice.Argument(0) Context applicationContext) { io.opentelemetry.instrumentation.api.internal.SpanKey agentSpanKey = SpanKeyBridging.toAgentOrNull(applicationSpanKey); if (agentSpanKey == null) { - return; + return null; } io.opentelemetry.context.Context agentContext = AgentContextStorage.getAgentContext(applicationContext); io.opentelemetry.api.trace.Span agentSpan = agentSpanKey.fromContextOrNull(agentContext); + if (agentSpan == null) { + // Bridged agent span was not found. Run the original method, there could be an unbridged + // span stored it the application context. + return null; + } - applicationSpan = agentSpan == null ? null : Bridging.toApplication(agentSpan); + return Bridging.toApplication(agentSpan); + } + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void onExit( + @Advice.Enter Span applicationSpan, @Advice.Return(readOnly = false) Span result) { + + if (applicationSpan != null) { + result = applicationSpan; + } } } } diff --git a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java index cd5078cd2211..127f7995bccb 100644 --- a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java +++ b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.instrumentationapi; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertNotNull; import io.opentelemetry.api.trace.Span; @@ -18,6 +19,7 @@ import io.opentelemetry.instrumentation.testing.junit.AgentInstrumentationExtension; import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension; import io.opentelemetry.javaagent.instrumentation.testing.AgentSpanTesting; +import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.semconv.ErrorAttributes; import io.opentelemetry.semconv.HttpAttributes; import java.util.Arrays; @@ -76,6 +78,18 @@ void testSpanKeyBridge() { }); } + @Test + void testSpanKeyBridge_unbridgedSpan() { + OpenTelemetrySdk openTelemetry = OpenTelemetrySdk.builder().build(); + // span is bridged only when it is created though a bridged OpenTelemetry instance obtained + // from GlobalOpenTelemetry + Span span = openTelemetry.getTracer("test").spanBuilder("test").startSpan(); + + Context context = SpanKey.HTTP_CLIENT.storeInContext(Context.current(), span); + assertThat(context).isNotNull(); + assertThat(SpanKey.HTTP_CLIENT.fromContextOrNull(context)).isEqualTo(span); + } + @Test void testHttpRouteHolder_SameSourceAsServerInstrumentationDoesNotOverrideRoute() { AgentSpanTesting.runWithHttpServerSpan( From 45a76676bd5d863e918239941898cad99a9332f9 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 24 Oct 2024 12:58:08 +0300 Subject: [PATCH 2/3] rename test method --- .../instrumentation/instrumentationapi/ContextBridgeTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java index 127f7995bccb..2ebea20102b6 100644 --- a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java +++ b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/ContextBridgeTest.java @@ -79,7 +79,7 @@ void testSpanKeyBridge() { } @Test - void testSpanKeyBridge_unbridgedSpan() { + void testSpanKeyBridge_UnbridgedSpan() { OpenTelemetrySdk openTelemetry = OpenTelemetrySdk.builder().build(); // span is bridged only when it is created though a bridged OpenTelemetry instance obtained // from GlobalOpenTelemetry From 84c2e56400c14ab957e2b0fd10235fda62ba6d96 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 25 Oct 2024 09:26:18 +0300 Subject: [PATCH 3/3] Update instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java Co-authored-by: Trask Stalnaker --- .../instrumentationapi/SpanKeyInstrumentation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java index dca71a1b81a8..b8cc4ea99f40 100644 --- a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java +++ b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/SpanKeyInstrumentation.java @@ -99,7 +99,7 @@ public static Span onEnter( io.opentelemetry.api.trace.Span agentSpan = agentSpanKey.fromContextOrNull(agentContext); if (agentSpan == null) { // Bridged agent span was not found. Run the original method, there could be an unbridged - // span stored it the application context. + // span stored in the application context. return null; }