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

feat(node): allow keepAlive override #6161

Merged
merged 4 commits into from Nov 9, 2022
Merged

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Nov 8, 2022

We want to reduce the network pressure during profiling and reuse the connection if possible so that we dont block test execution when profiling tests. This change allows us to leverage http connection keepalive by overriding the default SDK value (false). Since the tests that we profile run on node 18, we should not see any memory leaks and it is probably a good practice to allow an override here anyways.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How hard could it be to write a test? If it's too long, not worth the effort.

I also took the time to open a docs PR for this: getsentry/sentry-docs#5740

Once lint passes we can :shipit:

@JonasBa
Copy link
Member Author

JonasBa commented Nov 9, 2022

@AbhiPrasad thank you for the docs 🙏🏼 I added a test to check if connection: 'keep-alive' header is present on request.

@getsentry getsentry deleted a comment from github-actions bot Nov 9, 2022
@AbhiPrasad AbhiPrasad merged commit c59c70b into master Nov 9, 2022
@AbhiPrasad AbhiPrasad deleted the jb/feat/node-keepalive branch November 9, 2022 16:14
@AbhiPrasad
Copy link
Member

This change was part of the latest release https://github.com/getsentry/sentry-javascript/releases/tag/7.19.0!

docs merged also: getsentry/sentry-docs#5740

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

Successfully merging this pull request may close these issues.

None yet

2 participants