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

Size Based Rotating File Appender #865

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
198 changes: 169 additions & 29 deletions tracing-appender/src/inner.rs
Original file line number Diff line number Diff line change
@@ -1,80 +1,200 @@
use std::io::{BufWriter, Write};
use std::io::BufWriter;
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::marker::PhantomData;
use std::path::Path;

pub(crate) trait InnerAppenderTrait<R>: io::Write
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Something better named would be nice as pub(crate) trait already signifies this is a trait. Something like AppendInnder?

Copy link
Author

Choose a reason for hiding this comment

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

will rename

where
Self: Sized,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe specifying this will prevent making the trait into an object

https://doc.rust-lang.org/std/marker/trait.Sized.html

I may be wrong though. I've never used Sized before.

Copy link
Author

Choose a reason for hiding this comment

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

i need it for Result

{
fn new(rotation: R, log_directory: &Path, log_filename_prefix: &Path) -> io::Result<Self>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tying the constructor to a trait method will make the code brittle.

The constructor should only be implemented by InnerAppenderWrapper. InnerAppenderTrait I think would be more appropriate for only specifying the contract for how logs are appended.

Copy link
Author

Choose a reason for hiding this comment

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

i have to have this function since InnerAppenderWrapper need it in order to create the appenders.


fn refresh_writer(&mut self, size: &usize);
}

#[derive(Debug)]
pub(crate) struct InnerAppenderWrapper<R, T: InnerAppenderTrait<R>> {
appender: T,
phantom: PhantomData<R>,
}
impl<R, T: InnerAppenderTrait<R>> InnerAppenderWrapper<R, T> {
Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

nit let's add a line break here.

Copy link
Author

Choose a reason for hiding this comment

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

will fix

pub(crate) fn new(
rotation: R,
log_directory: &Path,
log_filename_prefix: &Path,
) -> io::Result<Self> {
let appender = T::new(rotation, log_directory, log_filename_prefix)?;
Ok(Self {
appender,
phantom: PhantomData,
})
}
}

impl<R, T: InnerAppenderTrait<R>> io::Write for InnerAppenderWrapper<R, T> {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
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 InnerAppender {
pub(crate) struct InnerRollingAppender {
log_directory: String,
log_filename_prefix: String,
writer: BufWriter<File>,
next_date: DateTime<Utc>,
rotation: Rotation,
roll: Roll,
}

impl io::Write for InnerAppender {
impl io::Write for InnerRollingAppender {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
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 InnerAppender {
pub(crate) fn new(
log_directory: &Path,
log_filename_prefix: &Path,
rotation: Rotation,
now: DateTime<Utc>,
) -> io::Result<Self> {
impl InnerAppenderTrait<Roll> for InnerRollingAppender {
fn new(roll: Roll, log_directory: &Path, log_filename_prefix: &Path) -> io::Result<Self> {
let now = Utc::now();
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,
})
}

fn write_timestamped(&mut self, buf: &[u8], date: DateTime<Utc>) -> io::Result<usize> {
// 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<Utc>) {
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,
Err(err) => eprintln!("Couldn't create writer for logs: {}", err),
}
}
}
}

impl InnerRollingAppender {
fn should_rollover(&self, date: DateTime<Utc>) -> bool {
date >= self.next_date
}
}

#[derive(Debug)]
pub(crate) struct InnerRotatingAppender {
log_directory: String,
log_filename_prefix: String,
writer: BufWriter<File>,
last_backup: usize,
current_size: usize,
rotation: Rotation,
}

impl io::Write for InnerRotatingAppender {
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
let buf_len = buf.len();
self.writer.write_all(buf).map(|_| buf_len)
}

fn flush(&mut self) -> io::Result<()> {
self.writer.flush()
}
Comment on lines +122 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to implement io::Write again for each appender impl. If you add a default method record/write then you can just call that from the impl of io::Write for InnerAppenderWrapper and not have to duplicate code for every appender.

Copy link
Author

Choose a reason for hiding this comment

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

i will still have to create these methods for each appender, I can't create a default method that will access the appenders buffers.

}

impl InnerAppenderTrait<Rotation> for InnerRotatingAppender {
fn new(
rotation: Rotation,
log_directory: &Path,
log_filename_prefix: &Path,
) -> io::Result<Self> {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: A new line between methods could be added in a few places.

Copy link
Author

Choose a reason for hiding this comment

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

will fix this.

Copy link
Author

Choose a reason for hiding this comment

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

done

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;
}
}

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);
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<BufWriter<File>> {
let file_path = Path::new(directory).join(filename);
Ok(BufWriter::new(open_file_create_parent_dirs(&file_path)?))
Expand All @@ -94,3 +214,23 @@ fn open_file_create_parent_dirs(path: &Path) -> io::Result<File> {

new_file
}

fn get_file_size(directory: &str, filename: &str) -> io::Result<usize> {
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)
}
27 changes: 25 additions & 2 deletions tracing-appender/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// TODO (cfirt): add rotating (01/08/2020)
//! Writers for logging events and spans
//!
//! # Overview
Expand All @@ -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
Expand All @@ -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()`,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
17 changes: 8 additions & 9 deletions tracing-appender/src/rolling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -30,7 +30,7 @@
//! let file_appender = RollingFileAppender::new(Rotation::HOURLY, "/some/directory", "prefix.log");
//! # }
//! ```
use crate::inner::InnerAppender;
use crate::inner::{InnerAppenderWrapper, InnerRollingAppender};
use chrono::{DateTime, Datelike, TimeZone, Timelike, Utc};
use std::fmt::Debug;
use std::io;
Expand All @@ -54,7 +54,7 @@ use std::path::Path;
/// ```
#[derive(Debug)]
pub struct RollingFileAppender {
inner: InnerAppender,
inner: InnerAppenderWrapper<Rotation, InnerRollingAppender>,
}

impl RollingFileAppender {
Expand Down Expand Up @@ -91,11 +91,10 @@ impl RollingFileAppender {
file_name_prefix: impl AsRef<Path>,
) -> RollingFileAppender {
RollingFileAppender {
inner: InnerAppender::new(
inner: InnerAppenderWrapper::new(
rotation,
directory.as_ref(),
file_name_prefix.as_ref(),
rotation,
Utc::now(),
)
.expect("Failed to create appender"),
}
Expand Down
Loading