Skip to content

rate limiter in logger, opt-in - #62

Open
c-cube wants to merge 5 commits into
masterfrom
sc/rate-limit-in-log
Open

rate limiter in logger, opt-in#62
c-cube wants to merge 5 commits into
masterfrom
sc/rate-limit-in-log

Conversation

@c-cube

@c-cube c-cube commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@c-cube
c-cube requested a review from yasunariw August 25, 2026 12:50

@raphael-proust raphael-proust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing important, just small potatoes feedback

Comment thread log.ml Outdated
Comment thread log.ml Outdated
Comment thread log.ml Outdated
@c-cube
c-cube force-pushed the sc/rate-limit-in-log branch from 2a93b89 to d6991e0 Compare August 25, 2026 18:39
Comment thread log.ml Outdated
open ExtLib
open Prelude

module Rate_limit : sig

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks generic enough to be in Cache (or Control?)

Comment thread log.ml Outdated
Comment thread log.ml

class logger facil =
let make_s (output_line:Logger.facil -> Time.t -> Logger.Pairs.t -> string -> unit) =
class logger ?(logger=State.logger) facil =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means changes to State will not matter once logger object is created? which goes against the purpose of the state

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently State.logger is already not changeable (only hooks and some internal things are). It hasn't really been changeable in a while, as far as I can tell. The override is only used for testing here, for all intent and purpose we always use Log.State.logger everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants