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

dedup #193

Closed
wants to merge 12 commits into from
Closed

dedup #193

Show file tree
Hide file tree
Changes from 10 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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ json_format = ["serde_json"]
toml_format = ["toml"]
xml_format = ["serde-xml-rs"]

console_appender = ["console_writer", "simple_writer", "pattern_encoder"]
console_appender = ["parking_lot","console_writer", "simple_writer", "pattern_encoder"]
file_appender = ["parking_lot", "simple_writer", "pattern_encoder"]
rolling_file_appender = ["parking_lot", "simple_writer", "pattern_encoder"]
compound_policy = []
Expand Down
8 changes: 8 additions & 0 deletions log4rs.code-workspace
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"folders": [
{
"path": "."
}
],
"settings": {}
}
Copy link
Owner

Choose a reason for hiding this comment

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

lets .gitignore this file, idk what that is

Copy link
Owner

Choose a reason for hiding this comment

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

I think you need to git rm --cached this file to remove it from the changeset

31 changes: 29 additions & 2 deletions src/append/console.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! Requires the `console_appender` feature.

use log::Record;
use parking_lot::Mutex;
#[cfg(feature = "file")]
use serde_derive::Deserialize;
use std::{
Expand All @@ -11,6 +12,7 @@ use std::{
io::{self, Write},
};

use crate::append::dedup::*;
#[cfg(feature = "file")]
use crate::encode::EncoderConfig;
#[cfg(feature = "file")]
Expand All @@ -28,14 +30,14 @@ use crate::{
},
priv_io::{StdWriter, StdWriterLock},
};

pm100 marked this conversation as resolved.
Show resolved Hide resolved
/// The console appender's configuration.
#[cfg(feature = "file")]
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ConsoleAppenderConfig {
target: Option<ConfigTarget>,
encoder: Option<EncoderConfig>,
dedup: Option<bool>,
}

#[cfg(feature = "file")]
Expand Down Expand Up @@ -112,6 +114,7 @@ impl<'a> encode::Write for WriterLock<'a> {
pub struct ConsoleAppender {
writer: Writer,
encoder: Box<dyn Encode>,
deduper: Option<Mutex<DeDuper>>,
}

impl fmt::Debug for ConsoleAppender {
Expand All @@ -125,6 +128,11 @@ impl fmt::Debug for ConsoleAppender {
impl Append for ConsoleAppender {
fn append(&self, record: &Record) -> Result<(), Box<dyn Error + Sync + Send>> {
let mut writer = self.writer.lock();
if let Some(dd) = &self.deduper {
if dd.lock().dedup(&mut writer, &*self.encoder, record)? == DedupResult::Skip {
return Ok(());
}
}
self.encoder.encode(&mut writer, record)?;
writer.flush()?;
Ok(())
Expand All @@ -139,6 +147,7 @@ impl ConsoleAppender {
ConsoleAppenderBuilder {
encoder: None,
target: Target::Stdout,
dedup: false,
}
}
}
Expand All @@ -147,6 +156,7 @@ impl ConsoleAppender {
pub struct ConsoleAppenderBuilder {
encoder: Option<Box<dyn Encode>>,
target: Target,
dedup: bool,
}

impl ConsoleAppenderBuilder {
Expand All @@ -163,6 +173,13 @@ impl ConsoleAppenderBuilder {
self.target = target;
self
}
/// Determines if the appender will reject and count duplicate messages.
///
/// Defaults to `false`.
pub fn dedup(mut self, dedup: bool) -> ConsoleAppenderBuilder {
self.dedup = dedup;
self
}

/// Consumes the `ConsoleAppenderBuilder`, producing a `ConsoleAppender`.
pub fn build(self) -> ConsoleAppender {
Expand All @@ -176,12 +193,19 @@ impl ConsoleAppenderBuilder {
None => Writer::Raw(StdWriter::stdout()),
},
};

pm100 marked this conversation as resolved.
Show resolved Hide resolved
let deduper = {
if self.dedup {
Some(Mutex::new(DeDuper::default()))
} else {
None
}
};
ConsoleAppender {
writer,
encoder: self
.encoder
.unwrap_or_else(|| Box::new(PatternEncoder::default())),
deduper,
}
}
}
Expand Down Expand Up @@ -233,6 +257,9 @@ impl Deserialize for ConsoleAppenderDeserializer {
if let Some(encoder) = config.encoder {
appender = appender.encoder(deserializers.deserialize(&encoder.kind, encoder.config)?);
}
if let Some(dedup) = config.dedup {
appender = appender.dedup(dedup);
}
Ok(Box::new(appender.build()))
}
}
94 changes: 94 additions & 0 deletions src/append/dedup.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
//! The dedup common handler
//!
use crate::append::Error;
use crate::encode::{Encode, Write};
use log::Record;

const REPEAT_COUNT: i32 = 1000;
/// dedup object to be used by deduping appender.
Copy link
Owner

Choose a reason for hiding this comment

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

Newline above please

/// internals are private to dedup
#[derive(Default)]
pub struct DeDuper {
count: i32,
last: String,
}
#[derive(PartialEq)]
Copy link
Owner

Choose a reason for hiding this comment

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

Newline above, comments first, then derives then struct decl please

/// Used by an appender that uses dedup.
/// Indicates whether or not the current message should be output.
///
/// sample use from console appender
/// if let Some(dd) = &self.deduper {
/// if dd.lock().dedup(&mut *file, &*self.encoder, record)? == DedupResult::Skip {
/// return Ok(());
/// }
/// ... output the message
pub enum DedupResult {
/// skip
Skip,
/// write
Write,
}
impl DeDuper {
// emits the extra line saying 'last line repeated n times'
fn write(
w: &mut dyn Write,
encoder: &dyn Encode,
record: &Record,
n: i32,
) -> Result<(), Box<dyn Error + Sync + Send>> {
if n == 1 {
encoder.encode(
w,
&Record::builder()
.args(format_args!("last message repeated, suppressing dups"))
.level(record.level())
.target(record.target())
.module_path_static(None)
.file_static(None)
.line(None)
.build(),
)
} else {
encoder.encode(
w,
&Record::builder()
.args(format_args!("last message repeated {} times", n))
.level(record.level())
.target(record.target())
.module_path_static(None)
.file_static(None)
.line(None)
.build(),
)
Copy link
Owner

Choose a reason for hiding this comment

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

instead of calling these encode fns in the if else branches can we just return the msg and call the encode after in one place? Seems like a lot of repeated lines

Copy link
Author

Choose a reason for hiding this comment

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

tried that, format_args is amazingly hard to refactor

Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking something like this

let args = if n == 1 {
  format_args!("last message repeated, suppressing dups")
} else {
  format_args!("last message repeated {} times", n)
};
encoder.encode(
    w,
    &Record::builder()
        .args(args)
        .level(record.level())
        .target(record.target())
        .module_path_static(None)
        .file_static(None)
        .line(None)
        .build(),
)

}
}

/// appender calls this.
pm100 marked this conversation as resolved.
Show resolved Hide resolved
/// If it returns Skip then appender should not write
/// If it returns Write then the appender should write as per normal
pub fn dedup(
&mut self,
w: &mut dyn Write,
encoder: &dyn Encode,
record: &Record,
) -> Result<DedupResult, Box<dyn Error + Sync + Send>> {
let msg = format!("{}", *record.args());
if msg == self.last {
self.count += 1;

// every now and then keep saying we saw lots of dups
if self.count % REPEAT_COUNT == 0 || self.count == 1 {
Self::write(w, encoder, record, self.count)?;
}
Ok(DedupResult::Skip)
} else {
self.last = msg;
let svct = self.count;
self.count = 0;
if svct > 1 {
Self::write(w, encoder, record, svct)?;
}
Ok(DedupResult::Write)
}
}
}
32 changes: 30 additions & 2 deletions src/append/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use crate::{
encode::{pattern::PatternEncoder, writer::simple::SimpleWriter, Encode},
};

use crate::append::dedup::*;

/// The file appender's configuration.
#[cfg(feature = "file")]
#[derive(Deserialize)]
Expand All @@ -31,13 +33,15 @@ pub struct FileAppenderConfig {
path: String,
encoder: Option<EncoderConfig>,
append: Option<bool>,
dedup: Option<bool>,
}

/// An appender which logs to a file.
pub struct FileAppender {
path: PathBuf,
file: Mutex<SimpleWriter<BufWriter<File>>>,
encoder: Box<dyn Encode>,
deduper: Option<Mutex<DeDuper>>,
}

impl fmt::Debug for FileAppender {
Expand All @@ -52,11 +56,16 @@ impl fmt::Debug for FileAppender {
impl Append for FileAppender {
fn append(&self, record: &Record) -> Result<(), Box<dyn Error + Sync + Send>> {
let mut file = self.file.lock();
if let Some(dd) = &self.deduper {
if dd.lock().dedup(&mut *file, &*self.encoder, record)? == DedupResult::Skip {
return Ok(());
}
}

self.encoder.encode(&mut *file, record)?;
file.flush()?;
Ok(())
}

pm100 marked this conversation as resolved.
Show resolved Hide resolved
fn flush(&self) {}
}

Expand All @@ -66,6 +75,7 @@ impl FileAppender {
FileAppenderBuilder {
encoder: None,
append: true,
dedup: false,
}
}
}
Expand All @@ -74,6 +84,7 @@ impl FileAppender {
pub struct FileAppenderBuilder {
encoder: Option<Box<dyn Encode>>,
append: bool,
dedup: bool,
}

impl FileAppenderBuilder {
Expand All @@ -90,6 +101,13 @@ impl FileAppenderBuilder {
self.append = append;
self
}
/// Determines if the appender will reject and count duplicate messages.
pm100 marked this conversation as resolved.
Show resolved Hide resolved
///
/// Defaults to `false`.
pub fn dedup(mut self, dedup: bool) -> FileAppenderBuilder {
self.dedup = dedup;
self
}

/// Consumes the `FileAppenderBuilder`, producing a `FileAppender`.
pub fn build<P: AsRef<Path>>(self, path: P) -> io::Result<FileAppender> {
Expand All @@ -103,10 +121,17 @@ impl FileAppenderBuilder {
.truncate(!self.append)
.create(true)
.open(&path)?;

pm100 marked this conversation as resolved.
Show resolved Hide resolved
let deduper = {
if self.dedup {
Some(Mutex::new(DeDuper::default()))
} else {
None
}
};
Ok(FileAppender {
path,
file: Mutex::new(SimpleWriter(BufWriter::with_capacity(1024, file))),
deduper,
encoder: self
.encoder
.unwrap_or_else(|| Box::new(PatternEncoder::default())),
Expand Down Expand Up @@ -150,6 +175,9 @@ impl Deserialize for FileAppenderDeserializer {
if let Some(append) = config.append {
appender = appender.append(append);
}
if let Some(dedup) = config.dedup {
appender = appender.dedup(dedup);
}
if let Some(encoder) = config.encoder {
appender = appender.encoder(deserializers.deserialize(&encoder.kind, encoder.config)?);
}
Expand Down
2 changes: 1 addition & 1 deletion src/append/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ use crate::filter::FilterConfig;

#[cfg(feature = "console_appender")]
pub mod console;
pub mod dedup;
#[cfg(feature = "file_appender")]
pub mod file;
#[cfg(feature = "rolling_file_appender")]
pub mod rolling_file;

pm100 marked this conversation as resolved.
Show resolved Hide resolved
/// A trait implemented by log4rs appenders.
///
/// Appenders take a log record and processes them, for example, by writing it
Expand Down