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

feat(relay): allow to configure default rate limiters #5584

Conversation

stormshield-frb
Copy link
Contributor

Description

Making the GenericRateLimiterConfig, rate_limiter::new_per_peer and rate_limiter::new_per_ip public to be able to configure the default rate limiters and still use them.

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

@dariusc93
Copy link
Member

I believe it was discussed previously that it was made private to allow others to implement their own RateLimiter if they did not wish to configure it via Config. What are your use case for wanting to use GenericRateLimiterConfig directly?

@stormshield-frb
Copy link
Contributor Author

stormshield-frb commented Aug 30, 2024

Yes I do think it is a good idea to allow people implementing their own RateLimiter. However, when using the default configuration of the relay Behaviour, the only choices are: using the default rate limiters with their default configuration, or not using them at all and implementing our own RateLimiter.

Currently we did not had a use case were we needed to implement our own rate limiter, we are perfectly happy with the generic ones that are provided in the libp2p. However, we wanted to update the default configurations of the default rate limiters.

We don't have many relay peers for now on our network and we were reaching the default rate limiters too fast. We just needed to set a different interval and limit for specific peers that are relays and that have the resources to handle more load.

@dariusc93
Copy link
Member

Yes I do think it is a good idea to allow people implementing their own RateLimiter. However, when using the default configuration of the relay Behaviour, the only choices are: using the default rate limiters with their default configuration, or not using them at all and implementing our own RateLimiter.

Correct, however unless I am missing something, would setting GenericRateLimiterConfig and the rate limiter functions public would be no different than using Config::{reservation_rate_per_peer, circuit_src_per_peer, reservation rate_per_ip. circuit_src_per_ip}? See #3742 (comment) for more info

Currently we did not had a use case were we needed to implement our own rate limiter, we are perfectly happy with the generic ones that are provided in the libp2p. However, we wanted to update the default configurations of the default rate limiters.

In that case, wouldnt you be able to update the default from the functions above?

We don't have many relay peers for now on our network and we were reaching the default rate limiters too fast. We just needed to set a different interval and limit for specific peers that are relays and that have the resources to handle more load.

Just for clarification, you want to be able to set different rate limits for different peers and not the ones set in the config, which would apply for every peer?

@stormshield-frb
Copy link
Contributor Author

Correct, however unless I am missing something, would setting GenericRateLimiterConfig and the rate limiter functions public would be no different than using Config::{reservation_rate_per_peer, circuit_src_per_peer, reservation rate_per_ip. circuit_src_per_ip}?

Oh yes indeed !! My bad. We did not see those function back then. You are totally right this PR has the same purpose than those functions, so no need for this PR 😉

Just for clarification, you want to be able to set different rate limits for different peers and not the ones set in the config, which would apply for every peer?

Hum I'm not sure what you mean. What we needed to do was to increase the rate limiters of the relay Behaviour on some specific peers that had more resources (cpu, storage, bandwith, etc) but let the default ones for the other peers (which did not have increased available resources). So we just needed a way to tell the Config of the Behaviour that. And since we did not see the functions mentioned above, making the GenericRateLimiterConfig public was the next best thing.

Thanks a lot for you patience and explanations. I'm closing this since your functions should perfectly do the job for us 😊

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.

2 participants