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

[node][7.102.0] Suspicion of Memory Leak #10790

Closed
3 tasks done
xr0master opened this issue Feb 22, 2024 · 31 comments
Closed
3 tasks done

[node][7.102.0] Suspicion of Memory Leak #10790

xr0master opened this issue Feb 22, 2024 · 31 comments
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug

Comments

@xr0master
Copy link
Contributor

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

7.102.0

Framework Version

espress.js

Link to Sentry event

No response

SDK Setup

Sentry.init({
  dsn: process.env.SENTRY_DNS,
  release: 'n/a',
  environment: process.env.NODE_ENV,
  integrations: [
    new Sentry.Integrations.Http({ tracing: true }),
    new Sentry.Integrations.Express({
      app,
      methods: ['post', 'get', 'delete'],
    }),
  ],

  tracesSampleRate: 0,
});

Steps to Reproduce

  1. add app.use(Sentry.Handlers.requestHandler());
  2. run request
  3. check profile

Expected Result

n/a

Actual Result

I would like to point out that the problem is not one-to-one. That is, not every request gets stuck in memory. After dozens of requests, only one object can be added, and sometimes more. I didn't find any particular pattern.

Screenshot 2024-02-22 at 21 37 15 Screenshot 2024-02-22 at 21 37 42

You may notice that multiple hubs are created, and the data (in locals) is not removed from the response object (and the response object is not deleted either).

if remove app.use(Sentry.Handlers.requestHandler());, there are no memory issues.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 22, 2024
@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label Feb 22, 2024
@Lms24
Copy link
Member

Lms24 commented Feb 23, 2024

Hey @xr0master thanks for writing in!

Unfortunately this is going to be quite hard to figure out without a reproducible example. Any chance you could provide one?

Another question:

You may notice that multiple hubs are created

Are you creating multiple hubs on purpose? Or is this just something you observed with the single Sentry.init call you mentioned in the issue?

@xr0master
Copy link
Contributor Author

xr0master commented Feb 23, 2024

Hey @Lms24

Unfortunately this is going to be quite hard to figure out without a reproducible example. Any chance you could provide one?

It's definitely not easy... I'll try to do it.

Are you creating multiple hubs on purpose? Or is this just something you observed with the single Sentry.init call you mentioned in the issue?

Second option. Just regular integration with express.js. As far as I remember, only one hub should be created for the entire application, is it correct?

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 2 Feb 23, 2024
@xr0master
Copy link
Contributor Author

@Lms24
https://github.com/xr0master/sentry-memo-leak

npm run build
npm run dev

get request http://localhost:5001/logs

Since the application is small, catching this issue is even more difficult. Dozens of requests with different intensities with several snapshots, and I managed to reproduce it. Good luck with this.

image

@Lms24
Copy link
Member

Lms24 commented Feb 26, 2024

@xr0master thanks for the repro and for all the details! Debugging such leaks is very hard but we'll try to get to the bottom of this. One question though: Did this only happen in the most recent version? Have you by any chance tried an older SDK version where this didn't happen before?

@Patrick-Ullrich
Copy link

Patrick-Ullrich commented Mar 3, 2024

We noticed a memory leak in our production environment and am pretty sure it might be the same issue. Just dumping some info in case any of this helps.

We were on version @sentry/node 7.91.0, and upgraded to 7.104.0 during our routine package maintenance. We noticed the memory leak in production. I took 7 or so memory snapshots that i still have. When comparing the snapshots I noticed a constant increase in the Delta size, especially in strings and objects. Looking more into the string ones they all looked something like this:
image
Always referring to hub.js -> transaction.js like in @xr0master screenshot.

As xr0master, we are also running express and the same set up as in the config. Node 18 on elastic beanstalk. We did have the node/tracing package installed which i believe is deprecated. We did remove it, but I assume that wasn't what fixed it.

We downgraded about 2 hours ago to 7.91.0 and memory has been normalized since.

@Lms24 let me know if there is anything we can provide to assist.

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 2 Mar 3, 2024
@daan-nu
Copy link

daan-nu commented Mar 4, 2024

Same here: memory leak after upgrading Sentry from 7.84.0 to 7.102.0. Reverted back to 7.84 and we're OK again. Node v18.17.0, also noticed this in Node v20.11.1

@heipei
Copy link

heipei commented Mar 16, 2024

Echoing this, we have upgraded from 6.2.2 to 7.101.1 and are encountering severe memory leaks with similar heap snapshots as those posted above!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Mar 16, 2024
@daan-nu
Copy link

daan-nu commented Mar 25, 2024

Fwiw, we don't use Sentry in our express config at all, only in our Vue application and also noticed this memory leak (after upgrading 7.84.0 to 7.102.0).

We simply execute Sentry.init with some config options: ignoreErrors, environment, attachStacktrace (true), release, debug (false, unless on dev/test), tracesSampleRate set to 0, sampleRate set to a very low figure (as we have lots of traffic), allowUrls (regex with only our domain), and the Vue app itself (for the Sentry.Vue, for server side rendering / Sentry.Node we can't supply the app).

Testing is quite time-consuming, but if you have a suspect version I'm happy to help with a loadtest on our application with a before/after.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Mar 25, 2024
@AbhiPrasad
Copy link
Member

@daan-nu if you could provide heap snapshots or similar would be a huge help for us to debug further.

for the Sentry.Vue, for server side rendering

I recommend you don't use @sentry/vue for SSR at all fwiw. It relies on browser APIs, and even does stacktrace parsing based expected stacktraces from a browser, so running it in a server environment is not recommended.

@sarunast
Copy link

sarunast commented Apr 17, 2024

We are experiencing this issue as well on production with next.js.

Adding a screenshot that shows the memory increase from version 7.95.0 -> 7.110.1. I can't provide reproduction sorry.

Snapshot from grafana memory tracker.
image

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Apr 17, 2024
@Lms24
Copy link
Member

Lms24 commented Apr 18, 2024

@sarunast did you check the order of our handlers as described in this comment above?

@lforst
Copy link
Member

lforst commented Apr 18, 2024

@Lms24 since he mentioned that this is a Next.js app any express handler shenanigans are unlikely (though not impossible).

@sarunast I'd be interested in whether you have any more involved Sentry code somewhere in your app or whether you just use what we recommend as a default. 🤔

@sarunast
Copy link

sarunast commented Apr 18, 2024

@Lms24 since he mentioned that this is a Next.js app any express handler shenanigans are unlikely (though not impossible).

@sarunast I'd be interested in whether you have any more involved Sentry code somewhere in your app or whether you just use what we recommend as a default. 🤔

As you said it's next.js so we don't configure it for express. We use very basic recommended next.js sentry setup so there is nothing much to add. There is one odd thing about the setup. We do not deploy via vercel rather our own custom docker'isd deployments to AWS. Due to this custom setup sentry works only for client and the backend reporting doesn't even work. Not sure if it helps it would be great to fix the backend reporting but I think it's not related to this issue?

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Apr 18, 2024
@Lms24
Copy link
Member

Lms24 commented Apr 22, 2024

As you said it's next.js so we don't configure it for express

Apologies, I completely missed this in your first comment.

Unfortunately, I think we're talking about multiple different memory leaks and scenarios by now. The only reproduction we received so far was for the Node SDK which no longer leaks memory if the order of registering the Sentry Node/Express handlers is corrected. Furthermore, users are mentioning different version ranges when the leak started happening.

If you're able to provide a minimal reproduction for the leak you experience in your NextJS, we'd greatly appreciate it!

@sarunast
Copy link

sarunast commented Apr 24, 2024

Perhaps it is @Lms24 but what caught my attention was this comment:

I also went through some versions to see when it was introduced. It seems to start in 7.100.0. 7.99 and before works just fine.

Which is true for us.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Apr 24, 2024
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Apr 24, 2024

Looking at https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md#71000

The related changes are:

What Node version is everyone using? What SDK features is everyone using (errors, performance, profiling, etc.). For Next.js in particular are you using the app or pages router?

Because of the variety of different permutations of possible setups, sharing your package.json, next.config.js, etc. would be extremely helpful to allow us to narrow this down!

@lforst
Copy link
Member

lforst commented Apr 25, 2024

@AbhiPrasad I think it is this change where we introduce strong references to scopes on spans #10492

@AbhiPrasad
Copy link
Member

Considering we now have a new major version that made some improvements here (8.0.0), and we've also done some work to address memory issues there, I think the recommendation is to upgrade your SDK to v8:

https://docs.sentry.io/platforms/javascript/guides/node/migration/v7-to-v8/

Closing this issue for now as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Bug
Projects
Archived in project
Development

No branches or pull requests

9 participants