Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:5.xfromkbond:add-get-limit
Oct 20, 2020

Conversation

@kbond
Copy link
Member

@kbondkbond commentedOct 16, 2020
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?no
Deprecations?no
TicketsFix#38489
LicenseMIT
Doc PRtodo

After discussing#38489 with@wouterj, he agreed we should addLimit::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 theRateLimit Header Fields for HTTP RFC.

sstok and wouterj reacted with thumbs up emoji
@javiereguiluz
Copy link
Member

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 aReservation, but it's not for the same purpose). So ... maybe we don't need neitherLimit norQuota and we could embed this login into the limiter object itself?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedOct 16, 2020
edited
Loading

@javiereguiluz I asked the same question to@wouterj and he gave a nice answer already, check#38257 (comment)

javiereguiluz reacted with thumbs up emoji

@wouterj
Copy link
Member

wouterj commentedOct 16, 2020
edited
Loading

This information is the last missing information from therate limiting RFC:

This proposal defines syntax and semantics for the following header fields:

o "RateLimit-Limit": containing the requests quota in the time window;
o "RateLimit-Remaining": containing the remaining requests quota in the current window;
o "RateLimit-Reset": containing the time remaining in the current window, specified in seconds or as a timestamp;

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 theLimit/Quota object, I think it makes most sense to expose the limit value also through this object.

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

javiereguiluz commentedOct 16, 2020
edited
Loading

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 calledPolicy instead ofQuota. Maybe not ... and maybe it's because I'm not native in English, butQuota doesn't feel entirely right to me.

@kbond
Copy link
MemberAuthor

kbond commentedOct 16, 2020
edited
Loading

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.

sstok reacted with laugh emoji

@javiereguiluz
Copy link
Member

javiereguiluz commentedOct 16, 2020
edited
Loading

After your comment, I re-read the RFC and now I think that:

  • We use "strategy" to refer to "fixed window", "token bucket", "sliding window", etc. But this might be renamed as "policy"
  • Instead ofLimit orQuota, the status object may be calledQuotaPolicy:
To help clients throttling their requests, servers may expose the counters used to evaluate quota policies via HTTP header fields.[...]A server MAY use one or more RateLimit response header fields defined in this document to communicate its quota policies.The returned values refers to the metrics used to evaluate if the current request respects the quota policy and MAY not apply to subsequent requests.Example: a successful response with the following header fieldsRateLimit-Limit: 10RateLimit-Remaining: 1RateLimit-Reset: 7

And the other "quota policy" examples also expose the entire state:

RateLimit-Limit: 10, 10;window=1, 50;window=60, 1000;window=3600, 5000;window=86400RateLimit-Limit: 10, 10;window=1;burst=1000, 1000;window=3600
sstok reacted with eyes emoji

@Mediagone
Copy link

At Google, they are using "quota" for API calls :https://developers.google.com/analytics/devguides/config/mgmt/v3/limits-quotas

@javiereguiluz
Copy link
Member

@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
Copy link
Contributor

ciaranmcnulty commentedOct 17, 2020
edited
Loading

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
Copy link
Member

wouterj commentedOct 17, 2020
edited
Loading

TheLimit object at this moment holds some data:

  • getLimit(): The actual limit for the current window (e.g.25, if the limit is 25 per minute or the like)
  • getRemainingTokens(): the quota left in the current window (called "remaining quota" by the RFC)
  • getRetryAfter(): the moment the window resets/the client should try to call again

In the RFC, they use:

  • "policy" to define the strategy (e.g. "fixed window", "token bucket", "sliding window", etc):policy="leaky bucket"
  • "request quota" to define the limit for the window/bucket:request-quota=4
  • "quota policy" to define the combination of "policy" and "request quota":100;window=60 ("An example policy of 100 quota-units per minute")

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 likeQuota,QuotaState,LimiterQuotas (as it holds all sorts of quotas, the quota policy, the request quota, the remaining quota, etc),LimiterState, etc

@kbond
Copy link
MemberAuthor

kbond commentedOct 17, 2020
edited
Loading

What about ?

  • QuotaStatus
  • Status
  • QuotaUsage
  • Usage

@javiereguiluz
Copy link
Member

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 reacted with thumbs up emoji

@ciaranmcnulty
Copy link
Contributor

@javiereguiluz Remaining sounds best to me

@kbond
Copy link
MemberAuthor

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 useLimiterState as@wouterj suggests above?

@derrabusderrabus added this to the5.2 milestoneOct 17, 2020
@craigh
Copy link

craigh commentedOct 18, 2020
edited
Loading

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

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:

  1. Allotment (noun), Allot , Allotted
  2. Allocation (noun), Allocate, Allocated
  3. Hit (noun), (hit is also a verb) so maybe MaximumHits (Hit seems like a logical extension of RateLimiter because you are limiting Hits)

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
Copy link

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.
I'd just go with
CurrentLimit
because it returns the actual available limit at any time. It does not have to be aware of Quota, Policy or QuotaPolicy.

@wouterj
Copy link
Member

wouterj commentedOct 19, 2020
edited
Loading

Thanks for the insights and perspectives from the native english speakers!

Let's go withRateLimit. That fits the current component terminology and@kbond pointed out in a Slack conversation that this also closely resembles the HTTP headers (RateLimit-Limit ->$rateLimit->getLimit()).

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 :)

@kbondkbond changed the title[RateLimiter] rename Limit to Quota and add Quota::getLimit()[RateLimiter] rename Limit to RateLimit and add RateLimit::getLimit()Oct 19, 2020
@kbond
Copy link
MemberAuthor

Ok, I think this PR is ready.

@fabpot
Copy link
Member

Thank you@kbond.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

[RateLimiter] the standard "X-RateLimit-Limit" HTTP header is not easy to implement

11 participants

@kbond@javiereguiluz@nicolas-grekas@wouterj@Mediagone@ciaranmcnulty@craigh@bytesystems@fabpot@derrabus@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp