Implement comprehensive logging system with configurable levels, file output, and WebSocket streaming#181
Merged
beniroquai merged 9 commits intomasterfrom Dec 19, 2025
Conversation
… support Co-authored-by: openuc2-gmbh <218447657+openuc2-gmbh@users.noreply.github.com>
Co-authored-by: openuc2-gmbh <218447657+openuc2-gmbh@users.noreply.github.com>
Co-authored-by: openuc2-gmbh <218447657+openuc2-gmbh@users.noreply.github.com>
Co-authored-by: openuc2-gmbh <218447657+openuc2-gmbh@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Improve logging system and add file output functionality
Implement comprehensive logging system with configurable levels, file output, and WebSocket streaming
Oct 10, 2025
Collaborator
Contributor
Author
|
Fixed the circular import issue in commit The fix:
The logging system now initializes correctly after all dependencies are loaded. Fixed the circular import issue in commit The fix:
The logging system now initializes correctly after all dependencies are loaded. |
Co-authored-by: beniroquai <4345528+beniroquai@users.noreply.github.com>
Contributor
|
@copilot I still get these errors: |
Contributor
Author
... Could you provide the complete error message? The error trace appears to be cut off. The full stack trace will help me identify and fix the issue. You can paste it directly in the comment or attach the full log file. |
Changed SignalEmittingHandler to emit log data as individual parameters to match expected signal signatures and added error handling for signal emission issues. Also updated log file size in ImSwitchServer to be returned as a string for consistency.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive logging system with configurable log levels, file output, and WebSocket streaming capabilities for ImSwitch. The implementation addresses circular import issues that were preventing application startup and adds REST API endpoints for log file management.
Key changes include:
- Configurable logging levels (DEBUG, INFO, WARNING, ERROR, CRITICAL) via ImSwitchConfig
- Automatic file logging with timestamp-based filenames in the ImSwitchConfig/logs directory
- WebSocket streaming of log messages to frontend clients in headless mode
- REST API endpoints for listing and downloading log files
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| imswitch/config.py | Adds logging configuration options (log_level, log_to_file, log_folder) |
| imswitch/imcommon/model/logging.py | Implements core logging system with file handlers, signal emission, and setup functions |
| imswitch/imcommon/model/init.py | Exports new logging functions (setup_logging, enable_signal_emission, get_log_folder) |
| imswitch/imcommon/framework/noqt.py | Adds sigLog signal and special handling for log message broadcasting via WebSocket |
| imswitch/main.py | Initializes logging system at startup and enables signal emission for headless mode |
| imswitch/imcontrol/controller/server/ImSwitchServer.py | Adds REST API endpoints for listing and downloading log files |
| imswitch/imcommon/_test/test_logging.py | Provides unit tests for logging setup and basic functionality |
| examples/logging_demo.py | Demonstrates logging system usage with example code |
| examples/README.md | Documents available examples |
| docs/LOGGING.md | Comprehensive documentation for the logging system |
| IMPLEMENTATION_SUMMARY.md | Technical implementation details and usage guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file_stat = os.stat(file_path) | ||
| log_files.append({ | ||
| 'filename': filename, | ||
| 'size': str(file_stat.st_size), |
There was a problem hiding this comment.
The 'size' field is returned as a string but represents a numeric value (file size in bytes). This should be returned as an integer to maintain type consistency and allow clients to perform numeric operations without conversion. Returning numeric values as strings is an anti-pattern in REST APIs.
Comment on lines
+633
to
+634
| 'modified': datetime.fromtimestamp(file_stat.st_mtime).isoformat(), | ||
| 'path': file_path |
There was a problem hiding this comment.
The 'path' field in the response exposes the full server-side file system path, which is a potential information disclosure issue. This field is not necessary for client functionality since they can use the 'filename' to download files. Consider removing this field or only including it when explicitly needed for administrative purposes.
Suggested change
| 'modified': datetime.fromtimestamp(file_stat.st_mtime).isoformat(), | |
| 'path': file_path | |
| 'modified': datetime.fromtimestamp(file_stat.st_mtime).isoformat() |
| # Security: prevent directory traversal | ||
| if '..' in filename or '/' in filename or '\\' in filename: | ||
| raise HTTPException(status_code=400, detail="Invalid filename") | ||
|
|
There was a problem hiding this comment.
The directory traversal check only blocks '..' and path separators, but doesn't validate that the file actually ends with '.log'. A malicious user could potentially request files like 'config.json' or other files in the log directory that aren't log files. Add validation to ensure the filename ends with '.log' to restrict access to only log files.
Suggested change
| # Security: only allow download of .log files | |
| if not filename.endswith('.log'): | |
| raise HTTPException(status_code=400, detail="Invalid log filename") | |
| _file_handler.close() | ||
|
|
||
| # Create new file handler | ||
| _file_handler = logging.FileHandler(log_filename, encoding='utf-8') |
There was a problem hiding this comment.
A new log file is created for every application start with a timestamp in the filename. Over time, this could lead to accumulation of many log files without automatic cleanup. Consider implementing log rotation (e.g., keeping only the last N files) or documenting that users need to manually manage old log files to prevent disk space issues.
| Enable emitting log messages via a signal. | ||
|
|
||
| Args: | ||
| signal: A signal object with an emit method that accepts a dict |
There was a problem hiding this comment.
The docstring states that the signal should accept a dict, but based on the SignalEmittingHandler implementation (lines 141-147), the signal actually receives 5 individual arguments (created, levelname, name, message, formatted_text), not a dictionary. Update the docstring to accurately reflect the signal signature or fix the implementation to match the documentation.
Suggested change
| signal: A signal object with an emit method that accepts a dict | |
| signal: A signal object with an ``emit`` method that accepts | |
| five positional arguments: | |
| ``created``, ``levelname``, ``name``, ``message``, | |
| and ``formatted_text``. |
| Unit tests for enhanced logging functionality. | ||
| """ | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
Suggested change
| import pytest |
| import os | ||
| import tempfile | ||
| import shutil | ||
| import logging |
There was a problem hiding this comment.
Import of 'logging' is not used.
Suggested change
| import logging |
| import tempfile | ||
| import shutil | ||
| import logging | ||
| from datetime import datetime |
There was a problem hiding this comment.
Import of 'datetime' is not used.
Suggested change
| from datetime import datetime |
| import shutil | ||
| import logging | ||
| from datetime import datetime | ||
| from unittest.mock import Mock, MagicMock |
There was a problem hiding this comment.
Import of 'Mock' is not used.
Import of 'MagicMock' is not used.
Suggested change
| from unittest.mock import Mock, MagicMock |
| from datetime import datetime | ||
| from unittest.mock import Mock, MagicMock | ||
|
|
||
| from imswitch.imcommon.model import setup_logging, enable_signal_emission, get_log_folder, initLogger |
There was a problem hiding this comment.
Import of 'enable_signal_emission' is not used.
Suggested change
| from imswitch.imcommon.model import setup_logging, enable_signal_emission, get_log_folder, initLogger | |
| from imswitch.imcommon.model import setup_logging, get_log_folder, initLogger |
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
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.
Implementation Plan for Logging System Enhancement
ImSwitchConfigclass (log level, log file settings)logging.pyto support configurable log levels from configLatest Changes (Commit: Fix circular import issue)
Fixed a critical circular import problem that was preventing ImSwitch from starting:
Problem
The logging setup was being called too early in the initialization process (before the framework was fully loaded), causing circular import dependencies:
__main__.pytried to importsetup_loggingfromimswitch.imcommon.model__init__.pywhich imports framework componentsSolution
setup_logging()call to AFTER the framework imports in__main__.pyfrom imswitch.imcommon.model import dirtoolsto direct importfrom imswitch.imcommon.model.dirtools import UserFileDirs~/ImSwitchConfigif dirtools can't be importedChanges Made
imswitch/__main__.py: Moved logging setup to after framework initializationimswitch/imcommon/model/logging.py: Fixed dirtools imports with try/except fallbackThis ensures logging is initialized at the right time in the startup sequence and prevents circular import issues.
Original prompt
Fixes #180
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.