tracing

tracing copied to clipboard

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

Motivation

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

Solution

Maximum bytes per log file may be specified in the Rotation struct. Written bytes are counted and when the specified maximum number of bytes is exceeded, log file is rolled over.

CI is failing with:

error: failed to parse manifest at `/home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/opentelemetry-0.18.0/Cargo.toml`

Caused by:
  feature `edition2021` is required

  this Cargo does not support nightly features, but if you
  switch to nightly channel you can add
  `cargo-features = ["edition2021"]` to enable this feature

error: process didn't exit successfully: `/home/runner/.rustup/toolchains/1.53.0-x86_64-unknown-linux-gnu/bin/cargo hack check --feature-powerset --no-dev-deps` (exit status: 1)

I believe this is unrelated to the changes in this PR.

This patch adds size-based rotation as an option. I think it is good enough to have a truncated log between files. The CI problems don't come for this patch.

Thank you @CBenoit for this contribution,

I rebased on master and addressed the comments

In my last commit, I addressed an issue I had with this patch: the overly specific date formatting was causing a new file to be created on start, always. So instead, I now insert an index into the file name allowing rotation when the max size is reached without conflicting with an already existing file whose name happened to be the same because of the formatting. Also, when the most recent file does not exceed the max size, we simply reuse it instead of creating yet another file.

Here is how it looks like when I run my program repeatedly the same day: If there is a "max files" limit (10 in my case), older files are properly removed like so: And on the next day, the index is back to 0:

Note that even when the timed rotation is set to "never", the date is still inserted using the daily date format anyway. I did that to avoid the index from growing forever until we overflow the u64 (admittedly, I don’t think this will happen anytime soon). In this case, the rotation will only happen when the maximum size is reached.

I addressed all comments made so far in https://github.com/tokio-rs/tracing/pull/2497/commits/9efcaecf2d4851ab3957c282ddd5d719dd835050 and https://github.com/tokio-rs/tracing/pull/2497/commits/3c86bd4c00bc4bdf662c6970c994ac4c24775f6d. I did not click on "resolve conversion" myself so the commenter can easily double check.

I cleaned a few last things. I want to add additional tests before merging, but otherwise I’m waiting for further reviews.

This would be really nice to get merged, I was looking for this feature and was surprised I couldn't rotate logs based on size.

Is there anything I can do to help get this pull request merged?

Is there anything I can do to help get this pull request merged?

To help me, you could add some additional tests (send a PR on my fork), but I don’t know if we can do much until one of the maintainers find enough time to review again.

Is there anything I can do to help get this pull request merged?

To help me, you could add some additional tests (send a PR on my fork), but I don’t know if we can do much until one of the maintainer find enough time to review again.

I can help with tests. Any particular test cases that are missing, or should I just be creative?

I can help with tests. Any particular test cases that are missing, or should I just be creative?

Thank you!

There are two functions that I would like covered more thoroughly because they are critical for this feature:

• filter_log_file
• find_filename_index

Here are three test cases I would like to have:

• Are we finding the proper filename index even when we have logs over several days (or hours if the rotation is hourly)? For example, on the next day, we should start from 0 again.
• Are we properly ignoring files which are not using the proper formatting? For the daily rotation with a size_based_rotation prefix and log suffix, size_based_rotation.1970-01-01.3.log should be accounted for when finding the next index, but files such as size_based_rotation.1970-01-01-10.3.log and hello.txt should be ignored and do not cause any error.
• Are we properly increasing the index even when the first file is deleted? (e.g.: size_based_rotation.1970-01-01.3.log exists but not size_based_rotation.1970-01-01.0.log, the next index should not be 0 but 4)

I already tested these properties in practice, and it appears to work as expected, but having a few automated tests would be much welcomed for future maintenance.

If you can think of more, it’s welcomed too.

I rebased on top of master.

This would be really nice to get merged, I was looking for this feature and was surprised I couldn't rotate logs based on size.

+1, i'm looking forward for this feature as well. Thank you for this @CBenoit and everyone involved!

Hi @CBenoit , I opened this PR https://github.com/tokio-rs/tracing/pull/3026 trying to do a similar thing, then I realized your PR was doing a better job and I closed mine to concentrate the attention on this one.

While I was reviewing your implementation I found an important difference about how we expect to rotate files by size. In your implementation, there is a certain degree of complexity around renaming files and keeping track of an index for same-frequency log files.

As a user, what I would expect is to read the {current-date}.log file as the current log file, and then {current-date}.{n}.log as the previous, archived, log files. This is the behavior I've seen in other systems like logrotate and also in some rust libraries like file_rotate. With this behavior, we could remove a big portion of the code of the filter_log_file and find_filename_index functions, avoiding a lot of system calls to read directories to figure out the next index + the files metadata. What do you think? I can help with this and also with the tests, just let me know and I'll open a PR towards your fork!

Hi @adrianbenavides!

As a user, what I would expect is to read the {current-date}.log file as the current log file, and then {current-date}.{n}.log as the previous, archived, log files. This is the behavior I've seen in other systems like logrotate and also in some rust libraries like file_rotate. With this behavior, we could remove a big portion of the code of the filter_log_file and find_filename_index functions, avoiding a lot of system calls to read directories to figure out the next index + the files metadata. What do you think?

Indeed, this sounds reasonable to me!

I can help with this and also with the tests, just let me know and I'll open a PR towards your fork!

That sounds great! I’ll be glad to merge your work :)

@CBenoit here's the PR with the proposed changes: https://github.com/CBenoit/tracing/pull/1

Hi all! Is there an estimation when this might be included in the library and supported? I see the PR has been open for quite some time but that it's also very active. If there is any way I can help, lmk. Would hope to use this asap

Thanks

Hi all! Is there an estimation when this might be included in the library and supported? I see the PR has been open for quite some time but that it's also very active. If there is any way I can help, lmk. Would hope to use this asap

@GlennOlsson

As a simple contributor, I can’t really give you any ETA. However, I can tell you that I haven’t received any updates from the tokio/tracing maintainers since last year, so I don’t think it’s a priority for the project at the moment. Also, it is very likely that the maintainers already have their plates full with other projects, as I haven’t seen many PRs merged recently.

As for the help, currently @adrianbenavides is simplifying the code and adding the remaining tests I wanted. That’s pretty much all I can see at the moment.