-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 |
@javiereguiluz I asked the same question to @wouterj and he gave a nice answer already, check #38257 (comment) |
This information is the last missing information from the rate 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 and GitHub. 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. |
@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. |
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. |
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 |
When developing a personal 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. |
After your comment, I re-read the RFC and now I think that:
And the other "quota policy" examples also expose the entire state:
|
At Google, they are using "quota" for API calls : https://developers.google.com/analytics/devguides/config/mgmt/v3/limits-quotas |
@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". |
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) |
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 |
What about ?
|
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 |
@javiereguiluz Remaining sounds best to me |
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 |
A quota is typically a quota of something (a quota of candy), but to my knowledge has no directly associated verbs. You might say you have 4 pieces of candy remaining in your quota. A quota is assigned to someone to indicate an allotted amount that one is indicated to receive, or more importantly, required to contribute. It can also be used as a minimum 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 the Reservation 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. |
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. |
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 :) |
Ok, I think this PR is ready. |
Thank you @kbond. |
After discussing #38489 with @wouterj, he agreed we should add
Limit::getLimit()
but this method name was unfortunate. We decided to rename theLimit
object toRateLimit
which, IMO, is a better name for the object and is inline with how this concept is described in the RateLimit Header Fields for HTTP RFC.