Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a log file rotation and compression system to prevent log files from growing indefinitely. The implementation adds automated rotation when log files exceed 10MB, compresses rotated logs into zip archives, and maintains a configurable number of backup files.
Key changes:
- Added
LogRotationutility class with methods for size checking, file rotation, compression, and backup cleanup - Modified
initLoggerto be asynchronous and perform rotation checks before initializing the file logger - Added
archivepackage dependency for zip compression functionality
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| pubspec.yaml | Added archive package dependency (version 4.0.7) for log compression support |
| pubspec.lock | Updated lock file with archive and transitive dependencies |
| lib/core/services/logger_service.dart | Implemented log rotation system with compression and automated backup management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final timestamp = DateFormat('yyyyMMdd_HHmmss').format(DateTime.now()); | ||
| final directory = currentFile.parent; | ||
| final fileName = currentFile.path.split(Platform.pathSeparator).last; | ||
| final nameWithoutExt = fileName.replaceAll('.log', ''); |
There was a problem hiding this comment.
Using replaceAll('.log', '') could incorrectly handle filenames that contain the substring '.log' multiple times or in unexpected places (e.g., 'my.log.log' or 'application.log.data'). Consider using a more precise method like checking if the string ends with '.log' and removing only the suffix.
| final nameWithoutExt = fileName.replaceAll('.log', ''); | |
| const logSuffix = '.log'; | |
| final nameWithoutExt = fileName.endsWith(logSuffix) | |
| ? fileName.substring(0, fileName.length - logSuffix.length) | |
| : fileName; |
| try { | ||
| LogRotation.checkAndRotateIfNeeded( | ||
| path: path, | ||
| maxFileSizeInBytes: 10 * 1024 * 1024, | ||
| maxBackupFiles: 1, | ||
| ); | ||
| } catch (e) { | ||
| print("Log rotation check failed: $e"); | ||
| } | ||
| logPrinter = MultiLogPrinter([ | ||
| _defaultConsolePrinter(), | ||
| FileLogPrinter(path), |
There was a problem hiding this comment.
There's a potential race condition between rotating the log file and the FileLogPrinter opening it. The rotation creates a new empty file at line 167, but FileLogPrinter is initialized at line 38 after the rotation check completes. If log rotation occurs, FileLogPrinter will open the newly created file in append mode, which should work correctly. However, if rotation fails partway through (e.g., after renaming but before creating the new file), FileLogPrinter may fail to open the file. Consider ensuring the new file exists before FileLogPrinter initialization or adding more robust error handling.
| print("Checking log file size for rotation: $path"); | ||
| final file = File(path); | ||
|
|
||
| if (!file.existsSync()) { | ||
| file.createSync(recursive: true); | ||
| return; | ||
| } | ||
|
|
||
| final fileSize = file.lengthSync(); | ||
| if (fileSize > maxFileSizeInBytes) { | ||
| _rotateLog(file, maxBackupFiles); | ||
| } | ||
| } | ||
|
|
||
| static void _rotateLog(File currentFile, int maxBackupFiles) { | ||
| try { | ||
| print("Rotating log file: ${currentFile.path}"); |
There was a problem hiding this comment.
Using print statements for logging instead of the configured logger is inconsistent with the rest of the codebase. Consider using debugPrint or the appLogger for consistency and to ensure these messages follow the same logging patterns.
| _compressFile(backupPath, zipPath); | ||
| // Delete the original log file after compression | ||
| File(backupPath).deleteSync(); |
There was a problem hiding this comment.
If compression fails (the catch block at line 195 is triggered), the backup .log file will still be deleted at line 165, resulting in data loss. The deletion should only occur if compression succeeds. Consider checking if the zip file was created successfully before deleting the backup.
| class LogRotation { | ||
| static void checkAndRotateIfNeeded({ | ||
| required String path, | ||
| required int maxFileSizeInBytes, | ||
| int maxBackupFiles = 2, | ||
| }) { |
There was a problem hiding this comment.
The LogRotation class and its public method checkAndRotateIfNeeded lack documentation. Consider adding class-level and method-level documentation that explains the purpose, parameters, behavior, and any important notes about usage (e.g., that it performs synchronous I/O operations).
|
|
||
| backupFiles | ||
| .sort((a, b) => a.lastModifiedSync().compareTo(b.lastModifiedSync())); | ||
|
|
||
| if (backupFiles.length > maxBackupFiles) { |
There was a problem hiding this comment.
The backup files are sorted by modification time in ascending order (oldest first), and then take() is used to select files for deletion. However, this will delete the oldest files, keeping the newest ones, which is the correct behavior. But the comment on line 168 says "Clean up old backups" which could be clearer. The logic appears correct but consider adding a comment explaining that files are sorted oldest-first and oldest files beyond the limit are deleted.
| backupFiles | |
| .sort((a, b) => a.lastModifiedSync().compareTo(b.lastModifiedSync())); | |
| if (backupFiles.length > maxBackupFiles) { | |
| // Sort by modification time in ascending order (oldest first). | |
| backupFiles | |
| .sort((a, b) => a.lastModifiedSync().compareTo(b.lastModifiedSync())); | |
| if (backupFiles.length > maxBackupFiles) { | |
| // Delete the oldest files beyond the maxBackupFiles limit, keeping the newest ones. |
|
|
||
| riverpod_lint: ^3.0.3 | ||
| ffigen: ^20.0.0 | ||
| archive: ^4.0.7 |
There was a problem hiding this comment.
The archive package should be added as a regular dependency rather than a dev dependency, since it's used in production code for log file compression. Dev dependencies are meant for development and testing tools only.
| } | ||
|
|
||
| void initLogger([String? path]) { | ||
| Future<void> initLogger([String? path]) async { |
There was a problem hiding this comment.
The initLogger function is now async but the call to it in main.dart (line 37) is not awaited. This means the log rotation check runs asynchronously and may not complete before the logger starts writing to files, potentially causing race conditions. Since checkAndRotateIfNeeded performs synchronous file operations, the function should either remain synchronous or the caller should await it.
| Future<void> initLogger([String? path]) async { | |
| void initLogger([String? path]) { |
| String currentPath, | ||
| int maxBackupFiles, |
There was a problem hiding this comment.
The currentPath parameter is declared but never used in this function. This parameter should either be removed or utilized if it was intended to exclude the current log file from cleanup operations.
| static void _rotateLog(File currentFile, int maxBackupFiles) { | ||
| try { | ||
| print("Rotating log file: ${currentFile.path}"); | ||
| final timestamp = DateFormat('yyyyMMdd_HHmmss').format(DateTime.now()); | ||
| final directory = currentFile.parent; | ||
| final fileName = currentFile.path.split(Platform.pathSeparator).last; | ||
| final nameWithoutExt = fileName.replaceAll('.log', ''); | ||
|
|
||
| // Create backup file path | ||
| final backupPath = | ||
| '${directory.path}${Platform.pathSeparator}${nameWithoutExt}_$timestamp.log'; | ||
| currentFile.renameSync(backupPath); | ||
|
|
||
| // Compress the backup file to zip | ||
| final zipPath = | ||
| '${directory.path}${Platform.pathSeparator}${nameWithoutExt}_$timestamp.zip'; | ||
| _compressFile(backupPath, zipPath); | ||
| // Delete the original log file after compression | ||
| File(backupPath).deleteSync(); | ||
| // Create new log file | ||
| File(currentFile.path).createSync(); | ||
| // Clean up old backups (now looking for .zip files) | ||
| _cleanupOldBackups( | ||
| directory, nameWithoutExt, currentFile.path, maxBackupFiles); | ||
|
|
||
| debugPrint("Log rotated and compressed: $zipPath"); | ||
| } catch (e, st) { | ||
| debugPrint("Failed to rotate log: $e\n$st"); | ||
| } | ||
| } | ||
|
|
||
| static void _compressFile(String sourcePath, String zipPath) { | ||
| try { | ||
| final sourceFile = File(sourcePath); | ||
| final bytes = sourceFile.readAsBytesSync(); |
There was a problem hiding this comment.
The log rotation performs several blocking I/O operations synchronously (file rename, read entire file into memory, compress, write, delete). For large log files (up to 10MB), reading the entire file into memory with readAsBytesSync() at line 181 could cause the application to freeze briefly. Consider making these operations asynchronous or performing them in an isolate to avoid blocking the main thread.
|
@copilot Make all changes you suggested, and create PR against this branch. |
This pull request introduces a log file rotation and compression system to the logging service, ensuring log files do not grow indefinitely and that old logs are archived and managed efficiently. The changes include a new utility class for handling log rotation, compression of rotated logs into zip files, and automated cleanup of old backups. Additionally, the necessary dependency for file compression is added.
Log rotation and compression:
LogRotationutility class tologger_service.dartthat checks the log file size, rotates the log when it exceeds a set limit, compresses old log files into zip archives, and deletes older backups beyond a configurable limit.initLoggerfunction to be asynchronous and to invoke log rotation checks before initializing the file log printer.Dependency management:
archivepackage topubspec.yamlto support log file compression.Imports:
logger_service.dartto includearchiveandintlfor compression and timestamp formatting.