rate limiter in logger, opt-in - #62
Conversation
raphael-proust
left a comment
raphael-proust
left a comment
There was a problem hiding this comment.
nothing important, just small potatoes feedback
2a93b89 to
d6991e0
Compare
| type t = | ||
| | None | ||
| | RL of { | ||
| mutable tokens: float; |
There was a problem hiding this comment.
tokens as float? i guess this helps avoid thinking about rounding but still feels strange
There was a problem hiding this comment.
I don't really know another way to deal with fractional refilling (what if we refill 3.4 tokens?). Well an alternative is to round up or down depending on Random.float (refill -. floor refill) (ie random(0.4) for refill=3.4) but that seems even weirder.
| | RL rl -> | ||
| let now = Time.now() in | ||
|
|
||
| if now > rl.last_update then ( |
There was a problem hiding this comment.
this is pretty much guaranteed to be always true unless it is called in a tight loop with nothing else to do (but then why rate limit noop action), idk if it is important.
There was a problem hiding this comment.
well sure, although this is using Unix.gettimeofday() so the clock could in theory step backwards.
we could use mtime if that's better?
There was a problem hiding this comment.
i would use integer division and refill only full tokens (and update timestamp when refill actually happens)
There was a problem hiding this comment.
I feel like it'd still lead to token loss.
Say we refill at 1.5 token/second, with an empty bucket, and emit one log every second. We should refill 1.5 token (time delta is 1.) but truncate to 1, so the log is accepted but the bucket remains empty. A burst of 2 log messages at once thus leads to a rejection of the second message, even though we emit logs at 1 log/second.
edit: looks like Go's stdlib uses float64 for the counter
rr0gi
left a comment
rr0gi
left a comment
There was a problem hiding this comment.
also lets enable some generous rate limit by default
| val create : ?burst_capacity:int -> allowed_per_sec:float -> unit -> t | ||
| (** Create a token-bucket rate limiter. The bucket starts full. | ||
| @param burst_capacity limits the size of a burst when token bucket is full. | ||
| [burst_capacity = N] means a full bucket contains [N * allowed_per_sec] tokens. |
There was a problem hiding this comment.
then this is not capacity but more like burst_factor?
i would make the comment less detailed overall : "create rate limiter with asymptotic maximum rate [allowed_per_sec] and handling up to [burst_factor] higher rate at peak", move implementation comments into .ml
3 participants
No description provided.