Allow users to provide a custom template for the RollingFileAppender

[Michael-F-Bryan]

Motivation

The current strategy for naming log files generates filenames like /var/log/MyApplication.log.2020-07-21.

I'd prefer my log files to have log as the extension (e.g. /var/log/MyApplication.2020-07-21.log) because OSs and VS Code use the extension to know how to open the file.

Solution

This PR extracts the generation of log filenames into a FilenameTemplate trait, with Prefixed being the default strategy to maintain existing behaviour. I've also introduced a WithNameAndExtension type (and a with_name_and_extension() function for creating one) which implements the behaviour I'd like.

By using a trait and a RollingFileAppender::with_custom_template() constructor, users are able to employ whatever strategy they want for generating filenames. FilenameTemplate is also implemented for Send + Sync + 'static closures as a convenience.

This is intended to be a fully backwards compatible change.

[hawkw]

@zekisherif can you give this a review? (we should probably add you to the CODEOWNERS file to get tagged on tracing-appender PRs...)

[zekisherif]

@zekisherif can you give this a review? (we should probably add you to the CODEOWNERS file to get tagged on tracing-appender PRs...)

Yep. Looking at it now

[zekisherif]

Looks great @Michael-F-Bryan! Left a few small comments but everything else looks fine to me

[Michael-F-Bryan]

Thanks for the feedback @zekisherif! I think I've addressed the comments you've made.

Another thing I'd like to point out is that this PR switches to using PathBuf for paths (and OsString for bits in a path) instead of a String. This is purely internal so nobody see it, but I felt like it might be a good idea from a code quality standpoint (e.g. "this log_file variable isn't any old string, it's a filesystem path").

[hawkw]

Here's a thought. I think it's possible to potentially provide a more compositional and intuitive API for building filename templates. It occurs to me that WithNameAndExtension is essentially a Prefixed plus a ...postfix. We could replace WithNameAndExtension with a Postfixed (or WithExtension) type that wraps another FilenameTemplate and appends a postfix.

Then, we could have an API like this:

let filename = tracing_appender::filename("/var/log")
    .with_prefix("myapp-")
    .with_extension(".log");

What do you think?

[davidbarsky]

+1 to Eliza's suggestion; it's also more consistent with the rest of the tracing ecosystem. Not setting .with_extension could default to the current extension.

[Michael-F-Bryan]

I like the idea of using this sort of fluent API.

How would the tracing_appender::filename("/var/log") call work, though? Does that return an impl FilenameTemplate (think filter() and map() on Iterator), or is it returning a builder type where successive method calls just update the prefix and extension fields?

[Michael-F-Bryan]

Instead of Prefixed and WithNameAndExtension, I've created an in_directory() function which takes the log directory and returns a Template (a builder type implementing FilenameTemplate). By default the Template will have a None extension and a prefix of log..

I'm not sold on the log. prefix, but it made more sense than accepting a prefix in in_directory() and having a with_prefix() builder method. I assume most people would be fine with setting the prefix manually.

To maintain backwards compatibility with the previous filename format we need to add a . between the timestamp and prefix, so @hawkw's with_prefix("myapp-") example wouldn't really work out as intended (/var/log/myapp-.2020-07-26 instead of /var/log/myapp-2020-07-26). If we didn't hard-code that ., users would need to remember it whenever they set the prefix.

[davidbarsky]

To maintain backwards compatibility with the previous filename format we need to add a . between the timestamp and prefix, so @hawkw's with_prefix("myapp-") example wouldn't really work out as intended (/var/log/myapp-.2020-07-26 instead of /var/log/myapp-2020-07-26). If we didn't hard-code that ., users would need to remember it whenever they set the prefix.

While this is up to Zeki, I'm personally in favor of cutting a breaking release (0.2) of tracing-appender to get a fluent interface. I'm not sure how many libraries are using tracing-appender at this time and I think we (the tracing project as a whole) can be more aggressive about cutting breaking changes for less-frequently used crates. Not everything's tracing-core.

[zekisherif]

To maintain backwards compatibility with the previous filename format we need to add a . between the timestamp and prefix, so @hawkw's with_prefix("myapp-") example wouldn't really work out as intended (/var/log/myapp-.2020-07-26 instead of /var/log/myapp-2020-07-26). If we didn't hard-code that ., users would need to remember it whenever they set the prefix.

While this is up to Zeki, I'm personally in favor of cutting a breaking release (0.2) of tracing-appender to get a fluent interface. I'm not sure how many libraries are using tracing-appender at this time and I think we (the tracing project as a whole) can be more aggressive about cutting breaking changes for less-frequently used crates. Not everything's tracing-core.

Oh totally forgot about this idea to do a breaking release. Yeah, if everyone is fine with that then me too :)

[hawkw]

I'd like to take a closer look at this code. I'm pretty sure a breaking change shouldn't be necessary; I'm surprised that we require one. I'll take a look at this change when I get the chance.

[Michael-F-Bryan]

@davidbarsky, this isn't a breaking change.

The previous RollingFileAppender::new() constructor hasn't changed signature and now just defers to the RollingFileAppender::with_custom_template(), using a FilenameTemplate that creates identical filenames to the previous behaviour.

The fluent interface creates a new FilenameTemplate that's passed to RollingFileAppender::with_custom_template(), and is completely additive.

Of course, if you want to bump the minor version for non-technical reasons (e.g. "look, we've added new functionality to the RollingFileAppender"), that's fine too.

[zekisherif]

@davidbarsky, this isn't a breaking change.

The previous RollingFileAppender::new() constructor hasn't changed signature and now just defers to the RollingFileAppender::with_custom_template(), using a FilenameTemplate that creates identical filenames to the previous behaviour.

The fluent interface creates a new FilenameTemplate that's passed to RollingFileAppender::with_custom_template(), and is completely additive.

Of course, if you want to bump the minor version for non-technical reasons (e.g. "look, we've added new functionality to the RollingFileAppender"), that's fine too.

I think David was thinking your comment about "." being hardcoded influencing your thoughts on the more fluent api and thus is proposing a breaking change where the public api is completely updated to where having the "." is not required anymore.

Maybe an api that lets users choose what delimiter they want can be added to the proposed api that @hawkw suggested.

let filename = tracing_appender::filename("/var/log")
    .with_prefix("myapp-")
    .with_extension("log")
    .with_extension_delimiter(".");

Also tying in the new api more cleanly with the existing RollingFileAppender constructors would be nice instead of having to construct a filename and then calling RollingFileAppender::with_custom_template

[Michael-F-Bryan]

Also tying in the new api more cleanly with the existing RollingFileAppender constructors would be nice instead of having to construct a filename and then calling RollingFileAppender::with_custom_template

If you want with_prefix() and friends to be methods directly callable on RollingFileAppender, you either need to make the RollingFileAppender generic over the FilenameTemplate or use the current approach of hard-coding the RollingFileAppender to use the one file name generation algorithm (which makes the whole FilenameTemplate trait redundant).

If RollingFileAppender was made generic it could be implemented like this:

struct RollingFileAppender<T> {
    inner: InnerAppender<T>,
}

impl RollingFileAppender<Template> {
    fn with_prefix(self, prefix: impl AsRef<OsStr>) -> Self { ... }

    fn with_extension(self, extension: impl AsRef<OsStr>) -> Self { ... }
}

Is that what you had in mind?

I would also like to point out that the current API isn't really fluent anyway... You still need to create things as local variables and pass them into successive functions which wrap those things.

fn main() {
    let file_appender = tracing_appender::rolling::hourly("/some/directory", "prefix.log");
    let (non_blocking, _guard) = tracing_appender::non_blocking(file_appender);
   tracing_subscriber::fmt()
       .with_writer(non_blocking)
       .init();
}

[zekisherif]

The comment about tying in these new methods with RollingFileAppender was just a thought but at this point I don't think it's worthwhile to go that far as it will require changes to the api as you've pointed out.

I'm fairly happy with the current implementation which can be later improved if we want something that's more fluid overall. As you said, the current api for constructing a non-blocking, rolling file appender and it would be out of the scope of this PR to try to fix all that now.

I'd wait for Eliza's take on this once she has a chance to take a closer look.

[JRAndreassen]

I'd like to see this change 👍 :)

[bryangarza]

@Michael-F-Bryan - should we close this PR? Or is it something that you still would like to merge?