feat: add TRACE log level and patch_logging()#5
Conversation
Add TRACE log level (DEBUG - 5 = 5) for fine-grained protocol-level debugging, mirroring the implementation in Appier. Includes patch_logging() to register the level with Python's logging module, TRACE-aware format templates with file path and line number, and comprehensive test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA TRACE logging level is introduced alongside a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Set stacklevel in trace(), debug(), info(), warning(), error(), and critical() wrapper functions so that %(pathname)s and %(lineno)d report the actual caller location instead of the wrapper code. Guarded by sys.version_info >= (3, 8) as stacklevel was introduced in Python 3.8. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f9be744cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is idempotent and safe to be called multiple times | ||
| log.patch_logging() | ||
|
|
||
| is_trace = level <= log.TRACE |
There was a problem hiding this comment.
start_log() now does level <= log.TRACE before any normalization, which raises TypeError on Python 3 when callers pass string levels (for example "DEBUG" or "TRACE"). This is a regression from the previous behavior where string levels could flow to logger.setLevel(...) without crashing, and it affects direct users of the public quorum.start_log API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TRACE logging level (numeric value 5) and associated helpers to support very fine-grained debugging in Flask-Quorum’s logging stack, including format templates optimized for protocol-level troubleshooting.
Changes:
- Added
TRACElog level,patch_logging()monkey-patch helper, andtrace()convenience logger inquorum.log. - Updated
start_log()to auto-register TRACE and to switch to TRACE-specific formats when configured at TRACE level. - Expanded unit tests to cover TRACE, level mapping,
patch_logging(),_level(),in_signature(), and handler helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/quorum/log.py |
Adds TRACE constants/formats plus patch_logging() and trace() helper functionality. |
src/quorum/base.py |
Calls patch_logging() from start_log(), selects TRACE-aware formats, adds is_trace(), and maps "TRACE" in _level(). |
src/quorum/test/log.py |
Adds new tests for TRACE behavior, patching behavior, and logging helpers. |
src/quorum/__init__.py |
Exposes TRACE-related APIs from the package root. |
CHANGELOG.md |
Documents TRACE level and patch_logging() addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def in_signature(callable, name): | ||
| has_full = hasattr(inspect, "getfullargspec") | ||
| if has_full: | ||
| spec = inspect.getfullargspec(callable) | ||
| else: | ||
| spec = inspect.getargspec(callable) | ||
| args, _varargs, kwargs = spec[:3] | ||
| return (args and name in args) or (kwargs and "secure" in kwargs) | ||
| return bool((args and name in args) or (kwargs and "secure" in kwargs)) |
There was a problem hiding this comment.
in_signature() is meant to check whether a callable accepts a parameter named by name, but the current implementation hard-codes 'secure' for the **kwargs case (kwargs and 'secure' in kwargs). With inspect.getfullargspec(), kwargs here is the name of the var-keyword parameter (eg 'kwargs'), so this check will almost always be false and doesn’t correctly represent “accepts arbitrary kwargs”. Consider updating this to (a) check name against positional/kw-only args, and (b) treat presence of spec.varkw as accepting the parameter.
| if sys.version_info >= (3, 8): | ||
| kwargs.setdefault("stacklevel", 2) | ||
| logger.trace(message, *args, **kwargs) |
There was a problem hiding this comment.
trace() calls logger.trace(...) unconditionally. If patch_logging() hasn’t been called yet (eg apps that don’t go through start_log()), this will raise AttributeError because logging.Logger won’t have a trace method. Consider calling patch_logging() inside trace() (idempotent) or falling back to logger.log(TRACE, ...) when .trace is missing.
| if sys.version_info >= (3, 8): | |
| kwargs.setdefault("stacklevel", 2) | |
| logger.trace(message, *args, **kwargs) | |
| patch_logging() | |
| if sys.version_info >= (3, 8): | |
| kwargs.setdefault("stacklevel", 2) | |
| if hasattr(logger, "trace"): | |
| logger.trace(message, *args, **kwargs) | |
| else: | |
| logger.log(TRACE, message, *args, **kwargs) |
| LOGGING_FORMAT_TRACE_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %s%%(pathname)s:%%(lineno)d | %%(message)s" | ||
| """ The format to be used when the logging level is set to TRACE, | ||
| includes file path and line number to allow for fine-grained debugging | ||
| of low-level protocol operations """ | ||
|
|
||
| LOGGING_FORMAT_TRACE_TID_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %s[%%(thread)d] %%(pathname)s:%%(lineno)d | %%(message)s" |
There was a problem hiding this comment.
The TRACE format templates already include %(name)s, but they are still parameterized with LOGGING_EXTRA (%s). If LOGGING_EXTRA is enabled (it expands to [%(name)s] ), TRACE logs will end up duplicating the logger name segment. Consider not applying LOGGING_EXTRA to TRACE formats (or making TRACE templates omit %(name)s and rely on LOGGING_EXTRA).
| LOGGING_FORMAT_TRACE_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %s%%(pathname)s:%%(lineno)d | %%(message)s" | |
| """ The format to be used when the logging level is set to TRACE, | |
| includes file path and line number to allow for fine-grained debugging | |
| of low-level protocol operations """ | |
| LOGGING_FORMAT_TRACE_TID_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %s[%%(thread)d] %%(pathname)s:%%(lineno)d | %%(message)s" | |
| LOGGING_FORMAT_TRACE_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %.0s%%(pathname)s:%%(lineno)d | %%(message)s" | |
| """ The format to be used when the logging level is set to TRACE, | |
| includes file path and line number to allow for fine-grained debugging | |
| of low-level protocol operations """ | |
| LOGGING_FORMAT_TRACE_TID_T = "%%(asctime)s [%%(name)s] [%%(levelname)s] %.0s[%%(thread)d] %%(pathname)s:%%(lineno)d | %%(message)s" |
| TRACE = logging.DEBUG - 5 | ||
| """ The trace level used for extremely detailed and verbose | ||
| logging of protocol-level operations, this is meant to be | ||
| used for fine-grained debugging of low-level operations | ||
| like raw byte transfers and frame parsing """ | ||
|
|
||
| LEVELS = ("DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL") | ||
| """ The sequence of levels from the least sever to the | ||
| most sever this sequence may be used to find all the |
There was a problem hiding this comment.
TRACE is introduced, but LEVELS doesn’t include 'TRACE'. As a result, MemoryHandler.get_messages_l() will treat TRACE records as an unknown level and won’t store them in messages_l, so get_latest(level=...) filtering can’t work for TRACE. Consider adding 'TRACE' to LEVELS (and possibly LEVEL_ALIAS) so TRACE messages participate in severity filtering consistently.
| # patches the logging infra-structure so that the TRACE level | ||
| # is properly registered and available for usage, this call | ||
| # is idempotent and safe to be called multiple times | ||
| log.patch_logging() | ||
|
|
||
| is_trace = level <= log.TRACE | ||
| format_base = format_base or ( | ||
| log.LOGGING_FORMAT_TRACE if is_trace else log.LOGGING_FORMAT | ||
| ) | ||
| format_tid = format_tid or ( | ||
| log.LOGGING_FORMAT_TRACE_TID if is_trace else log.LOGGING_FORMAT_TID | ||
| ) |
There was a problem hiding this comment.
start_log() computes is_trace = level <= log.TRACE, which will raise TypeError if callers pass a string level (eg 'INFO') or another non-int level value. Since start_log() is part of the public API, consider normalizing level to an int at the start (eg via _level(level) / logging._checkLevel) before comparing and before logger.setLevel(level).
| import os | ||
| import logging | ||
| import tempfile | ||
| import unittest |
There was a problem hiding this comment.
unittest is imported but not used in this test module. Consider removing the unused import to keep the test file minimal (and avoid potential lint failures in environments that enforce unused-import checks).
| import unittest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/quorum/__init__.py (1)
127-127: Consider the public API implications of exporting_level.The underscore prefix conventionally indicates an internal function, yet it's now part of the public exports. This is acceptable if intentional (tests use it directly as
quorum._level()), but consider renaming tolevelif it's meant to be public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/quorum/__init__.py` at line 127, The module currently exports an internal-looking symbol `_level` which makes it part of the public API despite the underscore convention; decide whether it should be public or internal and act accordingly: if it is intended for public use (e.g., tests call `quorum._level()`), rename `_level` to `level` and update the export list (e.g., `__all__`) and all call sites/tests to use `level`; otherwise remove `_level` from the public exports (and from `__all__`) so it remains internal and update any external callers/tests to use the supported public API.src/quorum/base.py (1)
686-697: Local variableis_traceshadows the module-level function.The local variable
is_traceon line 691 shadows the functionis_trace()defined at line 934. While this doesn't cause a bug (the function isn't called withinstart_log), it's confusing and could lead to issues if the code evolves.♻️ Suggested rename
- is_trace = level <= log.TRACE - format_base = format_base or ( - log.LOGGING_FORMAT_TRACE if is_trace else log.LOGGING_FORMAT + use_trace_format = level <= log.TRACE + format_base = format_base or ( + log.LOGGING_FORMAT_TRACE if use_trace_format else log.LOGGING_FORMAT ) format_tid = format_tid or ( - log.LOGGING_FORMAT_TRACE_TID if is_trace else log.LOGGING_FORMAT_TID + log.LOGGING_FORMAT_TRACE_TID if use_trace_format else log.LOGGING_FORMAT_TID )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/quorum/base.py` around lines 686 - 697, The local variable is_trace in start_log shadows the module-level is_trace() function; rename the local variable (e.g., trace_level or is_level_trace) where it's assigned and used (the assignment comparing level <= log.TRACE and subsequent selection of format_base/format_tid) to avoid shadowing the is_trace() function defined later, and update all references within start_log accordingly so the function name remains available and the intent is clearer.src/quorum/test/log.py (1)
177-196: Test manipulates global logging state - consider isolation.This test removes
logging._quorum_patchedandlogging.Logger.traceto simulate an unpatched state. While the cleanup infinallyis correct, this could cause flaky behavior if tests run in parallel or if an exception occurs between deletion and the try block. The current implementation is acceptable for sequential test execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/quorum/test/log.py` around lines 177 - 196, The test test_level_trace_before_patch manipulates globals logging._quorum_patched and logging.Logger.trace which can leak state; replace the manual delete/restore with a scoped patch so state is isolated: use unittest.mock.patch.object or pytest's monkeypatch to temporarily remove or set logging._quorum_patched and logging.Logger.trace around the call to quorum._level("TRACE") (e.g., patch.object(logging, "_quorum_patched", new=None, create=True) and patch.object(logging.Logger, "trace", new=DEFAULT, create=True) or monkeypatch.delattr/monkeypatch.setattr) so cleanup is automatic and no finally block is required, keeping the assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/quorum/log.py`:
- Around line 359-366: The in_signature function has two issues: it uses the
hardcoded string "secure" when checking kwargs instead of the provided name
parameter, and its parameter named callable shadows a Python builtin; fix by
renaming the parameter (e.g., to func or fn) and updating the kwargs check to
use the name variable (i.e., check if kwargs and name in kwargs). Ensure you
update all references inside in_signature (including the
inspect.getfullargspec/getargspec call) to use the new parameter name.
---
Nitpick comments:
In `@src/quorum/__init__.py`:
- Line 127: The module currently exports an internal-looking symbol `_level`
which makes it part of the public API despite the underscore convention; decide
whether it should be public or internal and act accordingly: if it is intended
for public use (e.g., tests call `quorum._level()`), rename `_level` to `level`
and update the export list (e.g., `__all__`) and all call sites/tests to use
`level`; otherwise remove `_level` from the public exports (and from `__all__`)
so it remains internal and update any external callers/tests to use the
supported public API.
In `@src/quorum/base.py`:
- Around line 686-697: The local variable is_trace in start_log shadows the
module-level is_trace() function; rename the local variable (e.g., trace_level
or is_level_trace) where it's assigned and used (the assignment comparing level
<= log.TRACE and subsequent selection of format_base/format_tid) to avoid
shadowing the is_trace() function defined later, and update all references
within start_log accordingly so the function name remains available and the
intent is clearer.
In `@src/quorum/test/log.py`:
- Around line 177-196: The test test_level_trace_before_patch manipulates
globals logging._quorum_patched and logging.Logger.trace which can leak state;
replace the manual delete/restore with a scoped patch so state is isolated: use
unittest.mock.patch.object or pytest's monkeypatch to temporarily remove or set
logging._quorum_patched and logging.Logger.trace around the call to
quorum._level("TRACE") (e.g., patch.object(logging, "_quorum_patched", new=None,
create=True) and patch.object(logging.Logger, "trace", new=DEFAULT, create=True)
or monkeypatch.delattr/monkeypatch.setattr) so cleanup is automatic and no
finally block is required, keeping the assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7344b27-aa1e-426e-b342-f7f4a64220cc
📒 Files selected for processing (5)
CHANGELOG.mdsrc/quorum/__init__.pysrc/quorum/base.pysrc/quorum/log.pysrc/quorum/test/log.py
| def in_signature(callable, name): | ||
| has_full = hasattr(inspect, "getfullargspec") | ||
| if has_full: | ||
| spec = inspect.getfullargspec(callable) | ||
| else: | ||
| spec = inspect.getargspec(callable) | ||
| args, _varargs, kwargs = spec[:3] | ||
| return (args and name in args) or (kwargs and "secure" in kwargs) | ||
| return bool((args and name in args) or (kwargs and "secure" in kwargs)) |
There was a problem hiding this comment.
Bug: in_signature() kwargs check uses hardcoded "secure" instead of name parameter.
The function checks if name is in args, but the kwargs check incorrectly uses the hardcoded string "secure" instead of the name parameter. This will cause incorrect results when checking for any parameter other than "secure".
Additionally, the static analysis correctly flags that callable shadows a Python builtin.
🐛 Proposed fix
-def in_signature(callable, name):
- has_full = hasattr(inspect, "getfullargspec")
- if has_full:
- spec = inspect.getfullargspec(callable)
- else:
- spec = inspect.getargspec(callable)
- args, _varargs, kwargs = spec[:3]
- return bool((args and name in args) or (kwargs and "secure" in kwargs))
+def in_signature(func, name):
+ has_full = hasattr(inspect, "getfullargspec")
+ if has_full:
+ spec = inspect.getfullargspec(func)
+ else:
+ spec = inspect.getargspec(func)
+ args, _varargs, kwargs = spec[:3]
+ return bool((args and name in args) or (kwargs and name in kwargs))🧰 Tools
🪛 Ruff (0.15.9)
[error] 359-359: Function argument callable is shadowing a Python builtin
(A002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/quorum/log.py` around lines 359 - 366, The in_signature function has two
issues: it uses the hardcoded string "secure" when checking kwargs instead of
the provided name parameter, and its parameter named callable shadows a Python
builtin; fix by renaming the parameter (e.g., to func or fn) and updating the
kwargs check to use the name variable (i.e., check if kwargs and name in
kwargs). Ensure you update all references inside in_signature (including the
inspect.getfullargspec/getargspec call) to use the new parameter name.
Include TRACE in the severity-ordered LEVELS tuple so that MemoryHandler.get_messages_l() properly stores and filters TRACE records. Add "TRAC" alias to LEVEL_ALIAS for consistent short-name resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
DEBUG - 5 = 5) for fine-grained protocol-level debugging, mirroring the Appier implementation (hivesolutions/appier#84)patch_logging()function to register TRACE with Python's logging module (idempotent, guarded bylogging._quorum_patched)trace()convenience function,is_trace()helper, and TRACE case in_level()in_signature()return inbool()for type correctnesspatch_logging(),_level(),in_signature(), androtating_handler()Divergences from Appier
DummyLogger.trace()DummyLoggerin flask-quorumis_trace()App.is_trace()methodis_trace()functionis_devel())_reload_logging()reload_format()logging._appier_patchedlogging._quorum_patched_level()App._level()classmethod_level()functionTest plan
PYTHONPATH=src python3 -m unittest quorum.test.log)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
trace()method for TRACE-level messagespatch_logging()function to register TRACE and custom logging levels