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

Compression middleware #270

Merged
merged 27 commits into from
Nov 30, 2020
Merged

Conversation

tpaschalis
Copy link

@tpaschalis tpaschalis commented Oct 12, 2020

Which problem is this PR solving?

This PR resolves #223 Add optional gzip compression for HTTP responses. I'm using the work of @KenanBek on #232 as a base.

The idea is to support the gzip and deflate compression schemes, as defined in section 3.5 of the HTTP/1.1 RFC with a clean up the public API, while also limiting the potential for errors on the user side.

Short description of the changes

  • Define a unified compression middleware
  • Use the builder pattern for creating and defining the middleware properties
  • Add tests both for the builder and the functionality
  • Introduce some new constants in the encoding package

Notes

  • Be ruthless in your review :) A fresh look might give new ideas on how to make things better
  • Add example
  • Add more extensive tests, for the other compression algorithms as well

@tpaschalis tpaschalis self-assigned this Oct 12, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #270 (f681aa5) into master (587a2c1) will decrease coverage by 0.03%.
The diff coverage is 73.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   72.17%   72.14%   -0.04%     
==========================================
  Files          61       61              
  Lines        3217     3278      +61     
==========================================
+ Hits         2322     2365      +43     
- Misses        804      820      +16     
- Partials       91       93       +2     
Impacted Files Coverage Δ
component/http/component.go 94.07% <64.28%> (-3.45%) ⬇️
component/http/middleware.go 83.76% <75.00%> (-2.68%) ⬇️
client/http/http.go 92.85% <81.81%> (-2.70%) ⬇️
component/async/kafka/simple/duration_client.go 92.72% <0.00%> (-3.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 587a2c1...f681aa5. Read the comment docs.

@tpaschalis tpaschalis changed the title [WIP] Compression middleware Compression middleware Oct 13, 2020
@tpaschalis tpaschalis marked this pull request as ready for review October 13, 2020 21:49
Copy link

@mantzas mantzas left a comment

Choose a reason for hiding this comment

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

Do we need to work on the client also for this meaning uncompress the payload?

component/http/middleware.go Outdated Show resolved Hide resolved
component/http/middleware.go Outdated Show resolved Hide resolved
component/http/middleware.go Outdated Show resolved Hide resolved
component/http/middleware.go Outdated Show resolved Hide resolved
component/http/middleware_test.go Outdated Show resolved Hide resolved
examples/eighth/main.go Outdated Show resolved Hide resolved
component/http/middleware.go Show resolved Hide resolved
@tpaschalis
Copy link
Author

Implementing the HTTP client reading of compressed data is quite simple; a POC can be seen here.

The only issue is that the LZW algorithm requires knowing the parameters that were used when compressing (order and litWidth) for decompressing correctly. In practice, these are inferred by the filetype, but this doesn't fit our use-case.

We could utilize two additional headers for LZW-compressed requests, so the receiving end knows what to do with that information. What do you think?

@mantzas
Copy link

mantzas commented Oct 14, 2020

Implementing the HTTP client reading of compressed data is quite simple; a POC can be seen here.

The only issue is that the LZW algorithm requires knowing the parameters that were used when compressing (order and litWidth) for decompressing correctly. In practice, these are inferred by the filetype, but this doesn't fit our use-case.

We could utilize two additional headers for LZW-compressed requests, so the receiving end knows what to do with that information. What do you think?

I would suggest to keep the scope small and not add LZW which also needs soem new headers. This way we add 2 basic compression algos, which are the most common ones, and keep LZW for a future addition. What do you think?

@tpaschalis
Copy link
Author

I would suggest to keep the scope small and not add LZW which also needs soem new headers. This way we add 2 basic compression algos, which are the most common ones, and keep LZW for a future addition. What do you think?

Well, it's either this or hardcoding the LZW parameters which might be confusing for other clients interacting with Patron.

So yes, I'd say I agree, let's remove LZW for now, and see how other OSS servers and clients handle this (eg. if they rely on file type).

@drakos74 what's your view on this?

// or pass middlewares to the HTTP component globally, like we do below
ctx := context.Background()
err = patron.New(name, version).
WithRoutesBuilder(routesBuilder).
Copy link

Choose a reason for hiding this comment

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

I propose the compression middleware to be always created inside patron HTTP component, much like the other middlewares already present e.g. recovery, caching etc.
The only thing that we need is a builder function for the ignore routes in the service builder.
This way the API is minimal and there is no need to pipe into patron the middleware at all...
No need for a builder etc... much simpler and less code.

Copy link
Author

Choose a reason for hiding this comment

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

@mantzas Hm, I think you're right, I'll do that.

Do you think we should also hardcode the compression level for 'deflate' to the maximum value to keep the API minimal? I'd think towards that.

For modern hardware and usual sizes of HTTP transports, the differences are miniscule.
For larger transport sizes, the speed/size trade-off is more apparent.

Here's some benchmarks I've run

size \ deflate |   lvl 1         lvl 9
------------------------------------------
1KiB data      |  0.18ms        0.17ms
1MiB data      |   3.5ms          23ms
1GiB data      |    4.5s           24s

Copy link

Choose a reason for hiding this comment

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

@tpaschalis could we provide this parameter as a Method in the service builder? This way we can have a sane default and also support configuring the value...

@tpaschalis tpaschalis changed the title Compression middleware [WIP] Compression middleware Nov 2, 2020
Paschalis Tsilias added 14 commits November 28, 2020 23:33
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
for all three possible compression algorithms

Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Paschalis Tsilias added 9 commits November 28, 2020 23:34
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Paschalis Tsilias added 4 commits November 28, 2020 23:38
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
Signed-off-by: Paschalis Tsilias <p.tsilias@thebeat.co>
@tpaschalis tpaschalis changed the title [WIP] Compression middleware Compression middleware Nov 29, 2020
@tpaschalis tpaschalis merged commit 3783bea into beatlabs:master Nov 30, 2020
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.

Add optional gzip compression for HTTP responses
3 participants