Repository navigation
fix(logging): log warnings without a traceback - #201
Conversation
A member who left the guild is an expected case in get_member_or_user, and the code falls back to the user lookup. It logged a full traceback at WARNING, which produced dozens of tracebacks on every start when scheduled unbans and unmutes were re-created. It now logs one DEBUG line with the id. Forbidden, HTTP errors and a missing user still log with the traceback. The user lookup messages now say user instead of guild member.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
- Coverage 67.20% 66.64% -0.56%
==========================================
Files 74 76 +2
Lines 3436 3586 +150
==========================================
+ Hits 2309 2390 +81
- Misses 1127 1196 +69 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The level stays as it was. Only the traceback goes.
A warning is a handled condition, so a traceback only buries real errors. No logger.warning call in src/ passes exc_info any more, and a test fails if one does. Errors keep their traceback. The dynamic role reload warning had only the traceback to explain what failed, so its message now includes the exception text.
ToxicBiohazard
left a comment
There was a problem hiding this comment.
Thanks for this. The ~80 NotFound tracebacks on every startup are real noise, and "no traceback for warnings, keep it for errors" makes sense to me. The AST test is a nice way to keep it that way, and good catch on the alembic fileConfig disabling the loggers.
Two things I'd like changed before merging:
- The HTTPException warnings in ban.py now don't tell us what actually went wrong. Those aren't routine like a member leaving, so we still need the cause in the log.
- The message renames in get_member_or_user were only done in one of the two fallback branches.
The rest are small nits.
| f"them..." | ||
| ) | ||
| except HTTPException: | ||
| logger.warning(f"HTTPException when trying to unban user with ID {member.id}") |
There was a problem hiding this comment.
Same problem as the ban HTTPException at line 361. If the DM fails, we don't know which Discord error it was. Can we add : {exc} here too (with except HTTPException as exc:)?
Also this says "unban" but it's the DM sent before a ban. Not from this PR, but could you fix it while you're on this line?
There was a problem hiding this comment.
Done in 7e72604. Added the cause and fixed the wording: HTTPException when trying to DM user with ID {id} about their ban: {exc}.
…okup wording The ban, ban DM and infraction DM warnings now end with the Discord error (status, reason and error code), so the cause is logged without a traceback. The ban DM warning also said unban; it now says what failed. The user fallback after an HTTPException on the member lookup now uses the same messages as the fallback after NotFound.
Types of changes
Proposed changes
Warnings no longer log a traceback. A warning is a handled condition, so a traceback only buries real errors. Errors keep their traceback, because it makes issues easier to resolve.
This started from the bot's startup log. On every start, the scheduled tasks re-schedule unbans and unmutes for users who left the guild. Each one logged
Could not find guild memberat WARNING with a fullNotFound 10007 Unknown Membertraceback, roughly 80 per start.logger.warningcall insrc/that passedexc_infodrops it: 7 insrc/bot.py, 6 insrc/helpers/ban.py, 1 insrc/services/role_manager.py. Levels and messages are unchanged.except ... as excbindings that are now unused are removed.role_manager.pyhad only the traceback to say what failed, so its message now ends with the exception text.tests/src/test_logging_rules.pyparsessrc/and fails if anylogger.warningcall passesexc_infoorstack_info, so the rule holds for new code.Checklist
doc.
Additional context
New tests:
tests/src/test_bot.py:get_member_or_user, each branch's return value, level, message, and whether a traceback is attached (WARNING never, ERROR always).tests/src/services/test_role_manager.py: a failed reload keeps the cache and logs the cause without a traceback.tests/src/test_logging_rules.py: the rule above. It failed with 14 call sites before the change.Full suite: 328 passed.