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

/cc @Sajmani

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

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.

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

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?

@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

@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).

What is the new API being proposed?

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())
}

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

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

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

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

Implemented in https://go-review.googlesource.com/c/time/+/423042

Change https://go.dev/cl/423042 mentions this issue: rate: add TokenAt and Tokens methods to Limiter.