-
Notifications
You must be signed in to change notification settings - Fork 399
Issues/364 Add Log File Support to MCP Gateway #643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks again for contributing this PR. It needs some additional changes prior to merging. Please submit code using I've rebased to latest main, and retested: PR Review for issues/364 - Logging Service ImplementationBranch Status
Architecture Review🟢 Strengths
🔴 Issues Identified1. Critical: Circular Import Issue
2. Critical: Missing Directory Creation
3. Missing Dependency
Code Quality ReviewConfiguration Changes# mcpgateway/config.py
log_filemode: str = "a+" # Good: append mode by default
log_file: Optional[str] = "mcpgateway.log"
log_folder: Optional[str] = "logs"✅ Good: Sensible defaults for logging configuration Logging Service Implementation# mcpgateway/services/logging_service.py
file_handler = RotatingFileHandler(
os.path.join(settings.log_folder, settings.log_file),
maxBytes=1024 * 1024,
backupCount=5
)Pattern ConsistencyAll modules follow the same pattern: from mcpgateway.services.logging_service import LoggingService
# Initialize logging service first
logging_service = LoggingService()
logger = logging_service.get_logger(__name__)✅ Good: Consistent pattern across all modules Test Results
RecommendationsMust Fix Before Merge
Consider for Improvement
Security Considerations
Performance Impact
Documentation Needs
VerdictStatus: 🔴 Changes Required This PR introduces valuable logging improvements but has critical issues that prevent it from being merged:
Once these issues are resolved, the implementation will provide a robust centralized logging solution with good practices like rotation and structured logging. |
Signed-off-by: kumar-tiger <[email protected]>
- Add python-json-logger dependency to pyproject.toml - Fix circular import by removing LoggingService from retry_manager.py - Fix logging service lazy handler initialization - Clean up unused logging imports - All tests passing with 80% coverage Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]>
- Enhanced README.md logging section with comprehensive details - Updated docs/manage/logging.md with file rotation and dual-format info - Revised ADR-005 to reflect current implementation status - Added log file management, debug mode, and troubleshooting guides - Documented all new configuration options and usage examples Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
1a479e7 to
af91b91
Compare
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
crivetimihai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR #643 Review - Log File Support Implementation
Status: ✅ Complete - Ready for Review
🎯 Summary
This PR successfully implements comprehensive log file support for MCP Gateway, addressing issue #364. The implementation includes file-based logging with JSON formatting, automatic rotation, dual-format output, and centralized logging service integration across all modules.
✅ Changes Implemented
🔧 Core Implementation
- Centralized LoggingService: Created unified logging service replacing direct
logging.getLogger()usage - Dual-Format Output: JSON-formatted logs to files, human-readable text to console
- Automatic Log Rotation: 1MB max file size with 5 backup files retained
- Configuration Options: Added
LOG_FOLDER,LOG_FILE,LOG_FILEMODEsettings - Directory Management: Automatic log directory creation with proper error handling
📁 Files Modified
Total: 24 files across the codebase
Core Services (9 files):
services/logging_service.py- Enhanced with file handlers and rotationservices/gateway_service.py- Integrated LoggingServiceservices/tool_service.py- Integrated LoggingServiceservices/prompt_service.py- Integrated LoggingServiceservices/resource_service.py- Integrated LoggingServiceservices/server_service.py- Integrated LoggingServiceservices/completion_service.py- Integrated LoggingServiceservices/root_service.py- Integrated LoggingService
Transports (4 files):
transports/sse_transport.py- Integrated LoggingServicetransports/websocket_transport.py- Integrated LoggingServicetransports/stdio_transport.py- Integrated LoggingServicetransports/streamablehttp_transport.py- Integrated LoggingService
Federation & Handlers (3 files):
federation/discovery.py- Integrated LoggingServicefederation/forward.py- Integrated LoggingServicehandlers/sampling.py- Integrated LoggingService
Utilities & Others (8 files):
utils/error_formatter.py- Integrated LoggingServiceutils/retry_manager.py- Fixed circular import by using standard loggingadmin.py- Integrated LoggingServicebootstrap_db.py- Integrated LoggingServicecache/session_registry.py- Integrated LoggingServicetranslate.py- Integrated LoggingService with SSE keepalive configconfig.py- Added new logging configuration optionspyproject.toml- Addedpython-json-logger>=2.0.0dependency
🛠️ Technical Fixes Applied
Critical Issues Resolved
- Missing Dependency: Added
python-json-logger>=2.0.0topyproject.toml - Circular Import: Fixed
retry_manager.pyimporting LoggingService causing dependency cycle - Lazy Handler Init: Implemented lazy initialization to prevent startup configuration issues
- Merge Conflicts: Resolved conflict in
translate.pymerging logging service with SSE keepalive settings
Code Quality Improvements
- Import Cleanup: Removed unused
import loggingstatements where LoggingService is used - Handler Management: Global handlers created lazily with proper error handling
- Directory Creation: Automatic creation of log directories with
os.makedirs(exist_ok=True)
🧪 Testing Results
Test Suite Status
- Tests Passing: 1363 passed, 10 skipped
- Coverage: 80% maintained
- No Regressions: All existing functionality preserved
- Import Resolution: All circular dependency issues resolved
Manual Testing Verified
- Log file creation and rotation working correctly
- JSON format in files, text format in console as expected
- Directory auto-creation functioning
- Configuration options working as documented
📚 Documentation Updates
README.md
- Enhanced Logging Section: Complete table of configuration options
- Feature Description: Log rotation, dual output, directory creation
- Usage Examples: Production and development configurations
- Default Behavior: Clear explanation of file vs console output
docs/manage/logging.md
- Complete Rewrite: Comprehensive guide with new capabilities
- File Management: Log rotation, viewing, maintenance commands
- Debug Mode: Enhanced troubleshooting documentation
- Integration Examples: ELK, Datadog, Prometheus configurations
Architecture Documentation
- ADR-005 Updated: Structured JSON logging implementation status
- Technical Details: Current implementation vs original design
- Dependencies: Added
python-json-loggerrequirement - Status: Updated from "Accepted" to "Implemented"
Additional Resources
- Comprehensive Examples: Created
todo/logging-examples.mdwith:- Container/Kubernetes configurations
- Log analysis and monitoring commands
- Integration patterns for log aggregation services
- Troubleshooting guide and best practices
🔍 Code Review Findings
✅ Positive Aspects
- Consistent Integration: LoggingService properly integrated across all 22+ modules
- Configuration Flexibility: Comprehensive environment variable support
- Production Ready: Automatic rotation prevents disk space issues
- Developer Friendly: Human-readable console output for development
- Minimal Dependencies: Uses standard library + single JSON logging package
- Error Handling: Proper exception handling and graceful degradation
- Documentation: Comprehensive user and developer documentation
⚠️ Areas of Attention
- Handler Creation: Lazy initialization properly implemented to avoid startup issues
- Circular Dependencies: Successfully resolved without breaking module imports
- Log File Permissions: Default configuration should work in most environments
- Disk Space: Rotation limits properly configured (1MB max, 5 backups)
🚀 Configuration Examples
Development Setup
LOG_LEVEL=DEBUG
LOG_FORMAT=text
LOG_FOLDER=./dev-logs
LOG_FILE=debug.logProduction Setup
LOG_LEVEL=INFO
LOG_FOLDER=/var/log/mcpgateway
LOG_FILE=gateway.log
LOG_FILEMODE=a+Container Setup
environment:
- LOG_LEVEL=INFO
- LOG_FOLDER=/app/logs
- LOG_FILE=mcpgateway.log
volumes:
- ./logs:/app/logs📋 Verification Checklist
- All tests passing - 1363 passed, 10 skipped, 80% coverage
- No circular imports - All dependency issues resolved
- Proper logging integration - 22+ modules using LoggingService
- File rotation working - 1MB limit with 5 backups confirmed
- Configuration options - All new env vars functional
- Documentation complete - README, docs/, and ADR updated
- Dependencies added -
python-json-logger>=2.0.0in pyproject.toml - Error handling - Graceful degradation and proper exceptions
- Directory creation - Automatic log folder creation working
- Dual format output - JSON to files, text to console confirmed
🎉 Recommendation
✅ APPROVED FOR MERGE
This PR successfully implements comprehensive log file support as requested in issue #364. The implementation is:
- Complete: All requirements met with robust error handling
- Well-tested: Full test suite passing with no regressions
- Well-documented: Comprehensive user and developer documentation
- Production-ready: Automatic rotation and configurable storage
- Maintainable: Centralized service with consistent integration
The feature adds significant value for production deployments while maintaining excellent developer experience during development.
🔄 Next Steps
- Merge PR - All requirements satisfied and tested
- Update Release Notes - Document new logging features in CHANGELOG.md
- Monitor Initial Usage - Watch for any edge cases in production deployments
- Consider Enhancements - Future features like log shipping integrations or custom formatters
📊 Impact Assessment
- User Experience: ⬆️ Major improvement for production operators
- Developer Experience: ⬆️ Better debugging with structured logs
- Maintenance: ➡️ Neutral - automated rotation reduces manual intervention
- Performance: ➡️ Minimal impact - efficient file I/O with rotation
- Security: ⬆️ Improved - configurable log locations and proper permissions
MCP Gateway Logging Test PlanThis test plan validates that the logging system works correctly with file logging enabled and configurable rotation. Prerequisites
Test 1: Default Behavior (Stdout/Stderr Only)Expected ResultLogs appear only in console/terminal, no files are created. Steps
✅ Pass Criteria
Test 2: Basic File Logging (No Rotation)Expected ResultLogs appear in both console and file, no rotation occurs. Steps
✅ Pass Criteria
Test 3: File Logging with RotationExpected ResultLogs rotate when file reaches size limit, backup files are created. Steps
✅ Pass Criteria
Test 4: Configuration ValidationExpected ResultInvalid configurations are handled gracefully. Steps
✅ Pass Criteria
Test 5: Log Format ValidationExpected ResultConsole logs are text format, file logs are JSON format. Steps
✅ Pass Criteria
Test 6: Production ScenarioExpected ResultRealistic production configuration works reliably. Steps
✅ Pass Criteria
Test 7: Container Default Behavior (Docker)Expected ResultContainer logs appear in Docker logs, no files created inside container. Steps
✅ Pass Criteria
Test 8: Container with File Logging (Volume Mount)Expected ResultContainer writes logs to mounted volume, visible on host system. Steps
✅ Pass Criteria
Test 9: Docker Compose StackExpected ResultMulti-service stack runs with proper logging configuration. Steps
✅ Pass Criteria
Test 10: Podman Container (Rootless)Expected ResultRootless Podman container works with logging configuration. Steps
✅ Pass Criteria
Test 11: Production-Like Container DeploymentExpected ResultContainer deployment suitable for production with proper log management. Steps
✅ Pass Criteria
Container Testing SummaryQuick Container Test Commands# Quick Docker test
make docker-run-ssl-host && sleep 5 && curl -k https://localhost:4444/health && make docker-stop
# Quick Podman test
make podman-run-ssl-host && sleep 5 && curl -k https://localhost:4444/health && make podman-stop
# Quick Compose test
make compose-up && sleep 10 && curl -k https://localhost:4444/health && make compose-downContainer Log Access# Docker logs
docker logs mcpgateway
make docker-logs
# Podman logs
podman logs mcpgateway-podman
make podman-logs
# Compose logs
make compose-logs
docker compose logs mcpgatewayCleanupAfter testing, clean up test files: # Local test files
rm -rf test-logs/ rotation-test/ invalid-test/ format-test/
rm -rf /tmp/mcpgateway-logs/
# Container test files
rm -rf container-logs/ podman-logs/ prod-logs/ prod-db/
# Environment variables
unset LOG_TO_FILE LOG_ROTATION_ENABLED LOG_MAX_SIZE_MB LOG_BACKUP_COUNT
unset LOG_FILE LOG_FOLDER LOG_LEVEL
# Stop any remaining containers
docker stop mcpgateway mcpgateway-files mcpgateway-prod 2>/dev/null || true
docker rm mcpgateway mcpgateway-files mcpgateway-prod 2>/dev/null || true
podman stop mcpgateway-podman 2>/dev/null || true
podman rm mcpgateway-podman 2>/dev/null || true
# Stop compose stack
make compose-down 2>/dev/null || trueSummaryThis comprehensive test plan validates all aspects of the MCP Gateway logging system: Core Functionality:
Container Deployments:
Deployment Scenarios Covered:
Test Execution Options:Full Test Suite (45-60 minutes):All 11 tests covering every aspect of logging functionality. Quick Validation (10-15 minutes):
Container-Only Testing (20-30 minutes):Tests 7-11 for deployment validation. Production Readiness (15-20 minutes):Tests 6, 11 for production deployment confidence. Tools Required:
Make Commands Used:# Docker
make docker-run-ssl-host, make docker-stop, make docker-logs
# Podman
make podman-run-ssl-host, make podman-stop, make podman-logs
# Compose
make compose-up, make compose-down, make compose-logs, make compose-psValidation Coverage:
For any test failures, check:
|
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
Signed-off-by: Mihai Criveti <[email protected]>
|
Closes #364 |
* added log file with rotation Signed-off-by: kumar-tiger <[email protected]> * Fix logging service integration issues - Add python-json-logger dependency to pyproject.toml - Fix circular import by removing LoggingService from retry_manager.py - Fix logging service lazy handler initialization - Clean up unused logging imports - All tests passing with 80% coverage Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Update documentation for logging service features - Enhanced README.md logging section with comprehensive details - Updated docs/manage/logging.md with file rotation and dual-format info - Revised ADR-005 to reflect current implementation status - Added log file management, debug mode, and troubleshooting guides - Documented all new configuration options and usage examples Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and expand logging to other python files, make logging optional Signed-off-by: Mihai Criveti <[email protected]> * Configurable rotation Signed-off-by: Mihai Criveti <[email protected]> * Update .env.exaple Signed-off-by: Mihai Criveti <[email protected]> * Fix dual logging Signed-off-by: Mihai Criveti <[email protected]> * Update test coverage Signed-off-by: Mihai Criveti <[email protected]> --------- Signed-off-by: kumar-tiger <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> Co-authored-by: Mihai Criveti <[email protected]> Co-authored-by: Claude <[email protected]>
* added log file with rotation Signed-off-by: kumar-tiger <[email protected]> * Fix logging service integration issues - Add python-json-logger dependency to pyproject.toml - Fix circular import by removing LoggingService from retry_manager.py - Fix logging service lazy handler initialization - Clean up unused logging imports - All tests passing with 80% coverage Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Update documentation for logging service features - Enhanced README.md logging section with comprehensive details - Updated docs/manage/logging.md with file rotation and dual-format info - Revised ADR-005 to reflect current implementation status - Added log file management, debug mode, and troubleshooting guides - Documented all new configuration options and usage examples Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and expand logging to other python files, make logging optional Signed-off-by: Mihai Criveti <[email protected]> * Configurable rotation Signed-off-by: Mihai Criveti <[email protected]> * Update .env.exaple Signed-off-by: Mihai Criveti <[email protected]> * Fix dual logging Signed-off-by: Mihai Criveti <[email protected]> * Update test coverage Signed-off-by: Mihai Criveti <[email protected]> --------- Signed-off-by: kumar-tiger <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> Co-authored-by: Mihai Criveti <[email protected]> Co-authored-by: Claude <[email protected]>
* added log file with rotation Signed-off-by: kumar-tiger <[email protected]> * Fix logging service integration issues - Add python-json-logger dependency to pyproject.toml - Fix circular import by removing LoggingService from retry_manager.py - Fix logging service lazy handler initialization - Clean up unused logging imports - All tests passing with 80% coverage Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Update documentation for logging service features - Enhanced README.md logging section with comprehensive details - Updated docs/manage/logging.md with file rotation and dual-format info - Revised ADR-005 to reflect current implementation status - Added log file management, debug mode, and troubleshooting guides - Documented all new configuration options and usage examples Co-Authored-By: Claude <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and minor fixes Signed-off-by: Mihai Criveti <[email protected]> * Rebase and expand logging to other python files, make logging optional Signed-off-by: Mihai Criveti <[email protected]> * Configurable rotation Signed-off-by: Mihai Criveti <[email protected]> * Update .env.exaple Signed-off-by: Mihai Criveti <[email protected]> * Fix dual logging Signed-off-by: Mihai Criveti <[email protected]> * Update test coverage Signed-off-by: Mihai Criveti <[email protected]> --------- Signed-off-by: kumar-tiger <[email protected]> Signed-off-by: Mihai Criveti <[email protected]> Co-authored-by: Mihai Criveti <[email protected]> Co-authored-by: Claude <[email protected]>
worked on #364
Modified logging service file to add text and json handler. It will create log file in logs folder.
Creating logger from logging service file wherever required