feat: separate logs into mcp_server.log and logs_server.log#924
feat: separate logs into mcp_server.log and logs_server.log#924
Conversation
- Configure MCP logs to write to (configurable via ). - Configure Log Viewer logs to write to (configurable via ). - Keep main application logs in . - Update configuration schema and documentation. - Add helper for consistent log path resolution. - Add tests for logging separation.
WalkthroughAdds two new logging configuration options ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 |
|
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. |
![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 (1)
webhook_server/config/schema.yaml (1)
13-18: Consider adding explicit default values for consistency.The descriptions mention default values (
default: mcp_server.loganddefault: logs_server.log), but the schema doesn't include adefaultfield like other properties do (e.g.,mask-sensitive-dataon line 22). While the defaults are handled in code, adding them to the schema would improve consistency and enable validation tools to understand the complete configuration contract.Apply this diff to add explicit defaults:
mcp-log-file: type: string description: File path for the MCP log file (default: mcp_server.log) + default: mcp_server.log logs-server-log-file: type: string description: File path for the Logs Server log file (default: logs_server.log) + default: logs_server.log
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(2 hunks)examples/config.yaml(1 hunks)webhook_server/app.py(3 hunks)webhook_server/config/schema.yaml(1 hunks)webhook_server/tests/test_logging_separation.py(1 hunks)webhook_server/utils/helpers.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
webhook_server/utils/helpers.py (1)
webhook_server/libs/config.py (2)
get_value(99-120)Config(11-142)
webhook_server/tests/test_logging_separation.py (2)
webhook_server/app.py (2)
get_log_viewer_controller(442-460)lifespan(96-248)webhook_server/utils/helpers.py (1)
get_log_file_path(95-114)
webhook_server/app.py (3)
webhook_server/utils/helpers.py (1)
get_logger_with_params(31-92)webhook_server/libs/config.py (2)
Config(11-142)get_value(99-120)webhook_server/web/log_viewer.py (1)
LogViewerController(20-1005)
🪛 Ruff (0.14.5)
webhook_server/tests/test_logging_separation.py
13-13: Probable insecure usage of temporary file or directory: "/tmp/data"
(S108)
20-20: Probable insecure usage of temporary file or directory: "/tmp/data"
(S108)
24-24: Probable insecure usage of temporary file or directory: "/tmp/data/logs/server.log"
(S108)
25-25: Probable insecure usage of temporary file or directory: "/tmp/data/logs"
(S108)
🪛 YAMLlint (1.37.1)
webhook_server/config/schema.yaml
[error] 15-15: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (8)
examples/config.yaml (1)
5-6: LGTM! Clear documentation of new log file configuration options.The example configuration properly demonstrates the new MCP and logs server log file settings with helpful comments indicating they support dynamic reload without server restart.
README.md (1)
229-230: LGTM! Documentation properly reflects new logging configuration.The README updates consistently document the new log file options in both the Advanced Configuration and Monitoring sections, making them easy to discover for users.
Also applies to: 369-370
webhook_server/app.py (2)
127-144: LGTM! Proper implementation of MCP logging separation.The MCP logging configuration correctly:
- Reads the config with appropriate default
- Creates a dedicated logger with file-specific configuration (rotation, masking)
- Copies handlers to prevent duplication issues
- Disables propagation to keep MCP logs out of webhook_server.log
The handler check (
if mcp_file_logger.handlers:) is appropriate sinceget_logger_with_paramsalways returns a logger with at least one handler (file or console).
453-459: LGTM! Log viewer controller initialization properly uses dedicated logger.The singleton initialization correctly:
- Fetches the logs-server-log-file config with proper default
- Creates a dedicated logger for the log viewer
- Passes it to LogViewerController for isolated logging
This ensures logs server operations are separated from the main webhook processing logs.
webhook_server/utils/helpers.py (2)
31-93: LGTM! Clean implementation of log file configuration with proper path resolution.The changes correctly:
- Add
log_file_nameparameter to support per-component log files- Fall back to config's
log-filewhen not explicitly provided- Resolve paths using the new helper function
- Update cache key to use resolved path basename (ensuring single handler per file)
The implementation maintains backward compatibility while enabling the new logging separation feature.
95-115: LGTM! Robust log file path resolution with proper directory creation.The helper function correctly:
- Handles absolute paths by returning them unchanged
- Resolves relative paths against
data_dir/logs- Creates the logs directory with
exist_ok=True(safe for concurrent calls)- Returns
NoneforNoneinput (explicit contracts)The implementation is simple, correct, and handles all edge cases appropriately.
webhook_server/tests/test_logging_separation.py (1)
11-107: LGTM! Comprehensive test coverage for logging separation.The tests properly verify:
- Path resolution (lines 11-31): Absolute, relative, and None cases
- MCP logging (lines 34-80): Configuration, handler attachment, and propagation disabling
- Log viewer initialization (lines 82-107): Dedicated logger creation and controller setup
The static analysis warnings about
/tmp/datausage (S108) are expected for test code and can be safely ignored.webhook_server/config/schema.yaml (1)
13-18: This is not a false positive—quote the description values to fix the YAML syntax error.The YAML parser confirms a real syntax error at line 15, column 57. The issue is that description values contain unquoted colons (e.g.,
default: mcp_server.log), which YAML interprets as mapping syntax. Quote both description values:mcp-log-file: type: string description: "File path for the MCP log file (default: mcp_server.log)" logs-server-log-file: type: string description: "File path for the Logs Server log file (default: logs_server.log)"Likely an incorrect or invalid review comment.
myakove-bot
added
commented-coderabbitai[bot]
and removed
commented-coderabbitai[bot]
labels
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.