Implement compression in tracing-appender

[pjankiewicz]

Motivation

This PR implements compression mechanism for tracing-appender crate. In our project we use tracing for event logging and they tend to be very big and compress quite well. We wanted to have a pure Rust solution to handle compression.

Solution

To overcome compatibility issues it looks at the file extension that is given by the user.

let appender = tracing_appender::rolling::minutely("/some/path", "rolling.log.gz")

will output compressed logs to /some/path/rolling.log.yyyy-MM-dd-HH-mm.gz

Note that I add non-optional dependency flate2 to Cargo.toml.

Let me know if this is ok so I can update the documentation.

Update

After @hawkw suggestion I have a new plan:

• implement builder for RollingFileAppender
• implement compression configuration behind a feature flag
• update docs
• add tests
• code formatting
• clippy

[davidbarsky]

Thanks for working on this! I'm not opposed to adding compression support to tracing-appender, but here are my initial thoughts on this PR:

• The compression feature should be behind a cargo feature gate named after the compression mechanism used. My understanding is that flate2 supports DEFLATE, zlib, and gzip, so each of these compression mechanism have their feature flag, especially if they require binding out to a C library. We might also want to add support for formats such as zstd in the future.
• tracing-appender shouldn't be looking at the suffix of the file to determine if/which compression mechanism is used. I think creating a builder for tracing-appender that allows the user to specify the folder, file name, and compression mechanism is probably best in this instance. The compression mechanism should probably be several consts implemented in a similar to how Level is in tracing-core—I think this is slightly easier to feature flag than an enum.
• All compression features should be off-by-default.

I'll add a few additional comments on the code, but those are my main thoughts at this time.

[hawkw]

• tracing-appender shouldn't be looking at the suffix of the file to determine if/which compression mechanism is used. I think creating a builder for tracing-appender that allows the user to specify the folder, file name, and compression mechanism is probably best in this instance.

+1. I'd expect the filename extensions to be added automatically based on which compression algorithm was selected in the builder, rather than selecting a compression algorithm automatically based on the user adding a filename extension.

[pjankiewicz]

@davidbarsky @hawkw Thank you for the feedback. I will implement at least 2 compression crates behind feature flags. About the file extensions it is quite common to use it as a selector for the compression method but I understand the need to more explicit 😄 .

The question is about the interface. There are 2 options I see:

1. Change existing RollingFileAppender

impl RollingFileAppender 
    pub fn new(
        rotation: Rotation,
        directory: impl AsRef<Path>,
        file_name_prefix: impl AsRef<Path>,
        compression: Option<CompressionParams> // new parameter
    ) -> RollingFileAppender  { ... }
}

struct CompressionParams {
     algorithm: SomeEnum,
     levl: SomeOtherEnum
}

and then change the constructors:

• minutely
• hourly
• daily
• never

by adding Option<CompressionParams>.

2. Implement new CompressedRollingFileAppender and minutely_compressed, hourly_compressed, daily_compressed, never_compressed

I like the second option because it is backward compatible but eventually th first one is better (maybe reserved for a major release?).

[hawkw]

@pjankiewicz Hmm, regarding the API design, I think the ideal approach would be adding a new builder type for construcing a rolling appender. I would expect the API to look sort of like this:

let compressed_appender = RollingFileAppender::builder()
     .compressed(Compression::gzip().level(3)) // or whatever
     .directory("/var/log")
     .file_name_prefix("myapp-")
     .build(Rotation::Hourly);

let non_compressed_appender = let compressed_appender = RollingFileAppender::builder()
     .directory("/var/log")
     .file_name_prefix("myapp-")
     .build(Rotation::Hourly);

I think it's fine if the new constructor and existing minutely, hourly, and daily functions don't have the option to construct a compressed appender; instead, users who want to do more advanced configuration, such as adding compression, can use the new builder. This is approach is also backwards-compatible, but adds less new API surface.

Also, this has the advantage that, because the compression feature is feature-flagged, we can also feature flag the .compressed() builder method, and add a #[doc(cfg(...))] attribute so that the required feature flags are clearly shown in the documentation. If we added a compression parameter to the new constructor, we would end up with a constructor argument that is sometimes just silently ignored based on a feature flag, which seems not great, to me.

[pjankiewicz]

@hawkw I managed to rewrite the PR according to your suggestions also I merged your latest changes from master. There were some hurdles with compression feature flag but I think it looks decent. I still need to update the docs, add tests and format the code.

The code will look like this:

RollingFileAppenderBuilder::new()
    .rotation(Roration::Hourly)
    .log_directory("/var/log")
    .log_filename_prefix("myapp-")
    .compression(compression_options::GZIP_BEST)
    .build()

I expect the rest will take me a few days more.

[hawkw]

overall, this looks like the right direction! i left some suggestions, hopefully they're helpful!

[pjankiewicz]

@hawkw thank you for the detailed code review, I really appreciate it. The issues you mentioned were fixed. It turned out that IntelliJ didn't even compile the code with the feature flag enabled even though it was selected. Not it compiles with and without the compression feature.

[hawkw]

overall, this looks very good. i had a bunch of small suggestions, but I think this should be ready to merge soon!

[hawkw]

a few questions about this:

• why does this need to be inside an Arc? as far as I can tell, we never clone it, so i don't think it needs to be reference counted?
• i don't love that the RwLock here adds a second lock that has to be acquired in the write path when compression is in use, but i see why it's necessary. we probably can't get rid of that without a fairly big rewrite of how the rest of RollingFileAppender works, so this is fine for now :/
• on that subject, I don't think this needs to be a RwLock. this is only ever locked mutably to write to it, and there's no case where multiple threads might concurrently access it immutably, so i think this should just be a Mutex, which should be a little more lightweight in cases where shared immutable access isn't needed.
• what's the motivation for wrapping the GzEncoder and the File itself in separate BufWriters? why do we need two layers of buffering? if this is important, a comment explaining it would be nice.

[pjankiewicz]

ad 1. Yeah, I might treat Arc<RwLock<...>> as an idiom by now.
ad 2. I didn't see another option. I tried for many hours to have it without any sort of lock but I always stumbled on the same problem that the GzEncoder couldn't be changed only behind a mut ref.
ad 3. I will change it to Mutex.
ad 4. I will check this again. You are right that it is confusing a bit.

[hawkw]

ad 2. I didn't see another option. I tried for many hours to have it without any sort of lock but I always stumbled on the same problem that the GzEncoder couldn't be changed only behind a mut ref.

yeah, i think this is fine, since it would probably take a lot of work to work around that part. don't worry about it.

[sw-dev-code]

@pjankiewicz @hawkw What is the current status of this merge? When we can expect it to be part of the official tracing crate?

I really like and need this feature. Thank you for your effort.

[pjankiewicz]

@sw-dev-code Sorry for the delay I had some other priorities. I will get back to it this week.

[sw-dev-code]

@sw-dev-code Sorry for the delay I had some other priorities. I will get back to it this week.

@pjankiewicz Thank you. Your solution is the most elegant one and the only one I found available to work with the Tracing crate. My devices would have limited memory so this feature is a must.

Can I somehow use it before the final release to the Tracing crate?

[hawkw]

@pjankiewicz

@sw-dev-code Sorry for the delay I had some other priorities. I will get back to it this week.

Great, I'm happy to help get this over the finish line --- I think it's pretty close to done.

@sw-dev-code

Can I somehow use it before the final release to the Tracing crate?

You'll need to backport this PR onto the v0.1.x branch, and take a git dependency on that branch. This is a bit harder than just using a Git dependency on this branch, because these changes need to be merged into v0.1.x to work with the rest of the released tracing ecosystem. However, because there aren't currently any major API changes in tracing-appender between the master and v0.1.x versions, it should be pretty easy to do the backport.

[pjankiewicz]

Hmm, interesting. I think we should figure out what's going on here before we merge this PR. It could maybe be worth adding a test that reproduces this.

It is easy to reproduce. Just writing decoder.read_to_string(&mut s).unwrap() throws this error. I will look into this tomorrow.

[pjankiewicz]

@hawkw I managed to find the issue with decoding. Before reading from the file the appender needs to be dropped to properly close the buffer.

let expected_value = "Hello";
write_to_log(&mut appender, expected_value);
drop(appender); // had to add this to read from the file
assert!(find_str_in_compressed_log(directory.path(), expected_value));

[pjankiewicz]

@hawkw @sw-dev-code I think the PR is ready. I went back to our previous discussions and resolved some issues - hopefully I didn't miss anything. I checked clippy, tests, formatting etc. I can remove WIP flag right now in my opinion.

[pjankiewicz]

@davidbarsky I might have misclicked to ask you for the review in this PR. Sorry about this I think @hawkw has everything under control.

[sw-dev-code]

@pjankiewicz Thank you so much for your effort. How will I know when this implementation becomes part of the official crate?

[pjankiewicz]

@pjankiewicz Thank you so much for your effort. How will I know when this implementation becomes part of the official crate?

@sw-dev-code It is not up to me. Assuming that it will be merged soon - it will be a part of some future release. As it is a change that doesn't break the current API I hope it will be pretty soon. In the meantime you could use this branch or manually apply the changes to 1.x version.

[sw-dev-code]

@pjankiewicz Ok, understood. Can you guide me on how to configure Rust to use your branch for tracing crate?

[pjankiewicz]

@pjankiewicz Ok, understood. Can you guide me on how to configure Rust to use your branch for tracing crate?

It is possible to specify dependencies from git / github. See here for more information https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories.

But I wouldn't suggest you use a specific version from any branch - it is much better to rely on stable releases of crates.

[sw-dev-code]

@pjankiewicz Thanks.

[bryangarza]

(Going through all open PRs)

Seems this PR is pretty close to getting merged, added myself as reviewer for now, will try to look at it later today or tomorrow.

[bryangarza]

@pjankiewicz seems the CI has some failures, could you resolve those? After that we can probably merge

[bryangarza]

And I think there might be some merge conflicts too

[TheGP]

What happened? I need compression and thinning, pls someone make it! XD

[gaoqiangz]

Is it possible to merge this feature?