feat: implement log rotation with safe stream swapping and backup management #47
Conversation
…ion.py/__init__.py
WalkthroughThe changes update the application's logging framework. The console handler has been removed from the configuration and replaced by a file-based handler, Changes
Sequence Diagram(s)sequenceDiagram
participant L as Logger
participant R as RotatingCLPFileHandler
participant FS as File System
L ->> R: Log a message
R ->> R: Check _should_rotate? (file size)
alt Rotation needed
R ->> FS: Flush current stream and write EOF
R ->> FS: Close current log file
R ->> R: Rotate log (_rotate)
R ->> FS: Create new log file (with timestamp)
R ->> FS: Remove old backups if necessary
end
R ->> FS: Write log message
sequenceDiagram
participant App as Application
participant OS as Operating System
participant Log as Logger
App ->> OS: os.makedirs("logs", exist_ok=True)
alt Configuration loads successfully
App ->> Log: Initialize global logger with config
else Configuration load fails
App ->> Log: Initialize global logger with basicConfig
Log ->> App: Log error and warning messages
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application/logger.py(1 hunks)log_config.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
application/logger.py
19-19: Boolean-typed positional argument in function definition
(FBT001)
19-19: Boolean default positional argument in function definition
(FBT002)
47-47: datetime.datetime.now() called without a tz argument
(DTZ005)
90-90: Use a context manager for opening files
(SIM115)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
log_config.yaml (2)
10-16: Confirm overwriting behavior for the file handler.Currently, the handler is configured with
mode: w, causing the log file to be truncated each time the application restarts. If you want to preserve logs across runs, considermode: a. Otherwise, this configuration is valid for clean-slate logging per run.
18-27: Configuration of RotatingCLPFileHandler appears consistent with your objectives.The new handler parameters match the file rotation requirements (5MB max, 20 backups). This setup should help prevent large log files and manage retention effectively.
application/logger.py
Outdated
| max_bytes: int, | ||
| backup_count: int = 5, | ||
| mode: str = "ab", | ||
| enable_compression: bool = True, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider replacing the boolean argument with a more descriptive parameter.
Some style guides discourage using boolean-typed parameters because they make function calls less self-explanatory. For instance, you could replace enable_compression: bool = True with an enum or separate methods to increase clarity. However, this is a minor style preference and not strictly required.
🧰 Tools
🪛 Ruff (0.8.2)
19-19: Boolean-typed positional argument in function definition
(FBT001)
19-19: Boolean default positional argument in function definition
(FBT002)
application/logger.py
Outdated
| except Exception as e: | ||
| print(f"Error deleting old log file {old_file}: {e}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Catch more targeted exceptions when removing old log files.
Catching Exception is overly broad and may inadvertently hide important errors. Consider using OSError (or a more specific exception) to handle file-related errors.
- except Exception as e:
+ except OSError as e:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| print(f"Error deleting old log file {old_file}: {e}") | |
| except OSError as e: | |
| print(f"Error deleting old log file {old_file}: {e}") |
🧰 Tools
🪛 Ruff (0.8.2)
119-119: Do not catch blind exception: Exception
(BLE001)
application/logger.py
Outdated
| new_stream = open(new_log_file, "ab") | ||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use more specific exception handling and consider raising exceptions with context.
Catching a broad Exception can make debugging more difficult and may mask unexpected issues. Also, raising with from e preserves the original traceback context. Using a context manager (e.g. with open(...) as f) isn’t straightforward here because you need to keep the stream open after initialization, but you can still refine your exception handling to be more explicit:
try:
- new_stream = open(new_log_file, "ab")
-except Exception as e:
- raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}")
+ new_stream = open(new_log_file, "ab")
+except OSError as err:
+ raise RuntimeError(f"Failed to open new log file {new_log_file}") from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new_stream = open(new_log_file, "ab") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}") | |
| new_stream = open(new_log_file, "ab") | |
| except OSError as err: | |
| raise RuntimeError(f"Failed to open new log file {new_log_file}") from err |
🧰 Tools
🪛 Ruff (0.8.2)
90-90: Use a context manager for opening files
(SIM115)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
application/logger.py
Outdated
| """ | ||
| Generate a log filename with the timestamp and prefix. | ||
| """ | ||
| start_time = datetime.now() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider specifying a timezone for consistent log timestamps.
Calling datetime.now() without a timezone defaults to local time, which may result in ambiguity if your system’s timezone changes. If you prefer UTC-based logs or consistent timestamps across systems, specify datetime.now(timezone.utc).
- start_time = datetime.now()
+ from datetime import timezone
+ start_time = datetime.now(timezone.utc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| start_time = datetime.now() | |
| from datetime import timezone | |
| start_time = datetime.now(timezone.utc) |
🧰 Tools
🪛 Ruff (0.8.2)
47-47: datetime.datetime.now() called without a tz argument
(DTZ005)
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application/__init__.py(1 hunks)application/log_handlers/handlers.py(1 hunks)log_config.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
application/log_handlers/handlers.py
20-20: Boolean-typed positional argument in function definition
(FBT001)
20-20: Boolean default positional argument in function definition
(FBT002)
48-48: datetime.datetime.now() called without a tz argument
(DTZ005)
90-90: Use a context manager for opening files
(SIM115)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
application/__init__.py (3)
30-30: Directory creation approach looks good.
No issues with creating thelogsdirectory if it doesn't exist.
36-36: Initialization logic is clear and consistent.
No issues; the logger is properly retrieved aslogging.getLogger(__name__)after reading config.
40-40: Fallback initialization matches the main path.
No issues; re-initializing the logger under the fallback scenario is properly handled.log_config.yaml (1)
10-23: Configuration is coherent.
Your newCLP_filehandler definition aligns well with theRotatingCLPFileHandlerin the code. Therootlogger’s handlers are updated accordingly, and the parameter choices (max file size, formatting, compression) appear consistent with your objectives.application/log_handlers/handlers.py (2)
90-90: Context manager usage may be unnecessary for persistent streams.
Static analysis suggests a context manager here, but since the handler requires a long-lived stream, manually opening and closing might be acceptable. Confirm that the file is properly closed in all execution paths to minimize resource leakage.🧰 Tools
🪛 Ruff (0.8.2)
90-90: Use a context manager for opening files
(SIM115)
122-128: Rotation logic integration looks solid.
Your_writemethod properly checks_should_rotate()and triggers_rotate()before writing logs. This ensures the file remains within the specified max size limit.
application/log_handlers/handlers.py
Outdated
| def _remove_old_backups(self) -> None: | ||
| """ | ||
| Remove old log files exceeding the backup count. | ||
| """ | ||
| if not self.backup_count: | ||
| return | ||
|
|
||
| log_files = sorted( | ||
| self.log_dir.glob(f"{self.filename_prefix}_*.clp.zst"), | ||
| key=os.path.getmtime, | ||
| ) | ||
| if len(log_files) > self.backup_count: | ||
| for old_file in log_files[:-self.backup_count]: | ||
| try: | ||
| old_file.unlink() | ||
| except Exception as e: | ||
| print(f"Error deleting old log file {old_file}: {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict broad exception handling or re-raise appropriately.
Catching Exception in _remove_old_backups can hinder troubleshooting for issues like PermissionError. Consider narrowing the exception type or re-raising from the original error.
🧰 Tools
🪛 Ruff (0.8.2)
119-119: Do not catch blind exception: Exception
(BLE001)
application/log_handlers/handlers.py
Outdated
| def _generate_log_filename(self) -> Path: | ||
| """ | ||
| Generate a log filename with the timestamp and prefix. | ||
| """ | ||
| start_time = datetime.now() | ||
| timestamp = start_time.strftime(self.timestamp_format) | ||
| self.current_log_file = Path(f"{self.log_dir}/{self.filename_prefix}_{timestamp}.clp.zst") | ||
|
|
||
| return self.current_log_file | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use a timezone-aware or UTC time for consistency.
Using datetime.now() without specifying a timezone can lead to ambiguous log timestamps if servers run in different timezones. Consider using datetime.now(tz=...) or datetime.utcnow() where appropriate.
🧰 Tools
🪛 Ruff (0.8.2)
48-48: datetime.datetime.now() called without a tz argument
(DTZ005)
…nges to the config file
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
application/log_handlers/handlers.py (4)
14-24: 🧹 Nitpick (assertive)Consider refactoring boolean default arguments to keyword-only.
Static analysis flags the boolean default argument
enable_compression=True. Converting it to a keyword-only argument clarifies its optional nature and avoids confusion in call signatures.def __init__( self, filename_prefix: str, log_dir: Path, max_bytes: int, backup_count: int = 20, mode: str = "ab", - enable_compression: bool = True, + *, + enable_compression: bool = True, timestamp_format: str = "%Y-%m-%d_%H-%M-%S", loglevel_timeout: Optional[CLPLogLevelTimeout] = None, ) -> None:🧰 Tools
🪛 Ruff (0.8.2)
21-21: Boolean-typed positional argument in function definition
(FBT001)
21-21: Boolean default positional argument in function definition
(FBT002)
92-92: 🧹 Nitpick (assertive)Use a timezone-aware or UTC time for consistency.
Using
datetime.now()without specifying a timezone can lead to ambiguous log timestamps if servers run in different timezones. Consider usingdatetime.now(tz=...)ordatetime.utcnow()where appropriate.- timestamp = datetime.now().strftime(self.timestamp_format) + # Use UTC for consistent timestamps regardless of server timezone + timestamp = datetime.now(datetime.UTC).strftime(self.timestamp_format)🧰 Tools
🪛 Ruff (0.8.2)
92-92:
datetime.datetime.now()called without atzargument(DTZ005)
140-144: 🛠️ Refactor suggestionUse context manager and improve exception handling.
The current implementation has several issues that could be improved:
- Not using a context manager for file operations
- Catching a blind
Exceptionwhich can mask specific errors- Not preserving the exception traceback with
from e- # Initialize the new stream - try: - new_stream = open(new_log_file, "ab") - except Exception as e: - raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}") + # Initialize the new stream + try: + new_stream = open(new_log_file, "ab") + except (IOError, PermissionError, OSError) as e: + raise RuntimeError(f"Failed to open new log file {new_log_file}") from eAlternatively, use a context manager approach for future implementation:
try: with open(new_log_file, "ab") as new_stream: # Store a reference to the file object self.stream = new_stream # Initialize with the stream self.init(new_stream) # Prevent the context manager from closing the file new_stream.detach() except (IOError, PermissionError, OSError) as e: raise RuntimeError(f"Failed to open new log file {new_log_file}") from e🧰 Tools
🪛 Ruff (0.8.2)
141-141: Use a context manager for opening files
(SIM115)
142-142: Do not catch blind exception:
Exception(BLE001)
143-143: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
172-173: 🛠️ Refactor suggestionRestrict broad exception handling or re-raise appropriately.
Catching
Exceptionin_remove_old_backupscan hinder troubleshooting for issues likePermissionError. Consider narrowing the exception type or re-raising from the original error.try: old_file.unlink() - except Exception as e: + except (IOError, PermissionError, OSError) as e: print(f"Error deleting old log file {old_file}: {e}")🧰 Tools
🪛 Ruff (0.8.2)
172-172: Do not catch blind exception:
Exception(BLE001)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application/log_handlers/handlers.py(1 hunks)log_config.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
application/log_handlers/handlers.py
21-21: Boolean-typed positional argument in function definition
(FBT001)
21-21: Boolean default positional argument in function definition
(FBT002)
69-69: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
92-92: datetime.datetime.now() called without a tz argument
(DTZ005)
141-141: Use a context manager for opening files
(SIM115)
142-142: Do not catch blind exception: Exception
(BLE001)
143-143: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
172-172: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
log_config.yaml (1)
10-19: LGTM! Clean configuration for the new rotating log handler.The configuration for
CLP_fileis well-structured with appropriate parameters for log rotation:
- Max file size of 5MB provides a good balance for log management
- 20 backup files is a reasonable number for history retention
- Compression is enabled to optimize storage usage
- Timestamp format is clear and sortable
application/log_handlers/handlers.py (3)
105-113: LGTM! Clean and error-resistant rotation check.The rotation check logic is straightforward and properly handles the case where the file doesn't exist.
175-181: LGTM! Clean implementation of write method with rotation check.The write method override is concise and correctly checks if rotation is needed before writing to the log file.
9-182: Great implementation of the rotating log handler.Overall, this is a well-designed implementation of a rotating log handler with:
- Clear separation of concerns (checking, rotating, cleanup)
- Thread-safety during rotation with proper lock handling
- Robust UUID generation for machine identification
- Appropriate error handling in most cases
The minor issues flagged above should be addressed, but the core functionality is solid.
🧰 Tools
🪛 Ruff (0.8.2)
21-21: Boolean-typed positional argument in function definition
(FBT001)
21-21: Boolean default positional argument in function definition
(FBT002)
69-69: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
92-92:
datetime.datetime.now()called without atzargument(DTZ005)
141-141: Use a context manager for opening files
(SIM115)
142-142: Do not catch blind exception:
Exception(BLE001)
143-143: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
172-172: Do not catch blind exception:
Exception(BLE001)
|
|
||
| root: | ||
| level: DEBUG | ||
| handlers: [console] | ||
| handlers: [CLP_file] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the removal of console logging.
The change from console to file-based logging improves log persistence, but users accustomed to seeing logs in the terminal may be surprised. Consider documenting this change in user-facing documentation.
…dle the case that someone manually deletes log files
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
application/log_handlers/handlers.py (2)
92-92: 🧹 Nitpick (assertive)Consider using timezone-aware timestamps for logs.
Using
datetime.now()without specifying a timezone can lead to ambiguous timestamps if servers run in different timezones.- timestamp = datetime.now().strftime(self.timestamp_format) + timestamp = datetime.now(tz=datetime.timezone.utc).strftime(self.timestamp_format)🧰 Tools
🪛 Ruff (0.8.2)
92-92:
datetime.datetime.now()called without atzargument(DTZ005)
179-179: 🛠️ Refactor suggestionAvoid catching overly broad
Exception.Catching a blind
Exceptioncan mask issues likePermissionError. Use narrow exception types or re-raise with the original error context for better troubleshooting.-except Exception as e: - print(f"Error deleting old log file {old_file}: {e}") +except OSError as e: + logger.error(f"Error deleting old log file {old_file}", exc_info=e)🧰 Tools
🪛 Ruff (0.8.2)
179-179: Do not catch blind exception:
Exception(BLE001)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application/__init__.py(1 hunks)application/log_handlers/handlers.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
application/log_handlers/handlers.py
21-21: Boolean-typed positional argument in function definition
(FBT001)
21-21: Boolean default positional argument in function definition
(FBT002)
69-69: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
92-92: datetime.datetime.now() called without a tz argument
(DTZ005)
116-116: Use a context manager for opening files
(SIM115)
117-117: Do not catch blind exception: Exception
(BLE001)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
application/log_handlers/handlers.py (2)
14-24: Consider makingenable_compressiona keyword-only argument.This mirrors a prior suggestion about refactoring boolean default arguments to keyword-only for clarity and avoidance of parameter confusion.
🧰 Tools
🪛 Ruff (0.8.2)
21-21: Boolean-typed positional argument in function definition
(FBT001)
21-21: Boolean default positional argument in function definition
(FBT002)
182-191: LGTM for rotation logic and concurrency handling.The locking mechanisms (
acquire()andrelease()) ensure thread safety during writes and log rotations. The approach looks well-structured.application/__init__.py (1)
35-39: Validation: The fallback ensures a logger is always defined.This approach guards against logging failures by defining the logger in both the
tryandexceptblocks.
| @staticmethod | ||
| def _get_persistent_machine_uuid() -> str: | ||
| """ | ||
| Retrieves a persistent machine-specific UUID: | ||
| - First checks the `iCtrl_LOG_UUID` environment variable. | ||
| - If not found, checks a system-wide file (`/etc/iCtrl_machine_log_uuid` or `~/.iCtrl_machine_log_uuid`). | ||
| - If still not found, generates a new UUID, saves it, and returns only the first 8 characters. | ||
| """ | ||
| env_var_name = "iCtrl_LOG_UUID" | ||
|
|
||
| # 1️⃣ Check if UUID exists in an environment variable | ||
| env_uuid = os.getenv(env_var_name) | ||
| if env_uuid: | ||
| return env_uuid.strip()[:8] # Limit to first 8 characters | ||
|
|
||
| # 2️⃣ Define a persistent file path | ||
| if os.name == "nt": # Windows | ||
| uuid_file = Path(os.getenv("APPDATA", "C:/ProgramData")) / "iCtrl_machine_log_uuid.txt" | ||
| else: # Linux/macOS | ||
| uuid_file = Path( | ||
| "/etc/iCtrl_machine_log_uuid") if os.geteuid() == 0 else Path.home() / ".iCtrl_machine_log_uuid" | ||
|
|
||
| # 3️⃣ Check if the file exists and read the UUID | ||
| if uuid_file.exists(): | ||
| with open(uuid_file, "r") as f: | ||
| machine_uuid = f.read().strip()[:8] # Limit to first 8 characters | ||
| os.environ[env_var_name] = machine_uuid # Store in environment for future use | ||
| return machine_uuid | ||
|
|
||
| # 4️⃣ Generate a new UUID and store only the first 8 characters | ||
| new_uuid = str(uuid.uuid4())[:8] | ||
|
|
||
| try: | ||
| with open(uuid_file, "w") as f: | ||
| f.write(new_uuid) | ||
| os.environ[env_var_name] = new_uuid # Store in environment | ||
| except PermissionError: | ||
| print(f"Warning: Could not save machine UUID to {uuid_file}. Using in-memory only.") | ||
|
|
||
| return new_uuid | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use logging instead of printing errors to maintain consistency.
At line 82, a print statement warns about permission issues. For consistency and configurability in production environments, consider using logger.warning(...) instead of printing to stdout.
-except PermissionError:
- print(f"Warning: Could not save machine UUID to {uuid_file}. Using in-memory only.")
+except PermissionError as err:
+ logger.warning(f"Could not save machine UUID to {uuid_file}. Using in-memory only. {err}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def _get_persistent_machine_uuid() -> str: | |
| """ | |
| Retrieves a persistent machine-specific UUID: | |
| - First checks the `iCtrl_LOG_UUID` environment variable. | |
| - If not found, checks a system-wide file (`/etc/iCtrl_machine_log_uuid` or `~/.iCtrl_machine_log_uuid`). | |
| - If still not found, generates a new UUID, saves it, and returns only the first 8 characters. | |
| """ | |
| env_var_name = "iCtrl_LOG_UUID" | |
| # 1️⃣ Check if UUID exists in an environment variable | |
| env_uuid = os.getenv(env_var_name) | |
| if env_uuid: | |
| return env_uuid.strip()[:8] # Limit to first 8 characters | |
| # 2️⃣ Define a persistent file path | |
| if os.name == "nt": # Windows | |
| uuid_file = Path(os.getenv("APPDATA", "C:/ProgramData")) / "iCtrl_machine_log_uuid.txt" | |
| else: # Linux/macOS | |
| uuid_file = Path( | |
| "/etc/iCtrl_machine_log_uuid") if os.geteuid() == 0 else Path.home() / ".iCtrl_machine_log_uuid" | |
| # 3️⃣ Check if the file exists and read the UUID | |
| if uuid_file.exists(): | |
| with open(uuid_file, "r") as f: | |
| machine_uuid = f.read().strip()[:8] # Limit to first 8 characters | |
| os.environ[env_var_name] = machine_uuid # Store in environment for future use | |
| return machine_uuid | |
| # 4️⃣ Generate a new UUID and store only the first 8 characters | |
| new_uuid = str(uuid.uuid4())[:8] | |
| try: | |
| with open(uuid_file, "w") as f: | |
| f.write(new_uuid) | |
| os.environ[env_var_name] = new_uuid # Store in environment | |
| except PermissionError: | |
| print(f"Warning: Could not save machine UUID to {uuid_file}. Using in-memory only.") | |
| return new_uuid | |
| @staticmethod | |
| def _get_persistent_machine_uuid() -> str: | |
| """ | |
| Retrieves a persistent machine-specific UUID: | |
| - First checks the `iCtrl_LOG_UUID` environment variable. | |
| - If not found, checks a system-wide file (`/etc/iCtrl_machine_log_uuid` or `~/.iCtrl_machine_log_uuid`). | |
| - If still not found, generates a new UUID, saves it, and returns only the first 8 characters. | |
| """ | |
| env_var_name = "iCtrl_LOG_UUID" | |
| # 1️⃣ Check if UUID exists in an environment variable | |
| env_uuid = os.getenv(env_var_name) | |
| if env_uuid: | |
| return env_uuid.strip()[:8] # Limit to first 8 characters | |
| # 2️⃣ Define a persistent file path | |
| if os.name == "nt": # Windows | |
| uuid_file = Path(os.getenv("APPDATA", "C:/ProgramData")) / "iCtrl_machine_log_uuid.txt" | |
| else: # Linux/macOS | |
| uuid_file = Path( | |
| "/etc/iCtrl_machine_log_uuid") if os.geteuid() == 0 else Path.home() / ".iCtrl_machine_log_uuid" | |
| # 3️⃣ Check if the file exists and read the UUID | |
| if uuid_file.exists(): | |
| with open(uuid_file, "r") as f: | |
| machine_uuid = f.read().strip()[:8] # Limit to first 8 characters | |
| os.environ[env_var_name] = machine_uuid # Store in environment for future use | |
| return machine_uuid | |
| # 4️⃣ Generate a new UUID and store only the first 8 characters | |
| new_uuid = str(uuid.uuid4())[:8] | |
| try: | |
| with open(uuid_file, "w") as f: | |
| f.write(new_uuid) | |
| os.environ[env_var_name] = new_uuid # Store in environment | |
| except PermissionError as err: | |
| logger.warning(f"Could not save machine UUID to {uuid_file}. Using in-memory only. {err}") | |
| return new_uuid |
🧰 Tools
🪛 Ruff (0.8.2)
69-69: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
| new_stream = open(new_log_file, "ab") | ||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a context manager and attach root cause when re-raising.
- Simplify file operations by using a
withstatement. - Add
from ewhen re-raising to preserve the original traceback.
-try:
- new_stream = open(new_log_file, "ab")
-except Exception as e:
- raise RuntimeError(f"Failed to open new log file {new_log_file}: {e}")
+try:
+ with open(new_log_file, "ab") as new_stream:
+ self.acquire()
+ try:
+ self.stream = new_stream
+ self.init(new_stream)
+ self.current_log_file = new_log_file
+ finally:
+ self.release()
+except (PermissionError, OSError) as e:
+ raise RuntimeError(f"Failed to open new log file {new_log_file}") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
116-116: Use a context manager for opening files
(SIM115)
117-117: Do not catch blind exception: Exception
(BLE001)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
| @@ -26,15 +26,17 @@ | |||
| from werkzeug.exceptions import HTTPException | |||
| from werkzeug.serving import WSGIRequestHandler | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
| global logger | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing unnecessary global logger.
When a variable is declared at the module level, it's already accessible within the module. Using global logger here may be unnecessary and can be removed for cleaner code.
Description
RotatingCLPFileHandler introduces a size-based log rotation mechanism to prevent excessive file growth while ensuring compatibility with Python’s logging framework. It enforces a 5MB file size limit to reduce I/O overhead and maintain fast access to logs. Before rotation, the system flushes buffered data and writes an EOF marker to ensure log integrity.
Logs follow a timestamp-based naming convention for better traceability. When a file exceeds max_bytes, rotation is triggered, creating a new file while retaining only the latest backup_count logs. Old logs are systematically removed, and a locking mechanism ensures thread safety and uninterrupted operation.
Design for RotatingCLPFileHandler
The RotatingCLPFileHandler extends CLPFileHandler, adding log rotation based on file size. It integrates file rotation and compression mechanisms for efficient log storage and management.
1.1 Class Diagram：
Attributes:
filename_prefix: str→ Log file prefix.log_dir: Path→ Directory for log files.max_bytes: int→ Max file size before rotation.backup_count: int→ Number of retained backup logs.timestamp_format: str→ Format for timestamp in filenames.current_log_file: Path→ Path to the active log file.1.2 Flow Chart：
Core Methods
_generate_log_filename()Creates a new log file name using the current timestamp and the
filename_prefix._should_rotate()Checks if the current log file size exceeds the specified
max_bytes.Trueif rotation is needed, otherwiseFalse._rotate()Handles the full rotation process:
ostream.backup_count._remove_old_backups()Manages old log files:
backup_count._write(loglevel, msg)Overrides the
_writemethod ofCLPFileHandlerto include rotation logic._should_rotate()._get_persistent_machine_uuid()Retrieves a persistent machine-specific UUID from an environment variable or a system-wide file.
If missing, generates a new UUID (first 8 characters) and saves it for future use.
_open_new_log_file()Creates and switches to a new log file.
self.current_log_file.Summary by CodeRabbit
New Features
Refactor