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: allow payload compression in POST requests #170

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rarruda
Copy link
Collaborator

@rarruda rarruda commented Mar 12, 2024

About the changes

  • only done if custom_http_headers config includes {'Content-Encoding' => 'gzip'}
  • only allow gzip.
  • 'Content-Encoding' header will get dropped in GET requests.
  • undocumented feature.

Discussion points

  • could have used helper class for compressing/decompression in the style of ActiveSuppport::Gzip
  • my guess is that for the vast majority of users this will not be worth enabling. But maybe for a few heavy users it could be useful. (therefore not documenting at the moment).
  • not tested against a raw unleash-server. Does it support naively decompressing gzipped content?

Creating as a draft PR to gather feedback.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8247551100

Details

  • 34 of 34 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 97.203%

Totals Coverage Status
Change from base Build 7444190517: 0.07%
Covered Lines: 2572
Relevant Lines: 2646

💛 - Coveralls

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Meant to respond last night but I needed to check some stuff!

So we do do support Gzip compression on the metrics endpoints (not formally tested so we'll need to poke the Unleash server to confirm). So this is a nice change

Some initial thoughts - this trades CPU for data transfer. No idea how good Ruby's Gzip compression is (imagine that's C behind the scenes?) but will this be worth it for very small payloads?

The mucking about with mutable state seems unfortunate (add/remove headers dynamically). Can we do this in a cleaner way? Something like provide a use_compression property on the config?

- only done if custom_http_headers config includes {'Content-Encoding' => 'gzip'}
- only allow gzip.
- 'Content-Encoding' header will get dropped in GET requests.
- undocumented feature.
@rarruda rarruda force-pushed the feat/enable_payload_compression branch from b6b548a to a4189de Compare March 27, 2024 09:34
Copy link

stale bot commented Apr 26, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 26, 2024
@rarruda rarruda added pinned Should not go stale and removed stale labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Should not go stale
Projects
Status: For later
Development

Successfully merging this pull request may close these issues.

3 participants