RSPEED-2437: Add configurable log level and unify logging#1148
RSPEED-2437: Add configurable log level and unify logging#1148tisnik merged 5 commits intolightspeed-core:mainfrom
Conversation
WalkthroughReplaces per-module use of Python's stdlib Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/endpoints/health.py (1)
18-29:⚠️ Potential issue | 🟡 MinorUpdate documentation reference to new endpoint logger names.
The logger name change from shared
"app.endpoints.handlers"to per-module names (e.g.,"app.endpoints.health") is reflected in code, but documentation indocs/a2a_protocol.mdstill references the old logger name. Update this reference to ensure monitoring and log filtering guidance remains accurate for users checking endpoint logs.No Python code actively references the old logger name, so the change is localized to configuration and documentation updates.
🧹 Nitpick comments (4)
src/utils/token_counter.py (1)
4-6: Coding guideline deviation:get_logger(__name__)replaces the mandatedlogging.getLogger(__name__)pattern.The project coding guidelines specify: "Use
logger = logging.getLogger(__name__)pattern for module logging". This PR intentionally replaces that pattern across ~45 modules. The newget_loggerwrapper internally delegates tologging.getLogger(name), so it's functionally compatible, but the guideline text should be updated to reflect the new convention (e.g., inCLAUDE.mdor equivalent) to avoid confusion in future reviews.This applies to all 8 files in this PR and the ~35 other modules mentioned in the commit description.
As per coding guidelines:
Use logger = logging.getLogger(__name__) pattern for module logging.tests/unit/authorization/test_azure_token_manager.py (1)
139-147: Correct workaround forpropagate=False, but consider extracting a reusable fixture.The
try/finallyto togglepropagateis necessary becauseget_loggerdisables propagation (which preventscaplogfrom capturing log records). This is correct, but if other tests face the same issue, a shared pytest fixture (or context manager) that temporarily enables propagation would reduce boilerplate and risk of copy-paste errors.♻️ Optional: reusable context manager
# conftest.py or a test utility module from contextlib import contextmanager import logging `@contextmanager` def propagate_logger(name: str): """Temporarily enable propagation on a logger so caplog can capture records.""" log = logging.getLogger(name) log.propagate = True try: yield log finally: log.propagate = FalseUsage:
with propagate_logger("authorization.azure_token_manager"): with caplog.at_level("WARNING"): result = token_manager.refresh_token() assert result is False assert "Failed to retrieve Azure access token" in caplog.textsrc/lightspeed_stack.py (2)
22-34: Duplicate log-level validation logic withsrc/log.py.Lines 22–32 replicate the same env-var read →
getattrvalidate → fallback flow thatget_logger()insrc/log.py(lines 32–42) already performs. If the validation rules ever diverge (e.g., supporting custom level names), you'd need to update both places.Consider extracting a shared helper (e.g.,
resolve_log_level() -> int) inlog.pythat both call sites can use.
26-31: Moveimport systo the top of the file.Placing a stdlib import inside a conditional block at module level is unconventional. Since this code runs unconditionally at import time (module-level
if),sysshould be with the other imports at the top.Proposed fix
import logging import os +import sys from argparse import ArgumentParserThen at line 26–27:
- import sys - print(
major
force-pushed
the
mhayden/configurable-log-level
branch
5 times, most recently
from
47b87fe to
fb4f58e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/a2a_protocol.md`:
- Line 774: The docs incorrectly instruct readers to check the
`app.endpoints.health` logger; update the A2A troubleshooting Debug Logging
section to reference the actual A2A logger created by get_logger(__name__)
(i.e., `app.endpoints.a2a`) instead of `app.endpoints.health`, and ensure any
log examples or grep commands use `app.endpoints.a2a` so they match the logger
name used by the A2A endpoint implementation.
🧹 Nitpick comments (2)
tests/unit/authorization/test_azure_token_manager.py (1)
139-147: Reasonable workaround forpropagate=False, minor fragility note.The hardcoded logger name
"authorization.azure_token_manager"will silently break if the module is ever renamed or moved. Consider importing the module and using its__name__to keep the test in sync:from authorization import azure_token_manager azure_logger = logging.getLogger(azure_token_manager.__name__)That said, the
try/finallyto restore propagation state is well done.src/log.py (1)
35-45: Warning for invalid log level is emitted before the RichHandler is attached.On lines 37-41,
logger.warning(...)fires before line 45 setslogger.handlers = [RichHandler()]. On the first call for a given logger name, the logger has no handlers yet, so this warning goes through Python'slastResorthandler (plain stderr) rather than the configured RichHandler — producing inconsistent formatting compared to all subsequent log output.Consider reordering: attach the RichHandler first (with a safe default level), then validate and adjust the level.
Proposed fix
logger = logging.getLogger(name) # Skip reconfiguration if logger already has a RichHandler from a prior call if any(isinstance(h, RichHandler) for h in logger.handlers): return logger + # Attach RichHandler early so any warnings use consistent formatting + logger.handlers = [RichHandler()] + logger.propagate = False + # Read log level from environment variable with default fallback level_str = os.environ.get(LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, DEFAULT_LOG_LEVEL) # Validate the level string and convert to logging level constant validated_level = getattr(logging, level_str.upper(), None) if not isinstance(validated_level, int): logger.warning( "Invalid log level '%s', falling back to %s", level_str, DEFAULT_LOG_LEVEL, ) validated_level = getattr(logging, DEFAULT_LOG_LEVEL) logger.setLevel(validated_level) - logger.handlers = [RichHandler()] - logger.propagate = False return logger
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/authorization/test_azure_token_manager.py (1)
140-148: Save and restore the originalpropagatevalue instead of hardcodingFalse.The
finallyblock assumespropagatewas originallyFalse. If the centralized logger's default changes, this will silently leave the logger in the wrong state for subsequent tests in the same process.♻️ Suggested fix
azure_logger = logging.getLogger(azure_token_manager.__name__) + original_propagate = azure_logger.propagate azure_logger.propagate = True try: with caplog.at_level("WARNING"): result = token_manager.refresh_token() assert result is False assert "Failed to retrieve Azure access token" in caplog.text finally: - azure_logger.propagate = False + azure_logger.propagate = original_propagate
…VEL env var This commit introduces runtime-configurable logging via the LIGHTSPEED_STACK_LOG_LEVEL environment variable, allowing deployment-time control of log verbosity without code changes. Key changes: - Added LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR and DEFAULT_LOG_LEVEL constants - Modified get_logger() to read log level from environment with defensive validation - Updated lightspeed_stack.py basicConfig() to respect the environment variable - Added force=True to basicConfig() to override llama_stack_client's early logging setup - Revived --verbose CLI flag to set DEBUG level and update all existing loggers - Added comprehensive unit tests covering default, custom, case-insensitive, invalid, and all valid log levels The --verbose flag now provides a convenient CLI shortcut for enabling debug logging, while the environment variable enables fine-grained control in containerized deployments. Signed-off-by: Pavel Tišnovský <ptisnovs@redhat.com> Signed-off-by: Major Hayden <major@redhat.com>
All 43+ modules now use the centralized get_logger() function from log.py, ensuring consistent log level configuration via the LIGHTSPEED_STACK_LOG_LEVEL environment variable. Non-standard logger names have been standardized to __name__ for clarity and consistency across the codebase. Signed-off-by: Major Hayden <major@redhat.com>
Signed-off-by: Major Hayden <major@redhat.com>
Signed-off-by: Major Hayden <major@redhat.com>
…ragile hardcoded names Address review feedback: move RichHandler attachment before log level validation so warnings use consistent formatting, use module.__name__ instead of hardcoded logger name string in tests, and fix stale logger reference in A2A docs. Signed-off-by: Major Hayden <major@redhat.com>
major
force-pushed
the
mhayden/configurable-log-level
branch
from
e53353b to
2517935
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lightspeed_stack.py (1)
23-34: Duplicate log-level validation logic — consider extracting tosrc/log.py.The env-var reading and validation (lines 23–31) duplicates the same logic inside
get_logger()insrc/log.py(lines 37–47). If validation rules change (e.g., accepting custom level names), both sites must be kept in sync.Consider exporting a small helper from
log.py(e.g.,resolve_log_level() -> int) that bothbasicConfighere andget_loggercan call.♻️ Sketch of shared helper in src/log.py
# In src/log.py def resolve_log_level() -> int: """Read and validate the log level from the environment variable. Returns: int: A valid logging level constant. """ level_str = os.environ.get(LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, DEFAULT_LOG_LEVEL) validated_level = getattr(logging, level_str.upper(), None) if not isinstance(validated_level, int): # Warning must go to stderr since loggers may not be configured yet print( f"WARNING: Invalid log level '{level_str}', falling back to {DEFAULT_LOG_LEVEL}", file=sys.stderr, ) validated_level = getattr(logging, DEFAULT_LOG_LEVEL) return validated_levelThen in
lightspeed_stack.py:-log_level_str = os.environ.get(LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, DEFAULT_LOG_LEVEL) -log_level = getattr(logging, log_level_str.upper(), None) -if not isinstance(log_level, int): - print( - f"WARNING: Invalid log level '{log_level_str}', falling back to {DEFAULT_LOG_LEVEL}", - file=sys.stderr, - ) - log_level = getattr(logging, DEFAULT_LOG_LEVEL) +from log import resolve_log_level +log_level = resolve_log_level()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightspeed_stack.py` around lines 23 - 34, Extract the duplicate environment-based log-level parsing/validation into a shared helper (e.g., resolve_log_level()) in src/log.py and have both get_logger() and the top-level basicConfig call in lightspeed_stack.py call it; specifically, move the logic that reads LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, validates via getattr(logging, ...), prints the stderr warning and falls back to DEFAULT_LOG_LEVEL into resolve_log_level() and replace the inline validation in lightspeed_stack.py (the block that computes log_level and calls logging.basicConfig) and the validation inside get_logger() to call resolve_log_level() and use its returned int when configuring the logger.
🤖 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/utils/token_counter.py`:
- Around line 4-6: Remove the unused custom logger: delete the import of
get_logger and the logger = get_logger(__name__) initialization in the module so
the unused symbol logger and its wrapper import are removed; if you later need
logging, replace them with the standard pattern by adding import logging and
logger = logging.getLogger(__name__) in this module (refer to the logger
variable and get_logger import to locate the lines to delete).
---
Nitpick comments:
In `@src/lightspeed_stack.py`:
- Around line 23-34: Extract the duplicate environment-based log-level
parsing/validation into a shared helper (e.g., resolve_log_level()) in
src/log.py and have both get_logger() and the top-level basicConfig call in
lightspeed_stack.py call it; specifically, move the logic that reads
LIGHTSPEED_STACK_LOG_LEVEL_ENV_VAR, validates via getattr(logging, ...), prints
the stderr warning and falls back to DEFAULT_LOG_LEVEL into resolve_log_level()
and replace the inline validation in lightspeed_stack.py (the block that
computes log_level and calls logging.basicConfig) and the validation inside
get_logger() to call resolve_log_level() and use its returned int when
configuring the logger.
| from log import get_logger | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger = get_logger(__name__) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if logger is used in the file
rg -n 'logger\.' src/utils/token_counter.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Also check the entire content of the file to understand context
wc -l src/utils/token_counter.py && cat -n src/utils/token_counter.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1567
🏁 Script executed:
# Check what get_logger is and where it comes from
find . -name "log.py" -o -name "log" -type d | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 88
🏁 Script executed:
# Search for how get_logger is defined and used across the codebase
rg -n "def get_logger" --type pyRepository: lightspeed-core/lightspeed-stack
Length of output: 134
🏁 Script executed:
# Check coding guideline compliance - look for logging.getLogger usage vs get_logger
rg -n "logging.getLogger|from log import get_logger" src/ --type py | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 1825
Remove unused logger and use standard logging pattern per coding guidelines.
The logger is initialized but never used in this module. Additionally, the import pattern violates the coding guidelines, which specify logger = logging.getLogger(__name__) directly instead of using the custom get_logger wrapper.
Remove lines 4-6 entirely. If logging is needed in the future, use the standard pattern:
import logging
logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/token_counter.py` around lines 4 - 6, Remove the unused custom
logger: delete the import of get_logger and the logger = get_logger(__name__)
initialization in the module so the unused symbol logger and its wrapper import
are removed; if you later need logging, replace them with the standard pattern
by adding import logging and logger = logging.getLogger(__name__) in this module
(refer to the logger variable and get_logger import to locate the lines to
delete).
3 participants
Description
Adds configurable log level via
LIGHTSPEED_STACK_LOG_LEVELenvironment variable and centralizes all logging through a singleget_logger()function insrc/log.py.Commits
feat(logging)— Introducessrc/log.pywithget_logger()that reads log level from the env var (defaults toINFO), validates the value with a warning on invalid input, and configuresRichHandlerfor consistent console output. The--verboseCLI flag overrides the env var. Movesimport systo top-level inlightspeed_stack.py.refactor(logging)— Mechanical refactor replacing alllogging.getLogger()calls withget_logger(__name__)across 52 modules. No logic or behavioral changes — every file gets the same two-line diff. Reviewable via--statand spot-checking. Uvicorn logging (src/runners/uvicorn.py) is intentionally excluded.docs: a2a_protocol.md— Updates stale logger name reference fromapp.endpoints.handlerstoapp.endpoints.health.docs: CLAUDE.md— Updates dev guide logging pattern fromlogging.getLogger(__name__)toget_logger(__name__).Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
LIGHTSPEED_STACK_LOG_LEVEL=DEBUGand verify all module loggers output at DEBUG levelLIGHTSPEED_STACK_LOG_LEVEL=INVALIDand verify a warning is printed with fallback to INFO--verboseflag and verify application loggers switch to DEBUGuv run make verifypasses (all linters/type checks clean)Summary by CodeRabbit
New Features
Documentation
Tests