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

@sentry/node slows server start times significantly #15034

Open
3 tasks done
nathan-knight opened this issue Jan 16, 2025 · 4 comments
Open
3 tasks done

@sentry/node slows server start times significantly #15034

nathan-knight opened this issue Jan 16, 2025 · 4 comments
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@nathan-knight
Copy link

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/nextjs

SDK Version

8.50.0

Framework Version

NestJS 10.4.2

Link to Sentry event

No response

Reproduction Example/SDK Setup

Sentry.init({
  dsn: '-',
  environment: process.env.ENV,
  enabled: !isLocal,

  integrations: [
    nodeProfilingIntegration(),
    Sentry.httpIntegration({
      breadcrumbs: true,
      ignoreOutgoingRequests: (urlPath) => {
        // Ignore requests to the Datadog agent
        return urlPath.startsWith('http://127.0.0.1:8126');
      },
    }),
  ],
});

Steps to Reproduce

Our API start times have been very slow with the vast majority of time being spent loading modules. I dug into the root cause by making Sentry run locally and profiling the launch of our app.

Hook._require.Module.require within require-in-the-middle which is used by @opentelemetry/instrumentation which is used by @sentry/node is responsible for a substantial amount of the launch time.

require-in-the-middle uses a package called resolve to determine if an import is the main module file or an internal one which is very slow, partially because it uses JSON.parse on each package.json for every file in that package that is imported (potentially multiple times). Is there anything the Sentry team can do to try to alleviate this issue for all of their customers? In my application I have patched require-in-the-middle to use Node internals (based on what DataDog does: https://github.com/DataDog/dd-trace-js/blob/6523d941297a8a08c8dcca91ab9fc202b26278b4/packages/dd-trace/src/ritm.js#L127) to resolve the correct file instead which cut our initial load time by 31%.

#14388 is probably related.

Expected Result

Start times to not be substantially affected by Sentry

Actual Result

Start times take substantially longer with Sentry

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 16, 2025
@github-actions github-actions bot added the Package: nextjs Issues related to the Sentry Nextjs SDK label Jan 16, 2025
@timfish timfish self-assigned this Jan 16, 2025
@mydea
Copy link
Member

mydea commented Jan 17, 2025

Hey, thanks for writing in. We'll see if we can make improvements to this upstream in require-in-the-middle - there will always be some overhead there but ideally we can find ways to cut this down as much as possible!

@timfish
Copy link
Collaborator

timfish commented Jan 17, 2025

I've had a go at reproducing this and have seen more like 6-10% overhead rather than 30% reported but that's no doubt due to my benchmarks being more synthetic than a a proper full app startup.

There's an old open issue at require-in-the-middle to replace resolve which would probably be a good start:

@timfish
Copy link
Collaborator

timfish commented Jan 19, 2025

The older PR does not pass the tests on all supported Node versions so I've created a PR that does:

If the minimum supported Node version of require-in-the-middle is bumped slightly, we can remove the need for resolve entirely.

@timfish
Copy link
Collaborator

timfish commented Jan 27, 2025

require-in-the-middle v7.5.0 has been released which no longer uses resolve on newer Node.js versions.

There's a planned v8.0.0 release which will bump the minimum supported Node version slightly and remove resolve entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
Status: No status
Development

No branches or pull requests

3 participants