dedup #193
dedup #193
Conversation
|
Cool, looks pretty good. The biggest issue here is that this should be implemented as a filter. Other than that below are some nit picks.
|
|
well I tried to implement as a filter given that that has a lot of advantages but could not work out how to make a filter generate extra messages, which it needs to do in this case. If you have a proposal of how to do that then please let me know. I will fix the other points |
|
Ah I see that about the filter! Ok well thats fine, lets just put the dedup behind a feature flag then since it will rarely be used. |
| } | ||
| ], | ||
| "settings": {} | ||
| } |
There was a problem hiding this comment.
lets .gitignore this file, idk what that is
There was a problem hiding this comment.
I think you need to git rm --cached this file to remove it from the changeset
| .file_static(None) | ||
| .line(None) | ||
| .build(), | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
tried that, format_args is amazingly hard to refactor
There was a problem hiding this comment.
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(),
)| count: i32, | ||
| last: String, | ||
| } | ||
| #[derive(PartialEq)] |
There was a problem hiding this comment.
Newline above, comments first, then derives then struct decl please
| use log::Record; | ||
|
|
||
| const REPEAT_COUNT: i32 = 1000; | ||
| /// dedup object to be used by deduping appender. |
|
Looking good, a few more nits, need to feature flag dedup and we're in business |
|
rust 1.38 doesnt allow #cfg on an if statement. I am reluctant to fix this in code because its gonna be real ugly (reminder its in all appenders) Thats why CI is failing well I fixed it but its kinda ugly |
| @@ -52,6 +58,14 @@ 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(); | |||
| #[cfg(feature = "dedup")] | |||
| let _ = { | |||
| @@ -150,6 +187,12 @@ impl Deserialize for FileAppenderDeserializer { | |||
| if let Some(append) = config.append { | |||
| appender = appender.append(append); | |||
| } | |||
| #[cfg(feature = "dedup")] | |||
| let _ = { | |||
|
This is looking great, there's a few tasks left. Do you have tests for this? |
|
closing stale |
new PR for dedup.