router: support offseting downstream provided grpc timeout#6628
router: support offseting downstream provided grpc timeout#6628snowp merged 11 commits intoenvoyproxy:masterfrom
Conversation
This adds support for modifying the grpc-timeout provided by the downstream by some offset. This is useful to make sure that Envoy is able to see timeouts before the gRPC client does, as the client will cancel the request when the deadline has been exceeded which hides the timeout from the outlier detector. Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
You anticipated all my review feedback, so LGTM. Thanks!
|
/retest |
|
🔨 rebuilding |
|
@snowp darn, looks like this needs master merge. |
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
It's unfortunate that the release notes files makes merge conflicts so common, though I don't really know how you'd do otherwise |
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
@mattklein123 Wanna give this one a quick look? |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small question/comment. Thank you!
/wait
source/common/router/router.cc
Outdated
| std::chrono::milliseconds grpc_timeout = Grpc::Common::getGrpcTimeout(request_headers); | ||
| if (route.grpcTimeoutOffset()) { | ||
| grpc_timeout = | ||
| std::max(std::chrono::milliseconds::zero(), (grpc_timeout - *route.grpcTimeoutOffset())); |
There was a problem hiding this comment.
Presumably std::chrono::milliseconds is signed and this won't wrap around? Can you potentially add a comment?
Also, if we end up setting this to zero, it will effectively disable the timeout, right? Should the default in this case actually be to use the supplied timeout in the header? That seems safer? I could go either way. Whatever you choose, can you update the docs to say what happens if the value is larger than the supplied header?
There was a problem hiding this comment.
Accoriding to https://en.cppreference.com/w/cpp/chrono/duration the value is signed:
std::chrono::milliseconds | duration</*signed integer type of at least 45 bits*/, std::milli>
There's code further down that handles the case where the timeout ends up at zero: it changes it to the value of route.maxGrpcTimout. That said, it does seem safer to not apply the offset in that case, because it would end up increasing the timeout instead of decreasing it which is likely the intended result
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Snow Pettersen <snowp@squareup.com>
|
/retest |
|
🔨 rebuilding |
* master: thread: remove ThreadFactorySingleton (envoyproxy#6658) router: support offseting downstream provided grpc timeout (envoyproxy#6628) router: defer per try timeout until downstream request is done (envoyproxy#6643) update bazel readme for clang-format-8 on mac (envoyproxy#6660) Implement some TODOs in quic_endian_impl.h (envoyproxy#6644) docs: add aspell to mac dependencies to fix check format script (envoyproxy#6661) config: fix delta xDS's use of (un)subscribe fields, more explicit protocol spec (envoyproxy#6545) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
3 participants
This adds support for modifying the grpc-timeout provided by the
downstream by some offset. This is useful to make sure that Envoy is
able to see timeouts before the gRPC client does, as the client will
cancel the request when the deadline has been exceeded which hides the
timeout from the outlier detector.
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Low, new optional feature
Testing: Added UT test cases
Docs Changes: Proto docs
Release Notes: added version note
Fixes #6566