tracing

tracing copied to clipboard

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:

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

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.

• 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.

@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 :smile: .

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?).

@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.

@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 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.

@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.

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

@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?

@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.

@hawkw

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.

Ok. So I need to merge this PR to v0.1.x and then use that locally merged Tracing crate in my code?

@hawkw Thank you for the offer to finish this PR. I want to push this through. What I did:

• resolved some minor conflicts with master
• added a test
• cleaned all of the warnings

I tested it manually too and seems to be writing compressed logs :boom:. In the meantime I saw your comments - I will review them tomorrow and if they are still applicable will apply the suggestions.

Sorry again that it took so long. I lost the train of thought.

@hawkw BTW I had a strange issue in reading the compressed log.

let mut s = String::new();
let _ = decoder.read_to_string(&mut s);

The string is read ok but the result of read_to_string method returns an error when I try to unwrap it

{ kind: InvalidInput, error: "corrupt deflate stream" }

I think there can be something wrong with flushing encoded buffer. Not sure about it to be honest. When I wrote manually to RollingFileAppender with compression I didn't have any issues when reading it. There is this issue I found in flate2 (https://github.com/rust-lang/flate2-rs/issues/251) but not sure if this is really related.

@hawkw BTW I had a strange issue in reading the compressed log.

let mut s = String::new();

let _ = decoder.read_to_string(&mut s);

The string is read ok but the result of read_to_string method returns an error when I try to unwrap it

{ kind: InvalidInput, error: "corrupt deflate stream" }

I think there can be something wrong with flushing encoded buffer. Not sure about it to be honest. When I wrote manually to RollingFileAppender with compression I didn't have any issues when reading it. There is this issue I found in flate2 (https://github.com/rust-lang/flate2-rs/issues/251) but not sure if this is really related.

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.

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.

@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));

@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.

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

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

@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.

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

@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.

@pjankiewicz Thanks.

(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.

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

And I think there might be some merge conflicts too