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: Compress uploads #286

Merged
merged 4 commits into from
Dec 21, 2020
Merged

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Dec 17, 2020

Fixes #285

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thank you, @timfish! LGTM.

As a suggestion, you could only trigger compression if the body exceeds a certain size, since gzip also comes with a CPU penalty.

src/main/transports/net.ts Outdated Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Dec 17, 2020

if the body exceeds a certain size

What would be a good threshold?

@jan-auer
Copy link
Member

I did some quick research, and without explicit benchmarking the threshold is hard to determine. Most resources I found talk about a limit lower than 1k, which would be almost every outgoing request.

There is also https://medium.com/@joehonton/the-gzip-penalty-d31bd697f1a2, which seems to tip over at 32k. This gets me thinking: We probably don't need compression for regular event submission, and events are usually under 32k, so let's go with that?

@HazAT
Copy link
Member

HazAT commented Dec 17, 2020

So if(request.body.length > 32000) zip else not zip?

@timfish
Copy link
Collaborator Author

timfish commented Dec 17, 2020

Now with streams and gzip threshold!

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for going the extra mile.

src/main/transports/net.ts Show resolved Hide resolved
@GintV
Copy link

GintV commented Dec 21, 2020

Any particular reason why this one is not merged yet?

@jan-auer jan-auer merged commit 8e8d3f5 into getsentry:master Dec 21, 2020
@GintV
Copy link

GintV commented Dec 22, 2020

Thanks for implementing this @timfish! @jan-auer, @HazAT any chance release could be made with this change?

@timfish timfish deleted the feat/compressed-uploads branch February 3, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minidumps are not compressed before upload
4 participants