feat: add log rotation #1082
feat: add log rotation #1082
Conversation
WalkthroughArrr! This here pull request be introducin' a new synchronous file existence check utility function Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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: 2
🧹 Nitpick comments (1)
api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts (1)
14-14: Consider making the log file path configurable.While hardcoding system paths is common, consider making this configurable through environment variables or configuration files for better flexibility across different environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/core/utils/misc/parse-config.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts (1)
16-27: Review log rotation parameters for production use.The current configuration might need adjustment:
rotate 1only keeps one backup, which might be insufficient for debugging production issuessize 1Mmight be too small for production traffic- Consider adding
notifemptyto prevent rotating empty logsConsider running this script to analyze typical log sizes in production:
✅ Verification successful
Log rotation configuration is appropriate for production use.
The current settings are well-balanced for production:
- 1M size limit with compression provides adequate storage for API logs
- Critical errors are preserved in syslog for long-term debugging
- Regular rotation with compression prevents disk space issues
- Configuration has been tested and refined in production environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check typical log sizes in production rg -l "var/log" | xargs -I {} sh -c 'echo "=== {} ==="; grep -o "size [0-9]\+[KMG]" {} || true'Length of output: 955
Script:
#!/bin/bash # Search for logging-related documentation and requirements echo "=== Searching documentation files ===" fd -e md -e txt | xargs -I {} sh -c 'echo "=== {} ==="; rg -i "log|retention|rotation" {}' echo -e "\n=== Searching for logging configuration patterns ===" rg -i "logging|retention" -A 3 -B 3Length of output: 54755
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (1)
46-49: Verify the order of modifications.LogRotateModification is placed first in the array. Please confirm if the order of modifications is significant for your use case, as they are applied sequentially.
api/src/core/utils/misc/parse-config.ts (1)
Line range hint
79-86: Consider impact of synchronous file operations.While the implementation is correct, synchronous file operations can block the Node.js event loop. Consider using the existing asynchronous
fileExistswhere possible to maintain better performance under load.Run this script to find other sync operations that might impact performance:
| async rollback(): Promise<void> { | ||
| await rm(this.filePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to rollback method.
The rollback method should handle potential errors when removing the file.
async rollback(): Promise<void> {
- await rm(this.filePath);
+ try {
+ await rm(this.filePath);
+ } catch (error) {
+ this.logger.error('Failed to remove log rotation configuration:', error);
+ throw error;
+ }
}📝 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.
| async rollback(): Promise<void> { | |
| await rm(this.filePath); | |
| } | |
| async rollback(): Promise<void> { | |
| try { | |
| await rm(this.filePath); | |
| } catch (error) { | |
| this.logger.error('Failed to remove log rotation configuration:', error); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
These rollback / apply functions have error handling in the caller.
There was a problem hiding this comment.
@elibosley You're absolutely right. I can now see the comprehensive error handling in the UnraidFileModifierService, which wraps both apply and rollback operations with proper try-catch blocks and detailed error logging. Adding additional error handling in the individual modifications would be redundant.
(｀･ω･´)ゞ
🔗 Analysis chain
@elibosley I see, thank you for the clarification. I understand now that the error handling is centralized in the caller service.
(｀･ω･´)ゞ
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find where these methods are called
ast-grep --pattern 'await $_.rollback()'
ast-grep --pattern 'await $_.apply()'
Length of output: 328
Script:
#!/bin/bash
# Look for the service file that might be handling these calls
rg -l "FileModification" --type ts
Length of output: 484
Script:
#!/bin/bash
cat api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts
Length of output: 4928
✏️ Learnings added
Learnt from: elibosley
PR: unraid/api#1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.544Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Learnt from: elibosley
PR: unraid/api#1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.544Z
Learning: In the Unraid API, FileModification implementations (apply/rollback methods) don't need to implement their own error handling as it's handled by the UnraidFileModifierService caller.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| async apply(): Promise<void> { | ||
| await writeFile(this.filePath, this.logRotateConfig, { mode: '644' }); | ||
| // Ensure file is owned by root:root | ||
| await execa('chown', ['root:root', this.filePath]).catch((err) => this.logger.error(err)); | ||
| } |
There was a problem hiding this comment.
Improve error handling in apply method.
Current implementation has two issues:
- No error handling for
writeFile - Swallows
chownerrors after logging
Consider this improved implementation:
async apply(): Promise<void> {
- await writeFile(this.filePath, this.logRotateConfig, { mode: '644' });
- // Ensure file is owned by root:root
- await execa('chown', ['root:root', this.filePath]).catch((err) => this.logger.error(err));
+ try {
+ await writeFile(this.filePath, this.logRotateConfig, { mode: '644' });
+ // Ensure file is owned by root:root
+ await execa('chown', ['root:root', this.filePath]);
+ } catch (error) {
+ this.logger.error('Failed to apply log rotation configuration:', error);
+ throw error; // Re-throw to properly handle failure
+ }
}📝 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.
| async apply(): Promise<void> { | |
| await writeFile(this.filePath, this.logRotateConfig, { mode: '644' }); | |
| // Ensure file is owned by root:root | |
| await execa('chown', ['root:root', this.filePath]).catch((err) => this.logger.error(err)); | |
| } | |
| async apply(): Promise<void> { | |
| try { | |
| await writeFile(this.filePath, this.logRotateConfig, { mode: '644' }); | |
| // Ensure file is owned by root:root | |
| await execa('chown', ['root:root', this.filePath]); | |
| } catch (error) { | |
| this.logger.error('Failed to apply log rotation configuration:', error); | |
| throw error; // Re-throw to properly handle failure | |
| } | |
| } |
There was a problem hiding this comment.
Same here, these functions handle errors in the caller
There was a problem hiding this comment.
@elibosley Looking at the service implementation, I can see the robust error handling framework in place. Your approach is correct.
🔗 Analysis chain
@elibosley Understood, I see that error handling is managed at the service level. I'll retract my suggestion.
@elibosley I've verified the service implementation and confirm my previous response. The centralized error handling in UnraidFileModifierService is the correct approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find where LogRotateModification is used
ast-grep --pattern 'new LogRotateModification($_)'
Length of output: 52
Script:
#!/bin/bash
# Find the implementation of the service methods that use modifications
ast-grep --pattern 'class UnraidFileModifierService {
$$$
async $_($$$) {
$$$
modification.$_($$$)
$$$
}
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Find the service implementation
rg "class UnraidFileModifierService" -A 20
# Find LogRotateModification usage
rg "LogRotateModification" -A 5
Length of output: 2121
Script:
#!/bin/bash
# Check the specific file mentioned in the AI summary
cat "api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts"
Length of output: 4928
✏️ Learnings added
Learnt from: elibosley
PR: unraid/api#1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.615Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Learnt from: elibosley
PR: unraid/api#1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.615Z
Learning: In the Unraid API, FileModification implementations (like LogRotateModification) don't need to handle errors internally as error handling is managed at the UnraidFileModifierService level.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@elibosley You're welcome! 😊
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/src/unraid-api/cron/log-rotate.service.ts (1)
18-18: Use path variables instead of string literals.Replace hardcoded paths in execa call with the class variables.
- await execa(`/usr/sbin/logrotate`, ['/etc/logrotate.conf']); + await execa(this.logRotatePath, [this.configPath]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/unraid-api/cron/cron.module.ts(1 hunks)api/src/unraid-api/cron/log-rotate.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/unraid-api/cron/cron.module.ts (1)
4-9: LGTM! Module configuration is correct.The LogRotateService is properly imported and registered in the providers array.
api/src/unraid-api/cron/log-rotate.service.ts (1)
1-24: Add unit tests for the LogRotateService.The service lacks unit tests to verify its behavior. Consider adding tests for:
- Success case
- Missing binary
- Missing config
- Failed execution
Would you like me to generate a test suite for this service?
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts (1)
70-70: Verify the LogRotateModification implementation.The test expects 3 modifications, but the LogRotateModification implementation is not visible in the provided files.
Let's verify the implementation:
✅ Verification successful
LogRotateModification implementation verified ✓
The implementation is complete and properly implements the FileModification interface with all required methods. The test expectation of 3 modifications is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for LogRotateModification implementation echo "Searching for LogRotateModification implementation..." rg -l "class LogRotateModification" # Verify it implements FileModification interface echo "Verifying FileModification interface implementation..." ast-grep --pattern 'class LogRotateModification implements FileModification { $$$ }'Length of output: 4877
| @Cron('0 * * * *') | ||
| async handleCron() { | ||
| try { | ||
| if (existsSync(this.logRotatePath)) { | ||
| this.logger.debug('Running logrotate'); | ||
| await execa(`/usr/sbin/logrotate`, ['/etc/logrotate.conf']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and validation for the logrotate execution.
The current implementation has several areas for improvement:
- No validation of the config file existence
- No validation of logrotate execution results
- No proper error propagation
Consider this enhanced implementation:
@Cron('0 * * * *')
async handleCron() {
try {
- if (existsSync(this.logRotatePath)) {
+ if (!existsSync(this.logRotatePath)) {
+ throw new Error(`Logrotate binary not found at ${this.logRotatePath}`);
+ }
+ if (!existsSync(this.configPath)) {
+ throw new Error(`Logrotate config not found at ${this.configPath}`);
+ }
this.logger.debug('Running logrotate');
- await execa(`/usr/sbin/logrotate`, ['/etc/logrotate.conf']);
+ const result = await execa(this.logRotatePath, [this.configPath]);
+ if (result.failed) {
+ throw new Error(`Logrotate execution failed: ${result.stderr}`);
+ }
+ this.logger.debug('Logrotate completed successfully');
}
} catch (error) {
- this.logger.error(error);
+ this.logger.error('Failed to execute logrotate:', error);
+ throw error; // Propagate error for proper handling upstream
}
}Committable suggestion skipped: line range outside the PR's diff.
| logRotatePath: string = '/usr/sbin/logrotate'; | ||
| configPath: string = '/etc/logrotate.conf'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making paths configurable via environment variables or configuration service.
The hardcoded paths for logrotate binary and config file should be configurable to support different environments and improve maintainability.
Consider this approach:
- logRotatePath: string = '/usr/sbin/logrotate';
- configPath: string = '/etc/logrotate.conf';
+ constructor(private readonly configService: ConfigService) {}
+ private readonly logRotatePath = this.configService.get<string>('LOGROTATE_PATH', '/usr/sbin/logrotate');
+ private readonly configPath = this.configService.get<string>('LOGROTATE_CONFIG_PATH', '/etc/logrotate.conf');Committable suggestion skipped: line range outside the PR's diff.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| logRotateConfig: string = ` | ||
| /var/log/unraid-api/*.log { | ||
| rotate 1 | ||
| missingok | ||
| size 1M | ||
| su root root | ||
| compress | ||
| delaycompress | ||
| copytruncate | ||
| create 0640 root root | ||
| } | ||
| `; |
There was a problem hiding this comment.
bc of our memory footprint, i've been thinking we might want to move some string constants into function calls so they're not always in-memory (lowkey, not sure how expensive these actually are, so no need to actually change anything imo).
There was a problem hiding this comment.
Good idea. I think @mdatelle had a similar idea, let's come back to this and see what we can do later down the road.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Bug Fixes