Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a configure_logging function to centralize log level configuration across uvicorn and application loggers, addressing issues with logging setup.
- Introduces
configure_loggingfunction inhypertrade/logging.pyto set log levels for uvicorn and app loggers - Calls
configure_loggingduring app initialization inhypertrade/daemon.pyto apply settings - Validates log levels with fallback to "INFO" for invalid values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hypertrade/logging.py | Adds new configure_logging function to configure log levels for uvicorn and hypertrade loggers |
| hypertrade/daemon.py | Imports and calls configure_logging during daemon initialization to apply log level settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| valid_levels = {"CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "NOTSET"} | ||
|
|
||
| if level not in valid_levels: | ||
| level = "INFO" | ||
|
|
There was a problem hiding this comment.
The validation logic here is redundant. The log_level parameter is already validated and normalized by the _normalize_level field validator in hypertrade/config.py (lines 94-99), which ensures that only valid log levels reach this function. This duplicate validation adds unnecessary complexity. Consider removing lines 19-23 and using log_level directly, or at minimum, just call .upper() without re-validation.
| valid_levels = {"CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "NOTSET"} | |
| if level not in valid_levels: | |
| level = "INFO" |
| def configure_logging(log_level: str = "INFO") -> None: | ||
| """Configure Python logging to use the specified level.""" | ||
| level = log_level.upper() | ||
| valid_levels = {"CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "NOTSET"} | ||
|
|
||
| if level not in valid_levels: | ||
| level = "INFO" | ||
|
|
||
| # Configure uvicorn loggers | ||
| pylog.getLogger("uvicorn").setLevel(level) | ||
| pylog.getLogger("uvicorn.access").setLevel(level) | ||
| pylog.getLogger("uvicorn.error").setLevel(level) | ||
|
|
||
| # Configure app logger | ||
| pylog.getLogger("hypertrade").setLevel(level) |
There was a problem hiding this comment.
The new configure_logging function lacks test coverage. The repository has comprehensive tests for other modules (e.g., test_health.py, test_webhook.py), but this new logging configuration functionality is not tested. Consider adding tests to verify:
- Correct log level configuration for all uvicorn and app loggers
- Behavior with valid and invalid log levels
- Integration with the daemon initialization
1 participant
No description provided.