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

Transaction name multiplies when upgrading to v10 with ESM #1646

Closed
Marcholio opened this issue May 22, 2023 · 20 comments · Fixed by #1729, #1760 or #1767
Closed

Transaction name multiplies when upgrading to v10 with ESM #1646

Marcholio opened this issue May 22, 2023 · 20 comments · Fixed by #1729, #1760 or #1767
Assignees
Labels

Comments

@Marcholio
Copy link

Marcholio commented May 22, 2023

Description

After updating agent from 9.15.0 to 10.1.1 transaction names have started to multiply paths, for example POST /api is reported as POST /api/api/api/api/api/api/api/api.

See screenshot:
Screenshot 2023-05-22 at 15 58 34

Expected Behavior

Transaction name should be reported as previously.

Troubleshooting or NR Diag results

Steps to Reproduce

  • Setup basic express app
  • install agent 9.15.0 with default config
  • Monitor transactions
  • Update agent to 10.1.1
  • Monitor transactions and see that names are multiplying

Your Environment

  • Express 4.18.0
  • Node.js 18.15.0
  • Newrelic agent 10.1.1

Additional context

We are using newrelic ESM loader for ESM codebase.

@workato-integration
Copy link

@newrelic-node-agent-team newrelic-node-agent-team added this to Triage Needed: Unprioritized Features in Node.js Engineering Board May 22, 2023
@jmartin4563
Copy link
Contributor

Hi @Marcholio, thank you for the bug report, and sorry for the issues you're having. I'll be taking a look at this, but in the meantime if you have a minimal repro app you can share, that would be very helpful in expediting our root cause analysis.

@jmartin4563 jmartin4563 self-assigned this May 22, 2023
@jmartin4563
Copy link
Contributor

@Marcholio I booted up a quick example app using agent version 10.1.1 (which you can see here), and I'm not seeing the behavior your screenshot shows. Could you describe more about how your express app is set up and what it does so I can try and get a good repro case? Additionally, which UI screen is that screenshot you posted from, is it Transactions or Distributed Tracing?

@Marcholio
Copy link
Author

@jmartin4563 Screenshot is from Transactions. I think this is not an issue with v10 itself, but most likely with ESM loader, because we have other apps using v10 that do not have the same problem. I'll see if I can come up with a repro case.

@jmartin4563
Copy link
Contributor

@Marcholio I'm also using the ESM loader in that example app, where I provide --experimental-loader in my start npm script. Out of curiosity, in your ESM app startup, are you specifying the --experimental-loader flag and including -r newrelic? I'm going to test that out on my end to see if it has an impact, but figured I'd ask you as well :)

@Marcholio
Copy link
Author

Yes we have specified --experimental-loader newrelic/esm-loader.mjs, but not including -r newrelic.

@jmartin4563
Copy link
Contributor

@Marcholio if you're unable to get a repro case, you could always go the self-service route of enabling trace level agent logs in your actual app, and looking at the logs to see the naming being applied as the transaction is generated. Short of that, we would recommend that you engage our support so they can work with you to triage and if they're unable to pinpoint the root cause, provide that triage information to my team.

@bizob2828
Copy link
Member

I'm going to close this. If you can get us a reproduction case, please reopen or continue to go the New Relic support route

Node.js Engineering Board automation moved this from Triage Needed: Unprioritized Features to Done: Issues recently completed Jun 12, 2023
@jordigh
Copy link
Contributor

jordigh commented Jun 22, 2023

We have just been able to reproduce this case internally by using both the newrelic.instrumentLoadedModule API and the ESM loader at the same time. @Marcholio, were you also using both?

@jordigh jordigh reopened this Jun 22, 2023
Node.js Engineering Board automation moved this from Done: Issues recently completed to In progress: Issues being worked on Jun 22, 2023
@Marcholio
Copy link
Author

I don't think we are using newrelic.instrumentLoadedModule API.

Node.js Engineering Board automation moved this from In progress: Issues being worked on to Done: Issues recently completed Jun 27, 2023
@bizob2828
Copy link
Member

I'm closing and will echo what @jmartin4563 said. @Marcholio if you're unable to get a repro case, you could always go the self-service route of enabling trace level agent logs in your actual app, and looking at the logs to see the naming being applied as the transaction is generated. Short of that, we would recommend that you engage our support so they can work with you to triage and if they're unable to pinpoint the root cause, provide that triage information to my team.

@wiked03
Copy link

wiked03 commented Jul 13, 2023

Hi @bizob2828 and team, I ran into this same issue and created a repo with an example app to reproduce it. The app does not use the newrelic.instrumentLoadedModule API

The steps to reproduce are in the README

Git Hub Repo

I originally posted my question on the Support Channel, so I will link to that here as a reference:
https://forum.newrelic.com/s/hubtopic/aAX8W0000015BOiWAM/new-nodeexpress-app-shows-transaction-names-with-repeated-path-values

Let me know if there are any issues with the repo or if there are any additional questions.

Thanks!!

@jmartin4563
Copy link
Contributor

Hiya @wiked03, thank you for the repro case! I'll be taking a look and will keep you posted on my findings

@jmartin4563
Copy link
Contributor

@wiked03 confirmed that I'm able to repro with your example, digging into why the ESM loader seems to be registering instrumentation twice for express, will keep you posted

@jmartin4563 jmartin4563 changed the title Transaction name multiplies when upgrading to v10 Transaction name multiplies when upgrading to v10 with ESM Jul 17, 2023
@jmartin4563
Copy link
Contributor

@Marcholio and @wiked03, we've just published v10.5.0 with a fix for this issue, give it a try and let us know how it works for you both, sorry again for the inconvenience.

@wiked03
Copy link

wiked03 commented Jul 20, 2023

@Marcholio and @wiked03, we've just published v10.5.0 with a fix for this issue, give it a try and let us know how it works for you both, sorry again for the inconvenience.

Well, It's better, but not quite fixed. It DOES report correctly for my simple example app, but the paths are still repeated in my real app. However, they are repeated FEWER times than before.

Examples of before and after updating to 10.5 (the correct path is /config):
image

I will try to update my example app to reproduce it and let you know.

@wiked03
Copy link

wiked03 commented Jul 22, 2023

@jmartin4563 I was finally able to figure out why that fix did not work for my app. It has something to do with swagger. Just importing the swagger-ui-express library causes the path to be repeated in the APM. Simply commenting out the import will fix it.

Git Hub Repo

@jmartin4563 jmartin4563 reopened this Jul 24, 2023
@jmartin4563
Copy link
Contributor

@wiked03 confirmed that I'm able to reproduce with your app, working internally to come up with a solution, the change we made previously didn't go far enough in this case, will keep you posted.

@jmartin4563
Copy link
Contributor

@wiked03 sorry for the delay but wanted to give you an update: the fix for this was a lot more involved than the previous change, and just wanted to let you know that we'll be shipping the fix in v11 of the agent (as it required us to make a breaking change). Thank you for your patience and help with resolving this issue!

bizob2828 added a commit that referenced this issue Aug 17, 2023
…ation firing for both commonjs and esm (#1760)

 * **Breaking Change**: Updated ESM loader that now requires to use both a loader and -r.
   * `node --experimental-loader newrelic/esm-loader.mjs -r newrelic path/to/app.js`
  * **Breaking change**: Removed `config.esm.custom_instrumentation_entrypoint` to register ESM instrumentation.
    * You can now just call the `newrelic.instrument*` APIs but you must pass in an object and specify `isEsm: true`.
  * Fixed issue where both CJS and ESM instrumentation tried to instrument the same package, thus causing transaction naming issues, see #1646 
  * Updated agent to only run in the main thread.  This is because running in a worker thread does not function out of the box and will just reduce the overhead for customers that are naively trying to load this.

```js
import newrelic from 'newrelic'
newrelic.instrument({ moduleName: 'parse-json', isEsm: true }, function wrap(shim, parseJson, moduleName) {
  shim.wrap(parseJson.default, function wrapParseJson(shim, orig) {
      return function wrappedParseJson() {
          const result = orig.apply(this, arguments)
          result.instrumented = true
          return true
      }
  })
})
```
This was referenced Aug 21, 2023
@bizob2828
Copy link
Member

@wiked03 @Marcholio v11.0.0 of the agent was released yesterday. It should fix your issue with transaction naming. Please be aware to use the esm loader you also now have to load the agent. You can refer to the README, or this example app

Before:

node --experimental-loader newrelic/esm-loader.mjs path/to/app.js

After:

node --experimental-loader newrelic/esm-loader.mjs -r newrelic path/to/app.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment