Enhance logging configuration to separate console and error outputs#3704
Enhance logging configuration to separate console and error outputs#3704jesbinjoseph wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLOGGING settings in base, deployment, and test modules now split stdout and stderr output by severity. Sub-ERROR records go to console, while ERROR-and-above records use console_error, with selected loggers and root handlers updated accordingly. ChangesSeverity-based logging handler split
Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None identified. Suggested labels: logging, config Suggested reviewers: None identified. Since three settings files got the same treatment, the copy-paste symmetry is at least doing one job well.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Greptile SummaryThis PR separates console log output by severity: DEBUG/INFO/WARNING records go to
Confidence Score: 5/5Safe to merge; the logging changes are isolated to configuration and carry no runtime logic changes. The change is a straightforward logging configuration refactor with no effect on application logic. The config/settings/base.py — the Important Files Changed
Reviews (3): Last reviewed commit: "pre-commit: fix lint" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "django.request": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, | ||
| "audit_log": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, | ||
| "celery": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, |
There was a problem hiding this comment.
The django.request, audit_log, and celery loggers all send records directly to console_error, but none have "propagate": False. Because the root logger also includes console_error, every ERROR record from these loggers will be written to stderr twice — once by the logger's own handler and once by the root logger's handler. The deployment.py loggers (django.db.backends, sentry_sdk) correctly use "propagate": False to prevent exactly this. Adding the same key to the three loggers in test.py would fix the duplicate output.
![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 (1)
config/settings/base.py (1)
341-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame filter/handler boilerplate duplicated across three settings files.
The
below_errorfilter andconsole/console_errorhandlers are copy-pasted verbatim inbase.py,deployment.py, andtest.py. Sincebase.pypresumably is the base for the others, consider defining this once and letting the other files inherit/extend it, rather than tripling the maintenance surface for any future tweak (e.g., changing the split threshold).
[recommended_refactor_note]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/settings/base.py` around lines 341 - 375, The logging filter/handler setup for `below_error`, `console`, and `console_error` is duplicated across `base.py`, `deployment.py`, and `test.py`; refactor it so the shared configuration lives in `base.py` and the other settings modules inherit or extend it instead of copying the same `filters`, `handlers`, and `root` entries verbatim. Keep the existing behavior for `time_logging_middleware` and `time_logging`, but centralize the common console split logic to reduce maintenance across the settings files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/settings/test.py`:
- Around line 91-105: Add propagate: False to the django.request, audit_log, and
celery logger entries in the logging config so their ERROR messages do not
bubble up to the root logger and get duplicated on stderr. Update the loggers
block in config/settings/test.py by setting propagate to False for each of these
named loggers while keeping their existing console_error handler and ERROR
level.
---
Nitpick comments:
In `@config/settings/base.py`:
- Around line 341-375: The logging filter/handler setup for `below_error`,
`console`, and `console_error` is duplicated across `base.py`, `deployment.py`,
and `test.py`; refactor it so the shared configuration lives in `base.py` and
the other settings modules inherit or extend it instead of copying the same
`filters`, `handlers`, and `root` entries verbatim. Keep the existing behavior
for `time_logging_middleware` and `time_logging`, but centralize the common
console split logic to reduce maintenance across the settings files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc246677-c2d1-4ba3-b680-b3294d7974c9
📒 Files selected for processing (3)
config/settings/base.pyconfig/settings/deployment.pyconfig/settings/test.py
| "loggers": { | ||
| "django.request": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, | ||
| "audit_log": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, | ||
| "celery": { | ||
| "handlers": ["console"], | ||
| "handlers": ["console_error"], | ||
| "level": "ERROR", | ||
| }, | ||
| }, | ||
| "root": {"level": "INFO", "handlers": ["console"]}, | ||
| "root": {"level": "INFO", "handlers": ["console", "console_error"]}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files config/settings && printf '\n--- test.py ---\n' && sed -n '1,220p' config/settings/test.py && printf '\n--- deployment.py ---\n' && sed -n '1,260p' config/settings/deployment.pyRepository: ohcnetwork/care
Length of output: 9939
Add propagate: False to these loggers. django.request, audit_log, and celery already write to console_error; letting them bubble to root just emits the same ERROR twice on stderr. config/settings/test.py:91-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/settings/test.py` around lines 91 - 105, Add propagate: False to the
django.request, audit_log, and celery logger entries in the logging config so
their ERROR messages do not bubble up to the root logger and get duplicated on
stderr. Update the loggers block in config/settings/test.py by setting propagate
to False for each of these named loggers while keeping their existing
console_error handler and ERROR level.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3704 +/- ##
========================================
Coverage 79.66% 79.66%
========================================
Files 479 479
Lines 23010 23011 +1
Branches 2379 2379
========================================
+ Hits 18330 18331 +1
Misses 4080 4080
Partials 600 600 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Can you make this as a plug, that was its fully safe for existing deployments ? |
2 participants
Proposed Changes
Associated Issue
Architecture changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit