Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mgr32 opened this issue Sep 27, 2023 · 17 comments · Fixed by #1544
Labels
bug Something isn't working as designed/intended

Comments

@mgr32
Copy link
Contributor

mgr32 commented Sep 27, 2023

Description

After fixing #1197, in Spring Boot 3 applications transactions for "standard" controllers annotated with @RestController / @Controller are named correctly (e.g. WebTransaction/SpringController/hello (GET)), but other requests are still getting non-meaningful names (WebTransaction/Servlet/dispatcherServlet), e.g. for:

  • "built-in" controllers that don't have @RestController-like annotations, e.g. /actuator/health (see Actuator endpoints),
  • custom controllers that don't use @RestController directly (e.g. using custom annotations - see below for an example),
  • requests returning 401s / 404s.

This is because SpringPointCut instrumentation (and possibly other instrumentations - see https://github.com/newrelic/newrelic-java-agent/pull/1505/files) still relies on javax instead of jakarta packages. The following table sums up the observed cases:

Spring Boot Request Controller Response Code Transaction name at one.newrelic.com Expected?
2.7.16 /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
3.1.4 /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
2.7.16 /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured (GET) Y
3.1.4 /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured (GET) Y
2.7.16 /hello-secured @RestController / secured / invalid auth 401 WebTransaction/SpringController/BasicErrorController/error Y
3.1.4 /hello-secured @RestController / secured / invalid auth 401 WebTransaction/Servlet/dispatcherServlet N
2.7.16 /hello-custom custom annotation 200 WebTransaction/SpringController/TestControllerWithCustomAnnotation/sayHello Y
3.1.4 /hello-custom custom annotation 200 WebTransaction/Servlet/dispatcherServlet N
2.7.16 /actuator/health registered automatically without annotation 200 WebTransaction/SpringController/OperationHandler/handle Y
3.1.4 /actuator/health registered automatically without annotation 200 WebTransaction/Servlet/dispatcherServlet N
2.7.16 /not-existing no handler mapping 404 WebTransaction/SpringController/BasicErrorController/error Y
3.1.4 /not-existing no handler mapping 404 WebTransaction/Servlet/dispatcherServlet N

/hello-custom is configured in the following way:

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
@RestController
public @interface CustomRestControllerAnnotation {
}
@CustomRestControllerAnnotation
public class TestControllerWithCustomAnnotation {
    @GetMapping("/hello-custom")
    public String sayHello() {
        return "hello-custom";
    }
}

/actuator-health is configured automatically due to the presence of spring-boot-starter-actuator.

Expected Behavior

Transaction names should include controller and method name whenever possible.

Steps to Reproduce

Described at https://github.com/mgr32/newrelic-spring-cloud-openfeign-issue/tree/springpointcut-sb3.

Your Environment

  • Ubuntu 22.04.3 x64
  • New Relic Java Agent 8.6.0
  • OpenJDK Temurin-17.0.6+10 x64
  • Spring Boot 3.1.4
@mgr32 mgr32 added the bug Something isn't working as designed/intended label Sep 27, 2023
@workato-integration
Copy link

@jtduffy
Copy link
Contributor

jtduffy commented Sep 27, 2023

@mgr32
Thanks for the detailed analysis and examples.

Our ultimate goal is to get rid of the legacy pointcut instrumentation and replace it with instrumentation modules, so we'll probably tackle this issue with an eye towards that. Thanks again!

@jtduffy
Copy link
Contributor

jtduffy commented Oct 10, 2023

The team has decided to go ahead and patch the existing pointcut implementation (#1544) in order to get the immediate need addressed. An instrumentation module will be explored in the future.

@blackr1234
Copy link

Hi all, may I know if the team has planned when this fix will be generally available?
Thanks!

@jtduffy
Copy link
Contributor

jtduffy commented Oct 11, 2023

@blackr1234 We are currently testing the changes via #1544. The hope is to have it out in the next release (~ Oct 19th) but that can change if we run into any issues.

@github-project-automation github-project-automation bot moved this from Triage to Code Complete/Done in Java Engineering Board Oct 11, 2023
@m-joon-ixix
Copy link

@jtduffy Hello, are you able to share the schedule for the next release? It seems that the latest release is still 8.6.0. Our team is waiting for this problem of missing NewRelic transaction names, to be resolved.
BTW, thanks for your contribution :)

@jtduffy
Copy link
Contributor

jtduffy commented Oct 23, 2023

Hi @m-joon-ixix
The next release should be out on Oct 26th. We had to delay the release by 1 week.

@jtduffy
Copy link
Contributor

jtduffy commented Nov 28, 2023

@mgr32 Since you were so proactive in opening this issue regarding Spring controller naming, I (and the team) were wondering if you would be willing to do a smoke test of a new Spring controller instrumentation module that supports annotated controller interfaces and controller inheritance (in a lower environment of course). I've done some pretty exhaustive testing, but it would be nice to get some feedback from end users as well.

If so, I can give you the branch name if you'd like to build the artifact on your own, or I can build a snapshot and forward you the link. Thanks!

@mgr32
Copy link
Contributor Author

mgr32 commented Nov 28, 2023

@jtduffy Sure, I can check the cases I prepared for reproducing this issue, please send me a link to the snapshot.

@jtduffy
Copy link
Contributor

jtduffy commented Nov 28, 2023

@mgr32 I will have a link for you tomorrow or Thursday. I have one more change I want to add. Thanks again.

@mgr32
Copy link
Contributor Author

mgr32 commented Dec 14, 2023

Hi @jtduffy, please find the results below - everything looks fine, and some cases (in bold) have improved. Also, I noticed that the current version of New Relic Java Agent (8.7.0) assigns wrong transaction names again on Spring Boot 3.2 - fortunately the snapshot version fixes it, so I'm looking forward to the release 🤞 Thanks!

Updated code used for testing can be found at https://github.com/mgr32/newrelic-spring-cloud-openfeign-issue/tree/springpointcut-sb3

Spring Boot 3.1.6

New Relic Java Agent Request Controller Response Code Transaction name at one.newrelic.com OK?
8.7.0 /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
8.8.0-SNAPSHOT /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
8.7.0 /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured Y
8.8.0-SNAPSHOT /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured (GET) Y
8.7.0 /hello-secured @RestController / secured / invalid auth 401 WebTransaction/SpringController/BasicErrorController/error Y
8.8.0-SNAPSHOT /hello-secured @RestController / secured / invalid auth 401 WebTransaction/SpringController/BasicErrorController/error Y
8.7.0 /hello-custom custom annotation 200 WebTransaction/SpringController/TestController.../sayHello Y
8.8.0-SNAPSHOT /hello-custom custom annotation 200 WebTransaction/SpringController/hello-custom (GET) Y
8.7.0 /actuator/health registered automatically without annotation 200 WebTransaction/SpringController/OperationHandler/handle Y
8.8.0-SNAPSHOT /actuator/health registered automatically without annotation 200 WebTransaction/SpringController/OperationHandler/handle Y
8.7.0 /not-existing no handler mapping 404 WebTransaction/SpringController/BasicErrorController/error Y
8.8.0-SNAPSHOT /not-existing no handler mapping 404 WebTransaction/SpringController/BasicErrorController/error Y
8.7.0 /hello-with-manual-instrumentation @RestController / unsecured 200 WebTransaction/Custom/com.example.demo. TestController/sayHelloWithManual... Y
8.8.0-SNAPSHOT /hello-with-manual-instrumentation @RestController / unsecured 200 WebTransaction/Custom/com.example.demo. TestController/sayHelloWithManual... Y
8.7.0 /hello-error @RestController / unsecured 400 WebTransaction/SpringController/hello-error (GET) Y
8.8.0-SNAPSHOT /hello-error @RestController / unsecured 400 WebTransaction/SpringController/hello-error (GET) Y
8.7.0 /hello-new/with-interface interface 200 WebTransaction/SpringController/TestController.../sayHelloWith... Y
8.8.0-SNAPSHOT /hello-new/with-interface interface 200 WebTransaction/SpringController/hello-new/with-interface (GET) Y
8.7.0 /hello-new/with-inheritance inheritance 200 WebTransaction/SpringController/TestController.../sayHelloWith... Y
8.8.0-SNAPSHOT /hello-new/with-inheritance inheritance 200 WebTransaction/SpringController/hello-new/with-inheritance (GET) Y

Spring Boot 3.2.0

New Relic Java Agent Request Controller Response Code Transaction name at one.newrelic.com OK?
8.7.0 /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
8.8.0-SNAPSHOT /hello @RestController / unsecured 200 WebTransaction/SpringController/hello (GET) Y
8.7.0 /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured (GET) Y
8.8.0-SNAPSHOT /hello-secured @RestController / secured / valid auth 200 WebTransaction/SpringController/hello-secured (GET) Y
8.7.0 /hello-secured @RestController / secured / invalid auth 401 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /hello-secured @RestController / secured / invalid auth 401 WebTransaction/SpringController/BasicErrorController/error Y
8.7.0 /hello-custom custom annotation 200 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /hello-custom custom annotation 200 WebTransaction/SpringController/hello-custom (GET) Y
8.7.0 /actuator/health registered automatically without annotation 200 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /actuator/health registered automatically without annotation 200 WebTransaction/SpringController/OperationHandler/handle Y
8.7.0 /not-existing no handler mapping 404 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /not-existing no handler mapping 404 WebTransaction/SpringController/BasicErrorController/error Y
8.7.0 /hello-with-manual-instrumentation @RestController / unsecured 200 WebTransaction/Custom/com.example.demo. TestController/sayHelloWithManual... Y
8.8.0-SNAPSHOT /hello-with-manual-instrumentation @RestController / unsecured 200 WebTransaction/Custom/com.example.demo. TestController/sayHelloWithManual... Y
8.7.0 /hello-error @RestController / unsecured 400 WebTransaction/SpringController/hello-error (GET) Y
8.8.0-SNAPSHOT /hello-error @RestController / unsecured 400 WebTransaction/SpringController/hello-error (GET) Y
8.7.0 /hello-new/with-interface interface 200 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /hello-new/with-interface interface 200 WebTransaction/SpringController/hello-new/with-interface (GET) Y
8.7.0 /hello-new/with-inheritance inheritance 200 WebTransaction/Servlet/dispatcherServlet N
8.8.0-SNAPSHOT /hello-new/with-inheritance inheritance 200 WebTransaction/SpringController/hello-new/with-inheritance (GET) Y

@jtduffy
Copy link
Contributor

jtduffy commented Dec 14, 2023

@mgr32 We truly appreciate you taking the time to verify the transaction name changes. I think we're targeting the next release for a mid-February date, but I'll keep you updated. Thanks again!

@jtduffy
Copy link
Contributor

jtduffy commented Jan 19, 2024

Hi @mgr32
I wanted to make you aware that the changes related to Spring annotation support have been merged. Also, we put a feature flag around the new transaction naming so as not to break existing customers who rely on the old transaction names. Details are in the PR #1675, but basically you just need the following config to enable the "new" transaction names:

class_transformer:
  enhanced_spring_transaction_naming: true

@pintomau
Copy link

pintomau commented Jan 24, 2024

Hi @jtduffy , I see you commented here recently, so trying to reach out here. Let me know if I should open a new ticket.

I'm using the use case mentioned here https://github.com/newrelic/newrelic-java-agent/pull/1675/files#diff-e4198bccc8b26ce850fad44652bc4d489a6037ea28c29689b5641936648cd396R21 (Interface with @RequestMapping annoted methods, as generated by the open api generator, and implemented in its own Controller class).

But still no instrumentation, only dispatcherServlet showing. Whereas before, we had proper naming.

Agent: 8.8.1
environment.class_transformer_enhanced_spring_transaction_naming: true
Spring Boot 3.2.2

@Generated(value = "org.openapitools.codegen.languages.SpringCodegen")
@Validated
public interface AbcApi {

  @RequestMapping(
        method = RequestMethod.POST,
        value = "/abc/def",
        produces = { "application/json" },
        consumes = { "application/json" }
    )
    default ResponseEntity<...> createDef(
         @Valid @RequestBody Object def
    ) {
        return new ResponseEntity<>(HttpStatus.NOT_IMPLEMENTED);
    }
}
@MyCustomAnnotationAnnotatedWithRestController
public class MyController implements AbcApi {

@Override
...

}

@jtduffy
Copy link
Contributor

jtduffy commented Jan 25, 2024

These changes I mentioned here haven't been released yet. They will be out in v8.9.0, which should be released within the next 3 weeks.

@pintomau
Copy link

Ah. Sorry for the misunderstanding. Thanks.

@jtduffy
Copy link
Contributor

jtduffy commented Jan 25, 2024

@pintomau No problem. This is a snapshot build that contains an earlier version of the instrumentation changes if you'd like to give it a go in a lower environment. This jar doesn't have the feature flag so the new transaction names happen automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as designed/intended
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants