Rename and clean up logging and starlette #48
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the logging system in the project. The changes involve migrating from a custom JSON formatter to the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Logging as Logging Setup
participant StructLog as Structlog
App->>Logging: Call setup_logging()
Logging->>StructLog: Configure logging processors
Logging->>Logging: Set up log handlers
Logging->>StructLog: Configure log formatting
Note over Logging,StructLog: Logging configured with optional JSON format
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
dnstapir/logging.py (1)
23-23: Ensure compatibility with Python versions prior to 3.9The use of built-in generic types like
list[Processor]requires Python 3.9 or higher. If the project supports earlier versions (e.g., Python 3.7 or 3.8), consider importingListfrom thetypingmodule.Apply this diff for compatibility:
+from typing import List -shared_processors: list[Processor] = [ +shared_processors: List[Processor] = [ structlog.contextvars.merge_contextvars,tests/test_logging.py (2)
9-9: Adjust logging statement to followstructlogbest practicesWhen using
structlog, it's recommended to pass variables as keyword arguments rather than using positional string formatting. The current use may not format the message as expected.Consider revising the logging statement:
- logger.warning("Hello %s", "world", foo="bar") + logger.warning("Hello world", foo="bar")Or, to include dynamic content:
- logger.warning("Hello %s", "world", foo="bar") + logger.warning(f"Hello {'world'}", foo="bar")
5-9: Add assertions to the test function to validate loggingThe
test_loggingfunction currently lacks assertions. Adding assertions ensures that the logging configuration behaves as expected and helps catch issues early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
dnstapir/logging.py(1 hunks)dnstapir/starlette.py(1 hunks)dnstapir/structlog.py(0 hunks)pyproject.toml(0 hunks)tests/test_logging.py(1 hunks)tests/test_structlog.py(0 hunks)
💤 Files with no reviewable changes (3)
- pyproject.toml
- tests/test_structlog.py
- dnstapir/structlog.py
✅ Files skipped from review due to trivial changes (1)
- dnstapir/starlette.py
🔇 Additional comments (1)
dnstapir/logging.py (1)
69-76: Verify the logging behavior for Uvicorn loggersThe configuration modifies handlers and propagation for Uvicorn loggers. Ensure that this setup captures logs as intended without suppressing important log messages.
Would you like assistance in verifying that Uvicorn logs are correctly captured and no critical logs are missing?
| return event_dict | ||
|
|
||
|
|
||
| def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None: |
There was a problem hiding this comment.
Validate the log_level parameter to prevent invalid log levels
The log_level parameter in setup_logging is not validated before use. Passing an invalid log level could lead to runtime errors. Consider validating it against the allowed log levels to ensure robustness.
Apply this diff to add validation:
def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None:
+ valid_log_levels = ["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "NOTSET"]
+ if log_level.upper() not in valid_log_levels:
+ raise ValueError(f"Invalid log level: {log_level}")
timestamper = structlog.processors.TimeStamper(fmt="iso")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None: | |
| def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None: | |
| valid_log_levels = ["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "NOTSET"] | |
| if log_level.upper() not in valid_log_levels: | |
| raise ValueError(f"Invalid log level: {log_level}") | |
| timestamper = structlog.processors.TimeStamper(fmt="iso") |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
dnstapir/logging.py (5)
1-3: Enhance module documentation.The docstring should include more details about the module's purpose, functionality, and usage examples.
""" -structlog helpers based on code from https://wazaari.dev/blog/fastapi-integration +Structured logging configuration for dnstapir. + +This module provides utilities for setting up structured logging using the structlog library. +It includes functionality for handling both JSON and console logging formats, with special +handling for Uvicorn logs. + +Based on code from: https://wazaari.dev/blog/fastapi-structlog-integration + +Example: + >>> from dnstapir.logging import setup_logging + >>> setup_logging(json_logs=True, log_level="INFO") """
10-10: Consider using Enum for log levels.Using an Enum for log levels would provide better type safety and IDE support.
-VALID_LOG_LEVELS = set(["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG"]) +from enum import Enum + +class LogLevel(str, Enum): + CRITICAL = "CRITICAL" + ERROR = "ERROR" + WARNING = "WARNING" + INFO = "INFO" + DEBUG = "DEBUG" + +VALID_LOG_LEVELS = set(level.value for level in LogLevel)
22-24: Improve function documentation.The docstring should include parameter descriptions, return type, and a note about global state modification.
- """Set up logging""" + """ + Configure structured logging for the application. + + This function modifies the global logging state by reconfiguring the root logger + and Uvicorn loggers. + + Args: + json_logs (bool): If True, use JSON formatter. Otherwise, use console formatter. + log_level (str): The logging level to use. Must be one of CRITICAL, ERROR, + WARNING, INFO, or DEBUG. + + Raises: + ValueError: If the provided log_level is invalid. + """
30-39: Consider moving shared processors to a module-level constant.The shared processors list could be defined at the module level to improve readability and reusability.
+DEFAULT_PROCESSORS: list[Processor] = [ + structlog.contextvars.merge_contextvars, + structlog.stdlib.add_logger_name, + structlog.stdlib.add_log_level, + structlog.stdlib.PositionalArgumentsFormatter(), + structlog.stdlib.ExtraAdder(), + drop_color_message_key, + structlog.processors.TimeStamper(fmt="iso"), + structlog.processors.StackInfoRenderer(), +] + def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None: # ... - shared_processors: list[Processor] = [ - structlog.contextvars.merge_contextvars, - structlog.stdlib.add_logger_name, - structlog.stdlib.add_log_level, - structlog.stdlib.PositionalArgumentsFormatter(), - structlog.stdlib.ExtraAdder(), - drop_color_message_key, - timestamper, - structlog.processors.StackInfoRenderer(), - ] + shared_processors = DEFAULT_PROCESSORS.copy()
22-83: Consider breaking down the setup_logging function.The function is quite long and handles multiple concerns. Consider splitting it into smaller, focused functions:
- Validation
- Processor configuration
- Root logger setup
- Uvicorn logger setup
+def _validate_log_level(log_level: str) -> None: + if log_level.upper() not in VALID_LOG_LEVELS: + raise ValueError(f"Invalid log level: {log_level}") + +def _configure_processors(json_logs: bool) -> tuple[list[Processor], structlog.stdlib.ProcessorFormatter]: + shared_processors = DEFAULT_PROCESSORS.copy() + if json_logs: + shared_processors.append(structlog.processors.format_exc_info) + + structlog.configure( + processors=shared_processors + [structlog.stdlib.ProcessorFormatter.wrap_for_formatter], + logger_factory=structlog.stdlib.LoggerFactory(), + cache_logger_on_first_use=True, + ) + + log_renderer = structlog.processors.JSONRenderer() if json_logs else structlog.dev.ConsoleRenderer() + return shared_processors, structlog.stdlib.ProcessorFormatter( + foreign_pre_chain=shared_processors, + processors=[ + structlog.stdlib.ProcessorFormatter.remove_processors_meta, + log_renderer, + ], + ) + +def _setup_root_logger(formatter: structlog.stdlib.ProcessorFormatter, log_level: str) -> None: + handler = logging.StreamHandler() + handler.setFormatter(formatter) + root_logger = logging.getLogger() + root_logger.addHandler(handler) + root_logger.setLevel(log_level.upper()) + +def _setup_uvicorn_loggers() -> None: + for _log in ["uvicorn", "uvicorn.error"]: + logging.getLogger(_log).handlers.clear() + logging.getLogger(_log).propagate = True + logging.getLogger("uvicorn.access").handlers.clear() + logging.getLogger("uvicorn.access").propagate = False + def setup_logging(json_logs: bool = False, log_level: str = "INFO") -> None: - # ... current implementation ... + _validate_log_level(log_level) + shared_processors, formatter = _configure_processors(json_logs) + _setup_root_logger(formatter, log_level) + _setup_uvicorn_loggers()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dnstapir/logging.py(1 hunks)
🔇 Additional comments (1)
dnstapir/logging.py (1)
13-19: LGTM! Well-documented utility function.The function is well-typed, properly documented, and correctly handles the removal of duplicate color messages from Uvicorn logs.
Summary by CodeRabbit
New Features
structlogfor more structured and flexible loggingImprovements
Dependency Changes
jsonformatterdependencyRefactor
structlogTests