Autoinstrumentation for Struts2 actions (#1628)

* Struts PoC

* Instrument ActionInvocation to get correct timings. Refactor to use latest o11y SDK and API.

* Fix license header.

* Revert accidental Gradle configuration change.

* First passing test for struts instrumentation.

* Cleanup

* Convert Struts test to HttpServerTest.

* Fix Spotless formatting issues.

* Manage scope properly in Struts advice. Use existing handlerSpan method to assert struts handler span detection. Make Struts controller behave similarly with other HttpServerTest implementations.

* Rename method.

* Update server span name when path params are used.

* Struts PoC

* Instrument ActionInvocation to get correct timings. Refactor to use latest o11y SDK and API.

* Fix license header.

* Revert accidental Gradle configuration change.

* First passing test for struts instrumentation.

* Cleanup

* Convert Struts test to HttpServerTest.

* Fix Spotless formatting issues.

* Manage scope properly in Struts advice. Use existing handlerSpan method to assert struts handler span detection. Make Struts controller behave similarly with other HttpServerTest implementations.

* Rename method.

* Update server span name when path params are used.

* Account for GStrings in asserted values.

* Use Groovy friendly Assert.

* Giving up on getting to work user-friendly assertion messages. Moving controller to a package, as ognl inside struts can't handle classes with no packagaes.

* Make codeNarc happy.

* Make spotless happy.

* Rename struts-2 to struts-2.3. Move autoinstrumentation to javaagent sub-folder to accommodate for library.

* Use tracer() instead of TRACER and other minor tweaks.

* Nicer way for asserting values returned from a method call.

* Fix formatting.

instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/ActionInvocationAdvice.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.struts2;
+
+import static io.opentelemetry.javaagent.instrumentation.struts2.Struts2Tracer.tracer;
+
+import com.opensymphony.xwork2.ActionInvocation;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Scope;
+import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
+import net.bytebuddy.asm.Advice;
+
+public class ActionInvocationAdvice {
+
+  @Advice.OnMethodEnter(suppress = Throwable.class)
+  public static void onEnter(
+      @Advice.This ActionInvocation actionInvocation,
+      @Advice.Local("otelSpan") Span span,
+      @Advice.Local("otelScope") Scope scope) {
+    span = tracer().startSpan(actionInvocation);
+    scope = tracer().startScope(span);
+
+    tracer()
+        .updateServerSpanName(Java8BytecodeBridge.currentContext(), actionInvocation.getProxy());
+  }
+
+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+  public static void stopSpan(
+      @Advice.Thrown Throwable throwable,
+      @Advice.Local("otelSpan") Span span,
+      @Advice.Local("otelScope") Scope scope) {
+    if (scope != null) {
+      scope.close();
+    }
+    if (throwable != null) {
+      tracer().endExceptionally(span, throwable);
+    } else {
+      tracer().end(span);
+    }
+  }
+}

instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/ActionInvocationInstrumentation.java

@@ -0,0 +1,39 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.struts2;
+
+import static io.opentelemetry.javaagent.tooling.ClassLoaderMatcher.hasClassesNamed;
+import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.implementsInterface;
+import static java.util.Collections.singletonMap;
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.isPublic;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
+import java.util.Map;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+
+public class ActionInvocationInstrumentation implements TypeInstrumentation {
+
+  @Override
+  public ElementMatcher<ClassLoader> classLoaderMatcher() {
+    return hasClassesNamed("com.opensymphony.xwork2.ActionInvocation");
+  }
+
+  @Override
+  public ElementMatcher<TypeDescription> typeMatcher() {
+    return implementsInterface(named("com.opensymphony.xwork2.ActionInvocation"));
+  }
+
+  @Override
+  public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
+    return singletonMap(
+        isMethod().and(isPublic()).and(named("invokeActionOnly")),
+        ActionInvocationAdvice.class.getName());
+  }
+}

instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2InstrumentationModule.java

@@ -0,0 +1,31 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.struts2;
+
+import static java.util.Collections.singletonList;
+
+import com.google.auto.service.AutoService;
+import io.opentelemetry.javaagent.tooling.InstrumentationModule;
+import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
+import java.util.List;
+
+@AutoService(InstrumentationModule.class)
+public class Struts2InstrumentationModule extends InstrumentationModule {
+
+  public Struts2InstrumentationModule() {
+    super("struts", "struts-2.3");
+  }
+
+  @Override
+  public String[] helperClassNames() {
+    return new String[] {packageName + ".Struts2Tracer"};
+  }
+
+  @Override
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return singletonList(new ActionInvocationInstrumentation());
+  }
+}

instrumentation/struts-2.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/struts2/Struts2Tracer.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.struts2;
+
+import com.opensymphony.xwork2.ActionInvocation;
+import com.opensymphony.xwork2.ActionProxy;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.servlet.ServletContextPath;
+import io.opentelemetry.instrumentation.api.tracer.BaseTracer;
+
+public class Struts2Tracer extends BaseTracer {
+
+  private static final Struts2Tracer TRACER = new Struts2Tracer();
+
+  public static Struts2Tracer tracer() {
+    return TRACER;
+  }
+
+  public Span startSpan(ActionInvocation actionInvocation) {
+    Object action = actionInvocation.getAction();
+    Class<?> actionClass = action.getClass();
+
+    String method = actionInvocation.getProxy().getMethod();
+    String spanName = spanNameForMethod(actionClass, method);
+
+    Span strutsSpan = tracer.spanBuilder(spanName).startSpan();
+
+    strutsSpan.setAttribute("code.namespace", actionClass.getName());
+    if (method != null) {
+      strutsSpan.setAttribute("code.function", method);
+    }
+
+    return strutsSpan;
+  }
+
+  // Handle cases where action parameters are encoded into URL path
+  public void updateServerSpanName(Context context, ActionProxy actionProxy) {
+    Span serverSpan = getCurrentServerSpan();
+    if (serverSpan == null) {
+      return;
+    }
+
+    // We take name from the config, because it contains the path pattern from the
+    // configuration.
+    String result = actionProxy.getConfig().getName();
+
+    String actionNamespace = actionProxy.getNamespace();
+    if (actionNamespace != null && !actionNamespace.isEmpty()) {
+      if (actionNamespace.endsWith("/") || result.startsWith("/")) {
+        result = actionNamespace + result;
+      } else {
+        result = actionNamespace + "/" + result;
+      }
+    }
+
+    if (!result.startsWith("/")) {
+      result = "/" + result;
+    }
+
+    if (!result.contains("{")) {
+      // If there are no braces, then there are no path parameters encoded in
+      // the action name, so let's not change existing server span name, because
+      // path is good enough. Wildcards like * in action name may glue
+      // several endpoints into one action name, which we do not want -- we want
+      // normalize parameters, not actions.
+      return;
+    }
+
+    serverSpan.updateName(ServletContextPath.prepend(context, result));
+  }
+
+  @Override
+  protected String getInstrumentationName() {
+    return "io.opentelemetry.auto.struts-2.3";
+  }
+}

instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy

@@ -0,0 +1,115 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import io.opentelemetry.api.trace.Span
+import io.opentelemetry.instrumentation.test.asserts.TraceAssert
+import io.opentelemetry.instrumentation.test.base.HttpServerTest
+import io.opentelemetry.sdk.trace.data.SpanData
+import javax.servlet.DispatcherType
+import org.apache.struts2.dispatcher.ng.filter.StrutsPrepareAndExecuteFilter
+import org.eclipse.jetty.server.Server
+import org.eclipse.jetty.servlet.DefaultServlet
+import org.eclipse.jetty.servlet.ServletContextHandler
+import org.eclipse.jetty.util.resource.FileResource
+
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
+
+class Struts2ActionSpanTest extends HttpServerTest<Server> {
+
+  @Override
+  boolean testNotFound() {
+    return false
+  }
+
+  @Override
+  boolean testPathParam() {
+    return true
+  }
+
+  @Override
+  boolean testExceptionBody() {
+    return false
+  }
+
+  @Override
+  boolean testError() {
+    return false
+  }
+
+  @Override
+  boolean testRedirect() {
+    return false
+  }
+
+  @Override
+  boolean hasHandlerSpan() {
+    return true
+  }
+
+  String expectedServerSpanName(ServerEndpoint endpoint) {
+    return endpoint == PATH_PARAM ? getContextPath() + "/path/{id}/param" : endpoint.resolvePath(address).path
+  }
+
+  String expectedHandlerName(ServerEndpoint serverEndpoint) {
+    return "GreetingAction." + expectedMethodName(serverEndpoint)
+  }
+
+  String expectedMethodName(ServerEndpoint endpoint) {
+    switch (endpoint) {
+      case QUERY_PARAM: return "query"
+      case EXCEPTION: return "exception"
+      case PATH_PARAM: return "pathParam"
+      default: return "success"
+    }
+  }
+
+  @Override
+  void handlerSpan(TraceAssert trace, int index, Object parent, String method, ServerEndpoint endpoint) {
+    trace.span(index) {
+      name expectedHandlerName(endpoint)
+      kind Span.Kind.INTERNAL
+      errored endpoint == EXCEPTION
+      if (endpoint == EXCEPTION) {
+        errorEvent(Exception, EXCEPTION.body)
+      }
+      def expectedMethodName = expectedMethodName(endpoint)
+      attributes {
+        'code.namespace' "io.opentelemetry.struts.GreetingAction"
+        'code.function' expectedMethodName
+      }
+      childOf((SpanData) parent)
+    }
+  }
+
+  @Override
+  String getContextPath() {
+    return "/context"
+  }
+
+  @Override
+  Server startServer(int port) {
+    def server = new Server(port)
+    ServletContextHandler context = new ServletContextHandler(0)
+    context.setContextPath(getContextPath())
+    def resource = new FileResource(getClass().getResource("/"))
+    context.setBaseResource(resource)
+    server.setHandler(context)
+
+    context.addServlet(DefaultServlet, "/")
+    context.addFilter(StrutsPrepareAndExecuteFilter, "/*", EnumSet.of(DispatcherType.REQUEST))
+
+    server.start()
+
+    return server
+  }
+
+  @Override
+  void stopServer(Server server) {
+    server.stop()
+    server.destroy()
+  }
+}

instrumentation/struts-2.3/javaagent/src/test/java/io/opentelemetry/struts/GreetingAction.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.struts;
+
+import com.opensymphony.xwork2.ActionSupport;
+import io.opentelemetry.instrumentation.test.base.HttpServerTest;
+
+public class GreetingAction extends ActionSupport {
+
+  String responseBody = "default";
+
+  public String success() {
+    responseBody =
+        HttpServerTest.controller(
+            HttpServerTest.ServerEndpoint.SUCCESS, HttpServerTest.ServerEndpoint.SUCCESS::getBody);
+
+    return "greeting";
+  }
+
+  public String query() {
+    responseBody =
+        HttpServerTest.controller(
+            HttpServerTest.ServerEndpoint.QUERY_PARAM,
+            HttpServerTest.ServerEndpoint.QUERY_PARAM::getBody);
+    return "greeting";
+  }
+
+  public String exception() {
+    responseBody =
+        HttpServerTest.controller(
+            HttpServerTest.ServerEndpoint.EXCEPTION,
+            () -> {
+              throw new Exception(HttpServerTest.ServerEndpoint.EXCEPTION.getBody());
+            });
+    return "exception";
+  }
+
+  public String pathParam() {
+    HttpServerTest.controller(
+        HttpServerTest.ServerEndpoint.PATH_PARAM,
+        () ->
+            "this does nothing, as responseBody is set in setId, but we need this controller span nevertheless");
+    return "greeting";
+  }
+
+  public void setId(String id) {
+    responseBody = id;
+  }
+
+  public void setSome(String some) {
+    responseBody = "some=" + some;
+    System.out.println("Setting query param some to " + some);
+  }
+
+  public String getResponseBody() {
+    return responseBody;
+  }
+}

instrumentation/struts-2.3/javaagent/src/test/resources/greeting.ftl

@@ -0,0 +1,2 @@
+<#-- @ftlvariable name="responseBody" type="java.lang.String" -->
+${responseBody}

instrumentation/struts-2.3/javaagent/src/test/resources/struts.xml

@@ -0,0 +1,32 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE struts PUBLIC
+  "-//Apache Software Foundation//DTD Struts Configuration 2.3//EN"
+  "http://struts.apache.org/dtds/struts-2.3.dtd">
+
+<struts>
+
+  <constant name="struts.devMode" value="true" />
+  <constant name="struts.enable.SlashesInActionNames" value="true"/>
+  <constant name="struts.mapper.alwaysSelectFullNamespace" value="false"/>
+  <constant name="struts.patternMatcher" value="namedVariable" />
+
+  <package name="basic-struts2" extends="struts-default">
+    <global-results>
+      <result name="exception" type="httpheader">
+        <param name="error">500</param>
+      </result>
+      <result type="freemarker" name="greeting">greeting.ftl</result>
+    </global-results>
+
+    <global-exception-mappings>
+      <exception-mapping exception="java.lang.Exception" result="exception" />
+    </global-exception-mappings>
+
+    <action name="success" class="io.opentelemetry.struts.GreetingAction" method="success"/>
+    <action name="query" class="io.opentelemetry.struts.GreetingAction" method="query"/>
+    <action name="exception" class="io.opentelemetry.struts.GreetingAction" method="exception"/>
+    <action name="/path/{id}/param" class="io.opentelemetry.struts.GreetingAction" method="pathParam"/>
+  </package>
+
+
+</struts>

instrumentation/struts-2.3/javaagent/struts-2.3-javaagent.gradle

@@ -0,0 +1,25 @@
+apply from: "$rootDir/gradle/instrumentation.gradle"
+
+muzzle {
+  pass {
+    group = "org.apache.struts"
+    module = "struts2-core"
+    versions = "[2.3.20,)"
+  }
+}
+
+dependencies {
+  library group: 'org.apache.struts', name: 'struts2-core', version: '2.3.20'
+
+  testImplementation(project(':testing-common')) {
+    exclude group: 'org.eclipse.jetty', module: 'jetty-server'
+  }
+  testLibrary group: 'org.eclipse.jetty', name: 'jetty-server', version: '8.0.0.v20110901'
+  testLibrary group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '8.0.0.v20110901'
+  testLibrary group: 'javax.servlet', name: 'javax.servlet-api', version: '3.0.1'
+  testLibrary group: 'javax.servlet', name: 'jsp-api', version: '2.0'
+
+  testImplementation project(":instrumentation:servlet:servlet-3.0")
+  testImplementation project(':instrumentation:jetty-8.0')
+}
+

settings.gradle

@@ -188,6 +188,7 @@ include ':instrumentation:spring:starters:jaeger-exporter-starter'
 include ':instrumentation:spring:starters:otlp-exporter-starter'
 include ':instrumentation:spring:starters:zipkin-exporter-starter'
 include ':instrumentation:spymemcached-2.12'
+include ':instrumentation:struts-2.3:javaagent'
 include ':instrumentation:twilio-6.6'
 include ':instrumentation:vertx-3.0'
 include ':instrumentation:vertx-reactive-3.5'

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy

@@ -128,6 +128,14 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
     true
   }
 
+  boolean testRedirect() {
+    true
+  }
+
+  boolean testError() {
+    true
+  }
+
   enum ServerEndpoint {
     SUCCESS("success", 200, "success"),
     REDIRECT("redirect", 302, "/redirected"),
@@ -208,7 +216,7 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
   }
 
   static <T> T controller(ServerEndpoint endpoint, Callable<T> closure) {
-    assert io.opentelemetry.api.trace.Span.current().getSpanContext().isValid(): "Controller should have a parent span."
+    assert Span.current().getSpanContext().isValid(): "Controller should have a parent span."
     if (endpoint == NOT_FOUND) {
       return closure.call()
     }
@@ -278,6 +286,7 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
 
   def "test redirect"() {
     setup:
+    assumeTrue(testRedirect())
     def request = request(REDIRECT, method, body).build()
     def response = client.newCall(request).execute()
 
@@ -297,6 +306,7 @@ abstract class HttpServerTest<SERVER> extends AgentTestRunner {
 
   def "test error"() {
     setup:
+    assumeTrue(testError())
     def request = request(ERROR, method, body).build()
     def response = client.newCall(request).execute()