Refactor ServerSpanNaming (in preparation for http.route) (#4852)

* Refactor ServerSpanNaming (in preparation for http.route) * fix tests * Add ServerSpanNaming to all HTTP server instrumentations * fix tests
open-telemetry · Dec 14, 2021 · a65e963 · a65e963
1 parent 331ce28
commit a65e963
Show file tree

Hide file tree

Showing 20 changed files with 104 additions and 265 deletions.
diff --git a/...tion-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/...tion-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java
@@ -8,6 +8,7 @@
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.ContextKey;
+import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
 import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
 import javax.annotation.Nullable;
 
@@ -17,14 +18,13 @@ public final class ServerSpanNaming {
   private static final ContextKey<ServerSpanNaming> CONTEXT_KEY =
       ContextKey.named("opentelemetry-servlet-span-naming-key");
 
-  public static Context init(Context context, Source initialSource) {
-    ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
-    if (serverSpanNaming != null) {
-      // TODO (trask) does this ever happen?
-      serverSpanNaming.updatedBySource = initialSource;
-      return context;
-    }
-    return context.with(CONTEXT_KEY, new ServerSpanNaming(initialSource));
+  public static <REQUEST> ContextCustomizer<REQUEST> get() {
+    return (context, request, startAttributes) -> {
+      if (context.get(CONTEXT_KEY) != null) {
+        return context;
+      }
+      return context.with(CONTEXT_KEY, new ServerSpanNaming(Source.CONTAINER));
+    };
   }
 
   private volatile Source updatedBySource;
@@ -37,34 +37,31 @@ private ServerSpanNaming(Source initialSource) {
   }
 
   /**
-   * If there is a server span in the context, and {@link #init(Context, Source)} has been called to
-   * populate a {@code ServerSpanName} into the context, then this method will update the server
-   * span name using the provided {@link ServerSpanNameSupplier} if and only if the last {@link
-   * Source} to update the span name using this method has strictly lower priority than the provided
-   * {@link Source}, and the value returned from the {@link ServerSpanNameSupplier} is non-null.
+   * If there is a server span in the context, and the context has been customized with a {@code
+   * ServerSpanName}, then this method will update the server span name using the provided {@link
+   * ServerSpanNameSupplier} if and only if the last {@link Source} to update the span name using
+   * this method has strictly lower priority than the provided {@link Source}, and the value
+   * returned from the {@link ServerSpanNameSupplier} is non-null.
    *
-   * <p>If there is a server span in the context, and {@link #init(Context, Source)} has NOT been
-   * called to populate a {@code ServerSpanName} into the context, then this method will update the
-   * server span name using the provided {@link ServerSpanNameSupplier} if the value returned from
-   * it is non-null.
+   * <p>If there is a server span in the context, and the context has NOT been customized with a
+   * {@code ServerSpanName}, then this method will update the server span name using the provided
+   * {@link ServerSpanNameSupplier} if the value returned from it is non-null.
    */
   public static <T> void updateServerSpanName(
       Context context, Source source, ServerSpanNameSupplier<T> serverSpanName, T arg1) {
     updateServerSpanName(context, source, OneArgAdapter.getInstance(), arg1, serverSpanName);
   }
 
   /**
-   * If there is a server span in the context, and {@link #init(Context, Source)} has been called to
-   * populate a {@code ServerSpanName} into the context, then this method will update the server
-   * span name using the provided {@link ServerSpanNameTwoArgSupplier} if and only if the last
-   * {@link Source} to update the span name using this method has strictly lower priority than the
-   * provided {@link Source}, and the value returned from the {@link ServerSpanNameTwoArgSupplier}
-   * is non-null.
+   * If there is a server span in the context, and the context has been customized with a {@code
+   * ServerSpanName}, then this method will update the server span name using the provided {@link
+   * ServerSpanNameTwoArgSupplier} if and only if the last {@link Source} to update the span name
+   * using this method has strictly lower priority than the provided {@link Source}, and the value
+   * returned from the {@link ServerSpanNameTwoArgSupplier} is non-null.
    *
-   * <p>If there is a server span in the context, and {@link #init(Context, Source)} has NOT been
-   * called to populate a {@code ServerSpanName} into the context, then this method will update the
-   * server span name using the provided {@link ServerSpanNameTwoArgSupplier} if the value returned
-   * from it is non-null.
+   * <p>If there is a server span in the context, and the context has NOT been customized with a
+   * {@code ServerSpanName}, then this method will update the server span name using the provided
+   * {@link ServerSpanNameTwoArgSupplier} if the value returned from it is non-null.
    */
   public static <T, U> void updateServerSpanName(
       Context context,

diff --git a/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java b/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java
@@ -11,6 +11,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.instrumentation.akkahttp.AkkaHttpUtil;
 
 public class AkkaHttpServerSingletons {
@@ -28,6 +29,7 @@ public class AkkaHttpServerSingletons {
             .setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesExtractor))
             .addAttributesExtractor(httpAttributesExtractor)
             .addRequestMetrics(HttpServerMetrics.get())
+            .addContextCustomizer(ServerSpanNaming.get())
             .newServerInstrumenter(AkkaHttpServerHeaders.INSTANCE);
   }
 

diff --git a/...ry/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTracingBuilder.java b/...ry/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTracingBuilder.java
@@ -21,6 +21,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.ArrayList;
 import java.util.List;
@@ -135,7 +136,8 @@ public ArmeriaTracing build() {
                 HttpSpanStatusExtractor.create(serverAttributesExtractor)))
         .addAttributesExtractor(new ArmeriaNetServerAttributesExtractor())
         .addAttributesExtractor(serverAttributesExtractor)
-        .addRequestMetrics(HttpServerMetrics.get());
+        .addRequestMetrics(HttpServerMetrics.get())
+        .addContextCustomizer(ServerSpanNaming.get());
 
     if (peerService != null) {
       clientInstrumenterBuilder.addAttributesExtractor(

diff --git a/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlySingletons.java b/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlySingletons.java
@@ -32,10 +32,8 @@ public final class GrizzlySingletons {
             .addAttributesExtractor(netAttributesExtractor)
             .addRequestMetrics(HttpServerMetrics.get())
             .addContextCustomizer(
-                (context, httpRequestPacket, startAttributes) -> {
-                  context = GrizzlyErrorHolder.init(context);
-                  return ServerSpanNaming.init(context, ServerSpanNaming.Source.CONTAINER);
-                })
+                (context, httpRequestPacket, startAttributes) -> GrizzlyErrorHolder.init(context))
+            .addContextCustomizer(ServerSpanNaming.get())
             .newServerInstrumenter(HttpRequestHeadersGetter.INSTANCE);
   }
 

diff --git a/...c/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java b/...c/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java
@@ -5,10 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.jetty.v11_0;
 
-import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;
-
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
-import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
 import io.opentelemetry.javaagent.instrumentation.jetty.common.JettyHelper;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
@@ -26,10 +23,7 @@ public final class Jetty11Singletons {
       INSTRUMENTER =
           ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
               .addContextCustomizer(
-                  (context, request, attributes) -> {
-                    context = ServerSpanNaming.init(context, CONTAINER);
-                    return new AppServerBridge.Builder().init(context);
-                  })
+                  (context, request, attributes) -> new AppServerBridge.Builder().init(context))
               .build(INSTRUMENTATION_NAME, Servlet5Accessor.INSTANCE);
 
   private static final JettyHelper<HttpServletRequest, HttpServletResponse> HELPER =

diff --git a/...src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java b/...src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java
@@ -5,10 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.jetty.v8_0;
 
-import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;
-
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
-import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
 import io.opentelemetry.javaagent.instrumentation.jetty.common.JettyHelper;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
@@ -26,10 +23,7 @@ public final class Jetty8Singletons {
       INSTRUMENTER =
           ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
               .addContextCustomizer(
-                  (context, request, attributes) -> {
-                    context = ServerSpanNaming.init(context, CONTAINER);
-                    return new AppServerBridge.Builder().init(context);
-                  })
+                  (context, request, attributes) -> new AppServerBridge.Builder().init(context))
               .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);
 
   private static final JettyHelper<HttpServletRequest, HttpServletResponse> HELPER =

diff --git a/...entelemetry/javaagent/instrumentation/liberty/dispatcher/LibertyDispatcherSingletons.java b/...entelemetry/javaagent/instrumentation/liberty/dispatcher/LibertyDispatcherSingletons.java
@@ -5,8 +5,6 @@
 
 package io.opentelemetry.javaagent.instrumentation.liberty.dispatcher;
 
-import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;
-
 import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
@@ -39,8 +37,7 @@ public final class LibertyDispatcherSingletons {
             .setSpanStatusExtractor(spanStatusExtractor)
             .addAttributesExtractor(httpAttributesExtractor)
             .addAttributesExtractor(netAttributesExtractor)
-            .addContextCustomizer(
-                (context, request, attributes) -> ServerSpanNaming.init(context, CONTAINER))
+            .addContextCustomizer(ServerSpanNaming.get())
             .addRequestMetrics(HttpServerMetrics.get())
             .newServerInstrumenter(LibertyDispatcherRequestGetter.INSTANCE);
   }

diff --git a/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java b/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java
@@ -5,10 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.liberty;
 
-import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER;
-
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
-import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletRequestContext;
@@ -25,10 +22,8 @@ public final class LibertySingletons {
       INSTRUMENTER =
           ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
               .addContextCustomizer(
-                  (context, request, attributes) -> {
-                    context = ServerSpanNaming.init(context, CONTAINER);
-                    return new AppServerBridge.Builder().recordException().init(context);
-                  })
+                  (context, request, attributes) ->
+                      new AppServerBridge.Builder().recordException().init(context))
               .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);
 
   private static final LibertyHelper<HttpServletRequest, HttpServletResponse> HELPER =

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerSingletons.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/NettyServerSingletons.java
@@ -10,6 +10,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.instrumentation.netty.common.NettyErrorHolder;
 import io.opentelemetry.javaagent.instrumentation.netty.v3_8.HttpRequestAndChannel;
 import org.jboss.netty.handler.codec.http.HttpResponse;
@@ -33,6 +34,7 @@ final class NettyServerSingletons {
             .addRequestMetrics(HttpServerMetrics.get())
             .addContextCustomizer(
                 (context, requestAndChannel, startAttributes) -> NettyErrorHolder.init(context))
+            .addContextCustomizer(ServerSpanNaming.get())
             .newServerInstrumenter(NettyHeadersGetter.INSTANCE);
   }
 

diff --git a/...lemetry/javaagent/instrumentation/netty/common/server/NettyServerInstrumenterFactory.java b/...lemetry/javaagent/instrumentation/netty/common/server/NettyServerInstrumenterFactory.java
@@ -31,12 +31,8 @@ public static Instrumenter<HttpRequestAndChannel, HttpResponse> create(
         .addAttributesExtractor(httpAttributesExtractor)
         .addAttributesExtractor(new NettyNetServerAttributesExtractor())
         .addRequestMetrics(HttpServerMetrics.get())
-        .addContextCustomizer(
-            (context, request, attributes) -> {
-              context = NettyErrorHolder.init(context);
-              // netty is not exactly a "container", but it's the best match out of these
-              return ServerSpanNaming.init(context, ServerSpanNaming.Source.CONTAINER);
-            })
+        .addContextCustomizer((context, request, attributes) -> NettyErrorHolder.init(context))
+        .addContextCustomizer(ServerSpanNaming.get())
         .newServerInstrumenter(HttpRequestHeadersGetter.INSTANCE);
   }
 

diff --git a/...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Singletons.java b/...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Singletons.java
@@ -5,12 +5,9 @@
 
 package io.opentelemetry.javaagent.instrumentation.servlet.v2_2;
 
-import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET;
-
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesExtractor;
-import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletInstrumenterBuilder;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletRequestContext;
 import io.opentelemetry.javaagent.instrumentation.servlet.ServletResponseContext;
@@ -33,8 +30,6 @@ public final class Servlet2Singletons {
             ServletRequestContext<HttpServletRequest>, ServletResponseContext<HttpServletResponse>>
         instrumenter =
             ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
-                .addContextCustomizer(
-                    (context, request, attributes) -> ServerSpanNaming.init(context, SERVLET))
                 .build(
                     INSTRUMENTATION_NAME,
                     Servlet2Accessor.INSTANCE,

diff --git a/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java b/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
@@ -9,7 +9,6 @@
 
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
-import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
 import io.opentelemetry.javaagent.bootstrap.servlet.AppServerBridge;
 import io.opentelemetry.javaagent.bootstrap.servlet.MappingResolver;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
@@ -39,52 +38,41 @@ public static void onEnter(
     if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
       return;
     }
+    HttpServletRequest httpServletRequest = (HttpServletRequest) request;
 
     callDepth = CallDepth.forClass(AppServerBridge.getCallDepthKey());
     callDepth.getAndIncrement();
 
-    HttpServletRequest httpServletRequest = (HttpServletRequest) request;
-
     Context currentContext = Java8BytecodeBridge.currentContext();
     Context attachedContext = helper().getServerContext(httpServletRequest);
-    if (attachedContext != null && helper().needsRescoping(currentContext, attachedContext)) {
-      MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
-      boolean servlet = servletOrFilter instanceof Servlet;
-      attachedContext =
-          helper().updateContext(attachedContext, httpServletRequest, mappingResolver, servlet);
-      scope = attachedContext.makeCurrent();
-      // We are inside nested servlet/filter/app-server span, don't create new span
-      return;
-    }
-
-    if (attachedContext != null || ServerSpan.fromContextOrNull(currentContext) != null) {
-      // Update context with info from current request to ensure that server span gets the best
-      // possible name.
-      // In case server span was created by app server instrumentations calling updateContext
-      // returns a new context that contains servlet context path that is used in other
-      // instrumentations for naming server span.
-      MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
-      boolean servlet = servletOrFilter instanceof Servlet;
-      Context updatedContext =
-          helper().updateContext(currentContext, httpServletRequest, mappingResolver, servlet);
-      if (currentContext != updatedContext) {
-        // updateContext updated context, need to re-scope
-        scope = updatedContext.makeCurrent();
-      }
-      // We are inside nested servlet/filter/app-server span, don't create new span
-      return;
-    }
+    Context contextToUpdate;
 
     requestContext = new ServletRequestContext<>(httpServletRequest, servletOrFilter);
-
-    if (!helper().shouldStart(currentContext, requestContext)) {
-      return;
+    if (attachedContext == null && helper().shouldStart(currentContext, requestContext)) {
+      context = helper().start(currentContext, requestContext);
+      helper().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response);
+
+      contextToUpdate = context;
+    } else if (attachedContext != null
+        && helper().needsRescoping(currentContext, attachedContext)) {
+      // Given request already has a context associated with it.
+      // see the needsRescoping() javadoc for more explanation
+      contextToUpdate = attachedContext;
+    } else {
+      // We are inside nested servlet/filter/app-server span, don't create new span
+      contextToUpdate = currentContext;
     }
 
-    context = helper().start(currentContext, requestContext);
-    scope = context.makeCurrent();
-
-    helper().setAsyncListenerResponse(httpServletRequest, (HttpServletResponse) response);
+    // Update context with info from current request to ensure that server span gets the best
+    // possible name.
+    // In case server span was created by app server instrumentations calling updateContext
+    // returns a new context that contains servlet context path that is used in other
+    // instrumentations for naming server span.
+    MappingResolver mappingResolver = Servlet3Singletons.getMappingResolver(servletOrFilter);
+    boolean servlet = servletOrFilter instanceof Servlet;
+    contextToUpdate =
+        helper().updateContext(contextToUpdate, httpServletRequest, mappingResolver, servlet);
+    scope = contextToUpdate.makeCurrent();
   }
 
   @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)