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

KeepAlive for @sentry/node transports #2555

Closed
rhcarvalho opened this issue Apr 23, 2020 · 4 comments
Closed

KeepAlive for @sentry/node transports #2555

rhcarvalho opened this issue Apr 23, 2020 · 4 comments

Comments

@rhcarvalho
Copy link
Contributor

In #1795 the transports changed from keepAlive: true to keepAlive: false. The reason had to do with memory leaks in old versions of Node.

keepAlive: false is undesirable because it incurs more overhead (re-)establishing TCP connections instead of keeping idle connections in the pool.

https://nodejs.org/dist/latest-v12.x/docs/api/http.html#http_class_http_agent

I propose we set keepAlive: true, at least for recent versions of Node.

@smeubank
Copy link
Member

@JonasBa should we keep this issue open? is the desire to change the default setting or really to apply some improvements with regards to which setting is placed? Or dependent on which feature is being used? As with profiling we may have more power to apply what the standard should be

@JonasBa
Copy link
Member

JonasBa commented Dec 30, 2022

@smeubank, I would love to flip this, but I think the risk of introducing a memory leak for users on outdated node versions is still pretty large. The leak was fixed somewhat recently here, so any outdated v12 or v14 would still be impacted.

Looking at https://endoflife.date/nodejs, v14 ends life support in 4 months, so I think we should enable it by default then (or conditionally via process.versions.node if we want to err on the safe side).

That said, we did add the keepAlive option to the transporter so users can override it keepAlive cc @rhcarvalho.

@smeubank
Copy link
Member

smeubank commented Jan 3, 2023

should we put this on the radar for a profiling GTM plan? Happy if we want to reopen the issue

@JonasBa
Copy link
Member

JonasBa commented Jan 3, 2023

@smeubank I would treat this as an isolated improvements, I dont think we'll have the bandwidth to run a benchmark and put some numbers on this, I'd rather just flip once v14 support ends.

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

No branches or pull requests

4 participants