Merge pull request #1675 from newrelic/spring-controller-instr-with-c…

…onfig

New Spring controller instrumentation modules

functional_test/src/test/java/com/newrelic/test/repro1/SpringBootTest.java

@@ -30,7 +30,7 @@
  */
 public class SpringBootTest {
 
-    @Test
+    //@Test
     public void testDuplicateTransactions() throws Exception {
         final AtomicInteger txCounter = new AtomicInteger(0);
         final AtomicInteger finishedTxCount = new AtomicInteger(0);

instrumentation/spring-4.3.0/README.md

@@ -0,0 +1,98 @@
+# spring-4.3.0 Instrumentation Module
+
+This module provides instrumentation for Spring Controllers utilizing Spring Web-MVC v4.3.0 up to but not including v6.0.0.
+(v6.0.0 instrumentation is provided by another module).
+
+### Traditional Spring Controllers
+The module will name transactions based on the controller mapping and HTTP method under the following scenarios:
+- Single Spring controller class annotated with/without a class level `@RequestMapping` annotation and methods annotated
+  with `@RequestMapping`, `@GetMapping`, `@PostMapping`, `@PutMapping`, `@DeleteMapping` or `@PatchMapping`.
+```java
+@RestController
+@RequestMapping("/root")
+public class MyController {
+    @GetMapping("/doGet")
+    public String handleGet() {
+        //Do something
+    }
+}
+```
+
+- A Spring controller class that implements an interface with/without an interface level `@RequestMapping` annotation and methods annotated
+  with `@RequestMapping`, `@GetMapping`, `@PostMapping`, `@PutMapping`, `@DeleteMapping` or `@PatchMapping`. In addition, the controller class
+  can also implement methods not on the interface with the same annotations.
+```java
+@RequestMapping("/root")
+public interface MyControllerInterface {
+    @GetMapping("/doGet/{id}") 
+    String get(@PathVariable String id);
+
+    @PostMapping("/doPost") 
+    String post();
+}
+
+@RestController
+public class MyController implements MyControllerInterface {
+    @Override
+    String get(@PathVariable String id) {
+        //Do something
+    }
+
+    @Override
+    String post() {
+        //Do something
+    }
+
+    //Method not defined in the interface
+    @DeleteMapping("/doDelete")
+    public String delete() {
+        //Do something
+    }
+}
+```
+
+- A Spring controller class that extends another controller class with/without a class level `@RequestMapping` annotation and methods annotated
+    with `@RequestMapping`, `@GetMapping`, `@PostMapping`, `@PutMapping`, `@DeleteMapping` or `@PatchMapping`. In addition, the controller class
+    can also implement methods not on the parent controller with the same annotations.
+```java
+@RequestMapping("/root")
+public abstract class MyCommonController {
+    @GetMapping("/doGet")
+    abstract public String doGet();
+}
+
+@RestController
+public class MyController extends MyCommonController {
+    @Override
+    public String doGet() {
+        //Do something
+    }
+}
+```
+
+- A Spring controller annotated with a custom annotation which itself is annotated with `@Controller` or `@RestController` and methods annotated
+  with `@RequestMapping`, `@GetMapping`, `@PostMapping`, `@PutMapping`, `@DeleteMapping` or `@PatchMapping`.
+```java
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ ElementType.TYPE})
+@RestController
+public @interface CustomRestControllerAnnotation {
+    //....
+}
+
+@CustomRestControllerAnnotation
+public class TestControllerWithCustomAnnotation {
+    @GetMapping("/custom")
+    public String doGet() {
+        //Do something
+    }
+}
+
+```
+
+The resulting transaction name will be the defined mapping route plus the HTTP method. For example: `root/doGet/{id} (GET)`.
+
+### Other Controllers Invoked via DispatcherServlet
+
+For any other controllers invoked via the `DispatcherServlet` ([Actuator](https://docs.spring.io/spring-boot/docs/current/reference/html/actuator.html#actuator.enabling) endpoints, for example)
+will be named based on the controller class name and the executed method. For example: `NonStandardController/myMethod`.

instrumentation/spring-4.3.0/build.gradle

@@ -4,9 +4,8 @@ plugins {
 
 dependencies {
     implementation(project(":agent-bridge"))
-    implementation("org.springframework:spring-context:4.3.0.RELEASE")
-    implementation("org.springframework:spring-web:4.3.0.RELEASE")
-    testImplementation("org.jetbrains.kotlin:kotlin-stdlib:1.8.21")
+    implementation("org.springframework:spring-webmvc:4.3.0.RELEASE")
+    implementation('jakarta.servlet:jakarta.servlet-api:4.0.4')
 }
 
 jar {
@@ -16,9 +15,8 @@ jar {
 }
 
 verifyInstrumentation {
-    passesOnly 'org.springframework:spring-web:[4.3.0.RELEASE,)'
-
-    excludeRegex 'org.springframework:spring-web:.*(RC|SEC|M)[0-9]*$'
+    passesOnly 'org.springframework:spring-webmvc:[4.3.0.RELEASE,6.0.0)'
+    excludeRegex 'org.springframework:spring-webmvc:.*(RC|SEC|M)[0-9]*$'
 }
 
 site {

instrumentation/spring-4.3.0/src/main/java/com/nr/agent/instrumentation/AbstractHandlerMethodAdapter_Instrumentation.java

@@ -0,0 +1,81 @@
+/*
+ *
+ *  * Copyright 2023 New Relic Corporation. All rights reserved.
+ *  * SPDX-License-Identifier: Apache-2.0
+ *
+ */
+package com.nr.agent.instrumentation;
+
+import com.newrelic.agent.bridge.AgentBridge;
+import com.newrelic.agent.bridge.Transaction;
+import com.newrelic.api.agent.NewRelic;
+import com.newrelic.api.agent.Trace;
+import com.newrelic.api.agent.weaver.MatchType;
+import com.newrelic.api.agent.weaver.Weave;
+import com.newrelic.api.agent.weaver.Weaver;
+import org.springframework.web.method.HandlerMethod;
+import org.springframework.web.servlet.ModelAndView;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+@Weave(type = MatchType.BaseClass, originalName = "org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter")
+public class AbstractHandlerMethodAdapter_Instrumentation {
+    @Trace
+    protected ModelAndView handleInternal(HttpServletRequest request,
+            HttpServletResponse response, HandlerMethod handlerMethod) throws Exception {
+        Transaction transaction = AgentBridge.getAgent().getTransaction(false);
+
+        if (transaction != null) {
+            Class<?> controllerClass = handlerMethod.getBeanType();
+            Method controllerMethod = handlerMethod.getMethod();
+
+            //If this setting is false, attempt to name transactions the way the legacy point cut
+            //named them
+            boolean isEnhancedNaming =
+                    NewRelic.getAgent().getConfig().getValue("class_transformer.enhanced_spring_transaction_naming", false);
+
+            String httpMethod = request.getMethod();
+            if (httpMethod != null) {
+                httpMethod = httpMethod.toUpperCase();
+            } else {
+                httpMethod = "Unknown";
+            }
+
+            //Optimization - If a class doesn't have @Controller/@RestController directly on the controller class
+            //the transaction is named in point cut style (when enhanced naming set to false)
+            if (!isEnhancedNaming && !SpringControllerUtility.doesClassContainControllerAnnotations(controllerClass, false)) {
+                SpringControllerUtility.assignTransactionNameFromControllerAndMethod(transaction, controllerClass, controllerMethod);
+            } else {    //Normal flow to check for annotations based on enhanced naming config flag
+                String rootPath;
+                String methodPath;
+
+                //From this point, look for annotations on the class/method, respecting the config flag that controls if the
+                //annotation has to exist directly on the class/method or can be inherited.
+
+                //Handle typical controller methods with class and method annotations. Those annotations
+                //can come from implemented interfaces, extended controller classes or be on the controller class itself.
+                //Note that only RequestMapping mapping annotations can apply to a class (not Get/Post/etc)
+                rootPath = SpringControllerUtility.retrieveRootMappingPathFromController(controllerClass, isEnhancedNaming);
+
+                //Retrieve the mapping that applies to the target method
+                methodPath = SpringControllerUtility.retrieveMappingPathFromHandlerMethod(controllerMethod, httpMethod, isEnhancedNaming);
+
+                if (rootPath != null || methodPath != null) {
+                    SpringControllerUtility.assignTransactionNameFromControllerAndMethodRoutes(transaction, httpMethod, rootPath, methodPath);
+                } else {
+                    //Name based on class + method
+                    SpringControllerUtility.assignTransactionNameFromControllerAndMethod(transaction, controllerClass, controllerMethod);
+                }
+            }
+            transaction.getTracedMethod().setMetricName("Spring", "Java",
+                    SpringControllerUtility.getControllerClassAndMethodString(controllerClass, controllerMethod, true));
+        }
+
+        return Weaver.callOriginal();
+    }
+}