RFC#1 - file-rotate version 1.0

[kstrafe]

File-rotate has existed for 6 years now, it's about time we create a version 1.0.

This is an RFC to address shortcomings of the current library and to improve upon them. Version 1.0 will be a breaking change.

Proposed Changes

Together with @Ploppz, we propose the following changes.

User-definable Rotation Schemes

The file rotation method must be user-definable to support use-cases that are beyond the scope of this library to support natively (#32, #27). file-rotate will provide a set of implementations for common cases (integer suffixes and timestamp suffixes). To accomplish this, we define a trait Rotator:

trait Rotator {
    /// Called when a limit has been exceeded, or rotation is manually called. Takes the current file, and returns a potentially new file to start writing to.
    /// After this call, any content limits are reset, even if we decide to not rotate, and instead write to the same file.
    fn rotate(&mut self, current: File) -> File;
}

With this trait we can instantiate FileRotate with an implementation of Rotator. The Rotator decides what suffix (#31) (if any) to use for the new file, to delete or overwrite the old file, or to run a post-compression algorithm on it (#5).

This method allows anyone to implement whatever is desired; one such case is setting file permissions for a specific OS.

User-definable Rotation Triggers

Currently, the library exposes fixed ContentLimits. In some cases, one might be interested in using available disk space or other, more complex scenarios. To support these cases, we define a trait.

trait Trigger {
    /// Decides whether to trigger a rotation, returning.
    fn trigger(&mut self, bytes: &[u8]) -> Action;
}

trigger returns an Action, an enum.

enum Action {
    Rotate { consumed: usize }, /// `consumed` is the amount of bytes to consume from the current buffer given to `trigger` before rotation, if it is less than `bytes.len()` in `trigger`, the remaining bytes will be consumed after `rotate`.
    None /// Do not rotate, write all bytes to output file.
}

For this trait, the library will provide the current ContentLimits. If common cases arise, they can be added to this crate.

RFC

We will have an open comment period to collect concerns and criticisms of this design.

@LinAGKar @ramyak-mehra @mbiggio @michalfita @Ploppz @craigdub @proski @PhilVince96 @jsdanielh @tuxmark5 @wizzardx @jb-alvarado @happybeing @vbmade2000 @rohitjoshi

[kstrafe]

WIP posted in branch 707b2e3

[michalfita]

Notes about Rotator trait:

1. It consumes File, is this really desired?
2. If all the duty for full management of the file is left to the user, what's left for the crate? IMVHO some more fine grained approach would serve better, for example: take responsibility for rotation names, but leave default behaviour intact.
3. Alternatively some default rotators can let used set closures for only specific stages of the process as customisation. Rust doesn't have inheritance to just extend existing functionality of a type.

Question about Trigger trait:

1. How this is supposed to work? Observing the content limits while writing data is easy, but how about watching other parameters?
2. Is it going to be called every time a write to the current log file takes place? It would then have to be very lightweight.

[proski]

I don't know why I was invited to comment, I don't use file-rotate in my projects and I have never contributed to it. Anyway, I'll comment from the point of view of a potential user.

I want the code to be readable without IDE or other tools. Let me see the example:

    let mut log = FileRotate::new("logs/log", AppendCount::new(2), ContentLimit::Lines(3), Compression::None, None);

What's the meaning of 2? Maybe the counter has two digits (01, 02 etc)? Maybe the counter starts at 2? The text below doesn't confirm either.

Further, what's the final None argument? I cannot tell from the function call.

The builder pattern can be used to improve readability. Instead of 5 arguments you can have 10 or 20 - and the user will only need to specify those of them that differ from the default or don't have a default.

[kstrafe]

@michalfita

1. It consumes File, is this really desired?

Not sure, any suggestions? In the WIP we use Option<File>. None indicates that it's the first file, so we are able to know when to create an initial file. We somehow need to indicate that this file is about to be dropped, since FileRotate relinquishes ownership of it.

2. If all the duty for full management of the file is left to the user, what's left for the crate? IMVHO some more fine grained approach would serve better, for example: take responsibility for rotation names, but leave default behaviour intact.

What's left of this crate is just a relatively simple API with some presets for rotators and triggers, basically a refactor of the current implementation into something more customizable.

Which default behavior do you mean?

3. Alternatively some default rotators can let used set closures for only specific stages of the process as customisation. Rust doesn't have inheritance to just extend existing functionality of a type.

The default rotators can use different arguments to customize them, if further customization is desired, then the trait can be implemented from the ground up. This way, pretty much anyone can get their desired behavior. The only thing we haven't dealt with yet is async.

Question about Trigger trait:

1. How this is supposed to work? Observing the content limits while writing data is easy, but how about watching other parameters?

Most likely via having the object own Rc<RefCell<_>> or reading from thread-local state. The alternative is to provide a Context object that FileRotate takes as a generic, or a type-erased Box<dyn Any> that's then typecast inside the trigger trait.

2. Is it going to be called every time a write to the current log file takes place? It would then have to be very lightweight.

It will be called upon every write. I don't know if we can get around this in any way to make it faster, we simply have to scan through the output to figure out when to split logs. It can be as simple as counting bytes, which should be the cheapest option.

@proski

I don't know why I was invited to comment, I don't use file-rotate in my projects and I have never contributed to it. Anyway, I'll comment from the point of view of a potential user.

You opened #29, I added anyone who might be interested. I forgot to mention it, but feel free to ignore.

I want the code to be readable without IDE or other tools. Let me see the example:

I completely agree with this.

[proski]

I forgot about #29 completely, it was at my previous job. Thank you for the reminder! Interestingly, it supports my argument for the build pattern initialization. Different OSes have different permissions. Some users may not care about Windows permissions, they shouldn't need to specify them. Others do care, they should have a way to supply the desired settings.

[frostyplanet]

    fn rotate(&mut self, current: File) -> File;

A suggestion, it would be better the type of current file argument were changed to Path, because some log sinks are not necessary File. The Rotator just moves it without the need to read or write. And the caller needs to "reopen" the file after rotate(). For example A raw file sink: https://github.com/NaturalIO/captains-log/blob/master/src/file_impl.rs#L137

So that we can make it possible to use the same API to monitor log file written by other process, in replace of system's log-rotate.

For example, I do have a service implemented as multiprocesses that write to the same log, flock needs to acquire and notify the process with signal when doing log-rotate

[Ploppz]

Hi all, sorry I've been inactive so far. I must say that I have been rather absentminded even concerning the initial suggestion due to life circumstance, that is why only now I share feedback on this which I wish I had readily available earlier in the process.

Rotator

I'm not completely satisfied with the Rotator trait. The old SuffixScheme trait does some things very well: implementing a new suffix scheme, you don't have to worry about any file operations at all. It's quite a clean and functional interface. The motivation behind a new Rotator trait was those issues that have been mentioned about callback functions and optional file ownership configuration. I would prefer to keep the suffix logic (as currently represented by SuffixScheme) separated from those concerns.

I think this also addresses @michalfita's concern

If all the duty for full management of the file is left to the user, what's left for the crate? IMVHO some more fine grained approach would serve better, for example: take responsibility for rotation names, but leave default behaviour intact.

But then, we need to find a different way to implement callbacks and control over file parameters.

I would be happy to draft something but tomorrow I go for a vacation until ca 13-14 August. After that I can - unless someone feels called before that.

Trigger

As for the Trigger trait: Yes this is one way to add more flexibility. We lose the convenience of the implementation of ContentLimit::Time where it's using a member variable modified which IIRC signifies when the file was created(?). Also the discrepancy between BytesSurpassed and Bytes I'm unsure how to implement with this simple trait, but maybe that is not so important.

Question: Is it a suitable alternative solution to actually just have another variant that holds a boxed closure?
And we can absolutely use the name Trigger, it's more fitting. So:

/// When to move files: Condition on which a file is rotated.
#[derive(Clone, Debug)]
pub enum Trigger {
    /// Cut the log at the exact size in bytes.
    Bytes(usize),
    /// Cut the log file at line breaks.
    Lines(usize),
    /// Cut the log at time interval.
    Time(TimeFrequency),
    /// Cut the log file after surpassing size in bytes (but having written a complete buffer from a write call.)
    BytesSurpassed(usize),
    /// Custom logic
    Custom(Box<dyn FnMut(&[u8]) + Send>),
    /// Don't do any rotation automatically
    None,
}

Alternatively, just the proposed trait but pass along the file creation date for example?

[Ploppz]

A suggestion, it would be better the type of current file argument were changed to Path, because some log sinks are not necessary File.

What do you mean with this @frostyplanet ? The library deals with rotation of files. And if given a Path, what else than a file do we expect to be at this location?

[frostyplanet]

@Ploppz What I suggesting: to seperate the logic that writing a file, and the logic moving the files.
I've opened another issue #36 , adapted file-rotate 0.8 to my log library (https://github.com/NaturalIO/captains-log/blob/master/src/rotation.rs#L312). Because I found file-rotate crate is the closest feature implemented compared to system logrotate. I have also added an extra option "archive_dir", which allows moving the old files to a specified directory different from the current file. （equals to old_dir option in system log-rotate）

I do not use std::fs::File to write I/O, instead I use libc::write to write raw unbuffered I/O to ensure the integrity of the line, not broken by the possibility of multi process writes (in scenario the program graceful restart, and in the future I and hoping to support the scenario of multi-process service).
I can only reuse the code in file_rotate::suffix, another half of the code had to be copied and modified from FileLogroate struct, in order to deal with Path. But most of the code is the same with the original FileLogrotate,
I am still waiting for file-rotate 1.0 API refactor, hope that I can reduce duplicate code by reusing the new abstraction.