Skip to content
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

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

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
06e2803
keep_last_n
ozgunozerk Aug 22, 2022
a98a627
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 22, 2022
30735ce
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 22, 2022
15f695b
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 22, 2022
e089385
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 22, 2022
d544f88
Update tracing-appender/Cargo.toml
ozgunozerk Sep 22, 2022
7d9a275
keep_last moved to builder
ozgunozerk Sep 22, 2022
1d0d5ea
builder method fix, and clippy fixes
ozgunozerk Sep 22, 2022
2881d40
typo
ozgunozerk Sep 22, 2022
6a82706
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
50034c2
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
cc1091c
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
803f48d
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
13a8c5a
suggestions
ozgunozerk Sep 23, 2022
e6b69af
logic fix
ozgunozerk Sep 23, 2022
371fc84
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 23, 2022
0f8f808
remainder of the suggestion
ozgunozerk Sep 23, 2022
13900ef
test for max_log_files
ozgunozerk Sep 23, 2022
c52d1df
Update tracing-appender/src/rolling.rs
ozgunozerk Sep 23, 2022
9bf0e24
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
86de637
Update tracing-appender/src/rolling/builder.rs
ozgunozerk Sep 23, 2022
08c4420
suggestions
ozgunozerk Sep 23, 2022
24f26bc
only parse format descr when creating appender
hawkw Sep 23, 2022
b224b1e
reduce test flakiness due to low resolution timestamps
hawkw Sep 23, 2022
1fd04ff
only turn filename into a string once
hawkw Sep 23, 2022
19e921f
never delete dirs or symlinks
hawkw Sep 23, 2022
5b3e24a
documentation fixes
ozgunozerk Sep 24, 2022
9032b44
remove `dbg!`
hawkw Sep 24, 2022
dfedf10
Merge pull request #1 from tokio-rs/eliza/one-format-descr
ozgunozerk Sep 24, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 69 additions & 1 deletion tracing-appender/src/rolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ struct Inner {
log_filename_suffix: Option<String>,
rotation: Rotation,
next_date: AtomicUsize,
keep_last: Option<usize>,
}

// === impl RollingFileAppender ===
Expand Down Expand Up @@ -152,6 +153,13 @@ impl RollingFileAppender {
.expect("initializing rolling file appender failed")
}

/// Keep the last `n` log entries on disk.
///
/// If no value is supplied, `RollingAppender` will not remove any files.
pub fn keep_last_n_logs(&mut self, n: usize) {
self.state.keep_last = Some(n);
}
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved

/// Returns a new [`Builder`] for configuring a `RollingFileAppender`.
///
/// The builder interface can be used to set additional configuration
Expand Down Expand Up @@ -187,6 +195,7 @@ impl RollingFileAppender {
ref rotation,
ref prefix,
ref suffix,
ref keep_last,
} = builder;
let directory = directory.as_ref().to_path_buf();
let now = OffsetDateTime::now_utc();
Expand All @@ -196,6 +205,7 @@ impl RollingFileAppender {
directory,
prefix.clone(),
suffix.clone(),
*keep_last,
)?;
Ok(Self {
state,
Expand Down Expand Up @@ -242,7 +252,7 @@ impl<'a> tracing_subscriber::fmt::writer::MakeWriter<'a> for RollingFileAppender
// Did we get the right to lock the file? If not, another thread
// did it and we can just make a writer.
if self.state.advance_date(now, current_time) {
self.state.refresh_writer(now, &mut *self.writer.write());
self.state.refresh_writer(now, &mut self.writer.write());
}
}
RollingWriter(self.writer.read())
Expand Down Expand Up @@ -538,6 +548,7 @@ impl Inner {
directory: impl AsRef<Path>,
log_filename_prefix: Option<String>,
log_filename_suffix: Option<String>,
keep_last: Option<usize>,
) -> Result<(Self, RwLock<File>), builder::InitError> {
let log_directory = directory.as_ref().to_path_buf();
let filename = rotation.join_date(
Expand All @@ -558,17 +569,73 @@ impl Inner {
.unwrap_or(0),
),
rotation,
keep_last,
};
Ok((inner, writer))
}

fn prune_old_logs(&self, keep_last: usize) {
let files = fs::read_dir(&self.log_directory).map(|dir| {
dir.filter_map(Result::ok)
.filter(|entry| {
entry.file_name().to_string_lossy().contains(
&self
.log_filename_prefix
.clone()
.expect("prefix is always present"),
)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.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

Copy link
Contributor Author

@ozgunozerk ozgunozerk Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

.collect::<Vec<_>>()
});
let mut files = match files {
Ok(files) => files,
Err(error) => {
eprintln!("Error reading the log directory/files: {}", error);
return;
}
};
if files.len() < keep_last {
return;
}
// sort the files by their creation timestamps.
files.sort_by_key(|file| {
file.metadata()
.and_then(|metadata| metadata.created().map(Some))
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
.unwrap_or_else(|error| {
eprintln!(
"Unable to get file creation time for {}: {}",
file.path().display(),
error
);
// if we couldn't determine the file's creation timestamp
// for whatever reason, it should compare as "less than"
// other files, so that it's not deleted.
None
})
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
});
// delete files, so that (n-1) files remain, because we will create another log file
for file in &files[..files.len() - (keep_last - 1)] {
if let Err(error) = fs::remove_file(file.path()) {
eprintln!(
"Failed to remove old log file {}: {}",
file.path().display(),
error
);
}
}
}

fn refresh_writer(&self, now: OffsetDateTime, file: &mut File) {
let filename = self.rotation.join_date(
self.log_filename_prefix.as_deref(),
&now,
self.log_filename_suffix.as_deref(),
);

if let Some(keep_last) = self.keep_last {
self.prune_old_logs(keep_last);
}

match create_writer(&self.log_directory, &filename) {
Ok(new_file) => {
if let Err(err) = file.flush() {
Expand Down Expand Up @@ -804,6 +871,7 @@ mod test {
directory.path(),
Some("test_make_writer".to_string()),
None,
None,
)
.unwrap();

Expand Down
29 changes: 29 additions & 0 deletions tracing-appender/src/rolling/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub struct Builder {
pub(super) rotation: Rotation,
pub(super) prefix: Option<String>,
pub(super) suffix: Option<String>,
pub(super) keep_last: Option<usize>,
}

/// Errors returned by [`Builder::build`].
Expand Down Expand Up @@ -48,6 +49,7 @@ impl Builder {
rotation: Rotation::NEVER,
prefix: None,
suffix: None,
keep_last: None,
}
}

Expand Down Expand Up @@ -181,6 +183,33 @@ impl Builder {
Self { suffix, ..self }
}

/// Keep the last `n` log entries on disk.
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
///
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
/// # Examples
///
/// Setting a suffix:
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```
/// use tracing_appender::rolling::RollingFileAppender;
///
/// # fn docs() {
/// let appender = RollingFileAppender::builder()
/// .keep_last_n_logs(5) // only the most recent 5 logs files will be kept
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
/// // ...
/// .build("/var/log")
/// .expect("failed to initialize rolling file appender");
/// # drop(appender)
/// # }
/// ```
///
/// If no value is supplied, `RollingAppender` will not remove any files.
ozgunozerk marked this conversation as resolved.
Show resolved Hide resolved
pub fn keep_last_n_logs(self, n: usize) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is definitely a better name, taking it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Self {
keep_last: Some(n),
..self
}
}

/// Builds a new [`RollingFileAppender`] with the configured parameters,
/// emitting log files to the provided directory.
///
Expand Down