rate limit - add unit_multiplier#38529
Conversation
Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
|
||
| // The multiplier for the unit of time. | ||
| // This value multiplies the base unit of time to define a custom time period for the rate limit. | ||
| // For example, if the unit is set to MINUTE and the unit_multiplier is set to 5, the rate limit | ||
| // will be applied over a 5-minute period. | ||
| uint32 unit_multiplier = 4; |
There was a problem hiding this comment.
I actually cannot get the value of this requirement. And this change will also make the API a little weird because the requests_per_unit means the allowed requests number of single unit. It's conflict with the unit_multiplier.
cc @envoyproxy/api-shepherds
/wait-any
There was a problem hiding this comment.
Currently, we only support single time unit, i.e for 1 minute, 1 hour or 1 day etc., but this PR is to support for time units like mentioned below.
X requests over 30 seconds
X requests over 5 minutes
X requests over 30 minutes
X requests over 12 hours
X requests over 7 days
which would allow you to represent any time period.
There was a problem hiding this comment.
I know the requirement. But my worry is this new feature will make the API become weird and not sure if it deserves that.
There was a problem hiding this comment.
But because @mattklein123 think it's fine, so I won't block this PR and will let the matt to make the decision. Thanks for all these contributions. 🌷
There was a problem hiding this comment.
I can help with the implementation if the matt give the LGTM to the API. :)
There was a problem hiding this comment.
If we add the new field which have overrlapping feature with exist unit, we may should deprecate the old unit.
I have no strong preference here, actually. But I think unit_multiplier may be easier to implment. Orz.
There was a problem hiding this comment.
Fine, let's go head with the unit_multiplier.
I think we need add a comment and limit about the zero value of the unit_multiplier?
There was a problem hiding this comment.
awesome! I will polish it a bit and reach out to you.
There was a problem hiding this comment.
Following this thread, see a proposal at #40222 (comment) on a related issue.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
stale
|
not stale |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
wbpcode
added
no stalebot
|
not stale |
|
working on this here: #40830 |
5 participants
Commit Message: add a unit multiplier field to rls proto
Additional Description: so that we can configure rate limits for n minutes/days etc.
Risk Level: Low
Testing: todo
Docs Changes: todo
Release Notes: todo
Fixes: #33277