From c5af7cc5bb28e4c5c55f54c0cacb4d4bc7b5d16e Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 10:21:33 -0700 Subject: [PATCH 1/6] appender: add a builder for constructing `RollingFileAppender`s Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953 --- tracing-appender/Cargo.toml | 1 + tracing-appender/src/rolling.rs | 145 ++++++++++++++-------- tracing-appender/src/rolling/builder.rs | 156 ++++++++++++++++++++++++ 3 files changed, 253 insertions(+), 49 deletions(-) create mode 100644 tracing-appender/src/rolling/builder.rs diff --git a/tracing-appender/Cargo.toml b/tracing-appender/Cargo.toml index 920629f475..3551dba08b 100644 --- a/tracing-appender/Cargo.toml +++ b/tracing-appender/Cargo.toml @@ -24,6 +24,7 @@ rust-version = "1.53.0" crossbeam-channel = "0.5.0" time = { version = "0.3", default-features = false, features = ["formatting"] } parking_lot = { optional = true, version = "0.12.0" } +thiserror = "1" [dependencies.tracing-subscriber] path = "../tracing-subscriber" diff --git a/tracing-appender/src/rolling.rs b/tracing-appender/src/rolling.rs index 3ff534d535..3a8b2420bc 100644 --- a/tracing-appender/src/rolling.rs +++ b/tracing-appender/src/rolling.rs @@ -31,11 +31,14 @@ use std::{ fmt::{self, Debug}, fs::{self, File, OpenOptions}, io::{self, Write}, - path::Path, + path::{Path, PathBuf}, sync::atomic::{AtomicUsize, Ordering}, }; use time::{format_description, Duration, OffsetDateTime, Time}; +mod builder; +pub use builder::{Builder, InitError}; + /// A file appender with the ability to rotate log files at a fixed schedule. /// /// `RollingFileAppender` implements the [`std:io::Write` trait][write] and will @@ -98,8 +101,8 @@ pub struct RollingWriter<'a>(RwLockReadGuard<'a, File>); #[derive(Debug)] struct Inner { - log_directory: String, - log_filename_prefix: String, + log_directory: PathBuf, + log_filename_prefix: Option, rotation: Rotation, next_date: AtomicUsize, } @@ -122,8 +125,10 @@ impl RollingFileAppender { /// - [`Rotation::daily()`][daily], /// - [`Rotation::never()`][never()] /// + /// Additional parameters can be configured using [`RollingFileAppender::builder`]. /// /// # Examples + /// /// ```rust /// # fn docs() { /// use tracing_appender::rolling::{RollingFileAppender, Rotation}; @@ -133,16 +138,58 @@ impl RollingFileAppender { pub fn new( rotation: Rotation, directory: impl AsRef, - file_name_prefix: impl AsRef, + filename_prefix: impl AsRef, ) -> RollingFileAppender { + let filename_prefix = filename_prefix + .as_ref() + .to_str() + .expect("filename prefix must be a valid UTF-8 string"); + Self::builder() + .rotation(rotation) + .filename_prefix(filename_prefix) + .build(directory) + .expect("initializing rolling file appender failed") + } + + /// Returns a new [`Builder`] for configuring a `RollingFileAppender`. + /// + /// The builder interface can be used to set additional configuration + /// parameters when constructing a new appender. + /// + /// Unlike [`RollingFileAppender::new`], the [`Builder::build`] method + /// returns a `Result` rather than panicking when the appender cannot be + /// initialized. Therefore, the builder interface can also be used when + /// appender initialization errors should be handled gracefully. + /// + /// # Examples + /// + /// ```rust + /// use tracing_appender::rolling::{RollingFileAppender, Rotation}; + /// + /// let file_appender = RollingFileAppender::builder() + /// .rotation(Rotation::HOURLY) // rotate log files once every hour + /// .directory("/some/directory") // log files will be stored in this directory. + /// . + #[must_use] + pub fn builder() -> Builder { + Builder::new() + } + + fn from_builder(builder: &Builder, directory: impl AsRef) -> Result { + let Builder { + ref rotation, + ref prefix, + } = builder; + let filename_prefix = prefix.clone(); + let directory = directory.as_ref().to_path_buf(); let now = OffsetDateTime::now_utc(); - let (state, writer) = Inner::new(now, rotation, directory, file_name_prefix); - Self { + let (state, writer) = Inner::new(now, rotation.clone(), directory, filename_prefix)?; + Ok(Self { state, writer, #[cfg(test)] now: Box::new(OffsetDateTime::now_utc), - } + }) } #[inline] @@ -428,35 +475,30 @@ impl Rotation { } } - pub(crate) fn join_date(&self, filename: &str, date: &OffsetDateTime) -> String { - match *self { + pub(crate) fn join_date(&self, filename: Option<&str>, date: &OffsetDateTime) -> String { + let date = match *self { Rotation::MINUTELY => { let format = format_description::parse("[year]-[month]-[day]-[hour]-[minute]") .expect("Unable to create a formatter; this is a bug in tracing-appender"); - - let date = date - .format(&format) - .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender"); - format!("{}.{}", filename, date) + date.format(&format) + .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender") } Rotation::HOURLY => { let format = format_description::parse("[year]-[month]-[day]-[hour]") .expect("Unable to create a formatter; this is a bug in tracing-appender"); - - let date = date - .format(&format) - .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender"); - format!("{}.{}", filename, date) + date.format(&format) + .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender") } - Rotation::DAILY => { + Rotation::DAILY | Rotation::NEVER => { let format = format_description::parse("[year]-[month]-[day]") .expect("Unable to create a formatter; this is a bug in tracing-appender"); - let date = date - .format(&format) - .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender"); - format!("{}.{}", filename, date) + date.format(&format) + .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender") } - Rotation::NEVER => filename.to_string(), + }; + match filename { + Some(filename) => format!("{}.{}", filename, date), + None => date, } } } @@ -480,20 +522,16 @@ impl Inner { now: OffsetDateTime, rotation: Rotation, directory: impl AsRef, - file_name_prefix: impl AsRef, - ) -> (Self, RwLock) { - let log_directory = directory.as_ref().to_str().unwrap(); - let log_filename_prefix = file_name_prefix.as_ref().to_str().unwrap(); - - let filename = rotation.join_date(log_filename_prefix, &now); + log_filename_prefix: Option, + ) -> Result<(Self, RwLock), builder::InitError> { + let log_directory = directory.as_ref().to_path_buf(); + let filename = rotation.join_date(log_filename_prefix.as_deref(), &now); let next_date = rotation.next_date(&now); - let writer = RwLock::new( - create_writer(log_directory, &filename).expect("failed to create appender"), - ); + let writer = RwLock::new(create_writer(log_directory.as_ref(), &filename)?); let inner = Inner { - log_directory: log_directory.to_string(), - log_filename_prefix: log_filename_prefix.to_string(), + log_directory, + log_filename_prefix, next_date: AtomicUsize::new( next_date .map(|date| date.unix_timestamp() as usize) @@ -501,11 +539,13 @@ impl Inner { ), rotation, }; - (inner, writer) + Ok((inner, writer)) } fn refresh_writer(&self, now: OffsetDateTime, file: &mut File) { - let filename = self.rotation.join_date(&self.log_filename_prefix, &now); + let filename = self + .rotation + .join_date(self.log_filename_prefix.as_deref(), &now); match create_writer(&self.log_directory, &filename) { Ok(new_file) => { @@ -552,20 +592,22 @@ impl Inner { } } -fn create_writer(directory: &str, filename: &str) -> io::Result { - let path = Path::new(directory).join(filename); +fn create_writer(directory: &Path, filename: &str) -> Result { + let path = directory.join(filename); let mut open_options = OpenOptions::new(); open_options.append(true).create(true); let new_file = open_options.open(path.as_path()); if new_file.is_err() { if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - return open_options.open(path); + fs::create_dir_all(parent).map_err(InitError::ctx("failed to create log directory"))?; + return open_options + .open(path) + .map_err(InitError::ctx("failed to create initial log file")); } } - new_file + new_file.map_err(InitError::ctx("failed to create initial log file")) } #[cfg(test)] @@ -673,19 +715,19 @@ mod test { let now = OffsetDateTime::parse("2020-02-01 10:01:00 +00:00:00", &format).unwrap(); // per-minute - let path = Rotation::MINUTELY.join_date("app.log", &now); + let path = Rotation::MINUTELY.join_date(Some("app.log"), &now); assert_eq!("app.log.2020-02-01-10-01", path); // per-hour - let path = Rotation::HOURLY.join_date("app.log", &now); + let path = Rotation::HOURLY.join_date(Some("app.log"), &now); assert_eq!("app.log.2020-02-01-10", path); // per-day - let path = Rotation::DAILY.join_date("app.log", &now); + let path = Rotation::DAILY.join_date(Some("app.log"), &now); assert_eq!("app.log.2020-02-01", path); // never - let path = Rotation::NEVER.join_date("app.log", &now); + let path = Rotation::NEVER.join_date(Some("app.log"), &now); assert_eq!("app.log", path); } @@ -702,8 +744,13 @@ mod test { let now = OffsetDateTime::parse("2020-02-01 10:01:00 +00:00:00", &format).unwrap(); let directory = tempfile::tempdir().expect("failed to create tempdir"); - let (state, writer) = - Inner::new(now, Rotation::HOURLY, directory.path(), "test_make_writer"); + let (state, writer) = Inner::new( + now, + Rotation::HOURLY, + directory.path(), + Some("test_make_writer".to_string()), + ) + .unwrap(); let clock = Arc::new(Mutex::new(now)); let now = { diff --git a/tracing-appender/src/rolling/builder.rs b/tracing-appender/src/rolling/builder.rs new file mode 100644 index 0000000000..c237fb5c2f --- /dev/null +++ b/tracing-appender/src/rolling/builder.rs @@ -0,0 +1,156 @@ +use super::{RollingFileAppender, Rotation}; +use std::{io, path::Path}; +use thiserror::Error; + +/// A [builder] for configuring [`RollingFileAppender`]s. +/// +/// [builder]: https://rust-unofficial.github.io/patterns/patterns/creational/builder.html +#[derive(Debug)] +pub struct Builder { + pub(super) rotation: Rotation, + pub(super) prefix: Option, +} + +/// Errors returned by [`Builder::build`]. +#[derive(Error, Debug)] +#[error("{context}: {source}")] +pub struct InitError { + context: &'static str, + #[source] + source: io::Error, +} + +impl InitError { + pub(crate) fn ctx(context: &'static str) -> impl FnOnce(io::Error) -> Self { + move |source| Self { context, source } + } +} + +impl Builder { + /// Returns a new `Builder` for configuring a [`RollingFileAppender`], with + /// the default parameters. + /// + /// # 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 + /// [`directory`]: Self::directory + #[must_use] + pub const fn new() -> Self { + Self { + rotation: Rotation::NEVER, + prefix: None, + } + } + + /// Sets the [rotation strategy] for log files. + /// + /// By default, this is [`Rotation::NEVER`]. + /// + /// # Examples + /// + /// ``` + /// use tracing_appender::rolling::{Rotation, RollingFileAppender}; + /// + /// let appender = RollingFileAppender::builder() + /// .rotation(Rotation::HOURLY) // rotate log files once every hour + /// .build("myapp.log") + /// .expect("failed to initialize rolling file appender"); + /// + /// # drop(appender) + /// ``` + /// + /// [rotation strategy]: Rotation + #[must_use] + pub fn rotation(self, rotation: Rotation) -> Self { + Self { rotation, ..self } + } + + /// Sets the prefix for log filenames. The prefix is output before the + /// timestamp in the file name, and if it is non-empty, it is followed by a + /// dot (`.`). + /// + /// By default, log files do not have a prefix. + /// + /// # Examples + /// + /// Setting a prefix: + /// + /// ``` + /// use tracing_appender::rolling::RollingFileAppender; + /// + /// let appender = RollingFileAppender::builder() + /// .prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" + /// // ... + /// .build("/var/log") + /// .expect("failed to initialize rolling file appender"); + /// # drop(appender) + /// ``` + /// + /// No prefix: + /// + /// ``` + /// use tracing_appender::rolling::RollingFileAppender; + /// + /// let appender = RollingFileAppender::builder() + /// .prefix("") // log files will have names like "2019-01-01" + /// // ... + /// .build("/var/log") + /// .expect("failed to initialize rolling file appender"); + /// # drop(appender) + /// ``` + /// + /// [rotation strategy]: Rotation + #[must_use] + pub fn filename_prefix(self, prefix: impl Into) -> Self { + let prefix = prefix.into(); + // If the configured prefix is the empty string, then don't include a + // separator character. + let prefix = if prefix.is_empty() { + None + } else { + Some(prefix) + }; + Self { prefix, ..self } + } + + /// Builds a new [`RollingFileAppender`] with the configured parameters, + /// emitting log files to the provided directory. + /// + /// Unlike [`RollingFileAppender::new`], this returns a `Result` rather than + /// panicking when the appender cannot be initialized. + /// + /// # Examples + /// + /// ``` + /// use tracing_appender::rolling::{Rotation, RollingFileAppender}; + /// + /// let appender = RollingFileAppender::builder() + /// .rotation(Rotation::DAILY) // rotate log files once per day + /// .prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" + /// .build("/var/log/myapp") // write log files to the '/var/log/myapp' directory + /// .expect("failed to initialize rolling file appender"); + /// # drop(appender); + /// ``` + /// + /// This is equivalent to + /// ``` + /// let appender = tracing_appender::rolling::daily("myapp.log", "/var/log/myapp"); + /// # drop(appender); + /// ``` + pub fn build(&self, directory: impl AsRef) -> Result { + RollingFileAppender::from_builder(self, directory) + } +} + +impl Default for Builder { + fn default() -> Self { + Self::new() + } +} From 1293bc111cbe67e8ad0557ac5c95b30523ef88dd Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 10:29:54 -0700 Subject: [PATCH 2/6] fix docs link Signed-off-by: Eliza Weisman --- tracing-appender/src/rolling/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing-appender/src/rolling/builder.rs b/tracing-appender/src/rolling/builder.rs index c237fb5c2f..d0e2c65352 100644 --- a/tracing-appender/src/rolling/builder.rs +++ b/tracing-appender/src/rolling/builder.rs @@ -40,7 +40,7 @@ impl Builder { /// | [`filename_prefix`] | `""` | By default, log file names will not have a prefix. | /// /// [`rotation`]: Self::rotation - /// [`directory`]: Self::directory + /// [`filename_prefix`]: Self::filename_prefix #[must_use] pub const fn new() -> Self { Self { From a9bab799793fd0d1e6d4208946dbc04987977434 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 10:40:53 -0700 Subject: [PATCH 3/6] only use date for NEVER rotation if there's no pfx Signed-off-by: Eliza Weisman --- tracing-appender/src/rolling.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tracing-appender/src/rolling.rs b/tracing-appender/src/rolling.rs index 3a8b2420bc..3a12058cc0 100644 --- a/tracing-appender/src/rolling.rs +++ b/tracing-appender/src/rolling.rs @@ -489,7 +489,19 @@ impl Rotation { date.format(&format) .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender") } - Rotation::DAILY | Rotation::NEVER => { + Rotation::DAILY => { + let format = format_description::parse("[year]-[month]-[day]") + .expect("Unable to create a formatter; this is a bug in tracing-appender"); + date.format(&format) + .expect("Unable to format OffsetDateTime; this is a bug in tracing-appender") + } + Rotation::NEVER => { + // If there's a name prefix, use that. + if let Some(filename) = filename { + return filename.to_owned(); + } + + // Otherwise, just use the date. let format = format_description::parse("[year]-[month]-[day]") .expect("Unable to create a formatter; this is a bug in tracing-appender"); date.format(&format) From 0fd4032eee14d2a494c77fd94ac23c34e8861268 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 11:02:58 -0700 Subject: [PATCH 4/6] fix wrong method name in examples lol. lmao. Signed-off-by: Eliza Weisman --- tracing-appender/src/rolling/builder.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tracing-appender/src/rolling/builder.rs b/tracing-appender/src/rolling/builder.rs index d0e2c65352..8e6a6eb605 100644 --- a/tracing-appender/src/rolling/builder.rs +++ b/tracing-appender/src/rolling/builder.rs @@ -60,6 +60,7 @@ impl Builder { /// /// let appender = RollingFileAppender::builder() /// .rotation(Rotation::HOURLY) // rotate log files once every hour + /// // ... /// .build("myapp.log") /// .expect("failed to initialize rolling file appender"); /// @@ -86,7 +87,7 @@ impl Builder { /// use tracing_appender::rolling::RollingFileAppender; /// /// let appender = RollingFileAppender::builder() - /// .prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" + /// .filename_prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" /// // ... /// .build("/var/log") /// .expect("failed to initialize rolling file appender"); @@ -99,7 +100,7 @@ impl Builder { /// use tracing_appender::rolling::RollingFileAppender; /// /// let appender = RollingFileAppender::builder() - /// .prefix("") // log files will have names like "2019-01-01" + /// .filename_prefix("") // log files will have names like "2019-01-01" /// // ... /// .build("/var/log") /// .expect("failed to initialize rolling file appender"); From 5c21389ef8a65196bea2d56089c78936b679144a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 11:26:11 -0700 Subject: [PATCH 5/6] don't try to write to /var/log in doctests Signed-off-by: Eliza Weisman --- tracing-appender/src/rolling.rs | 9 +++++++-- tracing-appender/src/rolling/builder.rs | 12 +++++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/tracing-appender/src/rolling.rs b/tracing-appender/src/rolling.rs index 3a12058cc0..5c3aad0345 100644 --- a/tracing-appender/src/rolling.rs +++ b/tracing-appender/src/rolling.rs @@ -164,12 +164,17 @@ impl RollingFileAppender { /// # Examples /// /// ```rust + /// # fn docs() { /// use tracing_appender::rolling::{RollingFileAppender, Rotation}; /// /// let file_appender = RollingFileAppender::builder() /// .rotation(Rotation::HOURLY) // rotate log files once every hour - /// .directory("/some/directory") // log files will be stored in this directory. - /// . + /// .filename_prefix("myapp") // log file names will be prefixed with `myapp.` + /// .build("/var/log") // try to build an appender that stores log files in `/var/log` + /// .expect("initializing rolling file appender failed"); + /// # drop(file_appender); + /// # } + /// ``` #[must_use] pub fn builder() -> Builder { Builder::new() diff --git a/tracing-appender/src/rolling/builder.rs b/tracing-appender/src/rolling/builder.rs index 8e6a6eb605..c9574bf05a 100644 --- a/tracing-appender/src/rolling/builder.rs +++ b/tracing-appender/src/rolling/builder.rs @@ -56,15 +56,17 @@ impl Builder { /// # Examples /// /// ``` + /// # fn docs() { /// use tracing_appender::rolling::{Rotation, RollingFileAppender}; /// /// let appender = RollingFileAppender::builder() /// .rotation(Rotation::HOURLY) // rotate log files once every hour /// // ... - /// .build("myapp.log") + /// .build("/var/log") /// .expect("failed to initialize rolling file appender"); /// /// # drop(appender) + /// # } /// ``` /// /// [rotation strategy]: Rotation @@ -86,12 +88,14 @@ impl Builder { /// ``` /// use tracing_appender::rolling::RollingFileAppender; /// + /// # fn docs() { /// let appender = RollingFileAppender::builder() /// .filename_prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" /// // ... /// .build("/var/log") /// .expect("failed to initialize rolling file appender"); /// # drop(appender) + /// # } /// ``` /// /// No prefix: @@ -99,12 +103,14 @@ impl Builder { /// ``` /// use tracing_appender::rolling::RollingFileAppender; /// + /// # fn docs() { /// let appender = RollingFileAppender::builder() /// .filename_prefix("") // log files will have names like "2019-01-01" /// // ... /// .build("/var/log") /// .expect("failed to initialize rolling file appender"); /// # drop(appender) + /// # } /// ``` /// /// [rotation strategy]: Rotation @@ -132,18 +138,22 @@ impl Builder { /// ``` /// use tracing_appender::rolling::{Rotation, RollingFileAppender}; /// + /// # fn docs() { /// let appender = RollingFileAppender::builder() /// .rotation(Rotation::DAILY) // rotate log files once per day /// .prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" /// .build("/var/log/myapp") // write log files to the '/var/log/myapp' directory /// .expect("failed to initialize rolling file appender"); /// # drop(appender); + /// # } /// ``` /// /// This is equivalent to /// ``` + /// # fn docs() { /// let appender = tracing_appender::rolling::daily("myapp.log", "/var/log/myapp"); /// # drop(appender); + /// # } /// ``` pub fn build(&self, directory: impl AsRef) -> Result { RollingFileAppender::from_builder(self, directory) From 24a4ee9a7d667c13758f5653b18991fe8d212842 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 18 Jul 2022 12:10:09 -0700 Subject: [PATCH 6/6] agh i missed one Signed-off-by: Eliza Weisman --- tracing-appender/src/rolling/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tracing-appender/src/rolling/builder.rs b/tracing-appender/src/rolling/builder.rs index c9574bf05a..82161f0cc6 100644 --- a/tracing-appender/src/rolling/builder.rs +++ b/tracing-appender/src/rolling/builder.rs @@ -141,7 +141,7 @@ impl Builder { /// # fn docs() { /// let appender = RollingFileAppender::builder() /// .rotation(Rotation::DAILY) // rotate log files once per day - /// .prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" + /// .filename_prefix("myapp.log") // log files will have names like "myapp.log.2019-01-01" /// .build("/var/log/myapp") // write log files to the '/var/log/myapp' directory /// .expect("failed to initialize rolling file appender"); /// # drop(appender);