go
go copied to clipboard
golang
proposal: x/time/rate: add Sometimes type
This proposal makes acting on simple rate throttlers more ergonomic. It adds an API that we use internally (I'm not the author). Adding it will ease open-sourcing a project I'm involved in.
package rate
import "time"
// Sometimes performs an action occasionally.
// The public fields govern the behavior of Do, which performs the action.
// A zero Sometimes performs the action exactly once.
//
// C++ users familiar with the glog package can use this mechanism instead
// of LOG_FIRST_N, LOG_EVERY_N, LOG_EVERY_N_SEC:
//
// var sometimes = rate.Sometimes{First: 3, Interval: 10*time.Second}
// func Spammy() {
// sometimes.Do(func() { log.Info("here I am!") })
// }
type Sometimes struct {
First int // if non-zero, the first N calls to Do will run f.
Every int // if non-zero, every Nth call to Do will run f.
Interval time.Duration // if non-zero and Interval has elapsed since f's last run, Do will run f.
}
// Do runs f, as governed by First, Every, and Interval.
//
// The model is is a union of filters. The first call to Do
// always runs f. Subsequent calls run f if allowed by any
// one of the Sometimes fields.
//
// If Do is called multiple times simultaneously, calls will block
// and run serially. It is therefore intended for lightweight operations.
//
// Because a call to Do may block until f returns, if f causes Do
// to be called, it will deadlock.
func (s *Sometimes) Do(f func())
I'm the original author of this type inside Google. It's simple, and the API has been stable for many years. If it's useful, I'm fine seeing this added to the existing rate package.
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
Change https://go.dev/cl/421915 mentions this issue: x/time/rate: add Sometimes, which runs a function occasionally.
I have 2 things.
One: given the code comment specifically mentions it being implemented as a union of the filters, it would be more flexible as an array of filters/filter functions which are then unioned. This implementation would come at the cost of complexity, where I acknowledge this is explicitly trying to be simple. This more of a personal preference but I think worth discussing.
Two: I continue to be of the opinion that before we expand rate any further, there should be a common rate limiter interface defined that different rate limiters can then implement.
One: given the code comment specifically mentions it being implemented as a union of the filters, it would be more flexible as an array of filters/filter functions which are then unioned. This implementation would come at the cost of complexity, where I acknowledge this is explicitly trying to be simple. This more of a personal preference but I think worth discussing.
Two: I continue to be of the opinion that before we expand rate any further, there should be a common rate limiter interface defined that different rate limiters can then implement.
There are many existing callers to this in the Google codebase, overwhelmingly rate-limited log statements. In those callsites, the simplicity of the API (as documented, modeled after sync.Once) is a big win. It decouples the particular log library from the Sometimes policy. And to date these have been the kinds of throttles that we've needed for these operations: I don't think adding abstraction would help. Adding flexibility might have allowed additional kinds of callers, but at the cost of complicating the setup, possibly significantly. I think it's worth the 20 LOC to be added as-is.
I don't think adding Checks [] func() bool to the struct would make setup that much more complicated as it could just be ignored if nil, but in retrospect, I also can not come up with much of a reasonable use of a Checks array where you wouldn't be better off just using some other mechanism. Also, adding the above can be done later if shown useful, so I withdraw my first remark.
I still believe in my second point that a common interface should be described before more changes to the rate package are added, but I can see this easily being made interface compatible to almost anything if one is ever introduced, so that's a bit of a consolation.
I have one final extra point. Is there similar code or similar acting helpers that are known elsewhere outside of google? I can see this being useful, but I want to make sure it meets the "address an important issue for many people" standard.
How often is this used for something other than a rate-limiting log statement? I'm curious whether it might belong nearer to logging if it's really log-specific.
The vast majority of Google callsites use this for logging. How would putting it closer to logging look like, though? Just copy the current type into packages log (and glog)? Does that need another proposal?
(BTW, while gathering this data, regexp lookaround assertions would have come in handy, though not strictly necessary.)
I forgot to mention, though, that it's also used to throttle t.Log and ctxlog. Those have similar requirements, but it would be odd to add Sometimes to packages testing and again to ctxlog for this; and it would also be odd to add Sometimes only to stdlib's log and have (say) tests do
var sometimes = log.Sometimes{...}
// ...
sometimes.Do(func() { t.Log(...) })
So personally I'm in favor of adding it to package rate.
Another option (which may also address @deefdragon 's concern) is to put the Sometimes type in a new package under x/time. Perhaps package some, type Times ;-)
I checked to see whether that would clash with any identifiers named some, and the answer is not really (there are a tiny number of cases with variables named some but not where the throttler is needed). So I think that could work.
package some // import "x/time/some"
type Times ...
func (t *Times) Do(...)
I've updated the CL and description based on the feedback above. PTAL and let me know if there's anything left to do in order to proceed with the proposal. Thanks.
"some.Times" is a very weird name. There's only one possible symbol you can put in "package some". It's cute but not functional. It seems like we should go back to rate.Sometimes.
Updated back.
Personally I prefer rate.Sometimes, as it would make my life easier.
OK it sounds like we are back to
type Sometimes struct {
First int // if non-zero, the first N calls to Do will run f.
Every int // if non-zero, every Nth call to Do will run f.
Interval time.Duration // if non-zero and Interval has elapsed since f's last run, Do will run f.
}
What are the semantics when combinations of these are set? For example if all three are set, or if First+Every are set, or if Every+Interval are set, or if First+Interval are set?
For clarity, here is the implementation of Do from inside Google, which makes the semantics a bit clearer:
// Do runs the function f as allowed by First, Every, and Interval.
//
// The model is a union (not intersection) of filters. The first call to Do
// always runs f. Subsequent calls to Do run f if allowed by First or Every or
// Interval.
//
// A non-zero First:N causes the first N Do(f) calls to run f.
//
// A non-zero Every:M causes every Mth Do(f) call, starting with the first, to
// run f.
//
// A non-zero Interval causes Do(f) to run f if Interval has elapsed since
// Do last ran f.
//
// Specifying multiple filters produces the union of these execution streams.
// For example, specifying both First:N and Every:M causes the first N Do(f)
// calls and every Mth Do(f) call, starting with the first, to run f. See
// Examples for more.
//
// If Do is called multiple times simultaneously, the calls will block and run
// serially. Therefore, Do is intended for lightweight operations.
//
// Because a call to Do may block until f returns, if f causes Do to be called,
// it will deadlock.
func (s *Sometimes) Do(f func()) {
s.mu.Lock()
defer s.mu.Unlock()
if s.count == 0 ||
(s.First != 0 && s.count < s.First) ||
(s.Every != 0 && s.count%s.Every == 0) ||
(s.Interval != 0 && !s.last.IsZero() && time.Since(s.last) >= s.Interval) {
f()
s.last = time.Now()
}
s.count++
}
Does anyone object to adding this API?
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group