new feature: keep only the last N log files in rotation #2169
Conversation
tracing-appender/src/rolling.rs
Outdated
| let date_str = | ||
| filename.clone()[(filename.rfind('.').unwrap() + 1)..].to_string(); | ||
| let date: usize = date_str.replace("-", "").parse().unwrap(); |
There was a problem hiding this comment.
It might not work if the underlying format of date would be changed.
I would extract Rotation::join_date into a type which would implement both Display and FromStr
tracing-appender/src/rolling.rs
Outdated
| }; | ||
| (inner, writer) | ||
| } | ||
|
|
||
| fn prune_old_logs(&self, keep_last: usize) { |
There was a problem hiding this comment.
If I understand this correctly, this function is collecting files into a Vec of Strings if the path contains the filename prefix and then sorting by the dates.
While this can work, I don't think this is how this should be implemented. Namely, I suggest the following:
- Use the path components of each path and convert
log_file_nameto anOsStr. - I don't recommend doing the adhoc string parsing in the latter half of the function. I believe we're emitting files with dates and I'm not opposed to enabling time's datetime parsing functionality in
tracing-appenderto better suppose this use-case, but depending on the compilation size hit, I might want to place this behind a Cargo feature.
There was a problem hiding this comment.
Use the path components of each path and convert log_file_name to an OsStr.
instead of using path components and iterating till the last Some(...), i think using the file_name() from DirEntry can be more useful. It already returns an OsStr :)
There was a problem hiding this comment.
I don't recommend doing the adhoc string parsing in the latter half of the function. I believe we're emitting files with dates and I'm not opposed to enabling time's datetime parsing functionality in tracing-appender to better suppose this use-case, but depending on the compilation size hit, I might want to place this behind a Cargo feature.
I implemented this, and pushed the change. However, I think it may be useful to compare two solutions.
- 1st (adhoc): less code, same logic, but more error-prone since we are using some string slice, and if any mistake happens, it will be harder to discover and debug
- 2nd (date parsing): extra imports, an extra code block of 18 lines just to create a
formatterfor the date parser. Safer code, since we will be treating the string slice as a properDateobject.
Please check the latest commit for the code changes related to above discussion
Important note: we actually already calculated the formatter in the code. I wanted to use it via storing this in an extra field in the Inner struct. But the type of formatter is FormatItem, which is a lifetimed variable. So the Inner itself should have a lifetime, and I did not want to disrupt the clean/simple code. Hence, the code duplication in my solution.
As you may guess, I'm not a mature Rust developer, but trying my best. If you think another solution may be better, please let me know. I'll try to implement it :)
There was a problem hiding this comment.
When you are happy with the solution, I can place all this behind a feature 👍
|
There is one test failing :/ |
tracing-appender/src/rolling.rs
Outdated
| let mut sorted_files = vec![]; | ||
| for file in files { | ||
| let filename = file.file_name().to_string_lossy().to_string(); | ||
| let date_str = filename[(filename.rfind('.').unwrap() + 1)..].to_string(); |
There was a problem hiding this comment.
Are you sure this will never panic?
There was a problem hiding this comment.
As long as the format of adding dates will stay the same, this should not panic imo.
Currently, the date is added to the filename with a ., so there should be a . just before the date :)
There was a problem hiding this comment.
I see! Probably a bit stretched, but imagine :
- my prefix doesn’t contain any extension (say for instance: "abc").
tracing-appenderwill generate files such asabc.2022.06.03and so on and so forth. - however, user can create a file called
abc - this file will be collected by the code at line 517, because the predicate is true ("abc" is contained in "abc")
rfind(’.’)will returnsNonebecause "abc" doesn’t contain any.
Quite an edge case though, maybe not worth addressing?
There was a problem hiding this comment.
I see. Yes, if the user manipulates the log folder, everything becomes possible :/
Let me try to handle it with a better error handling! Thank you very much for the suggestion
|
i want to make a few additional notes, but since i'm packing for my move this weekend, it might be a day or two. sorry! |
|
Any update on this? |
|
I'm willing to make further updates if necessary :) |
|
Sorry, it's been a busy month. Do you mind if I make commits directly to your branch/fork for the changes that I think we need? Since I believe this approach works for you, I'll also open a branch that targets the master branch—it'll make the subsequent branch maintenance on our side easier. |
|
Of course, no worries! |
|
Any update on this? :) |
|
What happened to this PR? GitHub says it's merged, but it's not as far as I can tell
|
|
Because unfortunately there were no responses from the tokio team, I decided to use the fork for my project, and thus synced the fork. Weird that it says merged. Let me re-open the PR, maybe we can get some attention :) |
|
New one is here: #2284 |
Motivation
tracing-appenderdoes not haveRotationbased on size yet.Also, it doesn't have the feature of keeping the most recent
Nlog filesI believe the second feature is more easy to implement, and also will partially solve the
Rotationbased on size problem. Because people may choosehourlyordailyrotation based on their needs, and put an extra boundary ofkeep the last 5 filesfor example. Of course it won't handle all the edge cases forRotationbased on size. But it will cover most of the scenarios. And also, it is a good feature to have on its own :)Solution
Introduce another field called
keep_last: Option<usize>to theInnerofRollingFileAppenderstruct.I managed to did not touch any of the existing functions, so it WON'T BE A BREAKING CHANGE. Yay :)
The solution is, whenever the rotation should happen, the
refresh_writer()is called. So I embed the following logic into that function:1- check the log folder and detect the log files
2- if there are more log files than the
last_keepamount3- store the filenames in a vector, and sort them by their dates (dates are already present in the filename)
4- keep deleting the oldest ones, till we have desired amount of log files in the log folder