newrelic / newrelic-java-agent

The New Relic Java agent
Apache License 2.0
202 stars 143 forks source link

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

Closed mgr32 closed 1 year ago

mgr32 commented 1 year ago

Description

After fixing https://github.com/newrelic/newrelic-java-agent/issues/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:

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

workato-integration[bot] commented 1 year ago

https://issues.newrelic.com/browse/NR-164950

jtduffy commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

jtduffy commented 1 year ago

@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.

m-joon-ixix commented 1 year ago

@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 commented 1 year ago

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

jtduffy commented 11 months ago

@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 commented 11 months ago

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

jtduffy commented 11 months ago

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

mgr32 commented 10 months ago

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 :crossed_fingers: 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 commented 10 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Ah. Sorry for the misunderstanding. Thanks.

jtduffy commented 9 months ago

@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.