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

Auto delete could potentially make a lot of requests #87

Closed
AnotherCat opened this issue Feb 4, 2021 · 15 comments
Closed

Auto delete could potentially make a lot of requests #87

AnotherCat opened this issue Feb 4, 2021 · 15 comments
Labels
help wanted Extra attention is needed

Comments

@AnotherCat
Copy link
Contributor

The Problem

Currently if auto_delete is True a GET request will be made for every guild the bot is in on every load.
This could potentially cause thousands of requests (depending on the size of the bot) and discord has indicated that they might be introducing rate limits for slash commands.
This could make the library unusable for some.

Proposed solution

Add a check_all_guilds parameter to SlashCommand which defaults to False.
If it's false then check all guilds that have commands registered to them, don't check all guilds.
If it's true check all guilds (like currently), possibly add a WARNING if guilds above a certain number.

The only problem I can see with this is that it won't delete commands in a guild if there are no commands registered to that guild in SlashCommand

Possible alternatives

  • Proposed solution above but default to False
  • Proposed solution above but require it
  • Leave it as is, causing high traffic at startup for large bots, and possibly reaching rate limits
  • The ideal solution would be an endpoint that could be used to get all guilds with commands currently. There's an issue on the discord repo but no one has responded yet
@eunwoo1104
Copy link
Contributor

Maybe we can just integrate every auto-things to PUT request. But this makes another breaking change, so not sure if it is the best option.

@AnotherCat
Copy link
Contributor Author

AnotherCat commented Feb 4, 2021

This issue still exists for PUT requests
While it does reduce the number of requests made, a request is still made for every guild the bot is in

Edit: i don't think there is really any good option atm, but i think there should at least be a way to opt out of the feature while still being able to use auto_delete
Ideally there would be an endpoint to get guilds that have commands

@i0bs
Copy link
Contributor

i0bs commented Feb 4, 2021

I've conducted a small experiment with my bot being in 15 separate guilds, and I can confirm that this is potentially a huge threat to the library itself if auto_delete is used in this manner. Putting everything into a PUT request would become a detriment since it would not fix the issue without further repercussions coming in place with some negative connotation.

My recommendation is to go with AnotherCat's suggestion by only reading slash commands with the guild_ids=[...] parameter assigned through the slash decorator, this way not every single guild is being checked. We can risk 429 errors if we're going through everything and rate limit the bot upon initialization, killing it all-in-all.

@i0bs
Copy link
Contributor

i0bs commented Feb 4, 2021

Another idea: we have the user define "shards" in some sense not through official d.py wrapper but with this internally so the API can handle "sections" or blocks of guilds it has to scan through, this might be a temporary or long-term workaround.

@AnotherCat
Copy link
Contributor Author

we have the user define "shards" in some sense not through official d.py wrapper but with this internally so the API can handle "sections" or blocks

Yeah like a list of guilds at start up which the bot will check every time

@AnotherCat
Copy link
Contributor Author

AnotherCat commented Feb 4, 2021

I think we should do this even though it's a breaking change because currently it means that people could potentially be API banned because of this.

I just remembered something, on all api requests (which i think includes slash commands) there is a global limit of 10k invalid requests per 10 mins. And a GET or POST request to guilds that haven't been authorised with application.commands is an invalid request.
Meaning that in large bots people could be api banned solely for using this library.

@i0bs
Copy link
Contributor

i0bs commented Feb 4, 2021

The shards idea might be very much a good idea then for the large bots with 10K+ servers. We just don't know right now how big slash commands are going to get, and it's better that we prepare for this now than never.

@AnotherCat
Copy link
Contributor Author

AnotherCat commented Feb 5, 2021

I'm not too sure how sharding works with slash commands, like do guilds with only slash commands get put into shards?
Also with d.py, all the sharding of the guild is done by Discord

Edit: (i do a lot of edits don't i)
I don't see how sharding would solve the issue though?

@eunwoo1104 eunwoo1104 added the help wanted Extra attention is needed label Feb 5, 2021
@eunwoo1104
Copy link
Contributor

For now it looks like these option can be used:

  • Get allowed guild ids from every command objects.
  • Warn about the rate-limit things if only auto_delete is enabled.
  • Wait until discord adds endpoint for scope added check.

@AnotherCat
Copy link
Contributor Author

AnotherCat commented Feb 5, 2021

This doesn't really change anything, just warning. How about implenting an arg for SlashCommand that defaults to True, that controls if all guilds are checked or not?
This would allow people to turn off the current behaviour, while still being able to use auto_delete on guilds they have commands on, but not introducing a breaking change

@muddyfish
Copy link

This looks extremely unusable for large bots; there's no way using your UI to say that you'll only ever use global commands, which is what I'm assuming most large bots will end up wanting

@i0bs
Copy link
Contributor

i0bs commented Feb 6, 2021

This is not only a severe design flaw for this API wrapper but in the Bot API itself. I'll be working at some attempts this weekend to see if a working model of Cat and I's idea can be made feasible.

@i0bs
Copy link
Contributor

i0bs commented Feb 9, 2021

Here is an update to the problem as referring to #92 :

tl;dr: This is a massive problem and we need to fix this ASAP pending v.1.0.9's release.

A user named knarfie#0001 described their issue of converting all of their guild commands to global commands in the Support Server. Their bot is in only 4 servers, and they have 25 commands. However, they experienced reaching a rate limit from the Discord Bot API despite being in very few servers.

  • Discord have introduced Slash Command rate limits to the API.
    discord_slash.error.RequestFailure: Request failed with resp: 400 | {"message": "Max number of daily application command creates has been reached (200)", "code": 0}

  • Bots with as small as 25 commands can hit the rate limit upon 200 creations.
    (25 commands X 4 servers X 2) = 200.

  • The rate limit includes both guild and global commands, too, but only it seems when the GET method is being used and not PUT. However, switching HTTP request methods could hold severe repercussions later down the line.

The biggest problem this serves is that Discord default to 24 hours per event of the quota of HTTP requests on a particular set action being reached on their Bot API, which means bot developers must wait 1 full day in total in order to process either smaller chunks of commands, or to even re-attempt it in total.

@AnotherCat
Copy link
Contributor Author

For anyone that's looking at this and is interested, we're discussing how to solve this on the discord server

@eunwoo1104
Copy link
Contributor

This should be fixed on new commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants