feat: add log rotation for forester - #1875
Conversation
Added log rotation with 48-hour retention (2 days) and fallback to stdout logging when file appender creation fails.
WalkthroughThe telemetry setup logic was refactored to use a builder pattern for configuring the rolling file appender, with improved error handling. If file logging cannot be initialized, the system now logs a warning and defaults to stdout-only logging. Logging filters were simplified to default to "info" level, and control flow was updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Telemetry
participant FileAppender
participant Stdout
App->>Telemetry: setup_telemetry()
Telemetry->>FileAppender: try create rolling file appender (builder)
alt Success
FileAppender-->>Telemetry: appender created
Telemetry->>Telemetry: setup file logging layer
Telemetry->>Stdout: setup stdout logging layer
Telemetry->>App: initialize subscriber with file and stdout layers
else Failure
FileAppender-->>Telemetry: error
Telemetry->>Telemetry: print warning to stderr
Telemetry->>Stdout: setup stdout logging layer only
Telemetry->>App: initialize subscriber with stdout layer only
end
Estimated code review effort2 (~15 minutes) Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
forester/src/telemetry.rs(1 hunks)
🔇 Additional comments (4)
forester/src/telemetry.rs (4)
11-26: LGTM: Well-implemented builder pattern with proper error handling.The builder pattern implementation for the rolling file appender is excellent. The configuration is clear and the error handling gracefully falls back to stdout-only logging when file operations fail.
The log retention of 48 files (2 days with hourly rotation) aligns perfectly with the PR objectives.
28-32: Good simplification of environment filters.The change from potentially complex filters to defaulting to "info" level is a sensible simplification that improves maintainability.
39-58: Well-structured conditional logging setup.The conditional initialization based on file appender success is well-implemented, ensuring logging continues regardless of file system issues.
15-15: Please verify disk usage for hourly log rotationThe code retains 48 hourly log files (2 days), which meets the PR objectives. Before approving, confirm this retention level is acceptable in your deployment:
- Measure current log sizes, for example:
du -sh logs/forester*.log | awk '{sum += $1} END {print "Total:", sum}' du -sh logs | grep forester- Calculate: average log size × 48
- Ensure the total fits within your available disk budget
[file: forester/src/telemetry.rs Lines 15]
.max_log_files(48) // 2 days
|
|
||
| // Keep _guard in scope to keep the non-blocking writer alive | ||
| std::mem::forget(_guard); | ||
| std::mem::forget(_guard); |
There was a problem hiding this comment.
Memory leak: Replace std::mem::forget with proper guard management.
Using std::mem::forget(_guard) intentionally leaks memory, which is problematic for long-running applications. The guard should be stored and managed properly.
Consider one of these solutions:
Solution 1 (Recommended): Store the guard in a static
+use std::sync::Mutex;
+use once_cell::sync::Lazy;
+
+static GUARD: Lazy<Mutex<Option<tracing_appender::non_blocking::WorkerGuard>>> =
+ Lazy::new(|| Mutex::new(None));
// Inside the function:
-std::mem::forget(_guard);
+*GUARD.lock().unwrap() = Some(_guard);Solution 2: Use a leaked Box for controlled memory management
-std::mem::forget(_guard);
+Box::leak(Box::new(_guard));Solution 3: Accept the guard will be dropped and document the trade-off
-std::mem::forget(_guard);
+// Note: Guard is intentionally dropped here, which may cause log loss
+// during shutdown, but prevents memory leaks in long-running processes🤖 Prompt for AI Agents
In forester/src/telemetry.rs at line 52, the use of std::mem::forget(_guard)
causes a memory leak by intentionally preventing the guard from being dropped.
To fix this, replace std::mem::forget with proper guard management by storing
the guard in a static variable to keep it alive for the program's duration, or
alternatively use a leaked Box to manage the memory more explicitly. Avoid
forgetting the guard without tracking it, ensuring it is properly stored and
managed to prevent leaks.
2 participants
Added log rotation with 48-hour retention (2 days) and fallback to stdout logging when file appender creation fails.
Summary by CodeRabbit
Bug Fixes
Refactor