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: add Sometimes type #54237
Comments
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 |
Change https://go.dev/cl/421915 mentions this issue: |
Does anyone object to accepting this proposal? |
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. |
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 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
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
|
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
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? |
The action is allowed if any one of the Sometimes conditions is met. |
For clarity, here is the implementation of Do from inside Google, which makes the semantics a bit clearer:
Does anyone object to adding this API? |
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
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.
// 2022-08-21 Edited: moved from "x/time/rate".Sometimes -> "x/time/some".Times based on feedback.
// 2022-08-31 Edited: moved back to "x/time/rate".Sometimes.
The text was updated successfully, but these errors were encountered: