DiscordBM for SSWG Incubation Proccess #81
Conversation
proposals/0022-discordbm.md
Outdated
|
|
||
| What happens is, the `GatewayManager` will start making a connection to the Discord Gateway using Websockets. For Websockets, DiscordBM uses a customized version of Vapor's `websocket-kit` which supports zlib decompression. After the websocket connection is established, Discord will send a 'hello' message to DiscordBM, and then DiscordBM sends an 'identify' payload to Discord which contains stuff like the `token`, `presence` and the `intents`. | ||
|
|
||
| In case Discord doesn't accept the identification, it will close the websocket connection with a proper close code. DiscordBM will catch that close code, and translate it to one of Discord's websocket close codes. If the code is a code that allows reconnection, DiscordBM will attempt that. Otherwise it will send a log message with `critical` level to users with a user-friendly description of the code, and instructions on how to try to solve the problem: |
There was a problem hiding this comment.
Please review the Log Levels guide.
There was a problem hiding this comment.
Logging critical logs is allowed for libraries, however as the name implies - only in the most critical situations. Most often this implies that the library will stop functioning after such log has been issued. End users are thought to expect that a logged critical error is very important, and they may have set up their systems to page people in the middle of the night to investigate the production system right now when such log statements are detected. So please be careful about logging these kinds of errors.
This very well matches what happens. Specifically, Most often this implies that the library will stop functioning after such log has been issued which is true about the Gateway part. (You possibly could still send stuff through DiscordClient but that might still not work, and most of the time you send requests in response to Gateway's events, which will not be received anymore.)
This is one of the 2 places that DiscordBM uses critical logging.
The other place is when you hit your global HTTP rate limit which is also something quite critical.
There was a problem hiding this comment.
Fair! Though I think rate limiting is normally not critical. You are taking plenty of steps to avoid this
There was a problem hiding this comment.
It's not a normal rate-limit. If you hit the global rate, you're probably genuinely hitting it and you might not be able to do much about it and might need to ask Discord for a rate-limit raise. It's kind of a critical situation in my eyes.
Co-authored-by: Joannis Orlandos <joannis@orlandos.nl>
proposals/NNNN-discordbm.md
Outdated
| } | ||
| ``` | ||
|
|
||
| This is also in line with the latest structured concurrency advancements, and users won't need to block a thread or call functions such as `RunLoop.main.run()`. Generally speaking, DiscordBM has gone full-in on structured concurrency and makes extensive use of actors. The whole library is already in full compliance with `-strict-concurrency=complete`. |
There was a problem hiding this comment.
Note: Including task cancellation?
There was a problem hiding this comment.
For DiscordClient stuff, it's just whatever AsyncHTTPClient does. For gateway, no. I did take a look at kind of rewriting the Gateway logic with Task cancellation support a few months ago, but it didn't go well. Can't exactly remember why, i think i couldn't come up with a plan and came to the conclusion that Task cancellation might not fit.
proposals/NNNN-discordbm.md
Outdated
| } | ||
| ``` | ||
|
|
||
| - The `cache` is a storage for the cached responses. |
There was a problem hiding this comment.
Should i explain that more in the proposal?
Caching is disabled by default. It just caches the http responses.
As to why, it's just the same reason you would guess. To do less http requests. It's faster, doesn't consume http limit etc...
proposals/NNNN-discordbm.md
Outdated
| For example if a bucket is exhausted, the `HTTPRateLimiter` will have no choice but to reject the request, Right? | ||
| That's not exactly what happens in DiscordBM. If there is a request that is disallowed by the bucket info, `HTTPRateLimiter` instead tries to return the time amount the `DiscordClient` needs to wait before making the request. Then `DiscordClient` takes a look at the `retryPolicy`, and if the `retryPolicy` specifies that requests failed with the `429 Too Many Requests` header can be retried based on headers, `DiscordClient` will just wait the time, and make the request after! This is best of the both worlds: | ||
|
|
||
| - Even if you make a lot of requests in a loop, `DiscordClient` not only won't fail, but also won't even let users notice anything. To users it will look like as if they don't even have a rate limit to worry about! |
There was a problem hiding this comment.
What if the number of messages per <timeunit> exceeds the amount that can be regained at the rate limiter?
There was a problem hiding this comment.
So to be clear, the whole mechanism doesn't stop you from exceeding your rate-limit.
It makes that happen way less often, but it can't stop you.
I didn't get what you exactly mean, but if you do requests in a loop, it should never fail. What happens is that it for example makes 5 requests immediately, then notices it needs to wait 4.5s before doing the next, and DiscordBM will just wait out that 4.5 before doing the next request, an then continues the loop.
Was that clear? Should i change what is in the proposal?
Co-authored-by: Joannis Orlandos <joannis@orlandos.nl>
This is a proposal for accepting DiscordBM into the SSWG incubation process.