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

new discord bot #45

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

Conversation

TheUbMunster
Copy link
Contributor

This is a proposed solution to this bug
image
by switching to the Discord.net library instead of DiscordSharpPlus

initial testing seems to be fine, some tweaks and fixes will need to be made.

Server/DiscordBot.cs Outdated Show resolved Hide resolved
Server/DiscordBot.cs Outdated Show resolved Hide resolved
@TheUbMunster TheUbMunster marked this pull request as ready for review July 11, 2023 11:05
@Istador
Copy link
Contributor

Istador commented Jul 12, 2023

It should not log these messages, in the default case where there is no Discord/Token configured in the settings.json, because it will confuse users:

logger.Info("Starting discord bot (ctor)");

logger.Error("Tried to run the discord bot, but the Token and/or communication channels are not specified in the settings.");

I'm not a big fan of it, but maybe we should introduce a Discord/Enabled setting, to be consistent with settings from other sections?

@TheUbMunster
Copy link
Contributor Author

It should not log these messages, in the default case where there is no Discord/Token configured in the settings.json, because it will confuse users:

logger.Info("Starting discord bot (ctor)");

logger.Error("Tried to run the discord bot, but the Token and/or communication channels are not specified in the settings.");

I'm not a big fan of it, but maybe we should introduce a Discord/Enabled setting, to be consistent with settings from other sections?

should be good now. lmk

Copy link
Contributor

@Istador Istador left a comment

Choose a reason for hiding this comment

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

Tested the bot for a few minutes and it seems to work.
Will need to do a longer test for a few days though.

I'll test this build for RCL 3.
docker

Server/Settings.cs Outdated Show resolved Hide resolved
Server/DiscordBot.cs Outdated Show resolved Hide resolved
} catch (Exception e) {
Logger.Error($"Failed to get log channel \"{Config.LogChannel}\"");
Logger.Error(e);
string message = a.Message ?? string.Empty + (a.Exception != null ? "Exception: " + a.Exception.ToString() : ""); //TODO: this crashes
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still an open TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's been so long I can't remember.

TheUbMunster and others added 2 commits October 14, 2024 16:09
disable the discord table by default

Co-authored-by: Robin C. Ladiges <[email protected]>
remove redundant warnings messages

Co-authored-by: Robin C. Ladiges <[email protected]>
@Istador
Copy link
Contributor

Istador commented Oct 17, 2024

Will need to do a longer test for a few days though.

Looks fine to me.


This branch has conflicts that must be resolved

Note that this branch is in conflict with the current master branch due to other changes (by me) from the last two years, so you should merge master into this branch or rebase it to resolve the merge conflict in Server/Server.cs.

@Istador
Copy link
Contributor

Istador commented Oct 21, 2024

The rate limit is something new that never happen with the old Discord client.
rate_limit

(Related to the already existing Broken pipe error, where the log is flooded with this error message in an endless loop.)

@TheUbMunster
Copy link
Contributor Author

@Istador is the

The rate limit is something new that never happen with the old Discord client.

thing known to be broken with the new bot as of now vs the old bot as of two years ago, or broken with the new bot as of now vs the old bot as of now? I wonder if the library I'm using is using a newer api version or something with stricter ratelimiting or something. I don't think I'd be doing much more work than the old bot

(Related to the already existing Broken pipe error, where the log is flooded with this error message in an endless loop.)

Is this problem referring to a error in the old bot or the new bot?

Also, I've been very busy lately, it's possible that I can't put any significant time into fixing any of this for a few months at least. It's been so long I can't really remember the conversation I had with sanae about why I felt redoing the bot code was a good idea. If you, or someone else, wants to take what I've done and clean it up/redo it/throw it away, that's fine with me.

@Istador
Copy link
Contributor

Istador commented Oct 22, 2024

@Istador is the

The rate limit is something new that never happen with the old Discord client.

thing known to be broken with the new bot as of now vs the old bot as of two years ago, or broken with the new bot as of now vs the old bot as of now? I wonder if the library I'm using is using a newer api version or something with stricter ratelimiting or something. I don't think I'd be doing much more work than the old bot

(Related to the already existing Broken pipe error, where the log is flooded with this error message in an endless loop.)

Is this problem referring to a error in the old bot or the new bot?

Also, I've been very busy lately, it's possible that I can't put any significant time into fixing any of this for a few months at least. It's been so long I can't really remember the conversation I had with sanae about why I felt redoing the bot code was a good idea. If you, or someone else, wants to take what I've done and clean it up/redo it/throw it away, that's fine with me.

The Broken pipe error is something that happens regardeless of the new or old bot being used. I assume it's caused by disconnected clients that the server still thinks are there and that it fails to send messages to. It just causes a lot of error messages in a short time over sometimes hours. (Discord likely doesn't like this and as someone scrolling trough these logs I dislike it too.)

With the old bot Discord just accepted that and allowed that flooding of messages (or they were silently rejected without me noticing). Now with the new bot the rate limit warning is appearing. (Speculation:) I also assume that's because the new bot is using a different API that has stricter rate limiting.

Maybe we could implement something to prevent duplicate messages. E.g. don't deliver messages that repeat more often than two times directly after another within a given timeframe. For that we could keep the last two send messages and their timestamps in memory.

@Istador
Copy link
Contributor

Istador commented Oct 22, 2024

Hitting such a rate limit will eventually lead to an Cloudflare IP address ban (also affecting the Discord desktop client).

During that time the server can't be (re)started, it hangs after the Starting discord bot (ctor) message.

@Istador
Copy link
Contributor

Istador commented Oct 23, 2024

Maybe we could implement something to prevent duplicate messages. E.g. don't deliver messages that repeat more often than two times directly after another within a given timeframe. For that we could keep the last two send messages and their timestamps in memory.

=> Istador@60d65ed

@TheUbMunster
Copy link
Contributor Author

TheUbMunster commented Oct 23, 2024

Maybe we could implement something to prevent duplicate messages. E.g. don't deliver messages that repeat more often than two times directly after another within a given timeframe. For that we could keep the last two send messages and their timestamps in memory.

=> Istador@60d65ed

this pr has a feature where you can define certain message prefixes, and if any message begins with that prefix, it doesn't get printed. There are certain messages that are "innocuous", but annoying. I'm still not really sure why some of these errors occur.

@Istador
Copy link
Contributor

Istador commented Oct 23, 2024

Maybe we could implement something to prevent duplicate messages. E.g. don't deliver messages that repeat more often than two times directly after another within a given timeframe. For that we could keep the last two send messages and their timestamps in memory.

=> Istador@60d65ed

this pr has a feature where you can define certain message prefixes, and if any message begins with that prefix, it doesn't get printed. There are certain messages that are "innocuous", but annoying. I'm still not really sure why some of these errors occur.

I'm aware of that feature but am not a fan of hiding them completely. Rather I'd see these error being catched in the code and handled in some way (or if they are really innocuous, ignore them there in the code instead of in the logger), because they are still logged to the standard output of the application (server log).

In this case I still want to see the Broken pipe error, just less frequently to avoid being banned, because sometimes it is a continous problem that doesn't go away on its own and might disturb normal operation of the server (not entirely sure about it but IIRC it prevents the player causing it to reconnect). When I see this error in the log I usually react with a server restart to fix it.

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.

3 participants