Size Based Rotating File Appender #865

CfirTsabari · 2020-07-30T18:12:48Z

Motivation

Adding an appender with max size per log file and max number of log files.
Addressing #858

Solution

Adding an appender very similar to the RollingFileAppender,
that will base the rotation on the size of the log file and the number of log files.

Motivation Adding an appender with max size per log file and max number of log files. Addressing tokio-rs#858 Solution Adding an appender very similar to the `RollingFileAppender`, that will base the rotation on size of log file and number of log files.

CfirTsabari · 2020-08-01T11:14:13Z

All of the CI failures are due to tracing-macros/examples/factorial.rs which is not part of the PR.

zekisherif · 2020-08-04T16:47:48Z

Thanks @CfirTsabari, I'll take a look at it later today

zekisherif · 2020-08-04T23:29:04Z

tracing-appender/src/rotating.rs

+/// ```
+///
+/// This will result in a log file located at `/some/path/rotating.log`.
+pub fn mb_100(


Is there some significance to 100 MB log files and max of 9 back ups?? I ask because I think creating helpers for different values will be too cumbersome. I think in this case it makes more sense to just take in the size of log files and max backups desired.

I just wanted to create at least one helper that will show an example on how to create a new appender\rotation.
If you think it's better to remove it I will remove it.

I think making the helper more generic taht takes in a size parameter would be good enough to show how it works and is probably the only helper we would need.

will change

zekisherif · 2020-08-04T23:34:56Z

tracing-appender/src/inner.rs

+        let buf_len = buf.len();
+        self.refresh_writer(&buf_len);
+        self.writer.write_all(buf).map(|_| buf_len)


This is fairly similar to what InnerRollingAppender does except it encapsulates it with a method call.

It might make more sense to have a parent struct called InnerAppender which wraps a trait called InnerAppenderTrait (not sure of a good name, just a placeholder). I feel it'll also help in the future when/if new types of appenders may be needed.

Both InnerRotatingAppender and InnerRollingAppender can implement this trait.

pub(crate) trait InnerAppenderTrait { record(&mut self, buf: &[u8]) -> Result<usize>; refresh_writer(&mut self); .... } pub(crate) struct InnerAppender<T: InnerAppenderTrait> { appender: T, } impl io::Write for InnerAppender<T: InnerAppenderTrait> { fn write(&mut self, buf: &[u8]) -> io::Result<usize> { self.appender.record(buf); } .... }

Maybe this is not ideomatic in rust and @hawkw feels differently.

I will try to create something like that, but I a little unsure of how to work correctly with traits.

zekisherif · 2020-08-05T00:11:08Z

tracing-appender/src/inner.rs

+            rotation,
+        })
+    }
+    fn refresh_writer(&mut self, size: &usize) {


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

will fix this.

Introduced InnerAppenderTrait. InnerAppenderWrapper will wrap structs that implement InnerAppenderTrait RotatingFileAppender and RollingFileAppender use this new wrapper. Added lines between functions.

zekisherif

This is some good progress. Still a few things to address concerning the major structures and then I'll look at the rest of the review.

zekisherif · 2020-08-11T18:51:42Z

tracing-appender/src/inner.rs

 use std::path::Path;

+pub(crate) trait InnerAppenderTrait<R>: io::Write


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

will rename

zekisherif · 2020-08-11T18:52:31Z

tracing-appender/src/inner.rs

+}
+impl<R, T: InnerAppenderTrait<R>> InnerAppenderWrapper<R, T> {


nit let's add a line break here.

zekisherif · 2020-08-11T18:55:42Z

tracing-appender/src/inner.rs

 use std::path::Path;

+pub(crate) trait InnerAppenderTrait<R>: io::Write
+where
+    Self: Sized,


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.

i need it for Result

zekisherif · 2020-08-11T18:59:02Z

tracing-appender/src/inner.rs

+where
+    Self: Sized,
+{
+    fn new(rotation: R, log_directory: &Path, log_filename_prefix: &Path) -> io::Result<Self>;


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.

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

zekisherif · 2020-08-11T19:05:34Z

tracing-appender/src/rotating.rs

+/// ```
+///
+/// This will result in a log file located at `/some/path/rotating.log`.
+pub fn mb_100(


I think making the helper more generic taht takes in a size parameter would be good enough to show how it works and is probably the only helper we would need.

zekisherif · 2020-08-11T19:09:52Z

tracing-appender/src/inner.rs

+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()
+    }


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.

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

CfirTsabari · 2020-08-18T12:50:04Z

Ok so after reading your comments, and reviewing the code again I did notice the flaws of using a Trait here.
I have started this trait in order to solve 3 duplicates lines, and ended up with ~50 more lines of code.
I failed to see the merits of using any trait here( other than io::Write).

so I thought to remove the entire trait, what do you think?
@zekisherif

zekisherif · 2020-08-18T21:41:51Z

Ok so after reading your comments, and reviewing the code again I did notice the flaws of using a Trait here.
I have started this trait in order to solve 3 duplicates lines, and ended up with ~50 more lines of code.
I failed to see the merits of using any trait here( other than io::Write).

so I thought to remove the entire trait, what do you think?
@zekisherif

I'll take a crack at it (can't until thursday). I feel like there's a cleaner approach here.

JRAndreassen · 2020-08-28T12:17:45Z

I'd like to see this change as well... :)

CfirTsabari · 2020-08-31T06:51:53Z

@zekisherif did you had the chance to look at it?

zekisherif · 2020-08-31T15:58:33Z

Hi @CfirTsabari

Thanks for the contribution and the time you spent working on it. However, I think that there might be a simpler solution and us asking you to write it through this PR would be draining and tiring for all parties. for that reason, we’ll close this PR.

CfirTsabari force-pushed the cfirtsabari/rotatingfileappender-858 branch 2 times, most recently from 7d23a15 to 36de84e Compare July 31, 2020 21:55

CfirTsabari force-pushed the cfirtsabari/rotatingfileappender-858 branch from 36de84e to c775505 Compare August 1, 2020 11:05

CfirTsabari changed the title ~~WIP Size Based Rotating File Appender~~ Size Based Rotating File Appender Aug 1, 2020

CfirTsabari marked this pull request as ready for review August 1, 2020 11:14

CfirTsabari requested a review from a team as a code owner August 1, 2020 11:14

zekisherif reviewed Aug 5, 2020

View reviewed changes

CR Fixes:

354731b

Introduced InnerAppenderTrait. InnerAppenderWrapper will wrap structs that implement InnerAppenderTrait RotatingFileAppender and RollingFileAppender use this new wrapper. Added lines between functions.

CfirTsabari removed the request for review from a team August 6, 2020 15:35

zekisherif reviewed Aug 11, 2020

View reviewed changes

davidbarsky closed this Aug 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Size Based Rotating File Appender #865

Size Based Rotating File Appender #865

CfirTsabari commented Jul 30, 2020 •

edited

Loading

CfirTsabari commented Aug 1, 2020

zekisherif commented Aug 4, 2020

zekisherif Aug 4, 2020

CfirTsabari Aug 6, 2020

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

zekisherif Aug 4, 2020

CfirTsabari Aug 6, 2020

CfirTsabari Aug 6, 2020

zekisherif Aug 5, 2020

CfirTsabari Aug 6, 2020

CfirTsabari Aug 6, 2020

zekisherif left a comment

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

zekisherif Aug 11, 2020

zekisherif Aug 11, 2020

CfirTsabari Aug 18, 2020

CfirTsabari commented Aug 18, 2020

zekisherif commented Aug 18, 2020

JRAndreassen commented Aug 28, 2020

CfirTsabari commented Aug 31, 2020

zekisherif commented Aug 31, 2020

		use std::path::Path;

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

		}
		impl<R, T: InnerAppenderTrait<R>> InnerAppenderWrapper<R, T> {

Size Based Rotating File Appender #865

Size Based Rotating File Appender #865

Conversation

CfirTsabari commented Jul 30, 2020 • edited Loading

Motivation

Solution

CfirTsabari commented Aug 1, 2020

zekisherif commented Aug 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zekisherif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CfirTsabari commented Aug 18, 2020

zekisherif commented Aug 18, 2020

JRAndreassen commented Aug 28, 2020

CfirTsabari commented Aug 31, 2020

zekisherif commented Aug 31, 2020

CfirTsabari commented Jul 30, 2020 •

edited

Loading