Skip to content

Conversation

@khepin
Copy link
Contributor

@khepin khepin commented Feb 21, 2024

Context

Sometimes a rate limit doesn't map directly to an increase of 1 at a time. Some examples would be:

  • a limit on bandwidth used
  • a limit on # of updates across batch calls where each call could mean multiple "hits"
  • a limit on time spent on some operations

For such rate limits today, the only available options via the RateLimiter class would be to loop and call ->hit for as many times as we want to increment.
This becomes impractical when the number we want to increment by increases since each hit requires updating a cache server.

Where do we use this?

At Square, we use a mechanism in queued jobs where we give higher / lower priority to certain users depending how much compute they've used across multiple jobs.
That is decided by counting the time spent across all queue workers up to a given limit over time. So rate limiting based not on each 1 job counting for 1 but instead counting for how many milliseconds were spent on the job.

So for a job running in 250ms, it would be easier to increment the number of attempts by 250 directly.

Proposal

Introduce additional increment public method on the RateLimiter class to enable incrementing by a custom amount.

@khepin khepin changed the title Custom rl increase Custom RateLimiter increase Feb 21, 2024
@driesvints driesvints changed the title Custom RateLimiter increase [10.x] Custom RateLimiter increase Feb 22, 2024
@bert-w
Copy link
Contributor

bert-w commented Feb 22, 2024

Is adding an optional third parameter $step = 1 to hit() a breaking change? if not, i think that is preferred because increment() now just is an alias.

@ahinkle
Copy link
Contributor

ahinkle commented Feb 23, 2024

I see the need here, but is method names like hit() and increment() ambiguous for developers?

@khepin
Copy link
Contributor Author

khepin commented Feb 23, 2024

Do you have a suggestion you think is clearer?
I'm struggling to come up with something else

@Anton5360
Copy link
Contributor

Do you have a suggestion you think is clearer? I'm struggling to come up with something else

I would suggest hitTimes naming.

This way we don`t have two ambiguous method names but just kind of "expand" the existing one

@taylorotwell taylorotwell merged commit e45d424 into laravel:10.x Feb 25, 2024
@christophrumpel
Copy link
Contributor

christophrumpel commented Feb 29, 2024

Hey, trying to understand how the new hit/increment method works. Let's say I have a limiter:

RateLimiter::for('api', function (Request $request) {
        return Limit::perMinute(5)->by($request->user()?->id ?: $request->ip());
});

What would be the key for the increment method now?

Or do you know if this only works when used like this?

if (RateLimiter::tooManyAttempts('send-message:'.$user->id, $perMinute = 5)) {
    return 'Too many attempts!';
}
 
RateLimiter::hit('send-message:'.$user->id);

@khepin
Copy link
Contributor Author

khepin commented Feb 29, 2024

I have only ever used it in the second way you show here so I would say that right now it only works when used that way yes.

@christophrumpel
Copy link
Contributor

Ok, thanks, I thought so, too, since this was the only way I made it work. Still great work 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants