Expose the current Limit() on existing ratelimiters #6235
Conversation
A rather minor but wide-touching change, but it will be useful for a later PR.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| return tm.limiter.Limit() | ||
| return float64(tm.limiter.Limit()) |
There was a problem hiding this comment.
though I'd like to add this....... it leads to a massive explosion in "well this is also the same value, so change that too..." chain of changes, with no good place to cut things off.
I think changing all these is very much worth it, float64 is rather obviously a worse type. But I'm not tackling that right now.
Groxx
changed the title
| // limiter. This is currently only used in tests. | ||
| func NewSimpleRateLimiter(t *testing.T, rps int) *RateLimiter { | ||
| t.Helper() // ensure a T has been passed |
There was a problem hiding this comment.
seems very likely that this will only ever be used in tests. almost all of our real limiters are dynamically configurable.
There was a problem hiding this comment.
I'd consider s/Simple/Test/ because it presumably means this has testing functionality and that makes it considerably easier to find when casting about for functions to use in tests
| func (rl *RateLimiter) Limit() float64 { | ||
| func (rl *RateLimiter) Limit() rate.Limit { |
There was a problem hiding this comment.
Had to change because this is also a quotas.Limiter, and the signature is incompatible.
|
Would you mind adding a line or two about the why to the PR description? The change makes sense, but it's not immediately clear from reading the description and code what the goal of your change is, besides that it seemed like a good thing to expose. |
yep, completely agreed, it was unclear. updated! |
A rather minor change, but it will be useful for a later PR:
I'm building some logic into the global ratelimiter that needs to know what the "local" limit would be, because that value is assumed to be "safe" in the absence of other info.
The easiest and IMO cleanest way to implement this is to allow interrogating the existing limiters (the global-fallback one specifically), rather than re-implementing the "get the limit and divide by the ring size" logic separately, because that logic may drift.
More generally: I think we'll eventually either get rid of
quotas.Limiter, or turn it into a complete read-only subset ofclock.Ratelimiter(because very few things need to write to limits). This is an incremental step towards that.I also attempted to change all the other things touching this to
rate.Limit, but... that turned out to be rather large. I think it's worth doing, primitive types are unnecessarily vague in nearly all cases, but not right now.