Skip to content

ref(node): Compression support for node http transport #5139

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

Merged
merged 2 commits into from
Jun 2, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented May 19, 2022

This is mostly stolen straight from the Electron transport.

This will help with attachments!

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.

🎉 🥳

Can we add tests for these?

@timfish timfish force-pushed the feat/node-http-compression branch from 2dba052 to ee433ee Compare May 19, 2022 21:26
@timfish
Copy link
Collaborator Author

timfish commented May 19, 2022

Tests added.

I also moved the TransportRequestExecutor tests into their own describe group which appears to have really messed with Github diff.

@timfish timfish force-pushed the feat/node-http-compression branch from ee433ee to f9c5484 Compare May 19, 2022 22:10
@timfish timfish force-pushed the feat/node-http-compression branch from f9c5484 to 8b1a761 Compare May 19, 2022 22:31
@timfish
Copy link
Collaborator Author

timfish commented May 31, 2022

I would hold off on merging this one for a moment. I've found a strange issue with some of the Electron tests throwing zlib incorrect header check for some envelopes for some as yet unknown reason.

It looks like that error is suppressed in these tests in the server here but everything decompresses fine after and is valid so I'm a little confused over what causes it and what the implications will be when these hit Relay.

@AbhiPrasad AbhiPrasad closed this May 31, 2022
@AbhiPrasad AbhiPrasad reopened this May 31, 2022
@timfish
Copy link
Collaborator Author

timfish commented Jun 2, 2022

This can now be merged. The test failures for Electron were caused by a bug in the transport!

@AbhiPrasad AbhiPrasad merged commit e80da53 into getsentry:7.x Jun 2, 2022
lobsterkatie added a commit that referenced this pull request Jun 7, 2022
Note: This is an exact duplicate of #5139 and #5203, both originally by @timfish, which accidentally got merged into the 7.x branch rather than master.

#5139:

This is mostly stolen straight from the [Electron transport](https://github.com/getsentry/sentry-electron/blob/master/src/main/transports/electron-net.ts).

This will help with attachments!

#5203:

#5139 introduced a subtle bug where `options.headers` was modified which causes headers to leak between requests. This means requests after a compressed request will be incorrectly marked with `content-encoding: gzip`.
@timfish timfish deleted the feat/node-http-compression branch August 31, 2022 16:03
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.

2 participants