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: ratelimit prediction #2188

Open
wants to merge 65 commits into
base: master
Choose a base branch
from
Open

feat: ratelimit prediction #2188

wants to merge 65 commits into from

Conversation

VincentRPS
Copy link
Member

Summary

This replaces the previous system of rate limit, which purely only handled rate limits, with
a new rate limit system which not only eases and handles rate limits but also tries to predict rate limits before they can happen and affect the bot. This system is made to be scalable and replaceable with something like Redis storage.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@VincentRPS VincentRPS self-assigned this Jul 31, 2023
@VincentRPS VincentRPS requested a review from a team as a code owner July 31, 2023 02:49
@pullapprove4
Copy link

pullapprove4 bot commented Jul 31, 2023

Please add a changelog entry

@pullapprove4
Copy link

pullapprove4 bot commented Jul 31, 2023

Changelog requirements met

CHANGELOG.md Outdated Show resolved Hide resolved
discord/client.py Outdated Show resolved Hide resolved
discord/http.py Show resolved Hide resolved
discord/rate_limiting.py Outdated Show resolved Hide resolved
discord/rate_limiting.py Show resolved Hide resolved
discord/rate_limiting.py Outdated Show resolved Hide resolved
discord/http.py Outdated Show resolved Hide resolved
VincentRPS and others added 3 commits July 31, 2023 15:51
pain. it was all pain.
Forgot to add bucket storage to slots.

Signed-off-by: VincentRPS <[email protected]>
@pullapprove4
Copy link

pullapprove4 bot commented Aug 2, 2023

Please add a changelog entry

@pullapprove4
Copy link

pullapprove4 bot commented Aug 2, 2023

Changelog requirements met

Copy link
Member

@Dorukyum Dorukyum left a comment

Choose a reason for hiding this comment

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

Could you respond to the comment below and also tell me how you've tested these changes?

discord/client.py Outdated Show resolved Hide resolved
discord/client.py Outdated Show resolved Hide resolved
@VincentRPS
Copy link
Member Author

Could you respond to the comment below and also tell me how you've tested these changes?

It's not hard to make the code yourself so I won't provide it but I did two different tests.

Creating / Deleting Channels inside a guild:
- 50 (surpasses limit)
- 100 (surpasses the limit even more)
- 150 (to fully test out rate limit prediction capabilities)
Creating / Deleting Messages inside a channel:
- 5 (really really limiting lol)
- 10 (more)
- 15 (I wouldn't go over that most of the time, message rate limits suck)

You can also do your own tests, anything that surpasses the limit of any endpoint is fine. Do, however, avoid testing interactions or anything webhook-related since due to the complexity at the moment of implementing that. I may do that in the future in a separate PR since it does seem feasible.

Mostly, to properly implement rate limit prediction into that, I would need to make an entirely different Webhook class for only interaction followups. A massive pain, and probably isn't worth doing for now.

Copy link
Contributor

@EmreTech EmreTech left a comment

Choose a reason for hiding this comment

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

This may be a helpful guide for this PR, if you haven't read it already.

CHANGELOG.md Outdated Show resolved Hide resolved
discord/client.py Outdated Show resolved Hide resolved
discord/client.py Outdated Show resolved Hide resolved
discord/client.py Outdated Show resolved Hide resolved
discord/http.py Outdated Show resolved Hide resolved
discord/http.py Show resolved Hide resolved
discord/rate_limiting.py Outdated Show resolved Hide resolved
discord/rate_limiting.py Outdated Show resolved Hide resolved
VincentRPS and others added 4 commits January 24, 2024 19:49
Co-authored-by: Emre Terzioglu <[email protected]>
Signed-off-by: VincentRPS <[email protected]>
Summary: if two or more requests get rate limited at the same time with the same bucket, they will override each other.

To fix this without using any locks, we just set the temp bucket before any rate limit can happen, and set it to rate limited once there is one, therefore the only thing being overridden in the `rate_limited` variable to True.
@VincentRPS
Copy link
Member Author

PR is mostly finalized by this point. The final goal now is to try and test this system on larger bots to identify any possible faults before it is merged into master.

This PR will not be refactoring webhooks, at least for now. That will be the goal of a second PR in 2.6 or 2.7, as the work for that would be a bit more complicated, especially with interaction followups. There is a chance we could make interactions use the HTTPClient instead of Webhooks though which would be much easier to handle since it would just integrate with the current rate limit prediction system.

discord/http.py Show resolved Hide resolved
VincentRPS and others added 3 commits January 25, 2024 07:10
rate_limited is now set to False by default.
There is a lock on .use to prevent multiple requests from all releasing requests at the same time.

Signed-off-by: VincentRPS <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Implements a feature PA: All Contributors pending priority: low Low Priority python Pull requests that update Python code status: awaiting review Awaiting review from a maintainer
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants