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% 54.84% -5.54%
==========================================
Files 411 594 +183
Lines 76609 80133 +3524
==========================================
- Hits 46253 43948 -2305
- Misses 27794 33149 +5355
- Partials 2562 3036 +474 ☔ View full report in Codecov by Sentry. |
| 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.
…or non-(2xx/409/400) response
jiekun
force-pushed
the
feature/respect-retry-after
branch
from
cea4491
to
3a6c7bd
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:
Signed-off-byline. Usegit commit -sto includeSigned-off-byyour commits. See this doc about how to sign your commits.make testto run all tests locally.make check-allto run all linters locally.Further checks are optional for External Contributions:
Include a link to the GitHub issue in the commit message, if issue exists.
Mention the change in the Changelog. Explain what has changed and why. If there is a related issue or documentation change - link them as well.
Tips for writing a good changelog message::
After your pull request is merged, please add a message to the issue with instructions for how to test the fix or try the feature you added. Here is an example
Do not close the original issue before the change is released. Please note, in some cases Github can automatically close the issue once PR is merged. Re-open the issue in such case.
If the change somehow affects public interfaces (a new flag was added or updated, or some behavior has changed) - add the corresponding change to documentation.