Skip to content

Commit

Permalink
appender: add a builder for constructing RollingFileAppenders (#2227)
Browse files Browse the repository at this point in the history 
## 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]>
  • Loading branch information
@hawkw
hawkw authored Jul 18, 2022
1 parent 968dd0d commit ab36c17
Show file tree
Hide file tree
Showing 3 changed files with 280 additions and 48 deletions.
1 change: 1 addition & 0 deletions tracing-appender/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
160 changes: 112 additions & 48 deletions tracing-appender/src/rolling.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
Expand All @@ -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};
Expand All @@ -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]
Expand Down Expand Up @@ -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,
}
}
}
Expand All @@ -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) => {
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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);
}

Expand All @@ -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 = {
Expand Down
Loading

0 comments on commit ab36c17

Please sign in to comment.