Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[RateLimiter] rename Limit to RateLimit and add RateLimit::getLimit()#38608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedOct 16, 2020
I've looked in the RateLimiter implementations of other languages (Golang, Java, Python, Rust and C#) and I couldn't find an equivalent object. (Golang has a |
nicolas-grekas commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@javiereguiluz I asked the same question to@wouterj and he gave a nice answer already, check#38257 (comment) |
wouterj commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This information is the last missing information from therate limiting RFC:
The limit value is the only value from these 3 that is based on configuration and not current limiter state. However, as the other information is exposed by the As mentioned in the issue, reference API rate limiters implement this RFC completely as well. See e.g.Laravel andGitHub. So I agree that we, as Symfony, should also have a DX-friendly way of exposing all information required to implement that RFC in your application. |
javiereguiluz commentedOct 16, 2020
@wouterj I agree that the Limiter object must provide a way to get these three HTTP headers in a trivial way ... but I don't understand why can't we provide that in the Limiter object itself. |
wouterj commentedOct 16, 2020
I tried to answer that one in the commented referenced by@nicolas-grekas#38257 (comment) Besides that comment, there is a fourth reason now: For compound limiters, the headers/returned state is of the limiter that is closest to reach its limit. If this information is fetched using getters, each getter on the compound limiter has to know which limiter is the closest to reach its limit. |
javiereguiluz commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Understood. We need this object. Reading the RFC (https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html) I wonder if this object should be called |
kbond commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
When developing apersonal POC rate limiting library, I spent an inordinate amount of time agonizing over the terminology. "Quota" was the best term I could think of the describe the current "state" of the rate limiter. That being said, I'm not 100% sold on Quota either... but had to move on... :) I don't think Policy makes sense, because the "Policy" is a property of this "state" (although we don't include this currently). Edit: re-reading the RFC, I do see the argument for calling this Policy. Some parts of the RFC are confusing and a lot of terms are used interchangeably. To me, a "Policy" is a static thing like "fixed window, 5/requests/minute" and shouldn't include dynamic info like remaining requests and reset at time. |
javiereguiluz commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
After your comment, I re-read the RFC and now I think that:
And the other "quota policy" examples also expose the entire state: |
Mediagone commentedOct 16, 2020
At Google, they are using "quota" for API calls :https://developers.google.com/analytics/devguides/config/mgmt/v3/limits-quotas |
javiereguiluz commentedOct 17, 2020
@Mediagone yes, but when they refer to "how much you can use". We agree that's called "quota". But the "status object" we're discussing in this PR is about "how much usage is left". In the RFC it looks like they call this "QuotaPolicy". |
ciaranmcnulty commentedOct 17, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just on a semantic basis, if it's a RateLimiter then it is applying a Limit to the Rate, so I don't see any problem with that naming If Limit::getLimit seems redundant then maybe call it Limit::asInt or similar to reflect what it's really doing (converting a value object to a scalar) |
wouterj commentedOct 17, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The
In the RFC, they use:
So imho, "quota policy" is a static configured thing and not a "current state". This object in Symfony holds the data for all 3 headers defined in the RFC and the RFC doesn't define a word for this. I think we should call it something like |
kbond commentedOct 17, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What about ?
|
javiereguiluz commentedOct 17, 2020
For English speakers: which are the words that usually go with "quota"? Used? Consumed? Remaining? Left? If that's the case, we could use e.g. RemainingQuota |
ciaranmcnulty commentedOct 17, 2020
@javiereguiluz Remaining sounds best to me |
kbond commentedOct 17, 2020
My issue with Remaining, is that is just one "property" of this object. Maybe we should drop this new "Quota" term and stick with the current component terminology and use |
craigh commentedOct 18, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
A quota is typically a quotaof something (a quota of candy), but to my knowledge has no directly associated verbs. You might say you have 4 pieces of candyremaining in your quota. A quota is assigned to someone to indicate an allotted amount that one isindicated to receive, or more importantly,required to contribute. It can also be used as aminimum amount one is to receive. It also has some very minor political overtones. In this light, I find Quota an odd choice and make these suggestions:
So far, I like Allocation the best because it has some parallels with CPU memory allocation or resource allocation in CS (at least I think it does, my degree is not in CS!) I would guess this is where theReservation you mentioned above comes from as well. After re-reading this post again (20+ minutes later) and reading my answer, I think I prefer "Limit" and "Remaining" best. |
bytesystems commentedOct 19, 2020
Neither status nor state fit. Status is a final result of an action -> 404, state on the other hand describes a stage in a process. |
wouterj commentedOct 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the insights and perspectives from the native english speakers! Let's go with Let's not forget that it's very easy to rename a class later on (even the BC layer is very solid when renaming classes). So let's not block this PR any longer. If someone dislikes the new term, please consider opening a new PR afterwards :) |
kbond commentedOct 19, 2020
Ok, I think this PR is ready. |
fabpot commentedOct 20, 2020
Thank you@kbond. |
Uh oh!
There was an error while loading.Please reload this page.
After discussing#38489 with@wouterj, he agreed we should add
Limit::getLimit()but this method name was unfortunate. We decided to rename theLimitobject toRateLimitwhich, IMO, is a better name for the object and is inline with how this concept is described in theRateLimit Header Fields for HTTP RFC.