New Spring controller instrumentation modules #1675

jtduffy · 2023-12-22T18:48:50Z

Overview

Resolves #1576
These instrumentation modules support proper transaction naming (route + HTTP method) of traditional annotated spring controllers as well as controllers that inherit annotations from interfaces, super classes or custom annotations. Check one of the README files in the PR for a full description.

spring-4.3.0: 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).
spring-6.0.0: Identical to spring-4.3.0 except for javax -> jakarta namespace.
spring-webflux-controller-mappings-5.0.0: Same functionality as the above modules except for Webflux rather than WebMVC.

Note that because the new instrumentation can change transaction names, enabling this "enhanced transaction naming" is gated by a configuration flag enhanced_spring_transaction_naming, which is false by default:

class_transformer:
    # Enhanced Spring transaction naming.
    # This feature will name any transaction that originates from a Spring controller after
    # the defined route and HTTP method. For example: "/customer/v1/edit (POST)".
    # This includes controllers that implement or extend interfaces/classes with WebMVC related
    # annotations (@RestController, @Controller, @RequestMapping, etc). By default, this is configured
    # to false, which will name transactions for those types of controllers based on the controller
    # class name and method. For example; "CustomerController/edit". This is the naming logic carried
    # over from previous agent versions. "Standard" controllers, with all relevant annotations
    # present on the actual class, will still get named based on route and HTTP method.
    enhanced_spring_transaction_naming: false

Testing

The agent includes a suite of tests which should be used to
verify your changes don't break existing functionality. These tests will run with
Github Actions when a pull request is made. More details on running the tests locally can be found
here,

Checks

Your contributions are backwards compatible with relevant frameworks and APIs.
Your code does not contain any breaking changes. Otherwise please describe.
Your code does not introduce any new dependencies. Otherwise please describe.

…c-java-agent into spring-controller-instr

meiao · 2024-01-11T23:50:06Z

settings.gradle

@@ -340,6 +341,8 @@ include 'instrumentation:spring-webflux-6.0.0'
 include 'instrumentation:spring-webflux-5.0.0'
 include 'instrumentation:spring-webflux-5.1.0'
 include 'instrumentation:spring-webflux-5.3.0'
+include 'instrumentation:spring-webflux-controller-mappings-5.0.0'
+include 'instrumentation:spring-webclient-5.0'


Looks like you re added this webclient that I had moved as it was not in alphabetical order.

codecov-commenter · 2024-01-12T16:26:48Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0d98b3a) 70.70% compared to head (3d39ef5) 69.89%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...relic/agent/config/ClassTransformerConfigImpl.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1675      +/-   ##
============================================
- Coverage     70.70%   69.89%   -0.82%     
+ Complexity     9929     9826     -103     
============================================
  Files           830      829       -1     
  Lines         39880    39861      -19     
  Branches       6030     6033       +3     
============================================
- Hits          28197    27860     -337     
- Misses         8960     9315     +355     
+ Partials       2723     2686      -37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

meiao · 2024-01-16T14:25:34Z

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

+        if (NewRelic.getAgent().getLogger().isLoggable(Level.FINEST)) {
+            NewRelic.getAgent()
+                    .getLogger()
+                    .log(Level.FINEST, "SpringControllerUtility::assignTransactionNameFromControllerAndMethodRoutes (6.0.0): calling transaction.setTransactionName to [{0}] " +


Was this 6.0.0 supposed to be the module version? This is in the 4.3.0 module.

meiao · 2024-01-16T14:35:37Z

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

+        if (NewRelic.getAgent().getLogger().isLoggable(Level.FINEST)) {
+            NewRelic.getAgent()
+                    .getLogger()
+                    .log(Level.FINEST, "SpringControllerUtility::assignTransactionNameFromControllerAndMethod (6.0.0): " +


Same 6.0.0 here.
And , on the line below.

meiao · 2024-01-16T14:37:00Z

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

+            NewRelic.getAgent()
+                    .getLogger()
+                    .log(Level.FINEST, "SpringControllerUtility::assignTransactionNameFromControllerAndMethodRoutes (6.0.0): calling transaction.setTransactionName to [{0}] " +
+                            "with FRAMEWORK_HIGH and override false, txn {1}, ", txnName, AgentBridge.getAgent().getTransaction().toString());


You already have the transaction here. Any reason to get it again from the API?
The log message is ending with , . This gives the impression that there would me more content on this log message. Was there supposed to be?

I'll address this in all 3 modules.

meiao · 2024-01-16T15:28:51Z

instrumentation/spring-4.3.0/build.gradle

-    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 {


Do we still need the weave-violation-filter?

I was going to create another issue to discuss this later. I don't know if it has any applicability outside of the spring controller stuff or not.

meiao · 2024-01-16T15:36:20Z

...instrumentation/AbstractHandlerMethodAdapterInstrumentation_withEnhancedNamingFalseTest.java

+    }
+
+    @Test
+    public void handleInternal_whenNoAnnotationPresent_namesTxnBasedOnControllerClassAndMethod() throws Exception {


Is this the case of a RouterFunction?

No, this checks the case of a controller method that has no Spring controller annotations at all (like actuator endpoints or error handlers).

meiao · 2024-01-16T15:39:54Z

...src/main/java/com/nr/agent/instrumentation/AbstractHandlerMethodAdapter_Instrumentation.java

+
+            //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);


This could be a constant in SpringControllerUtility.

I'll address this in all 3 modules.

meiao · 2024-01-16T16:14:29Z

instrumentation/spring-6.0.0/build.gradle

+jar {
+    manifest { attributes 'Implementation-Title': 'com.newrelic.instrumentation.spring-6.0.0',
+            'Implementation-Title-Alias': 'spring_annotations',
+            'Weave-Violation-Filter': 'METHOD_MISSING_REQUIRED_ANNOTATIONS,CLASS_MISSING_REQUIRED_ANNOTATIONS' }


Same for weave violation filter.

meiao · 2024-01-16T16:15:04Z

...instrumentation/AbstractHandlerMethodAdapterInstrumentation_withEnhancedNamingFalseTest.java

@@ -0,0 +1,193 @@
+package com.nr.agent.instrumentation;


All tests are missing copyright.

meiao · 2024-01-16T16:31:10Z

...s-5.0.0/src/main/java/com/nr/agent/instrumentation/web/reactive/SpringControllerUtility.java

@@ -0,0 +1,275 @@
+/*
+ *
+ *  * Copyright 2020 New Relic Corporation. All rights reserved.


Old copyright

meiao · 2024-01-16T16:31:38Z

...ntation/spring-6.0.0/src/main/java/com/nr/agent/instrumentation/SpringControllerUtility.java

@@ -0,0 +1,274 @@
+/*
+ *
+ *  * Copyright 2020 New Relic Corporation. All rights reserved.


Old Copyright

jtduffy and others added 11 commits November 17, 2023 11:35

v6 module

12a283b

Webflux module; name methdo tracer properly

2ed9017

Optimize annotation search; update tests and READMEs

6892f5e

v6 module

7a25ab0

Webflux module; name methdo tracer properly

c73368c

Optimize annotation search; update tests and READMEs

5a2b76a

Merge branch 'spring-controller-instr' of github.com:newrelic/newreli…

04dbcc5

…c-java-agent into spring-controller-instr

Add config flag and fallback to point cut behavior

6dad195

webflux controllers and tests

a248a12

Refactor webflux instrumentation so an additional module is not needed

2339da0

Normalize metric names for old and new transaction names

73f8757

jtduffy requested a review from a team December 22, 2023 18:48

Merge branch 'main' into spring-controller-instr-with-config

c11cc51

meiao reviewed Jan 11, 2024

View reviewed changes

Remove webclient5 from settings.gradle

5d754c0

meiao self-assigned this Jan 12, 2024

meiao approved these changes Jan 16, 2024

View reviewed changes

jtduffy added 4 commits January 16, 2024 15:31

Addressed PR comments

e97c839

Refactor config option and tests

91dc351

Disable functional test

d7bd6af

Revert config setting change

3d39ef5

jtduffy merged commit d1a6e27 into main Jan 19, 2024
103 checks passed

jtduffy deleted the spring-controller-instr-with-config branch January 19, 2024 14:24

This was referenced Jan 19, 2024

Improved Support For Spring Controller Methods Annotated To Handle Both GET and POST Methods #1696

Closed

Migrate Spring instrumentations from javax to jakarta to fix transaction names in Spring 6 / Spring Boot 3 #1523

Closed

jtduffy mentioned this pull request Feb 28, 2024

Support for Spring interface annotations #1207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Spring controller instrumentation modules #1675

New Spring controller instrumentation modules #1675

jtduffy commented Dec 22, 2023

meiao Jan 11, 2024 •

edited

codecov-commenter commented Jan 12, 2024 •

edited

meiao Jan 16, 2024

meiao Jan 16, 2024

meiao Jan 16, 2024

jtduffy Jan 16, 2024

meiao Jan 16, 2024

jtduffy Jan 16, 2024

meiao Jan 16, 2024

jtduffy Jan 16, 2024

meiao Jan 16, 2024

jtduffy Jan 16, 2024

meiao Jan 16, 2024

meiao Jan 16, 2024

meiao Jan 16, 2024

meiao Jan 16, 2024

New Spring controller instrumentation modules #1675

New Spring controller instrumentation modules #1675

Conversation

jtduffy commented Dec 22, 2023

Overview

Testing

Checks

meiao Jan 11, 2024 • edited

Choose a reason for hiding this comment

codecov-commenter commented Jan 12, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meiao Jan 11, 2024 •

edited

codecov-commenter commented Jan 12, 2024 •

edited