cortex icon indicating copy to clipboard operation
cortex copied to clipboard

Send Retry-After header with 429 response or 413 if samples exceed the burst of rate limiter

Open damnever opened this issue 3 years ago • 4 comments

What this PR does:

Which issue(s) this PR fixes: Fixes #

Checklist

  • [x] Tests updated
  • [ ] Documentation added
  • [x] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

damnever avatar Mar 15 '22 12:03 damnever

Retry-After doesn't support duration less than one second, this is hard since we may overload our service or lose some tokens..

damnever avatar Mar 16 '22 08:03 damnever

Hi damnever@ thank you for the PR, this is a nice change. May I ask you kindly add a CHANGELOG.md entry as well as add/update some tests? Thanks!

alvinlin123 avatar Apr 21 '22 18:04 alvinlin123

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jul 30 '22 22:07 stale[bot]

@alvinlin123 Somehow I have forgotten this patch. Please take another look.

damnever avatar Jul 31 '22 02:07 damnever

@damnever Thanks for the contribution! I think this change makes sense. But for the case where the ingestion path is already overloaded then retrying could make it worse. What about making it a flag on the distributor and we can choose the behavior of sending 429 retries or 413.

yeya24 avatar Nov 10 '22 21:11 yeya24

@yeya24 There are two changes:

  1. add Retry-After header to the existing 429 responses.
  2. change a part of 429 response to 413 since those requests contain too many samples, retry is useless until someone raises the limit(burst).

Maybe we should add an option to enable 413 since 413 may cause the client to drop samples without retrying.

damnever avatar Nov 11 '22 02:11 damnever

I see, thanks. If we only send 429 when it is retryable and otherwise 413 then I think it is a reasonable approach.

yeya24 avatar Nov 11 '22 17:11 yeya24

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 19 '23 05:03 stale[bot]