From 4f0baff3f87d7354d4f94a158d5778900d552947 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 14 Feb 2024 09:25:33 +0200 Subject: [PATCH] Set route only on the SERVER span (#10290) --- .../api/instrumenter/Instrumenter.java | 4 ++ .../api/internal/HttpRouteState.java | 26 +++++++- .../api/semconv/http/HttpServerRoute.java | 12 ++-- .../api/semconv/http/HttpServerRouteTest.java | 24 ++++++- .../InstrumentationApiContextBridging.java | 63 ++++++++++++++++--- .../HttpRouteStateInstrumentation.java | 20 ++++++ 6 files changed, 130 insertions(+), 19 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index 97f523bea61f..0d4ed4eedda4 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -11,6 +11,7 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.internal.HttpRouteState; import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess; import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; @@ -203,6 +204,9 @@ private Context doStart(Context parentContext, REQUEST request, @Nullable Instan if (localRoot) { context = LocalRootSpan.store(context, span); + if (spanKind == SpanKind.SERVER) { + HttpRouteState.updateSpan(context, span); + } } return spanSuppressor.storeInContext(context, spanKind, span); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java index 0eda2755da92..afd023467c16 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java @@ -5,6 +5,7 @@ package io.opentelemetry.instrumentation.api.internal; +import io.opentelemetry.api.trace.Span; import io.opentelemetry.context.Context; import io.opentelemetry.context.ContextKey; import io.opentelemetry.context.ImplicitContextKeyed; @@ -24,20 +25,36 @@ public static HttpRouteState fromContextOrNull(Context context) { return context.get(KEY); } + public static void updateSpan(Context context, Span span) { + HttpRouteState state = fromContextOrNull(context); + if (state != null) { + state.span = span; + } + } + + // this method is used reflectively from InstrumentationApiContextBridging public static HttpRouteState create( @Nullable String method, @Nullable String route, int updatedBySourceOrder) { - return new HttpRouteState(method, route, updatedBySourceOrder); + return create(method, route, updatedBySourceOrder, null); + } + + // this method is used reflectively from InstrumentationApiContextBridging + public static HttpRouteState create( + @Nullable String method, @Nullable String route, int updatedBySourceOrder, Span span) { + return new HttpRouteState(method, route, updatedBySourceOrder, span); } @Nullable private final String method; @Nullable private volatile String route; private volatile int updatedBySourceOrder; + @Nullable private volatile Span span; private HttpRouteState( - @Nullable String method, @Nullable String route, int updatedBySourceOrder) { + @Nullable String method, @Nullable String route, int updatedBySourceOrder, Span span) { this.method = method; this.updatedBySourceOrder = updatedBySourceOrder; this.route = route; + this.span = span; } @Override @@ -59,6 +76,11 @@ public String getRoute() { return route; } + @Nullable + public Span getSpan() { + return span; + } + public void update( @SuppressWarnings("unused") Context context, // context is used by the javaagent bridge instrumentation diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java index a252b6b22af1..69172ca72fb9 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java @@ -10,7 +10,6 @@ import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder; -import io.opentelemetry.instrumentation.api.instrumenter.LocalRootSpan; import io.opentelemetry.instrumentation.api.internal.HttpRouteState; import javax.annotation.Nullable; @@ -97,16 +96,17 @@ public static void update( HttpServerRouteBiGetter httpRouteGetter, T arg1, U arg2) { - Span serverSpan = LocalRootSpan.fromContextOrNull(context); + HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context); + if (httpRouteState == null) { + return; + } + Span serverSpan = httpRouteState.getSpan(); // 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; } - HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context); - if (httpRouteState == null) { - return; - } + // special case for servlet filters, even when we have a route from previous filter see whether // the new route is better and if so use it instead boolean onlyIfBetterRoute = diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRouteTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRouteTest.java index e2d36862f958..c57261c83ab7 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRouteTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRouteTest.java @@ -12,6 +12,7 @@ import static org.mockito.Mockito.when; import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.context.Context; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension; @@ -39,7 +40,7 @@ void setUp() { HttpServerRoute.builder(getter) .setKnownMethods(new HashSet<>(singletonList("GET"))) .build()) - .buildInstrumenter(); + .buildInstrumenter(s -> SpanKind.SERVER); } @Test @@ -61,6 +62,27 @@ void noLocalRootSpan() { span -> assertThat(span).hasName("parent"), span -> assertThat(span).hasName("test")); } + @Test + void nonServerRootSpan() { + Instrumenter testInstrumenter = + Instrumenter.builder(testing.getOpenTelemetry(), "test", s -> s) + .addContextCustomizer( + HttpServerRoute.builder(getter) + .setKnownMethods(new HashSet<>(singletonList("GET"))) + .build()) + .buildInstrumenter(s -> SpanKind.INTERNAL); + + Context context = testInstrumenter.start(Context.root(), "test"); + assertNull(HttpServerRoute.get(context)); + + HttpServerRoute.update(context, HttpServerRouteSource.SERVER, "/get/:id"); + + testInstrumenter.end(context, "test", null, null); + + assertNull(HttpServerRoute.get(context)); + assertThat(testing.getSpans()).satisfiesExactly(span -> assertThat(span).hasName("test")); + } + @Test void shouldSetRoute() { when(getter.getHttpRequestMethod("test")).thenReturn("GET"); diff --git a/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/context/InstrumentationApiContextBridging.java b/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/context/InstrumentationApiContextBridging.java index 2c1e36124395..eed3c4d281df 100644 --- a/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/context/InstrumentationApiContextBridging.java +++ b/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/context/InstrumentationApiContextBridging.java @@ -59,12 +59,14 @@ final class InstrumentationApiContextBridging { private static final MethodHandle AGENT_GET_METHOD; private static final MethodHandle AGENT_GET_ROUTE; private static final MethodHandle AGENT_GET_UPDATED_BY_SOURCE_ORDER; + private static final MethodHandle AGENT_GET_SPAN; private static final Class APPLICATION_HTTP_ROUTE_STATE; private static final MethodHandle APPLICATION_CREATE; private static final MethodHandle APPLICATION_GET_METHOD; private static final MethodHandle APPLICATION_GET_ROUTE; private static final MethodHandle APPLICATION_GET_UPDATED_BY_SOURCE_ORDER; + private static final MethodHandle APPLICATION_GET_SPAN; static { MethodHandles.Lookup lookup = MethodHandles.lookup(); @@ -74,11 +76,13 @@ final class InstrumentationApiContextBridging { MethodHandle agentGetMethod = null; MethodHandle agentGetRoute = null; MethodHandle agentGetUpdatedBySourceOrder = null; + MethodHandle agentGetSpan = null; Class applicationHttpRouteState = null; MethodHandle applicationCreate = null; MethodHandle applicationGetMethod = null; MethodHandle applicationGetRoute = null; MethodHandle applicationGetUpdatedBySourceOrder = null; + MethodHandle applicationGetSpan = null; try { agentHttpRouteState = @@ -87,7 +91,12 @@ final class InstrumentationApiContextBridging { lookup.findStatic( agentHttpRouteState, "create", - MethodType.methodType(agentHttpRouteState, String.class, String.class, int.class)); + MethodType.methodType( + agentHttpRouteState, + String.class, + String.class, + int.class, + io.opentelemetry.api.trace.Span.class)); agentGetMethod = lookup.findVirtual(agentHttpRouteState, "getMethod", MethodType.methodType(String.class)); agentGetRoute = @@ -95,15 +104,30 @@ final class InstrumentationApiContextBridging { agentGetUpdatedBySourceOrder = lookup.findVirtual( agentHttpRouteState, "getUpdatedBySourceOrder", MethodType.methodType(int.class)); + agentGetSpan = + lookup.findVirtual( + agentHttpRouteState, + "getSpan", + MethodType.methodType(io.opentelemetry.api.trace.Span.class)); applicationHttpRouteState = Class.forName("application.io.opentelemetry.instrumentation.api.internal.HttpRouteState"); - applicationCreate = - lookup.findStatic( - applicationHttpRouteState, - "create", - MethodType.methodType( - applicationHttpRouteState, String.class, String.class, int.class)); + try { + applicationCreate = + lookup.findStatic( + applicationHttpRouteState, + "create", + MethodType.methodType( + applicationHttpRouteState, String.class, String.class, int.class, Span.class)); + } catch (NoSuchMethodException exception) { + // older instrumentation-api has only the variant that does not take span + applicationCreate = + lookup.findStatic( + applicationHttpRouteState, + "create", + MethodType.methodType( + applicationHttpRouteState, String.class, String.class, int.class)); + } applicationGetMethod = lookup.findVirtual( applicationHttpRouteState, "getMethod", MethodType.methodType(String.class)); @@ -115,6 +139,13 @@ final class InstrumentationApiContextBridging { applicationHttpRouteState, "getUpdatedBySourceOrder", MethodType.methodType(int.class)); + try { + applicationGetSpan = + lookup.findVirtual( + applicationHttpRouteState, "getSpan", MethodType.methodType(Span.class)); + } catch (NoSuchMethodException ignored) { + // not present in older instrumentation-api + } } catch (Throwable ignored) { // instrumentation-api may be absent on the classpath, or it might be an older version } @@ -124,11 +155,13 @@ final class InstrumentationApiContextBridging { AGENT_GET_METHOD = agentGetMethod; AGENT_GET_ROUTE = agentGetRoute; AGENT_GET_UPDATED_BY_SOURCE_ORDER = agentGetUpdatedBySourceOrder; + AGENT_GET_SPAN = agentGetSpan; APPLICATION_HTTP_ROUTE_STATE = applicationHttpRouteState; APPLICATION_CREATE = applicationCreate; APPLICATION_GET_METHOD = applicationGetMethod; APPLICATION_GET_ROUTE = applicationGetRoute; APPLICATION_GET_UPDATED_BY_SOURCE_ORDER = applicationGetUpdatedBySourceOrder; + APPLICATION_GET_SPAN = applicationGetSpan; } @Nullable @@ -151,12 +184,16 @@ final class InstrumentationApiContextBridging { APPLICATION_CREATE, AGENT_GET_METHOD, AGENT_GET_ROUTE, - AGENT_GET_UPDATED_BY_SOURCE_ORDER), + AGENT_GET_UPDATED_BY_SOURCE_ORDER, + AGENT_GET_SPAN, + o -> o != null ? Bridging.toApplication((io.opentelemetry.api.trace.Span) o) : null), httpRouteStateConvert( AGENT_CREATE, APPLICATION_GET_METHOD, APPLICATION_GET_ROUTE, - APPLICATION_GET_UPDATED_BY_SOURCE_ORDER)); + APPLICATION_GET_UPDATED_BY_SOURCE_ORDER, + APPLICATION_GET_SPAN, + o -> o != null ? Bridging.toAgentOrNull((Span) o) : null)); } catch (Throwable ignored) { return null; } @@ -166,12 +203,18 @@ private static Function httpRouteStateConvert( MethodHandle create, MethodHandle getMethod, MethodHandle getRoute, - MethodHandle getUpdatedBySourceOrder) { + MethodHandle getUpdatedBySourceOrder, + MethodHandle getSpan, + Function convertSpan) { return httpRouteHolder -> { try { String method = (String) getMethod.invoke(httpRouteHolder); String route = (String) getRoute.invoke(httpRouteHolder); int updatedBySourceOrder = (int) getUpdatedBySourceOrder.invoke(httpRouteHolder); + if (create.type().parameterCount() == 4 && getSpan != null) { + Object span = convertSpan.apply(getSpan.invoke(httpRouteHolder)); + return create.invoke(method, route, updatedBySourceOrder, span); + } return create.invoke(method, route, updatedBySourceOrder); } catch (Throwable e) { return null; diff --git a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/HttpRouteStateInstrumentation.java b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/HttpRouteStateInstrumentation.java index e1438dc085ff..52f93afba7c5 100644 --- a/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/HttpRouteStateInstrumentation.java +++ b/instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/HttpRouteStateInstrumentation.java @@ -8,10 +8,12 @@ import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import application.io.opentelemetry.api.trace.Span; import application.io.opentelemetry.context.Context; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.context.AgentContextStorage; +import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.trace.Bridging; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -31,6 +33,11 @@ public void transform(TypeTransformer transformer) { .and(takesArgument(1, int.class)) .and(takesArgument(2, String.class)), this.getClass().getName() + "$UpdateAdvice"); + transformer.applyAdviceToMethod( + named("updateSpan") + .and(takesArgument(0, named("application.io.opentelemetry.context.Context"))) + .and(takesArgument(1, named("application.io.opentelemetry.api.trace.Span"))), + this.getClass().getName() + "$UpdateSpanAdvice"); } @SuppressWarnings("unused") @@ -54,4 +61,17 @@ public static void onEnter( agentRouteState.update(agentContext, updatedBySourceOrder, route); } } + + @SuppressWarnings("unused") + public static class UpdateSpanAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter( + @Advice.Argument(0) Context applicationContext, @Advice.Argument(1) Span applicationSpan) { + + io.opentelemetry.context.Context agentContext = + AgentContextStorage.getAgentContext(applicationContext); + io.opentelemetry.instrumentation.api.internal.HttpRouteState.updateSpan( + agentContext, Bridging.toAgentOrNull(applicationSpan)); + } + } }