Skip to content

Optional gzip compression of responses#157

Merged
bnusunny merged 6 commits intoaws:mainfrom
huntharo:compress-responses
Feb 17, 2023
Merged

Optional gzip compression of responses#157
bnusunny merged 6 commits intoaws:mainfrom
huntharo:compress-responses

Conversation

@huntharo
Copy link
Copy Markdown
Contributor

@huntharo huntharo commented Feb 16, 2023

Issue #, if available: None

Description of changes:

  • Compress responses when the COMPRESSION env var is true, when the response is not already compressed, and when the content-type starts with text/ or is one of a few known types such as application/javascript
  • The list of mime types to compress is not configurable to align with the way CloudFront handles automatic compression if enabled - the list of mime types that should / should not be compressed is something that is largely configurable one time and does not need to be adjusted or excruciatingly specified by each consumer (like nginx requires)
  • Set the Content-Length response header to have the correct compressed length
  • Set the Content-Encoding response header to gzip if compression is performed

Motivations

  • This project is awesome and I'm going to incorporate it into the documentation and examples for my MicroApps framework as the suggested method for quickly converting web apps to run as Lambda functions
  • Node.js is exceedingly slow at performing gzip compression, particularly of large response bodies
  • Lambda functions exposed via CloudFront connected to Function URLs with SigV4 signing do not have another place where compression of the response can be guaranteed, particularly on non-cacheable responses (which CloudFront either never or sometimes does not compress)
  • Rust compressing the response should be substantially faster (I didn't even measure it as I've done extensive measurements of Node.js gzip performance for other projects... it's bad)
  • This is a feature I need to allow Lambda to tie or beat the current deployment pattern for an existing site

To-Do / Seeking Input

  • Should the COMPRESSION env var name be the first name-spaced env var such as LAMBDA_ADAPTER_COMPRESSION?
  • Do we have deployed integration tests that will confirm this works in the wild? I wasn't sure where to put those
  • I assume the lambda http crate is handling marking the response as base64 encoded and handling the base64 encoding when it sees the gzip content-encoding - could someone confirm that? I see that nginx is sending gzip responses through the adapter so I assume this is all working correctly already
  • The use of unsafe in one of the tests (not deployed code) was gross, but there does not appear to be a body function that takes a byte array so I had to fake it as a non-validated string - Any suggestions for that test would be welcome

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bnusunny
Copy link
Copy Markdown
Contributor

bnusunny commented Feb 16, 2023

Thanks for the contribution!

Here are my suggestions.

  1. Let's use AWS_LWA_ENABLE_COMPRESSION as the environment variable name.
  2. We deploy a go-httpbin app in the pipeline, run e2e tests against it in the beta account, and tear it down when finished. You could add tests to the initial set of e2e tests here.
  3. Yes, lambda http crate will base64 encode any response with non-empty content-encoding.
  4. No need to convert the encoded bytes to a String. Hyper accepts Vec[u8] as body.

@bnusunny
Copy link
Copy Markdown
Contributor

bnusunny commented Feb 16, 2023

There are a few formatting issues. Could you run cargo fmt? It will fix them.

@huntharo
Copy link
Copy Markdown
Contributor Author

There are a few formatting issues. Could you run cargo fmt? It will fix them.

Yup will do shortly! Thanks for the quick feedback, that's very refreshing!

@huntharo
Copy link
Copy Markdown
Contributor Author

huntharo commented Feb 16, 2023

  1. No need to convert the encoded bytes to a String. Hyper accepts Vec[u8] as body.

I should clarify: It's httpmock that only accepts a string for the mocked body response. This is only in a unit test but there did not appear to be any other way to get httpmock to return unmodified bytes.

Ha ha... nope... httpmock does accept bytes. I just had to unwrap and pass as a ref. Done.

@huntharo huntharo marked this pull request as ready for review February 16, 2023 14:35
@huntharo
Copy link
Copy Markdown
Contributor Author

E2Es added, unsafe removed, env var renamed, formatting fixed, and docs updated.

I think this is ready for a deploy run and for review.

@huntharo
Copy link
Copy Markdown
Contributor Author

@bnusunny any chance this can be released quickly? I'm really wanting to use it in a project.

@bnusunny
Copy link
Copy Markdown
Contributor

Just need one minor update in the README. See my last comment above.

@huntharo
Copy link
Copy Markdown
Contributor Author

Just need one minor update in the README. See my last comment above.

I only see two comments? One about cargo fmt and the one answering my 4 questions?

Comment thread tests/integ_tests.rs Outdated
Comment thread tests/integ_tests.rs Outdated
Comment thread README.md Outdated
@bnusunny
Copy link
Copy Markdown
Contributor

bnusunny commented Feb 17, 2023

Just need one minor update in the README. See my last comment above.

I only see two comments? One about cargo fmt and the one answering my 4 questions?

Sorry, I forgot to summit the review comments.

@huntharo
Copy link
Copy Markdown
Contributor Author

Just need one minor update in the README. See my last comment above.

I only see two comments? One about cargo fmt and the one answering my 4 questions?

Sorry, I forgot to summit the review comments.

No problem! All issues addressed, thanks for the feedback!

@huntharo huntharo requested a review from bnusunny February 17, 2023 13:32
@bnusunny bnusunny merged commit 6b81bb2 into aws:main Feb 17, 2023
@bnusunny
Copy link
Copy Markdown
Contributor

Release v0.6.2 is out. The layers and docker images are published.

@huntharo huntharo deleted the compress-responses branch February 21, 2023 01:41
@huntharo
Copy link
Copy Markdown
Contributor Author

I can confirm this is working. I'm using it in a few projects already!

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