RSPEED-2437: Add configurable log level and unify logging#1148
RSPEED-2437: Add configurable log level and unify logging#1148major wants to merge 4 commits intolightspeed-core:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughThis PR centralizes logging configuration by replacing the standard Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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(
…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>
major
force-pushed
the
mhayden/configurable-log-level
branch
2 times, most recently
from
ca3b5ad to
121dfcc
Compare
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>
major
force-pushed
the
mhayden/configurable-log-level
branch
from
121dfcc to
c5aed53
Compare
1 participant
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)