Conversation
Fixed log rotation not working at 10MB threshold due to multiple logger instances creating separate RotatingFileHandler objects for the same log file. Changes: - Modified get_logger_with_params() to use fixed logger name based on log file path, ensuring single RotatingFileHandler per file - Removed unused 'name' parameter from get_logger_with_params() - Updated all callers (7 files) to remove name argument - Updated tests to reflect new logger naming pattern The issue was in our code, not in python-simple-logger. Multiple handlers writing to same file caused rotation to fail because each handler independently tracks file size. Now all code writing to same log file shares one logger instance with one handler, ensuring proper rotation at 10MB threshold.
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request refactors the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The review requires understanding the new cache-key-based logger naming strategy and verifying all call sites are correctly updated. While most changes are homogeneous (removing the name parameter), the core function signature change and new mask_sensitive_patterns logic add moderate complexity. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Extended mask_sensitive_patterns list to cover more credential and authentication scenarios found in the codebase. New patterns added: - Authentication: username, login, -u, --username, --creds - Tokens: api_key, github_token, GITHUB_TOKEN, pypi - Secrets: webhook_secret, webhook-secret - Private keys: private_key, private-key - GitHub App: github-app-id - Slack: slack-webhook-url, webhook-url - Command flags: --password (in addition to -p) This ensures sensitive data is masked in logs when using: - Docker/Podman login commands - Registry authentication - PyPI token publishing - GitHub API operations - Slack webhook notifications - Environment variables in commands All patterns are case-sensitive to avoid over-masking.
|
/verified |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webhook_server/utils/helpers.py (1)
75-82: Core fix logic is sound with one minor comment clarity issue.The fixed logger name strategy correctly ensures a single RotatingFileHandler per log file. The edge case handling (
log_file or 'console') is appropriate.However, the comment at Line 78 stating "The original 'name' parameter is preserved in log records via the logger name" may confuse readers since the
nameparameter was removed. Consider clarifying that log records will now contain the cache key (e.g.,webhook-server-<filepath>) rather than caller-provided names.Consider this comment revision for clarity:
- # The original 'name' parameter is preserved in log records via the logger name. + # Log records will use this cache key as the logger name for consistent identification.LOG_ROTATION_FIX.md (1)
1-153: Excellent documentation explaining the fix.The documentation clearly explains the problem, root cause, solution, and verification. This will be valuable for future maintainers.
The static analysis tool flagged some minor markdown formatting issues (missing blank lines around code blocks, ordered list numbering, missing language identifier on one code block at line 98). These are cosmetic but could be addressed for consistency with markdown standards.
If desired, you can address the markdown linting issues:
- Add blank lines before/after fenced code blocks
- Change line 53 list numbering from
3.to1.(for 1/1/1 style)- Add language identifier to code block at line 98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
LOG_ROTATION_FIX.md(1 hunks)webhook_server/app.py(2 hunks)webhook_server/tests/test_helpers.py(2 hunks)webhook_server/utils/app_utils.py(1 hunks)webhook_server/utils/github_repository_and_webhook_settings.py(1 hunks)webhook_server/utils/github_repository_settings.py(1 hunks)webhook_server/utils/helpers.py(6 hunks)webhook_server/utils/webhook.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
webhook_server/tests/test_helpers.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
webhook_server/utils/github_repository_settings.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
webhook_server/utils/webhook.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
webhook_server/app.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
webhook_server/utils/github_repository_and_webhook_settings.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
webhook_server/utils/app_utils.py (1)
webhook_server/utils/helpers.py (1)
get_logger_with_params(25-88)
🪛 markdownlint-cli2 (0.18.1)
LOG_ROTATION_FIX.md
53-53: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
67-67: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
98-98: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
146-146: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (11)
webhook_server/utils/helpers.py (3)
25-28: LGTM! Signature change correctly addresses the root cause.Removing the
nameparameter is the right fix. By deriving the logger name internally from the log file path, all code writing to the same file will share a single logger instance and RotatingFileHandler, ensuring correct rotation at the 10MB threshold.
29-60: Helpful reorganization of sensitive patterns.The categorization with comments improves readability and maintainability. While this refactor is technically independent of the log rotation fix, it's a valuable improvement.
106-106: LGTM! Internal call sites correctly updated.All internal calls to
get_logger_with_params()within this file have been properly updated to remove thenameargument, consistent with the new signature.Also applies to: 129-129, 190-190, 236-236
webhook_server/utils/github_repository_and_webhook_settings.py (1)
14-14: LGTM! Logger initialization correctly updated.The call to
get_logger_with_params()now uses the new signature without thenameparameter, ensuring this module shares the same logger instance (and RotatingFileHandler) with other code writing to the same log file.webhook_server/utils/webhook.py (1)
13-13: LGTM! Logger initialization correctly updated.The removal of
name="webhook"ensures this module shares the logger instance with other modules writing to the same log file, enabling proper log rotation.webhook_server/utils/github_repository_settings.py (1)
45-45: LGTM! Logger initialization correctly updated.Consistent with the log rotation fix, this change ensures a shared logger instance per log file.
webhook_server/utils/app_utils.py (1)
19-19: LGTM! Logger initialization correctly updated.The change aligns with the new
get_logger_with_params()signature, ensuring proper log rotation behavior.webhook_server/tests/test_helpers.py (2)
64-76: LGTM! Tests correctly updated for new signature.The test assertions properly validate the new logger naming pattern (
webhook-server-prefix) and confirm the function works correctly with and without therepository_nameparameter.
222-234: LGTM! Test correctly validates repository-specific logger behavior.The test appropriately verifies that
get_logger_with_params(repository_name="repo")creates loggers with the expected directory structure and naming.webhook_server/app.py (2)
46-46: LGTM! Global logger initialization correctly updated.The removal of
name="main"ensures this module participates in the shared logger instance strategy for proper log rotation.
213-213: LGTM! Repository-specific logger correctly configured.The per-repository logger appropriately uses the
repository_nameparameter to maintain repository context while benefiting from the shared logger instance behavior.
2 participants
Fixed log rotation not working at 10MB threshold due to multiple
logger instances creating separate RotatingFileHandler objects for
the same log file.
Changes:
log file path, ensuring single RotatingFileHandler per file
The issue was in our code, not in python-simple-logger. Multiple
handlers writing to same file caused rotation to fail because each
handler independently tracks file size.
Now all code writing to same log file shares one logger instance
with one handler, ensuring proper rotation at 10MB threshold.
Summary by CodeRabbit
Bug Fixes
Refactor