Size rolling

[x3ccd4828]

This patch adds size-based rotation to tracing-appender.

Closes https://github.com/tokio-rs/tracing/issues/1940 Closes https://github.com/tokio-rs/tracing/issues/858

cc @hawkw @IniterWorker @CfirTsabari @davidbarsky @CBenoit

There is another pull request (https://github.com/tokio-rs/tracing/pull/2497) for this feature but I thought I would try a different approach.

Motivation

In a constrained environment, we should be able to constrain the logging size and period.

Solution

Maximum bytes per log file may be specified in the max_file_size function when building the appender. The option will only be used when selecting Rotation::SIZE.

File size is checked every write and if the size is exceeded the log file is rolled over.

None of the other rotation types use sub-seconds in the logfile name but for size-based rotation, I used it since there could be a plethora of writes in a short period and this was the simple way to ensure filename uniqueness otherwise it would write to the same file even if the size was exceeded. This is a limitation since I tried to use the same methodology as the time-based rolling without rewriting the architecture.

[x3ccd4828]

Hi @hawkw @davidbarsky @hds,

What would it take to get this reviewed? Should I just close it since nobody will ever look at it?

[hds]

@x3ccd4828 I can look at this, but I'm currently very full up with family stuff, would you mind reminding me after the 25th of January? I'll be back at work then (and therefore at my computer) and will make some time to go through old PRs and issues.

[x3ccd4828]

Tha is understandable, I will post here at the end of January

[x3ccd4828]

@hds I have rebased the branch would you be able to review it?

[JoelSatkas]

@x3ccd4828 Could you update your branch with the latest official traceing-appenders? If i am not mistaken this branch is still on version 0.2.0?

Thats at least what my toml lock file is indicating, i was on version 0.2.3 before and had tracing working in both console and file appending using the time rotation but when i use your branch, i get errors indicating that the file appender doesnt implement MakeWriter?

expected a `Fn<()>` closure, found `RollingFileAppender`
the trait `Fn<()>` is not implemented for `RollingFileAppender`
wrap the `RollingFileAppender` in a closure with no arguments: `|| { /* code */ }`
the following other types implement trait `MakeWriter<'a>`:
  std::fs::File
  BoxMakeWriter
  TestWriter
  WithMaxLevel<M>
  WithMinLevel<M>
  WithFilter<M, F>
  tracing_subscriber::fmt::writer::OrElse<A, B>
  Tee<A, B>
and 2 others
required for `RollingFileAppender` to implement `for<'writer> MakeWriter<'writer>`

My code for tracing to the console and log file looks like this:

// Create a rolling file appender
let file_appender = RollingFileAppender::builder()
    .filename_prefix(SERVER_LOG_FILE_NAME)
    .filename_suffix("log")
    .max_file_size(5 * 1024 * 1024)
    .max_log_files(5)
    .rotation(Rotation::SIZE)
    .build(log_dir);

if file_appender.is_err() {
    tracing::error!("Failed to build file appender for server logs.");
    return EResultCode::Failure;
}

// Set up the console and file layers
let filter_layer = EnvFilter::try_from_default_env().unwrap_or_else(|_| {
    EnvFilter::new(
    "web-servicer=debug,web_servicer_library=debug,tower_http=debug",
)
});
let file_layer = fmt::layer()
    .with_writer(file_appender.unwrap())
    .with_ansi(false)
    .with_target(true)
    .with_level(true);
let console_layer = fmt::layer().with_target(true).with_level(true);

// Register the layers
tracing_subscriber::registry()
    .with(filter_layer)
    .with(console_layer)
    .with(file_layer)
    .init();

[x3ccd4828]

@JoelSatkas I rebased based on tracing master branch but for some reason master branch has tracing-appender at 0.2.0. From the looks of it, 0.2.3 was in a branch for release purposes only. I don't think I can rebase it based on that branch for this merge.

[JoelSatkas]

@JoelSatkas I rebased based on tracing master branch but for some reason master branch has tracing-appender at 0.2.0. From the looks of it, 0.2.3 was in a branch for release purposes only. I don't think I can rebase it based on that branch for this merge.

I see. Thats okay and thank you for checking. I would love to have this feature in the project im working on but for now the time rollover will have to suffice. I hope this PR will be properly reviewed by the developers soon and thank you for contributing

[bryanlarsen]

@JoelSatkas, this MR applies cleanly against v0.1.x. I've done so here: https://github.com/bryanlarsen/tracing/tree/v0.1.x-size-rolling

[x3ccd4828]

@hds would you be able to review this?

[bryanlarsen]

For what it's worth, we tried three log size rotation PR's, and settled on https://github.com/tokio-rs/tracing/pull/2497 .

This branch has two concerns:

First, it generates very oddly named files, with the milliseconds encoded in the filename. Our users found it off-putting.

Secondly, it does not allow rotation by date and size, you have to choose one or the other.

The big advantage this PR has is that it's small and easy to understand. That's why we tried this PR first -- small and non-disruptive is what you want when you're using a fork of the main project. However, I think anything that's merged into the main crate should cover the maximal number of use cases.

P.S. The other branch we tried and ruled out was https://github.com/CBenoit/tracing/pull/1

[bryanlarsen]

P.S. All three PR's apply cleanly against v0.1.x

[brunbjerg]

What is the status on this?

At the moment I am using the file_rotate instead of any tracing implementation. Is there still motivation to change this, so that we could use tracing instead?

[CBenoit]

What is the status on this?

At the moment I am using the file_rotate instead of any tracing implementation. Is there still motivation to change this, so that we could use tracing instead?

I’m thinking that using file_rotate is the right choice at this point. tracing-subscriber provides an API that works with any std::io::Write, making it possible to lift the advanced rotation complexity to a separate crate such as file_rotate. Probably not a bad thing to be fair.

As for the status, I think there are at least 3 PRs implementing size-based rotation, but it appears there is no bandwidth to review such changes as none of them are being seriously reviewed by maintainers. At this point, I suggest to forget about that and stick to file_rotate.

Use with_writer to get both tracing/tracing-subscriber and file_rotate.

In all fairness, and even though I myself opened a PR for that, I don’t think anymore it’s necessary to bring all the rotation features directly into the tracing-subscriber crate.

[x3ccd4828]

I have rebased this pull request and remove milliseconds from the generated file name. All appender tests pass locally.

Is there anyone that can review this merge?

[x3ccd4828]

@hds could you review this PR?