fix: prevent infrastructure logs from flooding webhook JSONL files (#1035)#1037
Merged
fix: prevent infrastructure logs from flooding webhook JSONL files (#1035)#1037
Conversation
…1035) Infrastructure loggers (mcp_server, logs_server) were writing to webhooks_*.json via JsonLogHandler, drowning out webhook data. - Only attach JsonLogHandler to the main webhook logger, not infra loggers - Filter infrastructure noise entries in log viewer streaming - Add tests for logging separation and noise filtering
WalkthroughThis PR implements infrastructure noise filtering for webhook log queries by modifying the JsonLogHandler attachment logic and introducing a new infrastructure noise detector in LogViewerController. It prevents infrastructure-only logs from polluting webhook-specific queries while preserving infrastructure logs with webhook context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Reasoning: Three files modified with 150+ net new lines spanning test code, utility conditional logic, and controller filtering logic. The infrastructure noise detection pattern is relatively straightforward (check logger name against frozenset + verify hook_id is None), but requires understanding the logging separation strategy and verifying correct application across multiple streaming paths. Test additions are extensive and follow consistent patterns. Changes are focused on one feature domain but demand verification across heterogeneous code sections. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Collaborator
Author
|
/reprocess |
Collaborator
|
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/tests/test_logging_separation.py`:
- Around line 123-174: The test
test_json_handler_attached_when_log_file_name_default is non-deterministic
because it patches os.makedirs but not os.path.isdir, so on CI where
/tmp/data/logs already exists makedirs may not be invoked; update the test to
also patch os.path.isdir (e.g., patch("os.path.isdir", return_value=False))
inside the same with(...) block used for get_logger_with_params so the code path
that calls os.makedirs is forced consistently when exercising
get_logger_with_params and JsonLogHandler attachment.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c22e3d9-d3f6-4b71-b974-d3c6326039b4
📒 Files selected for processing (3)
webhook_server/tests/test_logging_separation.pywebhook_server/utils/helpers.pywebhook_server/web/log_viewer.py
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
JsonLogHandlerto the main webhook logger, not infrastructure loggers (mcp_server, logs_server)Problem
Infrastructure loggers (MCP server, log viewer) were writing to
webhooks_*.jsonviaJsonLogHandler, flooding the file with context-less entries. When querying via MCP, these noise entries were the most recent (generated by the MCP call itself), drowning out actual webhook data and returning 0 useful results.Changes
webhook_server/utils/helpers.pyJsonLogHandlerwhen no explicitlog_file_nameis providedwebhook_server/web/log_viewer.py_is_infrastructure_noise()filter for streaming, skip entries from infra loggers with no webhook contextwebhook_server/tests/test_logging_separation.pyTest plan
mcpl call github-webhook-logs-myakove get_log_entries '{"limit": 5}'returns entriesCloses #1035
Summary by CodeRabbit
Release Notes
New Features
Tests