router: add x-envoy-overloaded header#1793
Merged
mattklein123 merged 2 commits intomasterfrom Oct 3, 2017
Merged
Conversation
This change reduces retry explosion: 1) Retry is never performed if local circuit breaking occurred. 2) If traffic was dropped (circuit breaking or maintenance mode), Envoy will set an x-envoy-overloaded header. (Right now it is set to "true" which has no real meaning. We might define more values in the future). 3) Envoy will not retry if it sees the x-envoy-overloaded header set by upstream. Of course this header can be propagated downstream by the caller if desired and application other than Envoy can set this header. However, even in the Envoy <-> Envoy single hop case this can reduce retry explosion when under duress. One downside of this change is that a single bad upstream might create a situation in which retries do not happen that might otherwise succeed. In general, it seems better to err on the side of less retries and bad single hosts should be ejected via proper outlier detection policies. Signed-off-by: Matt Klein <mklein@lyft.com>
mattklein123
commented
Oct 2, 2017
| return RetryStatus::NoOverflow; | ||
| } | ||
|
|
||
| if (!runtime_.snapshot().featureEnabled("upstream.use_retry", 100)) { |
Member
Author
There was a problem hiding this comment.
This was moved for perf reasons only. No point in doing lookup unless we are actually going to retry.
danielhochman
previously approved these changes
Oct 2, 2017
htuch
previously approved these changes
Oct 3, 2017
Member
htuch
left a comment
There was a problem hiding this comment.
LGTM, some nits, feel free to ship whenevs.
| bool RetryStateImpl::wouldRetry(const Http::HeaderMap* response_headers, | ||
| const Optional<Http::StreamResetReason>& reset_reason) { | ||
| // We never retry if the overloaded header is set. | ||
| if (response_headers && response_headers->EnvoyOverloaded() != nullptr) { |
Member
There was a problem hiding this comment.
Tiny nit: the first clause is a pointer and relies on implicit null checking, the second is also presumably a pointer and does an explicit nullptr comparison. So, might be nice to be consistent there.
source/common/router/router.cc
Outdated
| // This is a customized version of send local reply that allows us to set the overloaded | ||
| // header. | ||
| Http::Utility::sendLocalReply( | ||
| [&](Http::HeaderMapPtr&& headers, bool end_stream) -> void { |
| upstream_host->stats().rq_error_.inc(); | ||
| } | ||
| Http::Utility::sendLocalReply(*callbacks_, stream_destroyed_, code, body); | ||
| sendLocalReply(code, body, dropped); |
Member
There was a problem hiding this comment.
I think there are some other sites for sendLocalReply in this file, but maybe not all apply.
Member
Author
|
@htuch updated |
htuch
approved these changes
Oct 3, 2017
rshriram
pushed a commit
to rshriram/envoy
that referenced
this pull request
Oct 30, 2018
…" (envoyproxy#1793) This reverts commit e9cdb20.
mathetake
pushed a commit
that referenced
this pull request
Mar 3, 2026
**Description** Extend time-to-first-token histogram bucket boundaries from 10s max to 60s max to capture slow responses from upstream providers. Adds buckets: 15.0, 20.0, 30.0, 45.0, 60.0. Signed-off-by: Vein Kong <vk@modular.com>
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
3 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.
This change reduces retry explosion:
Envoy will set an x-envoy-overloaded header. (Right now it is
set to "true" which has no real meaning. We might define more
values in the future).
set by upstream. Of course this header can be propagated
downstream by the caller if desired and application other than
Envoy can set this header. However, even in the Envoy <-> Envoy
single hop case this can reduce retry explosion when under
duress.
One downside of this change is that a single bad upstream might
create a situation in which retries do not happen that might
otherwise succeed. In general, it seems better to err on the side
of less retries and bad single hosts should be ejected via proper
outlier detection policies.
Signed-off-by: Matt Klein mklein@lyft.com