-
Notifications
You must be signed in to change notification settings - Fork 550
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
Meter network activity #6885
Meter network activity #6885
Conversation
@@ -2600,7 +2600,12 @@ module Hooks = struct | |||
: (Get_epoch_ledger.query, Get_epoch_ledger.response) rpc | |||
|
|||
type rpc_handler = | |||
| Rpc_handler : ('q, 'r) rpc * ('q, 'r) rpc_fn -> rpc_handler | |||
| Rpc_handler : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to avoid repeating this type definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a while trying and it was difficult to do without repeating a lot of code or changing some stuff around significantly.
Need more info to make sure the thresholds are correct |
Can we call this "Rate_limiter" instead of "Meter"? |
That makes sense, will do that |
-> Envelope.Sender.t | ||
-> now:Time.t | ||
-> score:int | ||
-> [`Ok | `Capacity_exceeded] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change Ok
to something like Within_capacity
, to avoid confusion with an Or_error
type.
Some (do_ f) | ||
| Consensus_rpc rpc_a, Rpc_handler (Consensus_rpc rpc_b, f) -> | ||
Consensus.Hooks.Rpcs.match_handler (Rpc_handler (rpc_b, f)) rpc_a ~do_ | ||
| Consensus_rpc rpc_a, Consensus_rpc rpc_b -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the case for Get_telemetry_data
omitted intentionally?
@@ -626,7 +626,12 @@ module Rpcs = struct | |||
| Consensus_rpc : ('q, 'r) Consensus.Hooks.Rpcs.rpc -> ('q, 'r) rpc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment for ADDING A NEW RPC should probably mention updating match_handler
This PR "meters" all networking activity in the sense of putting a limit (for each peer ID and IP) on
in any given 5 minute interval.
I'm not 100% confident about all the concrete limits I put, so any feedback on that would be welcome.