pycord icon indicating copy to clipboard operation
pycord copied to clipboard

feat: ratelimit prediction

Open VincentRPS opened this issue 1 year ago • 13 comments

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.
  • [x] 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

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

VincentRPS avatar Jul 31 '23 02:07 VincentRPS

Please add a changelog entry

pullapprove4[bot] avatar Jul 31 '23 02:07 pullapprove4[bot]

Changelog requirements met

pullapprove4[bot] avatar Jul 31 '23 03:07 pullapprove4[bot]

Please add a changelog entry

pullapprove4[bot] avatar Aug 02 '23 17:08 pullapprove4[bot]

Changelog requirements met

pullapprove4[bot] avatar Aug 02 '23 17:08 pullapprove4[bot]

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.

VincentRPS avatar Jan 08 '24 11:01 VincentRPS

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.

VincentRPS avatar Jan 24 '24 13:01 VincentRPS

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

PR is mostly finalized by this point. — @VincentRPS

You will need to redo this PR if it is based on bluenix comment, which describes an incorrect implementation of rate limiting.

You can't predict rate limits based on when your bot sends requests, because your bot isn't the single source of truth of the rate limit.

The discord server determines which bucket (window) the request applies to and thus how the rate limit works. Your bot can send 50 requests per second at a technical level and still be rate limited depending on the timing of the rate limit bucket window and when the server receives the request.

You can read https://github.com/switchupcb/disgo/pull/14 and https://github.com/switchupcb/disgo/issues/22 for more information that validates these claims.

But I wouldn't read all that.


Bluenix admitted in the same thread his solution didn't work.

https://github.com/discord/discord-api-docs/issues/5144#issuecomment-1179625113

Did you test this at 50 requests per second?

I do hit the global ratelimit, yes. This could have to do with how requests take different amount of time and appears in later windows (which you brought up about my implementation for route-specific ratelimits), but I am not sure this explains everything, I suppose you can consider this issue reproduced by me.

switchupcb avatar Jul 09 '24 15:07 switchupcb

Yeah, no thanks. Where were you when other Discord.py forks updated their ratelimit handlers?

EmmmaTech avatar Jul 09 '24 15:07 EmmmaTech

Where were you when other Discord.py forks updated their ratelimit handlers? @EmreTech

The answer to that question doesn't change the truth or the tests used to assert the truth.

switchupcb avatar Jul 09 '24 16:07 switchupcb

Where were you when other Discord.py forks updated their ratelimit handlers? @EmreTech

The answer to that question doesn't change the truth or the tests used to assert the truth.

I want to know more about how "reliable" your method is. How many tests have you conducted? Have you tested your impelementation alongside Bluenix's proposal? I'm really just curious at this point, because you've seemed to dominate that original Discord API Docs issue I linked to with a bunch of your research (which was a weird thing to do btw, an issue in an issue tracker is not the right place to do that).

EmmmaTech avatar Jul 09 '24 16:07 EmmmaTech

I want to know more about how "reliable" your method is. How many tests have you conducted? — @EmreTech

https://github.com/switchupcb/disgo can handle over 50 RPS, but doesn't due to a correct rate limit implementation.

The rate limit test is located here: https://github.com/switchupcb/disgo/blob/v10/wrapper/tests/integration/ratelimit_test.go

You can also view the actions run on every commit and also read the information I linked, which ran a test indicating that Discord CANT be consumed using a "rolling" rate limit implementation.

I'm really just curious at this point, because you've seemed to dominate that original Discord API Docs issue I linked to with a bunch of your research (which was a weird thing to do btw, an issue in an issue tracker is not the right place to do that).

The entire point of that thread was to determine the actual rate limit implementation, so it could be included in the documentation.

Have you tested your impelementation alongside Bluenix's proposal?

Bluenix admitted in the same thread his solution didn't work.

https://github.com/discord/discord-api-docs/issues/5144#issuecomment-1179625113

Did you test this at 50 requests per second?

I do hit the global ratelimit, yes. This could have to do with how requests take different amount of time and appears in later windows (which you brought up about my implementation for route-specific ratelimits), but I am not sure this explains everything, I suppose you can consider this issue reproduced by me.

switchupcb avatar Jul 09 '24 17:07 switchupcb

Bluenix admitted in the same thread his solution didn't work.

Oh my god, I guess I missed that part. I suppose you're right then, sorry. I'm reading your documentation from switchupcb/disgo#14 now.

EmmmaTech avatar Jul 09 '24 17:07 EmmmaTech

needs a redo

Lulalaby avatar Aug 16 '24 12:08 Lulalaby