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

logging: disallow DEBUG channel logs & stop inheriting console log level #2419

Merged
merged 2 commits into from
Mar 18, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Mar 13, 2023

Description

Setting logging_channel_level = DEBUG sends so much to IRC that the bot got stuck waiting for flood protection to release.

The only sensible solution is to disallow setting channel logs to the DEBUG level, and along the way I've also made it so that channel logging always defaults to WARNING if not configured, instead of inheriting the console's logging_level setting.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

Indirectly related to #2255, both because I discovered that bug while working on this one last year and because I devised this solution while testing to see if that issue could be closed after #2410.

Setting DEBUG for the log channel made the bot unusable.
@dgw dgw added Bugfix Generally, PRs that reference (and fix) one or more issue(s) Breaking Change Stuff that probably should be mentioned in a migration guide labels Mar 13, 2023
@dgw dgw added this to the 8.0.0 milestone Mar 13, 2023
@dgw dgw requested a review from a team March 13, 2023 00:15
@dgw dgw force-pushed the logging-channel-max-level branch from 6cbe41c to bb4d5e8 Compare March 13, 2023 00:17
@Exirel
Copy link
Contributor

Exirel commented Mar 13, 2023

I'd dare to say that we could even prevent people from using the INFO level for channel logging.

@dgw
Copy link
Member Author

dgw commented Mar 13, 2023

we could even prevent people from using the INFO level

I didn't do that because "Channel joined:" log lines are emitted as INFO, and that seems like a useful thing to have in a log channel. Bucket, for example, also emits messages to its control channel when joining/parting a channel.

Regarding reporting channel joins/parts to IRC, we can treat that as a separate feature to be considered for 8.1. Bucket's join/part reporting is targeted at admin-requested actions, and we could extend our admin plugin with that functionality independent of channel logs.

About INFO logs: I think we still have work to do regarding which log level to use for various actions. As an example, see the logging for a kicked user, which is currently INFO but probably more appropriate for DEBUG.

Those two points aside, I think allowing INFO but defaulting to WARNING is appropriate for now. WARNING is low-volume enough that it should be fine on just about any bot, and it is safe for owners of low-to-medium traffic Sopel instances to drop it down to INFO if they want to see more of what's going on for a specific reason. I removed DEBUG because it breaks Sopel immediately upon joining IRC, even if it's only in one channel and no events have happened since startup—i.e. there's absolutely no chance that it would be useful.

Does all of this reasoning make sense? If I've missed considering something, please do bug me about it before I merge!

@dgw dgw mentioned this pull request Mar 13, 2023
4 tasks
@Exirel
Copy link
Contributor

Exirel commented Mar 13, 2023

Does all of this reasoning make sense?

Yes! With a small caveat: this applies if we keep sending technical logs to channels. I'd argue that a better feature (not to be done in this PR of course, and probably a new feature for 8.1/9.x) would be to completely separate these two types of logs:

  • technical logs, i.e. the code did something for technical reason
  • business logs, i.e. the code did something to support a feature (such as JOIN, INVITE)

The first one should probably not go to the channel logger unless it's a WARNING or an ERROR, and the second type could go as low as INFO without much issue. After all, we already separate application logs from raw IRC logs, so that would make sense to me to do as well for other types of logs.

@dgw
Copy link
Member Author

dgw commented Mar 13, 2023

a better feature (not to be done in this PR of course, and probably a new feature for 8.1/9.x)

Definitely. We need to think about how it should work. 😅

This should go on a tickler list. If there's no issue about it by merge time, I'll bang out a quick one.

Edit: Tickler issue #2422 created.

@dgw dgw merged commit 4bc3233 into master Mar 18, 2023
@dgw dgw deleted the logging-channel-max-level branch March 18, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants