Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements JSON-formatted logging for the application to improve log parsing and analysis in production environments.
Changes:
- Added json-log-formatter dependency (version 1.1.1)
- Created a custom JSONFormatter class that extends json-log-formatter to include log level in output
- Configured Django logging to use JSON formatting for all loggers including Django, root, and Gunicorn loggers
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| uv.lock | Updated dependency lock file with json-log-formatter package and various other package updates |
| src/snapy/settings.py | Added comprehensive LOGGING configuration with JSON formatter for all application loggers |
| src/snapy/logs.py | Implemented custom JSONFormatter class extending json-log-formatter to include level field |
| pyproject.toml | Added json-log-formatter>=1.1.1 to project dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LOGGING = { | ||
| "version": 1, | ||
| "disable_existing_loggers": False, | ||
| "formatters": { | ||
| "json": { | ||
| "()": "snapy.logs.JSONFormatter" | ||
| }, | ||
| }, | ||
| "handlers": { | ||
| "stdout": { | ||
| "class": "logging.StreamHandler", | ||
| "formatter": "json" | ||
| }, | ||
| }, | ||
| "loggers": { | ||
| "django": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "gunicorn.access": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "gunicorn.error": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The logging configuration is hardcoded to always use JSON formatting, regardless of the DEBUG environment variable. This makes local development debugging more difficult since developers will see JSON logs instead of human-readable formatted logs. Consider making the logging format configurable based on DEBUG or a separate environment variable to allow different formats for development vs production environments.
| LOGGING = { | ||
| "version": 1, | ||
| "disable_existing_loggers": False, | ||
| "formatters": { | ||
| "json": { | ||
| "()": "snapy.logs.JSONFormatter" | ||
| }, | ||
| }, | ||
| "handlers": { | ||
| "stdout": { | ||
| "class": "logging.StreamHandler", | ||
| "formatter": "json" | ||
| }, | ||
| }, | ||
| "loggers": { | ||
| "django": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "gunicorn.access": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| "gunicorn.error": { | ||
| "level": "INFO", | ||
| "handlers": ["stdout"], | ||
| "propagate": False, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The new LOGGING configuration may conflict with the existing basicConfig call in src/importer/management/commands/_launch_events.py at line 11, which sets its own formatter. When Django's LOGGING configuration is loaded, it may not fully override this basicConfig call, potentially leading to inconsistent log formatting across different parts of the application. Consider removing the basicConfig call from _launch_events.py to ensure consistent logging throughout the application.
|
@derkweijers I've opened a new pull request, #3755, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: derkweijers <11644998+derkweijers@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.