-
Notifications
You must be signed in to change notification settings - Fork 734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
appender: add file name suffix parameter #2225
Conversation
Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a suffix makes perfect sense. However, the current implementation is a breaking change, as it breaks the interface of the various methods that were modified to accept a suffix.
Instead of doing that, I think we should add a builder-style API, in order to allow more flexible configuration of the appender. Since there are a number of currently in flight PRs to add new configuration options to the appender, I've opened PR #2227 to just add the builder interface. That way, this branch and others that add new configurations can all be rebased onto that branch once it merges, and updated to use the builder.
With the builder, the prefix and suffix also both become optional. I think this is the best interface, because some users may want only a prefix (e.g. myapp-logs.<TIMESTAMP>
), other users may want only a suffix (e.g. <TIMESTAMP>.log
), and some users may want both (e.g. myapp.<TIMESTAMP>.log
).
Thanks!
I haven't looked at your PR yet, but I completely agree that a builder here would be a lot more flexible. Good idea! |
## Motivation Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. ## Solution Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953 Signed-off-by: Eliza Weisman <[email protected]>
okay, #2227 has merged! if you don't mind updating this branch from the latest |
## Motivation Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. ## Solution Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953 Signed-off-by: Eliza Weisman <[email protected]>
## Motivation Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. ## Solution Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953 Signed-off-by: Eliza Weisman <[email protected]>
## Motivation Several currently open PRs, such as #2225 and #2221, add new configuration parameters to the rolling file appender in `tracing-appender`. The best way to add new optional configuration settings without breaking existing APIs or creating a very large number of new constructors is to add a builder interface. ## Solution Since a number of PRs would all need to add the builder API, introducing potential conflicts, this branch _just_ adds the builder interface without adding any new configuration options. Once this merges, the existing in-flight PRs can be rebased onto this branch to use the builder interface without conflicting with each other. Also, the `Builder::build` method is fallible and returns a `Result`, rather than panicking. This is a relatively common pattern in Rust --- for example, `std::thread::Builder::spawn` returns a `Result` if a new thread cannot be spawned, while `std::thread::spawn` simply panics. This allows users to handle appender initialization errors gracefully without breaking the API of the existing `new` constructor. Fixes #1953 Signed-off-by: Eliza Weisman <[email protected]>
089dc7f
to
bf06460
Compare
@hawkw I rebased and updated the builder with the suffix. I'm not sure if there is a simpler way to do the join_date formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, this looks good to me!
i had one comment on a potential refactor we might want to do, but it's not a blocker for merging this PR.
filename: Option<&str>, | ||
date: &OffsetDateTime, | ||
suffix: Option<&str>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, take it or leave it: perhaps we want to bundle together the filename prefix and suffix into a single type that's passed into this method? we could even change this to just take an instance of the Builder
type, and have Inner
own a builder instead of all the pieces of the builder.
but, the current implementation here is fine, we can refactor this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea, but I won't be able to do it for a few days. If this gets merged before then I'll make a new PR with that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, let's just take care of that change later, then!
## Motivation The `RollingFileAppender` currently only supports a filename suffix. A lot of editors have support for log files using the `.log` extension. It would be nice to be able to configure what gets added after the date. ## Solution - Add a `Builder::filename_suffix` method, taking a string. - If the string is non-empty, this gets appended to the filename after the date. - This isn't an `AsRef<Path>` because it's not supposed to be a `Path` - Update the date appending logic to handle cases when the suffix or prefix is empty - Split each part with a `.` so the final output would be `prefix.date.suffix` - Make sure to remove unnecessary `.` when a prefix or suffix is empty - Add tests related to those changes ## Notes It would probably be nicer to simply have a completely configurable file name format, but I went with the easiest approach that achieved what I needed. Closes #1477
## Motivation The `RollingFileAppender` currently only supports a filename suffix. A lot of editors have support for log files using the `.log` extension. It would be nice to be able to configure what gets added after the date. ## Solution - Add a `Builder::filename_suffix` method, taking a string. - If the string is non-empty, this gets appended to the filename after the date. - This isn't an `AsRef<Path>` because it's not supposed to be a `Path` - Update the date appending logic to handle cases when the suffix or prefix is empty - Split each part with a `.` so the final output would be `prefix.date.suffix` - Make sure to remove unnecessary `.` when a prefix or suffix is empty - Add tests related to those changes ## Notes It would probably be nicer to simply have a completely configurable file name format, but I went with the easiest approach that achieved what I needed. Closes #1477
@hawkw are there any plans to make a tracing-appender release containing this PR? It's been over a year since this was merged so I'm wondering if there's any blockers. |
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (#2323) - **non_blocking**: allow worker thread name to be configured (#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (#2227) - **rolling**: add `Builder::filename_suffix` parameter (#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (#2607) - **non_blocking**: name spawned threads (#2219) ## Fixed - Fixed several documentation typos and issues (#2689, #2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (tokio-rs#2323) - **non_blocking**: allow worker thread name to be configured (tokio-rs#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (tokio-rs#2227) - **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (tokio-rs#2607) - **non_blocking**: name spawned threads (tokio-rs#2219) ## Fixed - Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (tokio-rs#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
# 0.2.3 (November 13, 2023) This release contains several new features. It also increases the minimum supported Rust version (MSRV) to Rust 1.63.0. ## Added - **rolling**: add option to automatically delete old log files (tokio-rs#2323) - **non_blocking**: allow worker thread name to be configured (tokio-rs#2365) - **rolling**: add a builder for constructing `RollingFileAppender`s (tokio-rs#2227) - **rolling**: add `Builder::filename_suffix` parameter (tokio-rs#2225) - **non_blocking**: remove `Sync` bound from writer for `NonBlocking` (tokio-rs#2607) - **non_blocking**: name spawned threads (tokio-rs#2219) ## Fixed - Fixed several documentation typos and issues (tokio-rs#2689, tokio-rs#2375) ## Changed - Increased minimum supported Rust version (MSRV) to 1.63.0+ (tokio-rs#2793) - Updated minimum `tracing-subscriber` version to [0.3.18][subscriber-v0.3.18] (tokio-rs#2790) [subscriber-v0.3.18]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.18
Motivation
The
RollingAppender
currently only supports a filename suffix. A lot of editor have support for log files using the.log
extension. It would be nice to be able to configure what gets added after the date.Solution
AsRef<Path>
because it's not supposed to be aPath
.
so the final output would beprefix.date.suffix
.
when a prefix or suffix is emptyNotes
It would probably be nicer to simply have a completely configurable file name format, but I went with the easiest approach that achieved what I needed.
Closes #1477