Changed instrumentation tracer" TRACER.startSpan() to never return `null` (#499)

agent-bootstrap/src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpServerTracer.java

@@ -51,10 +51,6 @@ public abstract class HttpServerTracer<REQUEST> {
   }
 
   public Span startSpan(REQUEST request, Method origin, String originType) {
-    if (getAttachedSpan(request) != null) {
-      return null;
-    }
-
     final Span.Builder builder =
         tracer
             .spanBuilder(spanNameForMethod(origin))
@@ -132,10 +128,6 @@ public abstract class HttpServerTracer<REQUEST> {
     // TODO set resource name from URL.
   }
 
-  public boolean sameTrace(Span oneSpan, Span otherSpan) {
-    return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId());
-  }
-
   /**
    * This method is used to generate an acceptable span (operation) name based on a given method
    * reference. Anonymous classes are named based on their parent.

instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpHandlerInstrumentation.java

@@ -79,6 +79,10 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default {
         @Advice.Argument(0) final Request request,
         @Advice.Local("otelSpan") Span span,
         @Advice.Local("otelScope") Scope scope) {
+      if (TRACER.getAttachedSpan(request) != null) {
+        return;
+      }
+
       request.addAfterServiceListener(SpanClosingListener.LISTENER);
 
       span = TRACER.startSpan(request, method, null);
@@ -96,10 +100,6 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default {
       }
       scope.close();
 
-      if (span == null) {
-        return;
-      }
-
       if (throwable != null) {
         TRACER.endExceptionally(span, throwable, response.getStatus());
       }
@@ -108,7 +108,6 @@ public class GrizzlyHttpHandlerInstrumentation extends Instrumenter.Default {
 
   public static class SpanClosingListener implements AfterServiceListener {
     public static final SpanClosingListener LISTENER = new SpanClosingListener();
-    public static final GrizzlyHttpServerTracer TRACER = new GrizzlyHttpServerTracer();
 
     @Override
     public void onAfterService(final Request request) {

instrumentation/grizzly-2.0/src/main/java/io/opentelemetry/auto/instrumentation/grizzly/GrizzlyHttpServerTracer.java

@@ -75,11 +75,12 @@ public class GrizzlyHttpServerTracer extends HttpServerTracer<Request> {
   }
 
   @Override
-  protected Span getAttachedSpan(Request request) {
+  public Span getAttachedSpan(Request request) {
     Object span = request.getAttribute(SPAN_ATTRIBUTE);
     return span instanceof Span ? (Span) span : null;
   }
 
+  @Override
   public void onRequest(Span span, Request request) {
     request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16());
     request.setAttribute("spanId", span.getContext().getSpanId().toLowerBase16());

instrumentation/servlet/request-2.3/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v2_3/Servlet2Advice.java

@@ -38,12 +38,16 @@ public class Servlet2Advice {
       @Advice.Local("otelSpan") Span span,
       @Advice.Local("otelScope") Scope scope) {
 
-    if (!(request instanceof HttpServletRequest)) {
+    if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
       return;
     }
 
     final HttpServletRequest httpServletRequest = (HttpServletRequest) request;
 
+    if (TRACER.getAttachedSpan(httpServletRequest) != null) {
+      return;
+    }
+
     // For use by HttpServletResponseInstrumentation:
     InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class)
         .put((HttpServletResponse) response, httpServletRequest);
@@ -64,21 +68,15 @@ public class Servlet2Advice {
     }
     scope.close();
 
-    if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) {
+    TRACER.setPrincipal((HttpServletRequest) request);
-      TRACER.setPrincipal((HttpServletRequest) request);
 
-      if (span == null) {
+    Integer responseStatus =
-        return;
+        InstrumentationContext.get(ServletResponse.class, Integer.class).get(response);
-      }
 
-      Integer responseStatus =
+    if (throwable == null) {
-          InstrumentationContext.get(ServletResponse.class, Integer.class).get(response);
+      TRACER.end(span, responseStatus);
-
+    } else {
-      if (throwable == null) {
+      TRACER.endExceptionally(span, throwable, responseStatus);
-        TRACER.end(span, responseStatus);
-      } else {
-        TRACER.endExceptionally(span, throwable, responseStatus);
-      }
     }
   }
 }

instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Advice.java

@@ -39,30 +39,17 @@ public class Servlet3Advice {
       @Advice.Argument(1) final ServletResponse response,
       @Advice.Local("otelSpan") Span span,
       @Advice.Local("otelScope") Scope scope) {
-    if (!(request instanceof HttpServletRequest)) {
+
+    if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
       return;
     }
 
     final HttpServletRequest httpServletRequest = (HttpServletRequest) request;
-    final Span existingSpan = TRACER.getAttachedSpan(httpServletRequest);
-    if (existingSpan != null) {
-      /*
-      Given request already has a span associated with it.
-      As there should not be nested spans of kind SERVER, we should NOT create a new span here.
 
-      But it may happen that there is no span in current Context or it is from a different trace.
+    Span attachedSpan = TRACER.getAttachedSpan(httpServletRequest);
-      E.g. in case of async servlet request processing we create span for incoming request in one thread,
+    if (attachedSpan != null) {
-      but actual request continues processing happens in another thread.
+      if (TRACER.needsRescoping(attachedSpan)) {
-      Depending on servlet container implementation, this processing may again arrive into this method.
+        scope = currentContextWith(attachedSpan);
-      E.g. Jetty handles async requests in a way that calls HttpServlet.service method twice.
-
-      In this case we have to put the span from the request into current context before continuing.
-      */
-      final boolean spanContextWasLost = !TRACER.sameTrace(TRACER.getCurrentSpan(), existingSpan);
-      if (spanContextWasLost) {
-        // Put span from request attribute into current context.
-        // We did not create a new span here, so return null instead
-        scope = currentContextWith(existingSpan);
       }
       // We are inside nested servlet/filter, don't create new span
       return;
@@ -88,34 +75,33 @@ public class Servlet3Advice {
     }
     scope.close();
 
-    if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) {
+    if (span == null) {
-      TRACER.setPrincipal((HttpServletRequest) request);
+      // an existing span was found
+      return;
+    }
 
-      if (throwable != null) {
+    TRACER.setPrincipal((HttpServletRequest) request);
-        TRACER.endExceptionally(span, throwable, ((HttpServletResponse) response).getStatus());
+
-        return;
+    if (throwable != null) {
+      TRACER.endExceptionally(span, throwable, ((HttpServletResponse) response).getStatus());
+      return;
+    }
+
+    final AtomicBoolean responseHandled = new AtomicBoolean(false);
+
+    // In case of async servlets wait for the actual response to be ready
+    if (request.isAsyncStarted()) {
+      try {
+        request.getAsyncContext().addListener(new TagSettingAsyncListener(responseHandled, span));
+      } catch (final IllegalStateException e) {
+        // org.eclipse.jetty.server.Request may throw an exception here if request became
+        // finished after check above. We just ignore that exception and move on.
       }
+    }
 
-      if (span == null) {
+    // Check again in case the request finished before adding the listener.
-        return;
+    if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) {
-      }
+      TRACER.end(span, ((HttpServletResponse) response).getStatus());
-
-      final AtomicBoolean responseHandled = new AtomicBoolean(false);
-
-      // In case of async servlets wait for the actual response to be ready
-      if (request.isAsyncStarted()) {
-        try {
-          request.getAsyncContext().addListener(new TagSettingAsyncListener(responseHandled, span));
-        } catch (final IllegalStateException e) {
-          // org.eclipse.jetty.server.Request may throw an exception here if request became
-          // finished after check above. We just ignore that exception and move on.
-        }
-      }
-
-      // Check again in case the request finished before adding the listener.
-      if (!request.isAsyncStarted() && responseHandled.compareAndSet(false, true)) {
-        TRACER.end(span, ((HttpServletResponse) response).getStatus());
-      }
     }
   }
 }

instrumentation/servlet/request-3.0/src/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java

@@ -29,6 +29,7 @@ import lombok.extern.slf4j.Slf4j;
 public class Servlet3HttpServerTracer extends ServletHttpServerTracer {
   public static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer();
 
+  @Override
   protected String getInstrumentationName() {
     return "io.opentelemetry.auto.servlet-3.0";
   }
@@ -44,6 +45,22 @@ public class Servlet3HttpServerTracer extends ServletHttpServerTracer {
     span.end();
   }
 
+  /*
+  Given request already has a span associated with it.
+  As there should not be nested spans of kind SERVER, we should NOT create a new span here.
+
+  But it may happen that there is no span in current Context or it is from a different trace.
+  E.g. in case of async servlet request processing we create span for incoming request in one thread,
+  but actual request continues processing happens in another thread.
+  Depending on servlet container implementation, this processing may again arrive into this method.
+  E.g. Jetty handles async requests in a way that calls HttpServlet.service method twice.
+
+  In this case we have to put the span from the request into current context before continuing.
+  */
+  public boolean needsRescoping(Span attachedSpan) {
+    return !sameTrace(tracer.getCurrentSpan(), attachedSpan);
+  }
+
   @Override
   public void onRequest(Span span, HttpServletRequest request) {
     super.onRequest(span, request);
@@ -75,4 +92,8 @@ public class Servlet3HttpServerTracer extends ServletHttpServerTracer {
       }
     }
   }
+
+  private static boolean sameTrace(final Span oneSpan, final Span otherSpan) {
+    return oneSpan.getContext().getTraceId().equals(otherSpan.getContext().getTraceId());
+  }
 }

instrumentation/servlet/servlet-common/src/main/java/io/opentelemetry/auto/instrumentation/servlet/ServletHttpServerTracer.java

@@ -29,6 +29,7 @@ import lombok.extern.slf4j.Slf4j;
 @Slf4j
 public abstract class ServletHttpServerTracer extends HttpServerTracer<HttpServletRequest> {
 
+  @Override
   protected String getVersion() {
     return null;
   }
@@ -73,6 +74,7 @@ public abstract class ServletHttpServerTracer extends HttpServerTracer<HttpServl
     return request.getMethod();
   }
 
+  @Override
   public void onRequest(Span span, HttpServletRequest request) {
     // we do this e.g. so that servlet containers can use these values in their access logs
     request.setAttribute("traceId", span.getContext().getTraceId().toLowerBase16());
@@ -89,6 +91,7 @@ public abstract class ServletHttpServerTracer extends HttpServerTracer<HttpServl
     return HttpServletRequestGetter.GETTER;
   }
 
+  @Override
   protected Throwable unwrapThrowable(Throwable throwable) {
     Throwable result = throwable;
     if (throwable instanceof ServletException && throwable.getCause() != null) {