fix(socket_mode): check the effective log level in debug guards - #1965
Shubham-Padkonde wants to merge 1 commit into
Conversation
The debug guards compared logger.level, which is NOTSET unless a level is set on that exact logger, so the guarded messages were built even when DEBUG is disabled through a parent logger. Use isEnabledFor(), which follows the effective level. Refs slackapi#1957 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1965 +/- ##
=======================================
Coverage 84.15% 84.15%
=======================================
Files 118 118
Lines 13566 13566
=======================================
Hits 11416 11416
Misses 2150 2150 ☔ View full report in Codecov by Harness. |
AmyScript
left a comment
There was a problem hiding this comment.
Nice fix. logger.level stays NOTSET (0) unless a level is set on that exact logger, so level <= DEBUG was effectively always true and the guarded strings (notably debug_redacted_message_string(message) on every inbound message) were built regardless of the effective level. isEnabledFor() follows getEffectiveLevel() and also respects logging.disable(). It's strictly safe too. Test genuinely fails before the change. LGTM. ✅
Non-blocking follow-ups (no need to hold this PR):
- Worth a quick
rg 'logger\.level\s*(<=|<|==)\s*logging\.' slack_sdk/socket_mode/
just to confirm nothing in these modules slipped through. - Adding an async-path test case wouldn't hurt
Fixes the Socket Mode part of #1957.
The debug guards compared
logger.level, which staysNOTSETunless a level is set on that exact logger, so the guarded messages (includingdebug_redacted_message_string(message)for every inbound message) were built even when DEBUG was disabled through a parent logger. They now useisEnabledFor(logging.DEBUG), which follows the effective level.I kept this to
slack_sdk/socket_mode/; the same pattern in the other modules can follow separately if you want it project-wide.The added test checks that the message is not built when a parent logger is at INFO and is built at DEBUG; it fails before this change. The Socket Mode test suites (sync and async), ruff and mypy pass.
🤖 Generated with Claude Code