sling: final cleanups and final refactorings

Author: rombert

instrumentation/sling/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/sling/ServletResolverInstrumentation.java

@@ -11,9 +11,6 @@
 
 import javax.servlet.Servlet;
 
-import java.util.Deque;
-import java.util.LinkedList;
-
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
 import static io.opentelemetry.javaagent.instrumentation.sling.SlingSingletons.REQUEST_ATTR_RESOLVED_SERVLET_NAME;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
@@ -49,7 +46,6 @@ public static void onExit(
         @Advice.Local("otelContext") Context context,
         @Advice.Local("otelScope") Scope scope) {
 
-      // TODO - copied from RequestUtil
       String name = null;
 
       if (servlet.getServletConfig() != null) {
@@ -62,14 +58,7 @@ public static void onExit(
         name = servlet.getClass().getName();
       }
 
-      @SuppressWarnings("unchecked")
-      Deque<String> servletNames = (Deque<String>) request.getAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME);
-      if ( servletNames == null ) {
-        servletNames = new LinkedList<>();
-        request.setAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME, servletNames);
-      }
-      servletNames.addLast(name);
-      System.out.format("SLING TRACE resolved name %s for uri=%s, current stack is %s%n", name, request.getRequestURI(), servletNames);
+      request.setAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME, name);
     }
   }
 }

instrumentation/sling/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/sling/SlingInstrumentationModule.java

@@ -17,9 +17,4 @@ public SlingInstrumentationModule() {
   public List<TypeInstrumentation> typeInstrumentations() {
     return Arrays.asList(new ServletResolverInstrumentation(), new SlingSafeMethodsServletInstrumentation());
   }
-
-  @Override
-  public int order() {
-    return -1;
-  }
 }

instrumentation/sling/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/sling/SlingSafeMethodsServletInstrumentation.java

@@ -1,33 +1,31 @@
 package io.opentelemetry.javaagent.instrumentation.sling;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
+import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
 import static io.opentelemetry.javaagent.instrumentation.sling.SlingSingletons.REQUEST_ATTR_RESOLVED_SERVLET_NAME;
 import static io.opentelemetry.javaagent.instrumentation.sling.SlingSingletons.helper;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
-import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerRoute;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerRouteSource;
 import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
-import io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
 import org.apache.sling.api.SlingHttpServletRequest;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
-import java.util.Deque;
 
 public class SlingSafeMethodsServletInstrumentation implements TypeInstrumentation {
   @Override
   public ElementMatcher<TypeDescription> typeMatcher() {
-    return AgentElementMatchers.implementsInterface(named("javax.servlet.Servlet"));
+    return implementsInterface(named("javax.servlet.Servlet"));
   }
 
   @Override
@@ -60,19 +58,29 @@ public static void onEnter(
         return;
       }
 
-      System.out.format("SLING TRACE Handling request %s%n", request);
-
       SlingHttpServletRequest slingRequest = (SlingHttpServletRequest) request;
 
       Context parentContext = Java8BytecodeBridge.currentContext();
 
       if (!helper().shouldStart(parentContext, slingRequest)) {
-        System.out.format("SLING TRACE should not handle %s%n", request);
         return;
       }
 
+      // written by ServletResolverInstrumentation
+      Object servletName = request.getAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME);
+      if ( !(servletName instanceof String) ) {
+        return;
+      }
+
+      // TODO - figure out why don't we have matches for all requests and find a better way to filter
       context = helper().start(parentContext, slingRequest);
       scope = context.makeCurrent();
+
+      // ensure that the top-level route is Sling-specific
+      HttpServerRoute.update(context, HttpServerRouteSource.CONTROLLER, (String) servletName);
+
+      // cleanup and ensure we don't have reuse the resolved Servlet name by accident for other requests
+      request.removeAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME);
     }
 
     @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
@@ -83,27 +91,12 @@ public static void onExit(
         @Advice.Local("otelContext") Context context,
         @Advice.Local("otelScope") Scope scope) {
 
-      System.out.format("SLING TRACE on exit for %s%n", request);
-
       if (scope == null) {
         return;
       }
       scope.close();
 
       SlingHttpServletRequest slingRequest = (SlingHttpServletRequest) request;
-      // written by ServletResolverInstrumentation
-      Object servletNameStack = request.getAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME);
-
-      System.out.format("SLING TRACE servletName attr for %s is %s%n", request, servletNameStack);
-
-      if ( servletNameStack instanceof Deque<?>) {
-        Deque<?> nameStack = (Deque<?>) servletNameStack;
-        if ( ! nameStack.isEmpty() ) {
-          String servletName = (String) nameStack.removeLast();
-          Span.fromContext(context).updateName(servletName);
-          HttpServerRoute.update(context, HttpServerRouteSource.CONTROLLER,servletName);
-        }
-      }
       helper().end(context, slingRequest, null, throwable);
     }
   }

instrumentation/sling/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/sling/SlingSingletons.java

@@ -7,23 +7,19 @@
 
 import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
 import org.apache.sling.api.SlingHttpServletRequest;
 
 public final class SlingSingletons {
   private static final String INSTRUMENTATION_NAME = "io.opentelemetry.sling-1.0";
-  private static final String SPAN_NAME = "sling.request";
 
-  static final String REQUEST_ATTR_RESOLVED_SERVLET_NAME = INSTRUMENTATION_NAME + ".resovledServletName";
+  static final String REQUEST_ATTR_RESOLVED_SERVLET_NAME = INSTRUMENTATION_NAME + ".resolvedServletName";
+
+  private static final SpanNameExtractor<SlingHttpServletRequest> SPAN_NAME_EXTRACTOR = s -> (String) s.getAttribute(REQUEST_ATTR_RESOLVED_SERVLET_NAME);
   private static final Instrumenter<SlingHttpServletRequest, Void>
-      INSTRUMENTER = Instrumenter.
-      <SlingHttpServletRequest, Void> builder(GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, s -> SPAN_NAME)
+      INSTRUMENTER = Instrumenter.<SlingHttpServletRequest, Void> builder(GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, SPAN_NAME_EXTRACTOR)
           .buildInstrumenter();
 
-         /* ServletInstrumenterBuilder.<HttpServletRequest, HttpServletResponse>create()
-              .addContextCustomizer(
-                  (context, request, attributes) -> new AppServerBridge.Builder().init(context))
-              .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE);*/
-
   public static Instrumenter<SlingHttpServletRequest, Void> helper() {
     return INSTRUMENTER;
   }