RateLimiter rate type does not match TocketBucketGroup rate about play-guard HOT 16 CLOSED

Regarding the number of buckets: the only limit I can think of is system resources. The bucket map only contains buckets that are not full, so under normal conditions it should be empty. I did some stress tests a while ago and the impact on throughput seemed to be negilible. I've been using this lib in production with multiple TokenBucketGroup instances for many years and have not encountered any memory or other resource consuption problems so far.

from play-guard.

(assuming you were waiting for me to close it)

from play-guard.

If I may also ask at the same time, I am curious if you have a general recommendation / experience on the maximum number of buckets you would generally recommend this solution for?

Thanks for this great library!

from play-guard.

thanks @tarmath. The rate parameter in the constructor of TokenBucketGroup was changed to Double because it is later used to calculate the ratePerNano, it saves a call to toDouble. But with the require(rate >= 0.000001f) it is questionable if ratePerNano has to be a Double in the first place.

from play-guard.

I still think it's safer to keep it as is, somebody might change or remove the require without taking taking this into account.

from play-guard.

It does seem a bit strange to keep it a mix of Float and Double rather than consolidate on one (no matter which), but happy to close this issue regardless as that's not something that would cause issues in this context anyhow...

from play-guard.

just noticed, similarly: Bucket size is Int, but consume returns a Long...

from play-guard.

bucket size seems to be Long, too:

class TokenBucketGroup(size: Long, rate: Double, clock: Clock = CurrentTimeClock)

What am I missing here?

from play-guard.

Right, but when creating these through RateLimiter, size is Int and rate is Float:

https://github.com/sief/play-guard/blob/master/module/app/com/digitaltangible/playguard/RateLimitActionFilter.scala#L152-L156

yet the only thing these 2 arguments do, is be passed to the TockenBucketGroup...

from play-guard.

As a sidenote, I am currently returning in the response headers the limit and number of remaining tokens, somewhat mimicking github's v3 APIs as documented here:

https://developer.github.com/v3/rate_limit/#response

In order for me to return the limit (aka bucket size), I currently have to hold that value somewhere else since it is not exposed by the RateLimiter / TocketBucketGroup.

Simply declaring size as val would be enough to make this work. Perhaps something you might be interested in changing?

Thank you again!

from play-guard.

right, the RateLimiter should use Long for the size, there is no require limit on size anyway.

Declaring size as val is fine for me, I'd even make rate a val as well.

Are you interested in making a PR?

from play-guard.

I already would have, but my hands are tied unfortunately...

from play-guard.

ok, done, I changed RateLimiter.rate to Double as well ;)

from play-guard.

oh great! fantastic! much appreciated! 🙏

from play-guard.

can these changes be released?

from play-guard.

releasing to maven central is quite a hassle, I'll see what I can do next week.

from play-guard.