-
Notifications
You must be signed in to change notification settings - Fork 734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
max_log_files_feature #2323
max_log_files_feature #2323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I previously approved this on the original PR against the v0.1.x
branch, so this looks good to me, provided CI passes! I'll make sure to give credit to @CBenoit when merging.
Thanks again for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed one last thing that I think was missed while porting this change over from your branch against v0.1.x
. It looks like this branch is missing the change to add the "parsing" feature flag to the dependency on time
, so this won't build when used as a dependency unless user code is already enabling time
's "parsing" feature. This isn't an issue for the tests, because they have a dev-dependency that already enables that feature:
tracing/tracing-appender/Cargo.toml
Line 38 in fb7cb4a
time = { version = "0.3.2", default-features = false, features = ["formatting", "parsing"] } |
I think before we can merge this, we have to change this line:
tracing/tracing-appender/Cargo.toml
Line 25 in fb7cb4a
time = { version = "0.3.2", default-features = false, features = ["formatting"] } |
so that it also enables the "parsing" feature flag. Then, the dev dependency can just be removed.
Once that's addressed, this will be good to go!
oh, i guess CI actually is failing due to the missing feature flag: https://github.com/tokio-rs/tracing/actions/runs/3119699075/jobs/5059829752#step:7:79 |
your CI's are fast, I'm impressed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me as soon as CI passes (which i think it ought to now!)
## Motivation `tracing-appender` does not have `Rotation` based on size yet. Also, it doesn't have the feature of keeping the most recent `N` log files I believe the second feature is more easy to implement, and also will partially solve the `Rotation` based on size problem. Because people may choose `hourly` or `daily` rotation based on their needs, and put an extra boundary of `keep the last 5 files` for example. Of course it won't handle all the edge cases for `Rotation` based 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 `max_files: Option<usize>` to the `Inner` of `RollingFileAppender` struct. 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 `max_files` amount 3- 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 P.S. this PR was opened before, but got closed since it would be easier for the maintainers to target `master` branch instead of `v0.1.x` Also, @CBenoit contributed to this PR, it would be great to give credit to him :) Co-authored-by: Benoît Cortier <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
## Motivation `tracing-appender` does not have `Rotation` based on size yet. Also, it doesn't have the feature of keeping the most recent `N` log files I believe the second feature is more easy to implement, and also will partially solve the `Rotation` based on size problem. Because people may choose `hourly` or `daily` rotation based on their needs, and put an extra boundary of `keep the last 5 files` for example. Of course it won't handle all the edge cases for `Rotation` based 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 `max_files: Option<usize>` to the `Inner` of `RollingFileAppender` struct. 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 `max_files` amount 3- 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 P.S. this PR was opened before, but got closed since it would be easier for the maintainers to target `master` branch instead of `v0.1.x` Also, @CBenoit contributed to this PR, it would be great to give credit to him :) Co-authored-by: Benoît Cortier <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
## Motivation `tracing-appender` does not have `Rotation` based on size yet. Also, it doesn't have the feature of keeping the most recent `N` log files I believe the second feature is more easy to implement, and also will partially solve the `Rotation` based on size problem. Because people may choose `hourly` or `daily` rotation based on their needs, and put an extra boundary of `keep the last 5 files` for example. Of course it won't handle all the edge cases for `Rotation` based 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 `max_files: Option<usize>` to the `Inner` of `RollingFileAppender` struct. 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 `max_files` amount 3- 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 P.S. this PR was opened before, but got closed since it would be easier for the maintainers to target `master` branch instead of `v0.1.x` Also, @CBenoit contributed to this PR, it would be great to give credit to him :) Co-authored-by: Benoît Cortier <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
## Motivation `tracing-appender` does not have `Rotation` based on size yet. Also, it doesn't have the feature of keeping the most recent `N` log files I believe the second feature is more easy to implement, and also will partially solve the `Rotation` based on size problem. Because people may choose `hourly` or `daily` rotation based on their needs, and put an extra boundary of `keep the last 5 files` for example. Of course it won't handle all the edge cases for `Rotation` based 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 `max_files: Option<usize>` to the `Inner` of `RollingFileAppender` struct. 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 `max_files` amount 3- 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 P.S. this PR was opened before, but got closed since it would be easier for the maintainers to target `master` branch instead of `v0.1.x` Also, @CBenoit contributed to this PR, it would be great to give credit to him :) Co-authored-by: Benoît Cortier <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
This feature is great. Do you have a release schedule? |
I have the same concern. |
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (#2323) - **non_blocking**: allow worker thread name to be configured (#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (#2227) - **rolling**: add `Builder::filename_suffix` parameter (#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (#2607) - **non_blocking**: name spawned threads (#2219) ## Fixed - Fixed several documentation typos and issues (#2689, #2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (tokio-rs#2323) - **non_blocking**: allow worker thread name to be configured (tokio-rs#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (tokio-rs#2227) - **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (tokio-rs#2607) - **non_blocking**: name spawned threads (tokio-rs#2219) ## Fixed - Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (tokio-rs#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (tokio-rs#2323) - **non_blocking**: allow worker thread name to be configured (tokio-rs#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (tokio-rs#2227) - **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (tokio-rs#2607) - **non_blocking**: name spawned threads (tokio-rs#2219) ## Fixed - Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (tokio-rs#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
Motivation
tracing-appender
does not haveRotation
based on size yet.Also, it doesn't have the feature of keeping the most recent
N
log filesI believe the second feature is more easy to implement, and also will partially solve the
Rotation
based on size problem. Because people may choosehourly
ordaily
rotation based on their needs, and put an extra boundary ofkeep the last 5 files
for example. Of course it won't handle all the edge cases forRotation
based 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 theInner
ofRollingFileAppender
struct.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_keep
amount3- 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
P.S. this PR was opened before, but got closed since it would be easier for the maintainers to target
master
branch instead ofv0.1.x
Also, @CBenoit contributed to this PR, it would be great to give credit to him :)