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 cee163f4a12e..a51990317067 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.context.ContextKey; import io.opentelemetry.instrumentation.api.internal.HttpRouteState; import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess; import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; @@ -41,6 +42,9 @@ */ public class Instrumenter { + private static final ContextKey START_OPERATION_LISTENERS = + ContextKey.named("instrumenter-start-operation-listeners"); + /** * Returns a new {@link InstrumenterBuilder}. * @@ -74,6 +78,7 @@ public static InstrumenterBuilder builder private final ContextCustomizer[] contextCustomizers; private final OperationListener[] operationListeners; private final ErrorCauseExtractor errorCauseExtractor; + private final boolean propagateOperationListenersToOnEnd; private final boolean enabled; private final SpanSuppressor spanSuppressor; @@ -89,6 +94,7 @@ public static InstrumenterBuilder builder this.contextCustomizers = builder.contextCustomizers.toArray(new ContextCustomizer[0]); this.operationListeners = builder.buildOperationListeners().toArray(new OperationListener[0]); this.errorCauseExtractor = builder.errorCauseExtractor; + this.propagateOperationListenersToOnEnd = builder.propagateOperationListenersToOnEnd; this.enabled = builder.enabled; this.spanSuppressor = builder.buildSpanSuppressor(); } @@ -198,6 +204,15 @@ private Context doStart(Context parentContext, REQUEST request, @Nullable Instan context = operationListeners[i].onStart(context, attributes, startNanos); } } + if (propagateOperationListenersToOnEnd || context.get(START_OPERATION_LISTENERS) != null) { + // when start and end are not called on the same instrumenter we need to use the operation + // listeners that were used during start in end to correctly handle metrics like + // http.server.active_requests that is recorded both in start and end + // + // need to also add when there is already START_OPERATION_LISTENERS, otherwise this + // instrumenter will call its parent's operation listeners in doEnd + context = context.with(START_OPERATION_LISTENERS, operationListeners); + } if (localRoot) { context = LocalRootSpan.store(context, span); @@ -228,6 +243,10 @@ private void doEnd( } span.setAllAttributes(attributes); + OperationListener[] operationListeners = context.get(START_OPERATION_LISTENERS); + if (operationListeners == null) { + operationListeners = this.operationListeners; + } if (operationListeners.length != 0) { long endNanos = getNanos(endTime); for (int i = operationListeners.length - 1; i >= 0; i--) { diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index 8c68cd0890d4..6b41ae6c60ab 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -66,6 +66,7 @@ public final class InstrumenterBuilder { SpanStatusExtractor spanStatusExtractor = SpanStatusExtractor.getDefault(); ErrorCauseExtractor errorCauseExtractor = ErrorCauseExtractor.getDefault(); + boolean propagateOperationListenersToOnEnd = false; boolean enabled = true; InstrumenterBuilder( @@ -370,6 +371,10 @@ private Set getSpanKeysFromAttributesExtractors() { .collect(Collectors.toSet()); } + private void propagateOperationListenersToOnEnd() { + propagateOperationListenersToOnEnd = true; + } + private interface InstrumenterConstructor { Instrumenter create(InstrumenterBuilder builder); @@ -406,6 +411,12 @@ public Instrumenter buildDownstreamInstrumenter( SpanKindExtractor spanKindExtractor) { return builder.buildDownstreamInstrumenter(setter, spanKindExtractor); } + + @Override + public void propagateOperationListenersToOnEnd( + InstrumenterBuilder builder) { + builder.propagateOperationListenersToOnEnd(); + } }); } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java index d8c40af3e47a..2c660577b17c 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java @@ -26,4 +26,7 @@ Instrumenter buildDownstreamInstrumenter( InstrumenterBuilder builder, TextMapSetter setter, SpanKindExtractor spanKindExtractor); + + void propagateOperationListenersToOnEnd( + InstrumenterBuilder builder); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java index 24648b4b2fb6..d61a6e8fe367 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java @@ -67,5 +67,11 @@ public static Instrumenter buildDownstrea builder, setter, spanKindExtractor); } + public static void propagateOperationListenersToOnEnd( + InstrumenterBuilder builder) { + // instrumenterBuilderAccess is guaranteed to be non-null here + instrumenterBuilderAccess.propagateOperationListenersToOnEnd(builder); + } + private InstrumenterUtil() {} } diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java index 240a96c39aca..0e7fe4081a1e 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java @@ -24,6 +24,7 @@ public final class Jetty11Singletons { ServletInstrumenterBuilder.create() .addContextCustomizer( (context, request, attributes) -> new AppServerBridge.Builder().init(context)) + .propagateOperationListenersToOnEnd() .build(INSTRUMENTATION_NAME, Servlet5Accessor.INSTANCE); private static final JettyHelper HELPER = diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java index ed503f12ab4e..fd26bb40a698 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java @@ -24,6 +24,7 @@ public final class Jetty8Singletons { ServletInstrumenterBuilder.create() .addContextCustomizer( (context, request, attributes) -> new AppServerBridge.Builder().init(context)) + .propagateOperationListenersToOnEnd() .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE); private static final JettyHelper HELPER = diff --git a/instrumentation/liberty/liberty-20.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java b/instrumentation/liberty/liberty-20.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java index 7468415faf50..7cfcb21c5538 100644 --- a/instrumentation/liberty/liberty-20.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java +++ b/instrumentation/liberty/liberty-20.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java @@ -24,6 +24,7 @@ public final class LibertySingletons { .addContextCustomizer( (context, request, attributes) -> new AppServerBridge.Builder().recordException().init(context)) + .propagateOperationListenersToOnEnd() .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE); private static final LibertyHelper HELPER = diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy index ca3187fc446f..f2f93e4c2ef9 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy @@ -149,13 +149,6 @@ class JettyServlet3TestAsync extends JettyServlet3Test { boolean isAsyncTest() { true } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-3.0" - } } class JettyServlet3TestFakeAsync extends JettyServlet3Test { @@ -164,13 +157,6 @@ class JettyServlet3TestFakeAsync extends JettyServlet3Test { Class servlet() { TestServlet3.FakeAsync } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-3.0" - } } class JettyServlet3TestForward extends JettyDispatchTest { @@ -247,13 +233,6 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest { true } - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-3.0" - } - @Override protected void setupServlets(ServletContextHandler context) { super.setupServlets(context) @@ -283,13 +262,6 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest { true } - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-3.0" - } - @Override protected void setupServlets(ServletContextHandler context) { super.setupServlets(context) diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy index d71f8ab8eab6..6b547b3dab93 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5Test.groovy @@ -123,13 +123,6 @@ class JettyServlet5TestAsync extends JettyServlet5Test { boolean errorEndpointUsesSendError() { false } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-5.0" - } } @IgnoreIf({ !jvm.java11Compatible }) @@ -139,13 +132,6 @@ class JettyServlet5TestFakeAsync extends JettyServlet5Test { Class servlet() { TestServlet5.FakeAsync } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-5.0" - } } @IgnoreIf({ !jvm.java11Compatible }) @@ -237,13 +223,6 @@ class JettyServlet5TestDispatchImmediate extends JettyDispatchTest { addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet5.DispatchImmediate) addServlet(context, "/dispatch/recursive", TestServlet5.DispatchRecursive) } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-5.0" - } } @IgnoreIf({ !jvm.java11Compatible }) @@ -275,13 +254,6 @@ class JettyServlet5TestDispatchAsync extends JettyDispatchTest { boolean errorEndpointUsesSendError() { false } - - @Override - String getMetricsInstrumentationName() { - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - "io.opentelemetry.servlet-5.0" - } } abstract class JettyDispatchTest extends JettyServlet5Test { diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java index 8aa9339e7c6b..10b221f613a1 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java @@ -14,6 +14,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder; import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerAttributesExtractor; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerAttributesGetter; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerMetrics; @@ -31,6 +32,8 @@ private ServletInstrumenterBuilder() {} private final List>> contextCustomizers = new ArrayList<>(); + private boolean propagateOperationListenersToOnEnd; + public static ServletInstrumenterBuilder create() { return new ServletInstrumenterBuilder<>(); } @@ -42,6 +45,12 @@ public ServletInstrumenterBuilder addContextCustomizer( return this; } + @CanIgnoreReturnValue + public ServletInstrumenterBuilder propagateOperationListenersToOnEnd() { + propagateOperationListenersToOnEnd = true; + return this; + } + public Instrumenter, ServletResponseContext> build( String instrumentationName, ServletAccessor accessor, @@ -85,6 +94,9 @@ public Instrumenter, ServletResponseContext(accessor)); } diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.java index e8863716d47e..651308f9634b 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.java +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatAsyncTest.java @@ -113,10 +113,6 @@ protected void configure(HttpServerTestOptions options) { options.setExpectedException(new ServletException(EXCEPTION.getBody())); options.setHasResponseSpan(endpoint -> endpoint == NOT_FOUND || endpoint == REDIRECT); - - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - options.setMetricsInstrumentationName(() -> "io.opentelemetry.servlet-5.0"); } @Override diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.java index a249300a5689..3e779cd3ef0e 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.java +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatAsyncTest.java @@ -108,10 +108,6 @@ protected void configure(HttpServerTestOptions options) { options.setExpectedException(new ServletException(EXCEPTION.getBody())); options.setHasResponseSpan(endpoint -> endpoint == NOT_FOUND || endpoint == REDIRECT); - - // with async requests the span is started in one instrumentation (server instrumentation) - // but ended from another (servlet instrumentation) - options.setMetricsInstrumentationName(() -> "io.opentelemetry.servlet-3.0"); } @Override diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java index 24f974f4c3ca..b89d70bc77d3 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java @@ -10,6 +10,7 @@ import io.opentelemetry.instrumentation.api.incubator.semconv.http.HttpServerExperimentalMetrics; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder; +import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerAttributesExtractor; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerMetrics; import io.opentelemetry.instrumentation.api.semconv.http.HttpServerRoute; @@ -61,6 +62,7 @@ public static Instrumenter create( .addAttributesExtractor(HttpExperimentalAttributesExtractor.create(httpAttributesGetter)) .addOperationMetrics(HttpServerExperimentalMetrics.get()); } + InstrumenterUtil.propagateOperationListenersToOnEnd(builder); return builder.buildServerInstrumenter(TomcatRequestGetter.INSTANCE); } } diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index fdade11a389d..54bcba8bbc26 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -173,10 +173,6 @@ abstract class HttpServerTest extends InstrumentationSpecification imple return true } - String getMetricsInstrumentationName() { - null - } - /** A list of additional HTTP server span attributes extracted by the instrumentation per URI. */ Set> httpAttributes(ServerEndpoint endpoint) { [ @@ -237,9 +233,6 @@ abstract class HttpServerTest extends InstrumentationSpecification imple options.sockPeerAddr = { endpoint -> HttpServerTest.this.sockPeerAddr(endpoint) } - options.metricsInstrumentationName = { - HttpServerTest.this.getMetricsInstrumentationName() - } options.responseCodeOnNonStandardHttpMethod = getResponseCodeOnNonStandardHttpMethod() options.testRedirect = testRedirect() diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java index 3246bad915f7..74c9e6c67af2 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java @@ -353,12 +353,8 @@ void httpServerMetrics() { spanData -> assertServerSpan(assertThat(spanData), method, SUCCESS, SUCCESS.status)); }); - String metricsInstrumentationName = options.metricsInstrumentationName.get(); - if (metricsInstrumentationName == null) { - metricsInstrumentationName = instrumentationName.get(); - } testing.waitAndAssertMetrics( - metricsInstrumentationName, + instrumentationName.get(), "http.server.request.duration", metrics -> metrics.anySatisfy( diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpServerTestOptions.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpServerTestOptions.java index ad0700b83359..970184967589 100644 --- a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpServerTestOptions.java +++ b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpServerTestOptions.java @@ -19,7 +19,6 @@ import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Predicate; -import java.util.function.Supplier; import javax.annotation.Nullable; public final class HttpServerTestOptions { @@ -42,7 +41,6 @@ public final class HttpServerTestOptions { Function sockPeerAddr = unused -> "127.0.0.1"; String contextPath = ""; Throwable expectedException = new Exception(EXCEPTION.body); - Supplier metricsInstrumentationName = () -> null; // we're calling /success in the test, and most servers respond with 200 anyway int responseCodeOnNonStandardHttpMethod = ServerEndpoint.SUCCESS.status; @@ -108,13 +106,6 @@ public HttpServerTestOptions setExpectedException(Throwable expectedException) { return this; } - @CanIgnoreReturnValue - public HttpServerTestOptions setMetricsInstrumentationName( - Supplier metricsInstrumentationName) { - this.metricsInstrumentationName = metricsInstrumentationName; - return this; - } - @CanIgnoreReturnValue public HttpServerTestOptions setResponseCodeOnNonStandardHttpMethod( int responseCodeOnNonStandardHttpMethod) {