🔖 Add gzip compression by default with option to opt-out#1310
Merged
Conversation
Sushisource
force-pushed
the
enable-transport-compression
branch
from
ae45f62 to
dc5e780
Compare
Contributor
|
Is the 💥 just a precaution, or are we aware of situations where the default opt-in policy could be problematic? |
Member
Author
Really just a precaution... maybe we need a different emoji for "requires lang changes but isn't breaking" which is what I really need here. |
Sushisource
changed the title
Sushisource
force-pushed
the
enable-transport-compression
branch
2 times, most recently
from
8c827e4 to
68e1b4f
Compare
eamsden
approved these changes
Jun 5, 2026
Comment on lines
+593
to
+611
| // The generated service clients don't share a trait exposing the compression setters, so | ||
| // a macro applies the same configuration to each concrete client type. | ||
| macro_rules! configure { | ||
| ($client:expr) => {{ | ||
| let client = $client.max_decoding_message_size(get_decode_max_size()); | ||
| match compression { | ||
| GrpcCompression::Gzip => client | ||
| .send_compressed(CompressionEncoding::Gzip) | ||
| .accept_compressed(CompressionEncoding::Gzip), | ||
| GrpcCompression::None => client, | ||
| } | ||
| }}; | ||
| } | ||
|
|
||
| let workflow_svc_client = Box::new(configure!(WorkflowServiceClient::new(svc.clone()))); | ||
| let operator_svc_client = Box::new(configure!(OperatorServiceClient::new(svc.clone()))); | ||
| let cloud_svc_client = Box::new(configure!(CloudServiceClient::new(svc.clone()))); | ||
| let test_svc_client = Box::new(configure!(TestServiceClient::new(svc.clone()))); | ||
| let health_svc_client = Box::new(configure!(HealthClient::new(svc.clone()))); |
Contributor
There was a problem hiding this comment.
I think a trait would be much clearer here, though the macro isn't horrible as macros go.
Member
Author
There was a problem hiding this comment.
It ends up being uglier since the trait would need to be defined down in the proto crate
Comment on lines
+69
to
+73
| /// Part 2 proves the *outbound request* bytes on the wire are genuinely gzip-compressed. We cannot | ||
| /// inspect the bytes of the real (TLS) connection, so this routes through an in-process tonic | ||
| /// server that lets us read the raw gRPC frame and confirm the compression flag, gzip magic bytes, | ||
| /// size reduction, and that the payload decompresses to the exact same protobuf as the | ||
| /// uncompressed request. |
jmaeagle99
approved these changes
Jun 5, 2026
jmaeagle99
reviewed
Jun 5, 2026
Sushisource
force-pushed
the
enable-transport-compression
branch
from
68e1b4f to
714eb9b
Compare
Sushisource
force-pushed
the
enable-transport-compression
branch
from
714eb9b to
6a6c8db
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
Added transport level compression by default
Lang layers will need to expose option to change compression type (should be modeled like enum as we've done here in case we add other compression kinds).
Why?
Be efficient!
Checklist
Closes
How was this tested:
Existing and new tests
Any docs updates needed?