subscriber: don't bail when timestamp formatting fails #1689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Motivation Currently, `tracing_subscriber::fmt` will bail out of formatting the log line when formatting the timestamp fails. Previously, when timestamps were formatted using the `chrono` crate, this was the correct behavior, as getting the timestamp was infallible, and an error would only be returned if *formatting* the timestamp to the buffer fails. This should never actually happen, because we are writing the timestamp to a string, which should never fail; using `?` is just a bit more efficient than `.expect` because it doesn't require generating unwinding code. However, this is no longer the case when using the `time` crate. In the `time` API, actually getting a timestamp is fallible, and in some cases, will fail. The current code will bail out of formatting the entire log line if getting the timestamp fails, which is not great --- using the wrong timestamp formatter will result in silently dropping all log lines. ## Solution This branch changes the `format_timestamp` method to print `<unknown time>` when the timestamp formatting fails, rather than bailing. This way, we at least print the rest of the log line for the event. This fixes half of the issue described in #1688 (the other half is improving documentation).
davidbarsky
approved these changes
Oct 26, 2021
hawkw
added a commit
that referenced
this pull request
Nov 30, 2021
## Motivation Currently, `tracing_subscriber::fmt` will bail out of formatting the log line when formatting the timestamp fails. Previously, when timestamps were formatted using the `chrono` crate, this was the correct behavior, as getting the timestamp was infallible, and an error would only be returned if *formatting* the timestamp to the buffer fails. This should never actually happen, because we are writing the timestamp to a string, which should never fail; using `?` is just a bit more efficient than `.expect` because it doesn't require generating unwinding code. However, this is no longer the case when using the `time` crate. In the `time` API, actually getting a timestamp is fallible, and in some cases, will fail. The current code will bail out of formatting the entire log line if getting the timestamp fails, which is not great --- using the wrong timestamp formatter will result in silently dropping all log lines. ## Solution This branch changes the `format_timestamp` method to print `<unknown time>` when the timestamp formatting fails, rather than bailing. This way, we at least print the rest of the log line for the event. This fixes half of the issue described in #1688 (the other half is improving documentation).
hawkw
added a commit
that referenced
this pull request
Dec 1, 2021
## Motivation Currently, `tracing_subscriber::fmt` will bail out of formatting the log line when formatting the timestamp fails. Previously, when timestamps were formatted using the `chrono` crate, this was the correct behavior, as getting the timestamp was infallible, and an error would only be returned if *formatting* the timestamp to the buffer fails. This should never actually happen, because we are writing the timestamp to a string, which should never fail; using `?` is just a bit more efficient than `.expect` because it doesn't require generating unwinding code. However, this is no longer the case when using the `time` crate. In the `time` API, actually getting a timestamp is fallible, and in some cases, will fail. The current code will bail out of formatting the entire log line if getting the timestamp fails, which is not great --- using the wrong timestamp formatter will result in silently dropping all log lines. ## Solution This branch changes the `format_timestamp` method to print `<unknown time>` when the timestamp formatting fails, rather than bailing. This way, we at least print the rest of the log line for the event. This fixes half of the issue described in #1688 (the other half is improving documentation).
hawkw
added a commit
that referenced
this pull request
Dec 23, 2021
# 0.3.4 (Dec 23, 2021) This release contains bugfixes for the `fmt` module, as well as documentation improvements. ### Fixed - **fmt**: Fixed `fmt` not emitting log lines when timestamp formatting fails ([#1689]) - **fmt**: Fixed double space before thread IDs with `Pretty` formatter ([#1778]) - Several documentation improvements ([#1608], [#1699], [#1701]) [#1689]: #1689 [#1778]: #1778 [#1608]: #1608 [#1699]: #1699 [#1701]: #1701 Thanks to new contributors @Swatinem and @rukai for contributing to this release!
hawkw
added a commit
that referenced
this pull request
Dec 23, 2021
# 0.3.4 (Dec 23, 2021) This release contains bugfixes for the `fmt` module, as well as documentation improvements. ### Fixed - **fmt**: Fixed `fmt` not emitting log lines when timestamp formatting fails ([#1689]) - **fmt**: Fixed double space before thread IDs with `Pretty` formatter ([#1778]) - Several documentation improvements ([#1608], [#1699], [#1701]) [#1689]: #1689 [#1778]: #1778 [#1608]: #1608 [#1699]: #1699 [#1701]: #1701 Thanks to new contributors @Swatinem and @rukai for contributing to this release!
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
# 0.3.4 (Dec 23, 2021) This release contains bugfixes for the `fmt` module, as well as documentation improvements. ### Fixed - **fmt**: Fixed `fmt` not emitting log lines when timestamp formatting fails ([tokio-rs#1689]) - **fmt**: Fixed double space before thread IDs with `Pretty` formatter ([tokio-rs#1778]) - Several documentation improvements ([tokio-rs#1608], [tokio-rs#1699], [tokio-rs#1701]) [tokio-rs#1689]: tokio-rs#1689 [tokio-rs#1778]: tokio-rs#1778 [tokio-rs#1608]: tokio-rs#1608 [tokio-rs#1699]: tokio-rs#1699 [tokio-rs#1701]: tokio-rs#1701 Thanks to new contributors @Swatinem and @rukai for contributing to this release!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently,
tracing_subscriber::fmtwill bail out of formatting the logline when formatting the timestamp fails. Previously, when timestamps
were formatted using the
chronocrate, this was the correct behavior,as getting the timestamp was infallible, and an error would only be
returned if formatting the timestamp to the buffer fails. This should
never actually happen, because we are writing the timestamp to a string,
which should never fail; using
?is just a bit more efficient than.expectbecause it doesn't require generating unwinding code.However, this is no longer the case when using the
timecrate. In thetimeAPI, actually getting a timestamp is fallible, and in some cases,will fail. The current code will bail out of formatting the entire log
line if getting the timestamp fails, which is not great --- using the
wrong timestamp formatter will result in silently dropping all log
lines.
Solution
This branch changes the
format_timestampmethod to print<unknown time>when the timestamp formatting fails, rather than bailing. Thisway, we at least print the rest of the log line for the event.
This fixes half of the issue described in #1688 (the other half is
improving documentation).