From c775505b128fbfd70d84303efe3b131bbcb56e66 Mon Sep 17 00:00:00 2001 From: CfirTsabari Date: Thu, 30 Jul 2020 18:10:12 +0000 Subject: [PATCH 1/2] Size Based Rotating File Appender Motivation Adding an appender with max size per log file and max number of log files. Addressing tokio-rs/tracing#858 Solution Adding an appender very similar to the `RollingFileAppender`, that will base the rotation on size of log file and number of log files. --- tracing-appender/src/inner.rs | 125 ++++++++- tracing-appender/src/lib.rs | 27 +- tracing-appender/src/rolling.rs | 14 +- tracing-appender/src/rotating.rs | 420 +++++++++++++++++++++++++++++++ 4 files changed, 565 insertions(+), 21 deletions(-) create mode 100644 tracing-appender/src/rotating.rs diff --git a/tracing-appender/src/inner.rs b/tracing-appender/src/inner.rs index f1e0fe0029..f05ace0c69 100644 --- a/tracing-appender/src/inner.rs +++ b/tracing-appender/src/inner.rs @@ -1,22 +1,24 @@ use std::io::{BufWriter, Write}; use std::{fs, io}; -use crate::rolling::Rotation; +use crate::rolling::Rotation as Roll; +use crate::rotating::Rotation; + use chrono::prelude::*; use std::fmt::Debug; use std::fs::{File, OpenOptions}; use std::path::Path; #[derive(Debug)] -pub(crate) struct InnerAppender { +pub(crate) struct InnerRollingAppender { log_directory: String, log_filename_prefix: String, writer: BufWriter, next_date: DateTime, - rotation: Rotation, + roll: Roll, } -impl io::Write for InnerAppender { +impl io::Write for InnerRollingAppender { fn write(&mut self, buf: &[u8]) -> io::Result { let now = Utc::now(); self.write_timestamped(buf, now) @@ -27,25 +29,25 @@ impl io::Write for InnerAppender { } } -impl InnerAppender { +impl InnerRollingAppender { pub(crate) fn new( log_directory: &Path, log_filename_prefix: &Path, - rotation: Rotation, + roll: Roll, now: DateTime, ) -> io::Result { let log_directory = log_directory.to_str().unwrap(); let log_filename_prefix = log_filename_prefix.to_str().unwrap(); - let filename = rotation.join_date(log_filename_prefix, &now); - let next_date = rotation.next_date(&now); + let filename = roll.join_date(log_filename_prefix, &now); + let next_date = roll.next_date(&now); - Ok(InnerAppender { + Ok(InnerRollingAppender { log_directory: log_directory.to_string(), log_filename_prefix: log_filename_prefix.to_string(), writer: create_writer(log_directory, &filename)?, next_date, - rotation, + roll, }) } @@ -59,9 +61,9 @@ impl InnerAppender { fn refresh_writer(&mut self, now: DateTime) { if self.should_rollover(now) { - let filename = self.rotation.join_date(&self.log_filename_prefix, &now); + let filename = self.roll.join_date(&self.log_filename_prefix, &now); - self.next_date = self.rotation.next_date(&now); + self.next_date = self.roll.next_date(&now); match create_writer(&self.log_directory, &filename) { Ok(writer) => self.writer = writer, @@ -75,6 +77,87 @@ impl InnerAppender { } } +#[derive(Debug)] +pub(crate) struct InnerRotatingAppender { + log_directory: String, + log_filename_prefix: String, + writer: BufWriter, + last_backup: usize, + current_size: usize, + rotation: Rotation, +} + +impl io::Write for InnerRotatingAppender { + fn write(&mut self, buf: &[u8]) -> io::Result { + let buf_len = buf.len(); + self.refresh_writer(&buf_len); + self.writer.write_all(buf).map(|_| buf_len) + } + + fn flush(&mut self) -> io::Result<()> { + self.writer.flush() + } +} + +impl InnerRotatingAppender { + pub(crate) fn new( + rotation: Rotation, + log_directory: &Path, + log_filename_prefix: &Path, + ) -> io::Result { + let log_directory = log_directory.to_str().unwrap(); + let log_filename_prefix = log_filename_prefix.to_str().unwrap(); + let current_size = get_file_size(log_directory, log_filename_prefix)?; + let last_backup = Self::find_last_backup(&rotation, log_directory, log_filename_prefix); + Ok(Self { + writer: create_writer(log_directory, log_filename_prefix)?, + log_directory: log_directory.to_string(), + log_filename_prefix: log_filename_prefix.to_string(), + last_backup, + current_size, + rotation, + }) + } + fn refresh_writer(&mut self, size: &usize) { + if self.rotation.should_rollover(self.current_size + size) { + self.current_size = 0; + if self.rotation.is_create_backup(self.last_backup) { + self.last_backup += 1; + } + self.rotate_files(); + match create_writer(&self.log_directory, &self.log_filename_prefix) { + Ok(writer) => self.writer = writer, + Err(err) => eprintln!("Couldn't create writer for logs: {}", err), + } + } + self.current_size += size; + } + fn rotate_files(&self) { + for x in (1..=self.last_backup).rev() { + let from = self.rotation.join_backup(&self.log_filename_prefix, x - 1); + let to = self.rotation.join_backup(&self.log_filename_prefix, x); + if let Err(err) = rename_file(&self.log_directory, &from, &to) { + eprintln!("Couldn't rename backup log file: {}", err); + } + } + } + fn find_last_backup( + rotation: &Rotation, + log_directory: &str, + log_filename_prefix: &str, + ) -> usize { + let mut last_backup = 0; + while rotation.is_create_backup(last_backup) { + let filename = rotation.join_backup(log_filename_prefix, last_backup + 1); + if file_exist(log_directory, &filename) { + last_backup += 1; + } else { + break; + } + } + last_backup + } +} fn create_writer(directory: &str, filename: &str) -> io::Result> { let file_path = Path::new(directory).join(filename); Ok(BufWriter::new(open_file_create_parent_dirs(&file_path)?)) @@ -94,3 +177,21 @@ fn open_file_create_parent_dirs(path: &Path) -> io::Result { new_file } +fn get_file_size(directory: &str, filename: &str) -> io::Result { + let file_path = Path::new(directory).join(filename); + if file_path.exists() { + Ok(std::fs::metadata(file_path)?.len() as usize) + } else { + Ok(0) + } +} +fn file_exist(directory: &str, filename: &str) -> bool { + let file_path = Path::new(directory).join(filename); + file_path.as_path().exists() +} + +fn rename_file(directory: &str, from: &str, to: &str) -> io::Result<()> { + let from = Path::new(directory).join(from); + let to = Path::new(directory).join(to); + std::fs::rename(from, to) +} diff --git a/tracing-appender/src/lib.rs b/tracing-appender/src/lib.rs index 56509d4436..4838dff1d8 100644 --- a/tracing-appender/src/lib.rs +++ b/tracing-appender/src/lib.rs @@ -1,3 +1,4 @@ +// TODO (cfirt): add rotating (01/08/2020) //! Writers for logging events and spans //! //! # Overview @@ -18,9 +19,11 @@ //! ``` //! //! This crate can be used in a few ways to record spans/events: -//! - Using a [`RollingFileAppender`][rolling_struct] to perform writes to a log file. This will block on writes. +//! - Using a [`RollingFileAppender`][rolling_struct] to perform writes to a log file, with time based roll. This will block on writes. +//! - Using a [`RotatingFileAppender`][rotating_struct] to perform writes to a log file, with size based rotation. This will block on writes. //! - Using *any* type implementing [`std::io::Write`][write] in a non-blocking fashion. -//! - Using a combination of [`NonBlocking`][non_blocking] and [`RollingFileAppender`][rolling_struct] to allow writes to a log file +//! - Using a combination of [`NonBlocking`][non_blocking] and one of the appenders( [`RollingFileAppender`][rolling_struct] or [`RotatingFileAppender`][rotating_struct]) +//! to allow writes to a log file //! without blocking. //! //! ## Rolling File Appender @@ -38,6 +41,21 @@ //! //! The [`rolling` module][rolling]'s documentation provides more detail on how to use this file appender. //! +//! ## Rotating File Appender +//! +//! ```rust +//! # fn docs() { +//! let file_appender = tracing_appender::rotating::mb_100("/some/directory", "prefix.log"); +//! # } +//! ``` +//! This creates an rotating file appender that writes to `/some/directory/prefix.log`, with a maximum +//! size of 100 MB for log file, and with 9 maximum backups log files. +//! +//! The file appender implements [`std::io::Write`][write]. To be used with [`tracing_subscriber::FmtSubscriber`][fmt_subscriber], +//! it must be combined with a [`MakeWriter`][make_writer] implementation to be able to record tracing spans/event. +//! +//! The [`rotating` module][rotating]'s documentation provides more detail on how to use this file appender. +//! //! ## Non-Blocking Writer //! //! The example below demonstrates the construction of a `non_blocking` writer with `std::io::stdout()`, @@ -89,8 +107,11 @@ //! [write]: https://doc.rust-lang.org/std/io/trait.Write.html //! [guard]: ./non_blocking/struct.WorkerGuard.html //! [rolling]: ./rolling/index.html +//! [rotating]: ./rotating/index.html //! [make_writer]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/trait.MakeWriter.html //! [rolling_struct]: ./rolling/struct.RollingFileAppender.html +//! [rotating_struct]: ./rotating/struct.RotatingFileAppender.html + //! [fmt_subscriber]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/fmt/struct.Subscriber.html //! //! ## Non-Blocking Rolling File Appender @@ -141,6 +162,8 @@ pub mod non_blocking; pub mod rolling; +pub mod rotating; + mod worker; /// Convenience function for creating a non-blocking, off-thread writer. diff --git a/tracing-appender/src/rolling.rs b/tracing-appender/src/rolling.rs index a2b1f9c1bc..ad0fb12172 100644 --- a/tracing-appender/src/rolling.rs +++ b/tracing-appender/src/rolling.rs @@ -9,13 +9,13 @@ //! //! The following helpers are available for creating a rolling file appender. //! -//! - [`Rotation::minutely()`][minutely]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd-HH-mm` +//! - [`rolling::minutely()`][minutely]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd-HH-mm` //! will be created minutely (once per minute) -//! - [`Rotation::hourly()`][hourly]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd-HH` +//! - [`rolling::hourly()`][hourly]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd-HH` //! will be created hourly -//! - [`Rotation::daily()`][daily]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd` +//! - [`rolling::daily()`][daily]: A new log file in the format of `some_directory/log_file_name_prefix.yyyy-MM-dd` //! will be created daily -//! - [`Rotation::never()`][never]: This will result in log file located at `some_directory/log_file_name` +//! - [`rolling::never()`][never]: This will result in log file located at `some_directory/log_file_name` //! //! [minutely]: fn.minutely.html //! [hourly]: fn.hourly.html @@ -30,7 +30,7 @@ //! let file_appender = RollingFileAppender::new(Rotation::HOURLY, "/some/directory", "prefix.log"); //! # } //! ``` -use crate::inner::InnerAppender; +use crate::inner::InnerRollingAppender; use chrono::{DateTime, Datelike, TimeZone, Timelike, Utc}; use std::fmt::Debug; use std::io; @@ -54,7 +54,7 @@ use std::path::Path; /// ``` #[derive(Debug)] pub struct RollingFileAppender { - inner: InnerAppender, + inner: InnerRollingAppender, } impl RollingFileAppender { @@ -91,7 +91,7 @@ impl RollingFileAppender { file_name_prefix: impl AsRef, ) -> RollingFileAppender { RollingFileAppender { - inner: InnerAppender::new( + inner: InnerRollingAppender::new( directory.as_ref(), file_name_prefix.as_ref(), rotation, diff --git a/tracing-appender/src/rotating.rs b/tracing-appender/src/rotating.rs new file mode 100644 index 0000000000..ba4f800adb --- /dev/null +++ b/tracing-appender/src/rotating.rs @@ -0,0 +1,420 @@ +//! A rotating file appender. +//! +//! Creates a new log file every fixed number of bytes and a fixed number of backups defined by [`Rotation`](struct.Rotation.html). +//! Logs will be written to this file and will automatically rotate +//! to the newly created log file once the file has reached a ceratin size. +//! When a rotation occurs, if allowed old logs will be saved under a new name. +//! +//! When allowing backups the appender will save old log files by appending the extensions ‘.1’, ‘.2’ etc., +//! to the filename. +//! For example, with a maximum backups allowed of 5 and a base file name of app.log, +//! you would get app.log, app.log.1, app.log.2, up to app.log.5. +//! The file being written to is always app.log. +//! When this file is filled, it is closed and renamed to app.log.1, and if files app.log.1, app.log.2, etc. exist, +//! then they are renamed to app.log.2, app.log.3 etc. respectively, app.log.5 will not be renamed to app.log.6. +//! +//! The log file is created at the specified directory and file name prefix which *may* be appended with +//! a backup index number. +//! +//! # Examples +//! +//! ```rust +//! # fn docs() { +//! use tracing_appender::rotating::{RotatingFileAppender, Rotation}; +//! let file_appender = RotatingFileAppender::new(Rotation::mb_100(), "/some/directory", "prefix.log"); +//! # } +//! ``` +use crate::inner::InnerRotatingAppender; +use std::io; +use std::path::Path; + +/// A file appender with the ability to rotate log files at a fixed size. +/// +/// `RotatingFileAppender` implements [`std:io::Write` trait][write] and will block on write operations. +/// It may be used with [`NonBlocking`][non-blocking] to perform writes without +/// blocking the current thread. +/// +/// [write]: https://doc.rust-lang.org/nightly/std/io/trait.Write.html +/// [non-blocking]: ../non_blocking/struct.NonBlocking.html +/// +/// # Examples +/// +/// ```rust +/// # fn docs() { +/// let file_appender = tracing_appender::rotating::mb_100("/some/directory", "prefix.log"); +/// # } +/// ``` +#[derive(Debug)] +pub struct RotatingFileAppender { + inner: InnerRotatingAppender, +} + +impl RotatingFileAppender { + /// Creates a new `RotatingFileAppender`. + /// + /// A `RotatingFileAppender` will have a fixed rotation whose size is + /// defined by [`Rotation`](struct.Rotation.html). The `directory` and + /// `file_name_prefix` arguments determine the location and file name's _prefix_ + /// of the log file. `RotatingFileAppender` will add an index to old logs. + /// + /// Alternatively, a `RotatingFileAppender` can be constructed using one of the following helpers: + /// + /// - [`rotating::mb_100()`][mb_100], + /// - [`rotating::never()`][never] + /// + /// [mb_100]: fn.mb_100.html + /// [never]: fn.never.html + /// + /// # Examples + /// ```rust + /// # fn docs() { + /// use tracing_appender::rotating::{RotatingFileAppender, Rotation}; + /// let file_appender = RotatingFileAppender::new(Rotation::mb_100(), "/some/directory", "prefix.log"); + /// # } + /// ``` + pub fn new( + rotation: Rotation, + directory: impl AsRef, + file_name_prefix: impl AsRef, + ) -> RotatingFileAppender { + RotatingFileAppender { + inner: InnerRotatingAppender::new( + rotation, + directory.as_ref(), + file_name_prefix.as_ref(), + ) + .expect("Failed to create appender"), + } + } +} + +impl io::Write for RotatingFileAppender { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.inner.write(buf) + } + + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +/// Creates an rotating file appender that will rotate after 100 MB and will allow 9 old log files. +/// +/// The appender returned by `rotating::mb_100` can be used with `non_blocking` to create +/// a non-blocking, size based rotating file appender. +/// +/// The directory of the log file is specified with the `directory` argument. +/// `file_name_prefix` specifies the _prefix_ of the log file. `RotatingFileAppender` +/// adds a backup index for old log files.. +/// +/// # Examples +/// +/// ``` rust +/// # #[clippy::allow(needless_doctest_main)] +/// fn main () { +/// # fn doc() { +/// let appender = tracing_appender::rotating::mb_100("/some/path", "rotating.log"); +/// let (non_blocking_appender, _guard) = tracing_appender::non_blocking(appender); +/// +/// let subscriber = tracing_subscriber::fmt().with_writer(non_blocking_appender); +/// +/// tracing::subscriber::with_default(subscriber.finish(), || { +/// tracing::event!(tracing::Level::INFO, "Hello"); +/// }); +/// # } +/// } +/// ``` +/// +/// This will result in a log file located at `/some/path/rotating.log`. +pub fn mb_100( + directory: impl AsRef, + file_name_prefix: impl AsRef, +) -> RotatingFileAppender { + RotatingFileAppender::new(Rotation::mb_100(), directory, file_name_prefix) +} + +/// Creates a non-rotating, file appender +/// +/// The appender returned by `rotating::never` can be used with `non_blocking` to create +/// a non-blocking, non-rotating appender. +/// +/// The location of the log file will be specified the `directory` passed in. +/// `file_name` specifies the prefix of the log file. No old log files will be provided. +/// +/// # Examples +/// +/// ``` rust +/// # #[clippy::allow(needless_doctest_main)] +/// fn main () { +/// # fn doc() { +/// let appender = tracing_appender::rotating::never("/some/path", "non-rotating.log"); +/// let (non_blocking_appender, _guard) = tracing_appender::non_blocking(appender); +/// +/// let subscriber = tracing_subscriber::fmt().with_writer(non_blocking_appender); +/// +/// tracing::subscriber::with_default(subscriber.finish(), || { +/// tracing::event!(tracing::Level::INFO, "Hello"); +/// }); +/// # } +/// } +/// ``` +/// +/// This will result in a log file located at `/some/path/non-rotating.log`. +pub fn never(directory: impl AsRef, file_name: impl AsRef) -> RotatingFileAppender { + RotatingFileAppender::new(Rotation::never(), directory, file_name) +} + +/// Defines a fixed size and number of backups for rotating of a log file. +/// +/// To use a `Rotation`, pick one of the following options use the [`new`](struct.Rotation.html#method.new) function: +/// +/// ### MB 100 Rotation, with 9 backups +/// ```rust +/// # fn docs() { +/// use tracing_appender::rotating::Rotation; +/// let rotation = tracing_appender::rotating::Rotation::mb_100(); +/// # } +/// ``` +/// +/// ### No Rotation +/// ```rust +/// # fn docs() { +/// use tracing_appender::rotating::Rotation; +/// let rotation = tracing_appender::rotating::Rotation::never(); +/// # } +/// ``` +#[derive(Clone, Eq, PartialEq, Debug)] +pub struct Rotation { + max_bytes: usize, + max_backups: usize, + max_backups_digit: usize, +} + +impl Rotation { + /// Provides a 100 MB rotation with 9 max_backups + pub fn mb_100() -> Self { + Rotation::new(100_000_000, 9) + } + + /// Provides a rotation that never rotates. + pub fn never() -> Self { + Rotation::new(0, 0) + } +} +impl Rotation { + /// Creates a new `Rotation`. + /// + /// A `Rotation` will have a fixed number whose size is + /// defined by [`Rotation`](struct.Rotation.html). The `max_bytes` argument + /// define the number of bytes per log file. The `max_backups` argument define the number + /// of old log files allowed. + /// + /// Alternatively, a `Rotation` can be constructed using one of the following helpers: + /// + /// - [`Rotation::mb_100()`][mb_100] + /// - [`Rotation::never()`][never] + /// + /// [mb_100]: struct.Rotation.html#method.mb_100 + /// [never]: struct.Rotation.html#method.never + /// + /// # Examples + /// ```rust + /// # fn docs() { + /// use tracing_appender::rotating::Rotation; + /// let rotation = tracing_appender::rotating::Rotation::new(100_000_000,99); + /// # } + /// ``` + pub fn new(max_bytes: usize, max_backups: usize) -> Self { + Self { + max_backups_digit: max_backups.to_string().len(), + max_bytes, + max_backups, + } + } + pub(crate) fn should_rollover(&self, size: usize) -> bool { + size > self.max_bytes && self.max_bytes != 0 + } + pub(crate) fn is_create_backup(&self, last_backup: usize) -> bool { + last_backup < self.max_backups + } + pub(crate) fn join_backup(&self, filename: &str, backup_index: usize) -> String { + match backup_index { + 0 => filename.to_string(), + _ => format!("{}.{}", filename, self.backup_index_to_str(backup_index)), + } + } + fn backup_index_to_str(&self, backup_index: usize) -> String { + let backup_index_str = backup_index.to_string(); + let backup_index_len = backup_index_str.len(); + if backup_index_len < self.max_backups_digit { + std::iter::repeat("0") + .take(self.max_backups_digit - backup_index_len) + .collect::() + + &backup_index_str + } else { + backup_index_str + } + } +} +#[cfg(test)] +mod test { + use super::*; + use std::fs; + use std::io::Write; + use tempdir::TempDir; + + fn find_str_in_log(dir_path: &Path, expected_value: &str) -> bool { + let dir_contents = fs::read_dir(dir_path).expect("Failed to read directory"); + + for entry in dir_contents { + let path = entry.expect("Expected dir entry").path(); + let result = fs::read_to_string(path).expect("Failed to read file"); + + if result.as_str() == expected_value { + return true; + } + } + + false + } + + fn write_to_log(appender: &mut RotatingFileAppender, msg: &str) { + appender + .write_all(msg.as_bytes()) + .expect("Failed to write to appender"); + appender.flush().expect("Failed to flush!"); + } + + fn test_appender(rotation: Rotation, directory: TempDir, file_prefix: &str) { + let mut appender = RotatingFileAppender::new(rotation, directory.path(), file_prefix); + + let expected_value = "Hello"; + write_to_log(&mut appender, expected_value); + assert!(find_str_in_log(directory.path(), expected_value)); + + directory + .close() + .expect("Failed to explicitly close TempDir. TempDir should delete once out of scope.") + } + #[test] + fn rotating_write() { + let r = Rotation::new(10, 2); + let directory = TempDir::new("rotating").expect("Failed to create tempdir"); + let file_prefix = "rotating.log"; + let mut appender = RotatingFileAppender::new(r, directory.path(), file_prefix); + write_to_log(&mut appender, "1111"); + write_to_log(&mut appender, "2222"); + write_to_log(&mut appender, "3333"); + write_to_log(&mut appender, "4444"); + write_to_log(&mut appender, "5555"); + write_to_log(&mut appender, "6666"); + write_to_log(&mut appender, "7777"); + write_to_log(&mut appender, "8888"); + + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log")).unwrap(), + "77778888" + ); + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log.1")).unwrap(), + "55556666" + ); + + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log.2")).unwrap(), + "33334444" + ); + assert_eq!(directory.path().join("rotating.log.3").exists(), false); + } + #[test] + fn rotating_double_write() { + let directory = TempDir::new("rotating").expect("Failed to create tempdir"); + let file_prefix = "rotating.log"; + { + let r = Rotation::new(10, 2); + let mut appender = RotatingFileAppender::new(r, directory.path(), file_prefix); + write_to_log(&mut appender, "1111"); + write_to_log(&mut appender, "2222"); + write_to_log(&mut appender, "3333"); + } + let r = Rotation::new(10, 2); + let mut appender = RotatingFileAppender::new(r, directory.path(), file_prefix); + write_to_log(&mut appender, "4444"); + write_to_log(&mut appender, "5555"); + write_to_log(&mut appender, "6666"); + write_to_log(&mut appender, "7777"); + write_to_log(&mut appender, "8888"); + + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log")).unwrap(), + "77778888" + ); + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log.1")).unwrap(), + "55556666" + ); + + assert_eq!( + fs::read_to_string(directory.path().join("rotating.log.2")).unwrap(), + "33334444" + ); + assert_eq!(directory.path().join("rotating.log.3").exists(), false); + } + #[test] + fn write_mb_100_log() { + test_appender( + Rotation::mb_100(), + TempDir::new("mb_100").expect("Failed to create tempdir"), + "mb_100.log", + ); + } + + #[test] + fn write_never_log() { + test_appender( + Rotation::never(), + TempDir::new("never").expect("Failed to create tempdir"), + "never.log", + ); + } + + #[test] + fn test_should_rollover() { + let r = Rotation::new(200, 0); + assert_eq!(r.should_rollover(999), true); + assert_eq!(r.should_rollover(0), false); + assert_eq!(r.should_rollover(200), false); + assert_eq!(r.should_rollover(201), true); + + let r = Rotation::new(0, 0); + assert_eq!(r.should_rollover(99999), false); + } + + #[test] + fn test_is_create_backup() { + let r = Rotation::new(0, 999); + assert_eq!(r.is_create_backup(999), false); + assert_eq!(r.is_create_backup(0), true); + assert_eq!(r.is_create_backup(100), true); + + let r = Rotation::new(0, 0); + assert_eq!(r.is_create_backup(999), false); + } + #[test] + fn test_join_backup() { + let r = Rotation::new(0, 999); + let joined_date = r.join_backup("Hello.log", 0); + assert_eq!(joined_date, "Hello.log"); + let joined_date = r.join_backup("Hello.log", 1); + assert_eq!(joined_date, "Hello.log.001"); + let joined_date = r.join_backup("Hello.log", 22); + assert_eq!(joined_date, "Hello.log.022"); + let joined_date = r.join_backup("Hello.log", 333); + assert_eq!(joined_date, "Hello.log.333"); + let joined_date = r.join_backup("Hello.log", 4444); + assert_eq!(joined_date, "Hello.log.4444"); + let r = Rotation::new(0, 0); + let joined_date = r.join_backup("Hello.log", 22); + assert_eq!(joined_date, "Hello.log.22"); + } +} From 354731bb43b102de44387b46e7dd7046bfe421ba Mon Sep 17 00:00:00 2001 From: CfirTsabari Date: Thu, 6 Aug 2020 15:20:55 +0000 Subject: [PATCH 2/2] CR Fixes: Introduced InnerAppenderTrait. InnerAppenderWrapper will wrap structs that implement InnerAppenderTrait RotatingFileAppender and RollingFileAppender use this new wrapper. Added lines between functions. --- tracing-appender/src/inner.rs | 83 +++++++++++++++++++++++--------- tracing-appender/src/rolling.rs | 9 ++-- tracing-appender/src/rotating.rs | 14 ++++-- 3 files changed, 76 insertions(+), 30 deletions(-) diff --git a/tracing-appender/src/inner.rs b/tracing-appender/src/inner.rs index f05ace0c69..08c7e92b11 100644 --- a/tracing-appender/src/inner.rs +++ b/tracing-appender/src/inner.rs @@ -1,4 +1,4 @@ -use std::io::{BufWriter, Write}; +use std::io::BufWriter; use std::{fs, io}; use crate::rolling::Rotation as Roll; @@ -7,8 +7,49 @@ use crate::rotating::Rotation; use chrono::prelude::*; use std::fmt::Debug; use std::fs::{File, OpenOptions}; +use std::marker::PhantomData; use std::path::Path; +pub(crate) trait InnerAppenderTrait: io::Write +where + Self: Sized, +{ + fn new(rotation: R, log_directory: &Path, log_filename_prefix: &Path) -> io::Result; + + fn refresh_writer(&mut self, size: &usize); +} + +#[derive(Debug)] +pub(crate) struct InnerAppenderWrapper> { + appender: T, + phantom: PhantomData, +} +impl> InnerAppenderWrapper { + pub(crate) fn new( + rotation: R, + log_directory: &Path, + log_filename_prefix: &Path, + ) -> io::Result { + let appender = T::new(rotation, log_directory, log_filename_prefix)?; + Ok(Self { + appender, + phantom: PhantomData, + }) + } +} + +impl> io::Write for InnerAppenderWrapper { + fn write(&mut self, buf: &[u8]) -> io::Result { + let buf_len = buf.len(); + self.appender.refresh_writer(&buf_len); + self.appender.write(buf) + } + + fn flush(&mut self) -> io::Result<()> { + self.appender.flush() + } +} + #[derive(Debug)] pub(crate) struct InnerRollingAppender { log_directory: String, @@ -20,22 +61,17 @@ pub(crate) struct InnerRollingAppender { impl io::Write for InnerRollingAppender { fn write(&mut self, buf: &[u8]) -> io::Result { - let now = Utc::now(); - self.write_timestamped(buf, now) + let buf_len = buf.len(); + self.writer.write_all(buf).map(|_| buf_len) } fn flush(&mut self) -> io::Result<()> { self.writer.flush() } } - -impl InnerRollingAppender { - pub(crate) fn new( - log_directory: &Path, - log_filename_prefix: &Path, - roll: Roll, - now: DateTime, - ) -> io::Result { +impl InnerAppenderTrait for InnerRollingAppender { + fn new(roll: Roll, log_directory: &Path, log_filename_prefix: &Path) -> io::Result { + let now = Utc::now(); let log_directory = log_directory.to_str().unwrap(); let log_filename_prefix = log_filename_prefix.to_str().unwrap(); @@ -51,15 +87,9 @@ impl InnerRollingAppender { }) } - fn write_timestamped(&mut self, buf: &[u8], date: DateTime) -> io::Result { - // Even if refresh_writer fails, we still have the original writer. Ignore errors - // and proceed with the write. - let buf_len = buf.len(); - self.refresh_writer(date); - self.writer.write_all(buf).map(|_| buf_len) - } + fn refresh_writer(&mut self, _size: &usize) { + let now = Utc::now(); - fn refresh_writer(&mut self, now: DateTime) { if self.should_rollover(now) { let filename = self.roll.join_date(&self.log_filename_prefix, &now); @@ -71,7 +101,9 @@ impl InnerRollingAppender { } } } +} +impl InnerRollingAppender { fn should_rollover(&self, date: DateTime) -> bool { date >= self.next_date } @@ -90,7 +122,6 @@ pub(crate) struct InnerRotatingAppender { impl io::Write for InnerRotatingAppender { fn write(&mut self, buf: &[u8]) -> io::Result { let buf_len = buf.len(); - self.refresh_writer(&buf_len); self.writer.write_all(buf).map(|_| buf_len) } @@ -99,8 +130,8 @@ impl io::Write for InnerRotatingAppender { } } -impl InnerRotatingAppender { - pub(crate) fn new( +impl InnerAppenderTrait for InnerRotatingAppender { + fn new( rotation: Rotation, log_directory: &Path, log_filename_prefix: &Path, @@ -118,6 +149,7 @@ impl InnerRotatingAppender { rotation, }) } + fn refresh_writer(&mut self, size: &usize) { if self.rotation.should_rollover(self.current_size + size) { self.current_size = 0; @@ -132,6 +164,9 @@ impl InnerRotatingAppender { } self.current_size += size; } +} + +impl InnerRotatingAppender { fn rotate_files(&self) { for x in (1..=self.last_backup).rev() { let from = self.rotation.join_backup(&self.log_filename_prefix, x - 1); @@ -141,6 +176,7 @@ impl InnerRotatingAppender { } } } + fn find_last_backup( rotation: &Rotation, log_directory: &str, @@ -158,6 +194,7 @@ impl InnerRotatingAppender { last_backup } } + fn create_writer(directory: &str, filename: &str) -> io::Result> { let file_path = Path::new(directory).join(filename); Ok(BufWriter::new(open_file_create_parent_dirs(&file_path)?)) @@ -177,6 +214,7 @@ fn open_file_create_parent_dirs(path: &Path) -> io::Result { new_file } + fn get_file_size(directory: &str, filename: &str) -> io::Result { let file_path = Path::new(directory).join(filename); if file_path.exists() { @@ -185,6 +223,7 @@ fn get_file_size(directory: &str, filename: &str) -> io::Result { Ok(0) } } + fn file_exist(directory: &str, filename: &str) -> bool { let file_path = Path::new(directory).join(filename); file_path.as_path().exists() diff --git a/tracing-appender/src/rolling.rs b/tracing-appender/src/rolling.rs index ad0fb12172..f33e95cda1 100644 --- a/tracing-appender/src/rolling.rs +++ b/tracing-appender/src/rolling.rs @@ -30,7 +30,7 @@ //! let file_appender = RollingFileAppender::new(Rotation::HOURLY, "/some/directory", "prefix.log"); //! # } //! ``` -use crate::inner::InnerRollingAppender; +use crate::inner::{InnerAppenderWrapper, InnerRollingAppender}; use chrono::{DateTime, Datelike, TimeZone, Timelike, Utc}; use std::fmt::Debug; use std::io; @@ -54,7 +54,7 @@ use std::path::Path; /// ``` #[derive(Debug)] pub struct RollingFileAppender { - inner: InnerRollingAppender, + inner: InnerAppenderWrapper, } impl RollingFileAppender { @@ -91,11 +91,10 @@ impl RollingFileAppender { file_name_prefix: impl AsRef, ) -> RollingFileAppender { RollingFileAppender { - inner: InnerRollingAppender::new( + inner: InnerAppenderWrapper::new( + rotation, directory.as_ref(), file_name_prefix.as_ref(), - rotation, - Utc::now(), ) .expect("Failed to create appender"), } diff --git a/tracing-appender/src/rotating.rs b/tracing-appender/src/rotating.rs index ba4f800adb..74db23e940 100644 --- a/tracing-appender/src/rotating.rs +++ b/tracing-appender/src/rotating.rs @@ -24,7 +24,7 @@ //! let file_appender = RotatingFileAppender::new(Rotation::mb_100(), "/some/directory", "prefix.log"); //! # } //! ``` -use crate::inner::InnerRotatingAppender; +use crate::inner::{InnerAppenderWrapper, InnerRotatingAppender}; use std::io; use std::path::Path; @@ -46,7 +46,7 @@ use std::path::Path; /// ``` #[derive(Debug)] pub struct RotatingFileAppender { - inner: InnerRotatingAppender, + inner: InnerAppenderWrapper, } impl RotatingFileAppender { @@ -78,7 +78,7 @@ impl RotatingFileAppender { file_name_prefix: impl AsRef, ) -> RotatingFileAppender { RotatingFileAppender { - inner: InnerRotatingAppender::new( + inner: InnerAppenderWrapper::new( rotation, directory.as_ref(), file_name_prefix.as_ref(), @@ -231,18 +231,22 @@ impl Rotation { max_backups, } } + pub(crate) fn should_rollover(&self, size: usize) -> bool { size > self.max_bytes && self.max_bytes != 0 } + pub(crate) fn is_create_backup(&self, last_backup: usize) -> bool { last_backup < self.max_backups } + pub(crate) fn join_backup(&self, filename: &str, backup_index: usize) -> String { match backup_index { 0 => filename.to_string(), _ => format!("{}.{}", filename, self.backup_index_to_str(backup_index)), } } + fn backup_index_to_str(&self, backup_index: usize) -> String { let backup_index_str = backup_index.to_string(); let backup_index_len = backup_index_str.len(); @@ -296,6 +300,7 @@ mod test { .close() .expect("Failed to explicitly close TempDir. TempDir should delete once out of scope.") } + #[test] fn rotating_write() { let r = Rotation::new(10, 2); @@ -326,6 +331,7 @@ mod test { ); assert_eq!(directory.path().join("rotating.log.3").exists(), false); } + #[test] fn rotating_double_write() { let directory = TempDir::new("rotating").expect("Failed to create tempdir"); @@ -360,6 +366,7 @@ mod test { ); assert_eq!(directory.path().join("rotating.log.3").exists(), false); } + #[test] fn write_mb_100_log() { test_appender( @@ -400,6 +407,7 @@ mod test { let r = Rotation::new(0, 0); assert_eq!(r.is_create_backup(999), false); } + #[test] fn test_join_backup() { let r = Rotation::new(0, 999);