Skip to content
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

fix(relay): export RateLimiter type #3742

Merged
merged 20 commits into from
Jul 31, 2023

Conversation

dariusc93
Copy link
Member

@dariusc93 dariusc93 commented Apr 6, 2023

Description

The rate-limiter module was not made publicly available. This change exports the RateLimiter trait defined within that module which allows users to define their own rate limiters. To make common usecases easy, we also provide a set of constructor functions for common rate limiters.

Resolves #3741.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger
Copy link
Contributor

Thanks for the patch! I am wondering how we can best follow the conventions laid out in #2217 with this.

Currently, all the the fields on relay::Config are public (they should probably be private so we don't expose such a large API).

I am thinking, instead of just exposing the rate_limiter module, why don't we:

  • Export the RateLimiter trait on the crate root (chore: enforce unreachable_pub lint #3735 would have enforced that)
  • Add functions on Config to create the two predefined rate limiters (per_peer and per_ip).
  • As parameters to the above functions, I'd recommend that we don't expose GenericRateLimiterConfig but just the two parameters required. That is one less type in our API.

This way, we don't have to expose the entire module but still allow users to use the existing configs and build their own.

Does that work for your usecase?

@dariusc93
Copy link
Member Author

Sounds good to me @thomaseizinger

@mxinden
Copy link
Member

mxinden commented Apr 9, 2023

Sorry for the trouble @dariusc93. I am assuming that this was my mistake back from #2059.

Agreed with the suggestions above by @thomaseizinger. Thanks!

@dariusc93 do you want to tackle these suggestions?

@mergify

This comment was marked as resolved.

@thomaseizinger
Copy link
Contributor

Thanks for the patch! I am wondering how we can best follow the conventions laid out in #2217 with this.

Currently, all the the fields on relay::Config are public (they should probably be private so we don't expose such a large API).

I am thinking, instead of just exposing the rate_limiter module, why don't we:

* Export the `RateLimiter` trait on the crate root ([chore: enforce `unreachable_pub` lint #3735](https://github.com/libp2p/rust-libp2p/pull/3735) would have enforced that)

* Add functions on `Config` to create the two predefined rate limiters (`per_peer` and `per_ip`).

* As parameters to the above functions, I'd recommend that we _don't_ expose `GenericRateLimiterConfig` but just the two parameters required. That is one less type in our API.

This way, we don't have to expose the entire module but still allow users to use the existing configs and build their own.

Does that work for your usecase?

@dariusc93 In case you missed this message, I am currently waiting on a response if the above suggestion would work for you.

@dariusc93
Copy link
Member Author

@dariusc93 In case you missed this message, I am currently waiting on a response if the above suggestion would work for you.

Yes it does work for me, just havent had the time to implement the suggestions myself (meant to do it this past weekend though but been busy).

@thomaseizinger
Copy link
Contributor

@dariusc93 In case you missed this message, I am currently waiting on a response if the above suggestion would work for you.

Yes it does work for me, just havent had the time to implement the suggestions myself (meant to do it this past weekend though but been busy).

No worries (and rush) at all :)
Just wanted to make sure there isn't a misunderstanding!

@dariusc93 dariusc93 marked this pull request as draft May 13, 2023 19:14
@@ -61,6 +61,32 @@ pub struct Config {
pub circuit_src_rate_limiters: Vec<Box<dyn rate_limiter::RateLimiter>>,
}

impl Config {
pub fn per_peer(mut self, limit: NonZeroU32, interval: Duration) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want these function to be per reservation_rate_limiters and circuit_src_rate_limiters or do we want to have it so we populate both vectors per function?

Copy link
Member

Choose a reason for hiding this comment

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

I am not 100% sure I follow the question. I would expect a user to want to set a different limit for reservations than for circuits, with the rational that a reservation is less expensive than a circuit. Does that make sense? I am not sure though whether we really need to support this use-case via the easy-path, instead of deferring the user to the rate-limiter primitives directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mxinden see #3742 (comment).

I think splitting as better as per @mxinden's reasoning. We only provide these functions to be able to hide more details from the public API. The user should still be able to make fine-grained configuration changes otherwise there is no point in providing them I think.

@thomaseizinger
Copy link
Contributor

@dariusc93 Let me know when this is ready for another review or you have any questions.

@dariusc93
Copy link
Member Author

@dariusc93 Let me know when this is ready for another review or you have any questions.

Will do! I havent forgotten about this PR and look to tackle this before this week is over.

@dariusc93
Copy link
Member Author

dariusc93 commented Jul 29, 2023

Sorry for the delay @thomaseizinger. I did push changes to split it for both reservation_rate_limiters and circuit_src_rate_limiters, though im not sure on the naming itself but should satisfy using GenericRateLimiterConfig internally while still giving those who implement RateLimiter the option to push into those fields, although I am wondering if we should make the Config fields private and have functions to set the value to the respected fields. Thoughts? We could just mark the fields as deprecated pointing users to use the corresponding functions instead if it is done.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice, thank you for the changes!

I think this design makes sense:

  • Easy things are easy: Users can call one of the pre-defined constructor functions on Config to create several rate limiters
  • Hard things are possible: Users can implement their own RateLimiter and push it to the field directly
  • The design is open: Users can access all values of the config.
  • The API surface is minimal: GenericRateLimiterConfig is an implementation detail

There might be an argument to not making the fields public and instead provide getter functions. Those would give us slightly more options in case we ever want to change these fields. For the moment, I am not too worried about that though.

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger marked this pull request as ready for review July 29, 2023 16:58
@thomaseizinger thomaseizinger changed the title fix(relay): Export rate-limiter module fix(relay): export rate-limiter module Jul 29, 2023
@thomaseizinger thomaseizinger changed the title fix(relay): export rate-limiter module fix(relay): export RateLimiter type Jul 31, 2023
@mergify mergify bot merged commit 8ff2cf3 into libp2p:master Jul 31, 2023
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
The `rate-limiter` module  was not made publicly available. This change exports the `RateLimiter` trait defined within that module which allows users to define their own rate limiters. To make common usecases easy, we also provide a set of constructor functions for common rate limiters.

Resolves #3741.

Pull-Request: #3742.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publicly export libp2p_relay::behaviour::rate_limiter
3 participants