Skip to content
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

x/time/rate: expose the number of remaining tokens #50035

Open
ryanmcnamara opened this issue Dec 7, 2021 · 14 comments
Open

x/time/rate: expose the number of remaining tokens #50035

ryanmcnamara opened this issue Dec 7, 2021 · 14 comments

Comments

@ryanmcnamara
Copy link

@ryanmcnamara ryanmcnamara commented Dec 7, 2021

rate.Limiter holds the remaining tokens in a bucket here https://github.com/golang/time/blob/master/rate/rate.go#L59

The feature request is to expose this value in a read only way, perhaps as a public function Tokens.

The use case is that when using a limiter it would be nice to be able to see how many tokens remain without attempting to use one. For my specific case, I want to emit a metric from my kubernetes controller indicating how many tokens are left in the client side rate limiter for a kubernetes clientset (which uses a rate.Limiter under the hood). I think this should be useful in other situations as well though.

Open to any other workarounds as well!

I'd be happy to contribute this, the Tokens func is probably just

// Tokens returns the remaining tokens in the bucket
func (lim *Limiter) Tokens() float64 {
	lim.mu.Lock()
	defer lim.mu.Unlock()
	lim.advance(time.Now())
	return lim.tokens
}

I'll create a real PR (and get my contribution environment set up) if this idea sounds reasonable. Thanks!

@gopherbot gopherbot added this to the Unreleased milestone Dec 7, 2021
@seankhliao seankhliao removed the pkgsite label Dec 7, 2021
@seankhliao seankhliao changed the title x/time: Feature request: ability to read only remaining tokens in Limiter bucket proposal: x/time/rate: expose the number of remaining tokens Dec 7, 2021
@seankhliao seankhliao removed this from the Unreleased milestone Dec 7, 2021
@seankhliao seankhliao added this to the Proposal milestone Dec 7, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Dec 8, 2021
@rsc rsc moved this from Incoming to Active in Proposals Dec 8, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 8, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Dec 15, 2021

/cc @Sajmani

@Sajmani
Copy link
Contributor

@Sajmani Sajmani commented Dec 15, 2021

If we expose the tokens, I think we'd instead expose TokensAt(time.Time), externalizing the time.Now() to the caller.

I'm curious about the metric you want to expose, though. You can't really interpret Tokens without Burst and Limit. Since we expose SetBurst and SetLimit, in theory you'd want to read tokens, burst, and limit simultaneously with the lock held. That suggests we might want a single method StateAt(time.Time) (limit Limit, burst int, tokens float64). But perhaps this is overkill, since most use cases never call SetBurst or SetLimit.

Assuming you're OK calling TokensAt separately from burst & limit (or just saving those from construction), TokensAt seems fine. (And maybe we should include Tokens as a wrapper for TokensAt(time.Now()) for completeness).

@deefdragon
Copy link

@deefdragon deefdragon commented Dec 15, 2021

On surface I think this would be a good addition, but part of me feels that the default rate limiter is becoming too much of a catch-all right now.

Beyond metrics, I can see this being used to allow an N-priority messages passed the rate limiter only if there are M tokens remaining. Then Higher-priority messages could still pushed through up until the rate limiter is completely full even if swamped with lower priority messages. However, this example feels like it might be better implemented with a custom rate limiter where you can pass options to the reserve, and in a safe manner.

@Sajmani
Copy link
Contributor

@Sajmani Sajmani commented Dec 16, 2021

@ryanmcnamara
Copy link
Author

@ryanmcnamara ryanmcnamara commented Dec 16, 2021

I'm curious about the metric you want to expose, though. You can't really interpret Tokens without Burst and Limit.

True, but most of the time I see this being used burst and limit aren't being changed. Narrowing to just the metric case, I'm probably going to be viewing this as a line over time, and changes to qps and limit would be very obvious (qps change means line recovers faster, limit change means line maxes out higher).

I realize that's a pretty specific justification for my use case, but just wanted to point out that a best effort mechanism was all I was looking for. Happy to see the api expanded though like you describe @Sajmani

@rsc
Copy link
Contributor

@rsc rsc commented Jan 5, 2022

It sounds like adding Tokens by itself is not useful, and then it's unclear how much more API to add after that.

Given that rate.Limiter is <50 lines of code, perhaps it would make sense for limiters with special requirements to just start there (or not) and live outside the x repos?

@ryanmcnamara
Copy link
Author

@ryanmcnamara ryanmcnamara commented Jan 5, 2022

@rsc rate.Limiter and the code it uses is 400 lines, not 50 (afaict everything in the file I linked above is for it)
I'm not sure why you say Tokens alone isn't useful, it's useful for the reasons mentioned above
The current pitch is to just add the Tokens or TokensAt func, not more api than that, so it seems clear to me

@Sajmani
Copy link
Contributor

@Sajmani Sajmani commented Jan 6, 2022

@rsc Given that rate.Limiter already exposes Burst and Limit as thread-safe accessors, I think it's reasonable to expose Tokens as well. Since Tokens is calculated relative to a specific time using Limiter.advance, I propose defining Tokens as TokensAt(time.Now()), and TokensAt(t) as returning the newTokens from advance(t).

@rsc
Copy link
Contributor

@rsc rsc commented Jan 12, 2022

What is the new API being proposed?

@Sajmani
Copy link
Contributor

@Sajmani Sajmani commented Jan 14, 2022

Add TokensAt method to resolve this issue:

func (lim *Limiter) TokensAt(t time.Time) float64 {
  lim.mu.Lock()
  _, _, tokens := lim.advance(t) // does not mutute lim
  lim.mu.Unlock()
  return tokens
}

Optionally add Tokens method for consistency with the rest of the API:

func (lim *Limiter) Tokens() float64 {
  return lim.TokensAt(time.Now())
}

@Sajmani Sajmani self-assigned this Jan 14, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jan 19, 2022

Does anyone object to the API in #50035 (comment)?

@ryanmcnamara does that API solve your use case? Thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 27, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Jan 27, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals Feb 2, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Feb 2, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/time/rate: expose the number of remaining tokens x/time/rate: expose the number of remaining tokens Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

6 participants