Gzip compress gRPC responses #3387
Conversation
There was a problem hiding this comment.
Simply copies the changes from square/wire@17a3d9e
misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt
Outdated
Show resolved
Hide resolved
| requestAdapter = "com.squareup.protos.test.grpc.HelloRequest#ADAPTER", | ||
| responseAdapter = "com.squareup.protos.test.grpc.HelloReply#ADAPTER" |
There was a problem hiding this comment.
drive-by: fix the test setup
| /** | ||
| * If true gRPC responses which are larger than the minGzipSize will be compressed. | ||
| */ | ||
| val grpcGzip: Boolean = false, |
There was a problem hiding this comment.
Should this instead be an enum to allow for the other wire-supported encoding?
| val grpcGzip: Boolean = false, | |
| val grpcEncoding: GrpcEncoding = GrpcEncoding.IDENTITY, |
There was a problem hiding this comment.
Yes, I'd lean towards the encoding enum, not just a boolean
There was a problem hiding this comment.
Not sure if it's actually worth it:
- The client only supports
identityandgzip: https://github.com/square/wire/blob/master/wire-grpc-client/src/commonMain/kotlin/com/squareup/wire/internal/GrpcDecoder.kt - The http response compression only supports gzip:
- We also need to introduce
WebConfig.minGrpcMessageToCompressinstead of reusingWebConfig.minGzipSize.
There was a problem hiding this comment.
I think the current implementation makes more sense to the user:
- Why do you configure
grpcEncodingto anything that's not supported? - Why do you want different configs for min compression size for http vs grpc over http?
cc @Hexcles
There was a problem hiding this comment.
Personally, I'd say @zpingcai 's current impl is better in terms of consistency.
grpcGzip makes a lot of sense along with gzip. If we want grpcEncoding, I'd expect to have an enum for plain old HTTP, too. And in fact, not just an enum but a config object as different compression algos have different knobs (minGzipSize obviously doesn't make sense for non-gzip compression algos).
| sink.writeByte(0) // 0 = Not encoded. | ||
| sink.writeInt(encodedMessage.size.toInt()) | ||
| sink.writeAll(encodedMessage) | ||
| } else { |
There was a problem hiding this comment.
Should we explicitly check that gzip type is set instead of this being the else clause? Maybe else should throw unsupported operation exception or something like that.
There was a problem hiding this comment.
I don't think that's needed. The grpcEncoding.toGrpcEncoder() call will throw if the encoding is not supported:
| /** | ||
| * If true gRPC responses which are larger than the minGzipSize will be compressed. | ||
| */ | ||
| val grpcGzip: Boolean = false, |
There was a problem hiding this comment.
Yes, I'd lean towards the encoding enum, not just a boolean
misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt
Outdated
Show resolved
Hide resolved
| writer.write(HelloRequest("localhost")) | ||
| writer.close() | ||
|
|
||
| assertEquals( |
There was a problem hiding this comment.
Can you add an assertion on the min size and the actual size? Not just a snapshot assertion on expected output (still keep that but add the extra assertion).
There was a problem hiding this comment.
The actual size is asserted in helloRequestSize, but I can move it here if that makes the test more readable.
There was a problem hiding this comment.
Added. I simply repeated the same assertion multiple times:
val message = HelloRequest("localhost")
val encodedMessage = HelloRequest.ADAPTER.encode(message)
assertEquals(encodedMessage.size, 11)
Can definitely create a helper function if needed.
| log.log( | ||
| level = mapper.loggingLevel(th), | ||
| th = th, | ||
| tags = *mdcTags.toTypedArray(), | ||
| ) { "exception dispatching to $actionName" } |
There was a problem hiding this comment.
IDE auto formatted this because the line is too long.
|
merged as cd10157 |
Description
Currently, all gRPC responses are not compressed. This is not optimal for endpoints with huge responses as compression may save us a decent amount of network resource. This PR adds support to compress gRPC responses using gzip, which can be enabled by setting
WebConfig.grpcGzipto true.Testing Strategy
GrpcSourceSinkTest.GrpcConnectivityTestdoesn't run in CI due to GrpcClientProviderTest is flaky #1853. So I ran it locally and confirmed all test cases passed.Related Work
Rollout Strategy and Risk Mitigation
WebConfig.grpcGzipis false by default, so users won't see surprises when they upgrade misk. They can manually turn it on if needed.Checklist