Make configured retry backoff authoritative - #4528
Conversation
| // retry duration shouldn't exceed the configured timeout period (skip cap when Timeout==0, which means no timeout) | ||
| if c.Cfg.HTTPClient.Timeout > 0 { | ||
| retryVal = math.Min(float64(c.Cfg.HTTPClient.Timeout/time.Second), retryVal) |
There was a problem hiding this comment.
this one was a bit false in the implem because it was capped by "c.Cfg.HTTPClient.Timeout" which is the timeout of the httpRequest not the "configured timeout period"
I replaced it with
if deadline, ok := ctx.Deadline(); ok && *retryDuration >= time.Until(deadline) {
return resp, requestErr
}
| retryDuration := time.Duration(retryVal) * time.Second | ||
|
|
||
| if isRateLimited { | ||
| retryDuration = max(retryDuration, parseRateLimitReset(response.Header.Get(rateLimitResetHeader))) |
There was a problem hiding this comment.
this one was a bit false in the implem because it was capped by "c.Cfg.HTTPClient.Timeout" which is the timeout of the httpRequest not the "configured timeout period"
I replaced it with
if deadline, ok := ctx.Deadline(); ok && *retryDuration >= time.Until(deadline) {
return resp, requestErr
}
| if err != nil || response.StatusCode == 429 || response.StatusCode >= 500 { | ||
| // Calculate the retry val (base * multiplier^retryCount) | ||
| retryVal := c.Cfg.RetryConfiguration.BackOffBase * math.Pow(c.Cfg.RetryConfiguration.BackOffMultiplier, float64(retryCount)) | ||
| // retry duration shouldn't exceed the configured timeout period (skip cap when Timeout==0, which means no timeout) | ||
| if c.Cfg.HTTPClient.Timeout > 0 { | ||
| retryVal = math.Min(float64(c.Cfg.HTTPClient.Timeout/time.Second), retryVal) |
There was a problem hiding this comment.
I inlined the expression, because the triple nested if was too convoluted to my taste
jaxalo
added
the
changelog/Changed
| if resp.Body != nil { | ||
| resp.Body.Close() | ||
| } |
1 participant
What does this PR do?
This stacked PR builds on #4517 and makes configured retry backoff authoritative.
Notable changes
X-Ratelimit-Resetvalue completely replaced configured backoff for a 429 response, even when it requested a shorter delay. This PR usesmax(configured exponential backoff, X-Ratelimit-Reset), so the server may extend but never shorten the configured delay.HTTPClient.Timeoutcould shorten configured retry delays even though it is the timeout for an individual HTTP request. It no longer caps backoff.Small changes
int64seconds could also overflow when converted totime.Duration. Both now fall back to configured backoff.HTTPRetryTimeoutbudget waited until the deadline expired. Because the previous request has already returned, waiting is useless when the delay alone would exhaust the remaining budget; the client now returns that response immediately without starting another attempt.retryCount >= MaxRetries; five retries still allow six total attempts.Preserved behavior
Retry-Afterhandling and transport-error retries remain out of scope.Jitter is introduced by the stacked PR #4517. This PR applies it after selecting the authoritative delay.
Additional Notes
Validated with:
go test ./api/datadoggo test ./api/datadog -run "^TestRetryJitter$" -count=20go test ./api/datadog -run "^TestRetryDeadlineAllowsInFlightAttemptToFinish$" -count=10cd tests && go test ./apigit diff --checkReview checklist