Review fixes - to be continued

Author: pjankiewicz

tracing-appender/Cargo.toml

@@ -20,6 +20,9 @@ keywords = ["logging", "tracing", "file-appender", "non-blocking-writer"]
 edition = "2018"
 rust-version = "1.51.0"
 
+[features]
+compression = ["flate2"]
+
 [dependencies]
 crossbeam-channel = "0.5.0"
 time = { version = "0.3", default-features = false, features = ["formatting"] }
@@ -36,6 +39,3 @@ features = ["fmt", "std"]
 tracing = { path = "../tracing", version = "0.2" }
 time = { version = "0.3", default-features = false, features = ["formatting", "parsing"] }
 tempfile = "3"
-
-[features]
-compression = ["flate2"]

tracing-appender/src/builder.rs

@@ -1,3 +1,4 @@
+use std::path::Path;
 use crate::rolling::{create_writer_file, Inner, RollingFileAppender, Rotation};
 use crate::sync::RwLock;
 use std::sync::atomic::AtomicUsize;
@@ -8,69 +9,53 @@ use crate::compression::CompressionConfig;
 
 use crate::writer::WriterChannel;
 
+
+#[derive(Debug)]
 pub struct RollingFileAppenderBuilder {
-    log_directory: Option<String>,
-    log_filename_prefix: Option<String>,
+    log_directory: String,
+    log_filename_prefix: String,
     rotation: Option<Rotation>,
-    next_date: Option<AtomicUsize>,
     #[cfg(feature = "compression")]
     compression: Option<CompressionConfig>,
 }
 
 impl RollingFileAppenderBuilder {
-    pub fn new() -> Self {
+    /// Creates an instance of RollingFileAppnderBuilder
+    pub fn new(log_directory: impl AsRef<Path>,
+               log_filename_prefix: impl AsRef<Path>) -> Self {
+        let log_directory = log_directory.as_ref().to_str().expect("Cannot convert log_directory Path to str").to_string();
+        let log_filename_prefix = log_filename_prefix.as_ref().to_str().expect("Cannot convert log_filename_prefix Path to str").to_string();
         RollingFileAppenderBuilder {
-            log_directory: None,
-            log_filename_prefix: None,
+            log_directory,
+            log_filename_prefix,
             rotation: None,
-            next_date: None,
             #[cfg(feature = "compression")]
+            #[cfg_attr(docsrs, doc(cfg(feature = "compression")))]
             compression: None,
         }
     }
 
-    pub fn log_directory(mut self, log_directory: String) -> Self {
-        self.log_directory = Some(log_directory);
-        self
-    }
-
-    pub fn log_filename_prefix(mut self, log_filename_prefix: String) -> Self {
-        self.log_filename_prefix = Some(log_filename_prefix);
-        self
-    }
-
+    /// Sets Rotation
     pub fn rotation(mut self, rotation: Rotation) -> Self {
         self.rotation = Some(rotation);
         self
     }
 
-    pub fn next_date(mut self, next_date: AtomicUsize) -> Self {
-        self.next_date = Some(next_date);
-        self
-    }
-
     #[cfg(feature = "compression")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "compression")))]
     pub fn compression(mut self, compression: CompressionConfig) -> Self {
         self.compression = Some(compression);
         self
     }
 
     pub fn build(self) -> RollingFileAppender {
         let now = OffsetDateTime::now_utc();
-        let log_directory = self
-            .log_directory
-            .expect("log_directory is required to build RollingFileAppender");
-        let log_filename_prefix = self
-            .log_filename_prefix
-            .expect("log_filename_prefix is required to build RollingFileAppender");
-        let rotation = self
-            .rotation
-            .expect("rotation is required to build RollingFileAppender");
-
-        let filename = rotation.join_date(log_filename_prefix.as_str(), &now, false);
+        let rotation = self.rotation.unwrap_or(Rotation::NEVER);
+        let filename = rotation.join_date(self.log_filename_prefix.as_str(), &now, false);
         let next_date = rotation.next_date(&now);
+
         let writer = RwLock::new(WriterChannel::File(
-            create_writer_file(log_directory.as_str(), &filename)
+            create_writer_file(self.log_directory.as_str(), &filename)
                 .expect("failed to create appender"),
         ));
 
@@ -82,10 +67,10 @@ impl RollingFileAppenderBuilder {
 
         RollingFileAppender {
             state: Inner {
-                log_directory,
-                log_filename_prefix,
+                log_directory: self.log_directory,
+                log_filename_prefix: self.log_filename_prefix,
                 next_date,
-                rotation,
+                rotation: rotation,
                 #[cfg(feature = "compression")]
                 compression: self.compression,
             },

tracing-appender/src/compression.rs

@@ -1,11 +1,13 @@
 use flate2::Compression;
 
+#[derive(Debug, Clone)]
 pub enum GzipCompressionLevelLiteral {
     None,
     Fast,
     Best,
 }
 
+#[derive(Debug, Clone)]
 pub enum GzipCompressionLevelNumerical {
     Level0,
     Level1,
@@ -19,44 +21,58 @@ pub enum GzipCompressionLevelNumerical {
     Level9,
 }
 
+#[derive(Debug, Clone)]
 pub enum GzipCompressionLevel {
     Literal(GzipCompressionLevelLiteral),
     Numerical(GzipCompressionLevelNumerical),
 }
 
 impl Into<Compression> for GzipCompressionLevel {
     fn into(self) -> Compression {
-        match GzipCompressionLevel {
+        match self {
             GzipCompressionLevel::Literal(lit) => match lit {
                 GzipCompressionLevelLiteral::None => Compression::none(),
                 GzipCompressionLevelLiteral::Fast => Compression::fast(),
                 GzipCompressionLevelLiteral::Best => Compression::best(),
             },
             GzipCompressionLevel::Numerical(num) => match num {
-                GzipCompressionLevelNumerical::Level0 => Compression(0),
-                GzipCompressionLevelNumerical::Level1 => Compression(1),
-                GzipCompressionLevelNumerical::Level2 => Compression(2),
-                GzipCompressionLevelNumerical::Level3 => Compression(3),
-                GzipCompressionLevelNumerical::Level4 => Compression(4),
-                GzipCompressionLevelNumerical::Level5 => Compression(5),
-                GzipCompressionLevelNumerical::Level6 => Compression(6),
-                GzipCompressionLevelNumerical::Level7 => Compression(7),
-                GzipCompressionLevelNumerical::Level8 => Compression(8),
-                GzipCompressionLevelNumerical::Level9 => Compression(9),
+                GzipCompressionLevelNumerical::Level0 => Compression::new(0),
+                GzipCompressionLevelNumerical::Level1 => Compression::new(1),
+                GzipCompressionLevelNumerical::Level2 => Compression::new(2),
+                GzipCompressionLevelNumerical::Level3 => Compression::new(3),
+                GzipCompressionLevelNumerical::Level4 => Compression::new(4),
+                GzipCompressionLevelNumerical::Level5 => Compression::new(5),
+                GzipCompressionLevelNumerical::Level6 => Compression::new(6),
+                GzipCompressionLevelNumerical::Level7 => Compression::new(7),
+                GzipCompressionLevelNumerical::Level8 => Compression::new(8),
+                GzipCompressionLevelNumerical::Level9 => Compression::new(9),
             },
         }
     }
 }
 
+#[derive(Debug, Clone)]
 pub struct GzipCompression {
     pub level: GzipCompressionLevel,
 }
 
 /// Data structure to pass compression parameters
+#[derive(Debug, Clone)]
 pub enum CompressionConfig {
     Gzip(GzipCompression),
 }
 
+impl CompressionConfig {
+    pub(crate) fn gz_compress_level(&self) -> Compression {
+        match self {
+            CompressionConfig::Gzip(gz) => {
+                let level = gz.level.into();
+                level
+            }
+        }
+    }
+}
+
 mod compression_options {
     use super::*;
     pub const GZIP_NONE: CompressionConfig = CompressionConfig::Gzip(GzipCompression {

tracing-appender/src/lib.rs

@@ -166,7 +166,7 @@ mod writer;
 #[cfg(feature = "compression")]
 mod compression;
 
-mod builder;
+pub mod builder;
 
 /// Convenience function for creating a non-blocking, off-thread writer.
 ///

tracing-appender/src/rolling.rs

@@ -39,6 +39,7 @@ use std::{
     sync::atomic::{AtomicUsize, Ordering},
 };
 use time::{format_description, Duration, OffsetDateTime, Time};
+use tracing_subscriber::fmt::format::Writer;
 
 /// A file appender with the ability to rotate log files at a fixed schedule.
 ///
@@ -88,6 +89,8 @@ use time::{format_description, Duration, OffsetDateTime, Time};
 pub struct RollingFileAppender {
     pub(crate) state: Inner,
     pub(crate) writer: RwLock<WriterChannel>,
+    #[cfg(features = "compression")]
+    pub(crate) compression: Option<CompressionConfig>,
 }
 
 /// A [writer] that writes to a rolling log file.
@@ -140,10 +143,8 @@ impl RollingFileAppender {
         directory: impl AsRef<Path>,
         file_name_prefix: impl AsRef<Path>,
     ) -> RollingFileAppender {
-        RollingFileAppenderBuilder::new()
+        RollingFileAppenderBuilder::new(directory, file_name_prefix)
             .rotation(rotation)
-            .log_directory(directory.as_ref().to_str().unwrap().to_string())
-            .log_filename_prefix(file_name_prefix.as_ref().to_str().unwrap().to_string())
             .build()
     }
 }
@@ -478,20 +479,18 @@ impl io::Write for RollingWriter<'_> {
 
 impl Inner {
     #[cfg(feature = "compression")]
-    fn refresh_writer(
-        &self,
-        now: OffsetDateTime,
-        file: &mut WriterChannel,
-        compression: CompressionConfig,
-    ) {
+    fn refresh_writer(&self, now: OffsetDateTime, file: &mut WriterChannel) {
         debug_assert!(self.should_rollover(now));
 
         let filename = self
             .rotation
             .join_date(&self.log_filename_prefix, &now, false);
 
-        let writer =
-            WriterChannel::new_with_compression(&self.log_directory, &filename, compression);
+        let writer = if let Some(compression) = self.compression.clone() {
+            WriterChannel::new_with_compression(&self.log_directory, &filename, compression)
+        } else {
+            WriterChannel::new_without_compression(&self.log_directory, &filename)
+        };
 
         Self::refresh_writer_channel(file, writer);
     }
@@ -547,7 +546,7 @@ impl Inner {
     }
 }
 
-pub fn create_writer_file(directory: &str, filename: &str) -> io::Result<File> {
+pub(crate) fn create_writer_file(directory: &str, filename: &str) -> io::Result<File> {
     let path = Path::new(directory).join(filename);
     let mut open_options = OpenOptions::new();
     open_options.append(true).create(true);

tracing-appender/src/writer.rs

@@ -1,29 +1,37 @@
-#[cfg(feature = "compression")]
-use crate::compression::CompressionConfig;
 use crate::rolling::create_writer_file;
 use crate::sync::RwLock;
-#[cfg(feature = "compression")]
-use flate2::write::GzEncoder;
-use std::borrow::BorrowMut;
+use std::borrow::{Borrow, BorrowMut};
 use std::fs::{File, OpenOptions};
 use std::io::{BufWriter, Write};
 use std::ops::{Deref, DerefMut};
 use std::path::Path;
 use std::{fs, io};
 
+#[cfg(feature = "compression")]
+use flate2::write::GzEncoder;
+
+#[cfg(feature = "compression")]
+use crate::compression::CompressionConfig;
+
+#[derive(Debug)]
+struct CompressedGzip {
+    compression: CompressionConfig,
+    buffer: BufWriter<GzEncoder<BufWriter<File>>>,
+}
+
 #[derive(Debug)]
 pub enum WriterChannel {
     File(File),
     #[cfg(feature = "compression")]
-    CompressedFileGzip(BufWriter<GzEncoder<BufWriter<File>>>),
+    CompressedFileGzip(CompressedGzip),
 }
 
 impl WriterChannel {
     #[cfg(feature = "compression")]
     pub fn new(
         directory: &str,
         filename: &str,
-        #[cfg(feature = "compression")] compression: CompressionConfig,
+        #[cfg(feature = "compression")] compression: Option<CompressionConfig>,
     ) -> io::Result<Self> {
         if let Some(compression) = compression {
             Self::new_with_compression(directory, filename, compression)
@@ -50,9 +58,13 @@ impl WriterChannel {
     ) -> io::Result<Self> {
         let file = create_writer_file(directory, filename)?;
         let buf = BufWriter::new(file);
-        let gzfile = GzEncoder::new(buf, compression.into());
+        let gzfile = GzEncoder::new(buf, compression.gz_compress_level());
         let writer = BufWriter::new(gzfile);
-        Ok(WriterChannel::CompressedFileGzip(writer))
+        let compressed_gz = CompressedGzip {
+            compression: compression.clone(),
+            buffer: writer,
+        };
+        Ok(WriterChannel::CompressedFileGzip(compressed_gz))
     }
 }
 
@@ -61,15 +73,15 @@ impl io::Write for WriterChannel {
         match self {
             WriterChannel::File(f) => f.write(buf),
             #[cfg(feature = "compression")]
-            WriterChannel::CompressedFileGzip(buf) => f.write(buf),
+            WriterChannel::CompressedFileGzip(gz) => gz.buffer.write(buf),
         }
     }
 
     fn flush(&mut self) -> io::Result<()> {
         match self {
             WriterChannel::File(f) => f.flush(),
             #[cfg(feature = "compression")]
-            WriterChannel::CompressedFileGzip(buf) => buf.flush(),
+            WriterChannel::CompressedFileGzip(gz) => gz.buffer.flush(),
         }
     }
 }
@@ -79,15 +91,15 @@ impl io::Write for &WriterChannel {
         match self {
             WriterChannel::File(f) => (&*f).write(buf),
             #[cfg(feature = "compression")]
-            WriterChannel::CompressedFileGzip(buf) => f.write(buf),
+            WriterChannel::CompressedFileGzip(gz) => gz.buffer.write(buf),
         }
     }
 
     fn flush(&mut self) -> io::Result<()> {
         match self {
             WriterChannel::File(f) => (&*f).flush(),
             #[cfg(feature = "compression")]
-            WriterChannel::CompressedFileGzip(buf) => buf.flush(),
+            WriterChannel::CompressedFileGzip(gz) => gz.buffer.flush(),
         }
     }
 }