vmagent: remote write respect Retry-After in header #6124
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6124 +/- ##
==========================================
- Coverage 60.37% 58.32% -2.06%
==========================================
Files 411 609 +198
Lines 76609 82852 +6243
==========================================
+ Hits 46253 48322 +2069
- Misses 27794 31311 +3517
- Partials 2562 3219 +657 ☔ View full report in Codecov by Sentry. |
app/vmagent/remotewrite/client.go
Outdated
| var retryAfterDuration time.Duration | ||
|
|
||
| // Retry-After could be in "Mon, 02 Jan 2006 15:04:05 GMT" format. | ||
| parsedTime, err := time.Parse(http.TimeFormat, retryAfterString) |
There was a problem hiding this comment.
I think header parsing should be a separate function. It can be tested separately and calculateRetryDuration will be simplified signifacntly.
There was a problem hiding this comment.
It's now seperated from calculateRetryDuration and called in sendBlockHTTP
| // testFunc evaluate if the result of calculateRetryDuration is | ||
| // 1. >= expectMinDuration | ||
| // 2. <= expectMinDuration + 10% (see timeutil.AddJitterToDuration) | ||
| testFunc := func(name string, retryAfterString string, retryDuration, expectMinDuration time.Duration) { |
There was a problem hiding this comment.
wdyt about emulating retryDuration persistence in this test? This would make it similar to real use case - consecutive calls to calculateRetryDuration slowly increase the retryDuration.
There was a problem hiding this comment.
I love this idea, and it brings up some scenarios that I had already forgotten.
Please review the test cases in the Call calculateRetryDuration for multiple times block.
app/vmagent/remotewrite/client.go
Outdated
|
|
||
| // calculateRetryAfterDuration calculate the retry duration. | ||
| // 1. Calculate next retry duration by backoff policy (x2) and max retry duration limit. | ||
| // 2. If Retry-After exists in response header, use max(Retry-After duration, next retry duration). |
There was a problem hiding this comment.
// 2. If Retry-After exists in response header, use max(Retry-After duration, next retry duration).
This function doesn't know about this header. This info needs to be placed elsewhere
There was a problem hiding this comment.
I have simplified it and removed unrelated content.
jiekun
force-pushed
the
feature/respect-retry-after
branch
from
cea4491
to
3a6c7bd
Compare
…or non-(2xx/409/400) response
Co-authored-by: Zakhar Bessarab <me@zekker-dev.tk>
jiekun
force-pushed
the
feature/respect-retry-after
branch
from
19accfd
to
850ad71
Compare
Describe Your Changes
related issue: #6097
Changed
vmagentis changed into:Retry-Afterexists in the response header, usemax(Retry-After duration, next retry duration), which may exceed the max retry duration limit (1m). (Added in this PR)Docs
CHANGELOG.md.Note regarding backoff and
Retry-AfterRetry-Afteris lower than 1s (which is the initial value of backoff policy retry), it will be ignored.Retry-Afteris lower than 1m, thenext retrywill still act like the current backoff policy, multiplied by 2 until reaching 1m.Retry-Afteris higher than 1m, thenext retryalways uses the duration retrieved fromRetry-After.This modification will reduce the number of retries when
Retry-Afteris greater than 1m. WhenRetry-Afteris less than 1m, it will behave like the existing back-off policy (with a different initial retry duration).Also see the test cases for more info.
Checklist
The following checks are mandatory: