new feature: keep only the last N log files in rotation

[ozgunozerk]

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 keep_last: 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 last_keep 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 I had to sync the fork, so I re-opened it

[IniterWorker]

I think we should consider a more generic way from a library perspective point of view that can solve #1940 and/or provide a customizable abstraction.

[hawkw]

Thanks for working on this @ozgunozerk, and I'm sorry it took so long to get your PR reviewed! This is definitely something I'd like to add support for, I just haven't had the free time to review tracing PRs recently.

I commented on some potential changes to the implementation that I think we might want to make.

Also, we generally prefer to base PRs for changes like this on the master branch, rather than v0.1.x, when the changed code is the same between the two branches. That makes it easier to ensure changes made to the release version won't be "lost" when the tracing 0.2 ecosystem is released. Would you mind changing this PR to be based on the master branch? Once it merges, a maintainer will handle backporting the change to v0.1.x so that it can be released.

@ozgunozerk, if you no longer have the time to continue working on this change, I'd be happy to make the suggested changes to get this over the finish line, and merge it (with credit to you, of course). Please let me know if you'd like me to do that!

Thanks again for the PR!

[ozgunozerk]

@ozgunozerk, if you no longer have the time to continue working on this change, I'd be happy to make the suggested changes to get this over the finish line, and merge it (with credit to you, of course). Please let me know if you'd like me to do that!

@hawkw I'm still willing to spend effort on this, thank you very much for the kind offer :)
I'll start on fixes/suggestions today!

[ozgunozerk]

@hawkw when I change the base to master, there are approx. 540 commits inherited because I originally based my changes on v0.1.x

When the changes are ok, I can create clean PR for master branch if you like and close this one

[hawkw]

Overall, this is looking close to ready. I had a few more minor suggestions, mostly for the documentation.

If you have the time, it would also be great to have a test for this?

[hawkw]

take it or leave it: maybe a clearer name for this would be max_log_files? what do you think?

[ozgunozerk]

this is definitely a better name, taking it :)

[ozgunozerk]

I'm also changing keep_last to max_files, we can always revert it you think keep_last was a better name.

[ozgunozerk]

@hawkw
Will add a test tonight or tomorrow, please let me know if it is good to go otherwise :)

[hawkw]

The CI failures are unrelated, it looks like the latest Rust release broke the UI tests for the #[instrument] macro's error messages: https://github.com/tokio-rs/tracing/actions/runs/3114487346/jobs/5050405220#step:5:1233

Don't worry about that for now.

[ozgunozerk]

I've written a test, that will generate 3 log files,
and set the max_files to 2,
so the first created log file should be deleted.
@hawkw please check the last commit for the test. I mostly copy pasted the test_make_writer to generate this one.

[hawkw]

This is definitely getting close, and thanks for adding the test --- it looks right to me.

I have one remaining concern about this change: it looks like the code for determining whether or not a file in the log file directory is a log file or not works by checking if it starts with the appender's filename prefix, and panicking if self.log_filename_prefix is None. But, the appender can be configured with a filename prefix, a filename suffix, both, or neither. I don't think that the max_log_files setting should panic if the appender was configured without a prefix, since that's a valid configuration some users may be using. We should probably fix that before merging this branch.

Also, one last small docs change: the Builder::new() function's documentation has a list of the builder parameter's default values, here:

tracing/tracing-appender/src/rolling/builder.rs

Lines 34 to 44 in a77be16

	/// # Default Values
	///
	/// The default values for the builder are:
	///
	/// | Parameter | Default Value | Notes |
	/// | :-------- | :------------ | :---- |
	/// | [`rotation`] | [`Rotation::NEVER`] | By default, log files will never be rotated. |
	/// | [`filename_prefix`] | `""` | By default, log file names will not have a prefix. |
	///
	/// [`rotation`]: Self::rotation
	/// [`filename_prefix`]: Self::filename_prefix

let's add that there's no default max file count.

Besides that, I commented on some style tweaks you may want to make, but none of them are blockers for this change, as long as we take care of the issue with the filename prefix being required.

[hawkw]

i don't think this is correct. we can't expect that the prefix is always present --- the user may not have configured a log filename prefix. I think instead, we should check if the filename starts with the configured prefix if there is one, check for the suffix if there is one, and if there's neither a prefix nor a suffix, we could...see if its name parses as a date, i guess?

[hawkw]

also, we shouldn't have to clone the filename prefix here? we should be able to compare a &String to a String...

[ozgunozerk]

.log_filename_prefix is Option<String>.
Without clone(), the borrow checker is not happy:

move occurs because `self.log_filename_prefix` has type `Option<String>`, which does not implement the `Copy` trait

[ozgunozerk]

I think you are completely right on your first comment. When I wrote the code, I wrote it for my specific needs and forgot to change this part to general purpose.
Your suggestions sound pretty reasonable to me, implementing now 💯

[ozgunozerk]

Adding the check of parsing the filename as a date required me to add some previous stuff back (adding Date and Format related things). If there is better way doing that, I'm open to suggestions.

If not, you may want to reintroduce the parsing feature again maybe?

[ozgunozerk]

Thank you for spending this much effort&time into this @hawkw ,
I still recall your request on basing this PR against main. As I said, please let me know when you think this PR is in good shape, so I can close this one and open a new one against main branch.
(if I just change the base target, it becomes more than 530 commits against main)

[hawkw]

I still recall your request on basing this PR against main. As I said, please let me know when you think this PR is in good shape, so I can close this one and open a new one against main branch.

Yup, this looks good to me now, and if you're able to open a new PR against the main branch with these changes, that would make things much more convenient for the maintainers. I don't think there's anything else we need to address before merging this, once a new PR against the main branch is created. Thank you for all your hard work on this!

[ozgunozerk]

Please follow from #2323