appender: add a builder for constructing RollingFileAppenders (#2227)

## Motivation

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.

## Solution

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

Signed-off-by: Eliza Weisman <[email protected]>

Author: hawkw

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"

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<String>,
     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,63 @@ impl RollingFileAppender {
     pub fn new(
         rotation: Rotation,
         directory: impl AsRef<Path>,
-        file_name_prefix: impl AsRef<Path>,
+        filename_prefix: impl AsRef<Path>,
     ) -> 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
+    /// # fn docs() {
+    /// use tracing_appender::rolling::{RollingFileAppender, Rotation};
+    ///
+    /// let file_appender = RollingFileAppender::builder()
+    ///     .rotation(Rotation::HOURLY) // rotate log files once every hour
+    ///     .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()
+    }
+
+    fn from_builder(builder: &Builder, directory: impl AsRef<Path>) -> Result<Self, InitError> {
+        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 +480,42 @@ 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 => {
                 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 => {
+                // 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)
+                    .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,32 +539,30 @@ impl Inner {
         now: OffsetDateTime,
         rotation: Rotation,
         directory: impl AsRef<Path>,
-        file_name_prefix: impl AsRef<Path>,
-    ) -> (Self, RwLock<File>) {
-        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<String>,
+    ) -> Result<(Self, RwLock<File>), 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)
                     .unwrap_or(0),
             ),
             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 +609,22 @@ impl Inner {
     }
 }
 
-fn create_writer(directory: &str, filename: &str) -> io::Result<File> {
-    let path = Path::new(directory).join(filename);
+fn create_writer(directory: &Path, filename: &str) -> Result<File, InitError> {
+    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 +732,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 +761,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 = {