subscriber::fmt: Print all error sources #1460

nightkr · 2021-07-09T03:20:48Z

Motivation

Solution

Change the format from err=message, err: source1 to err=message err.sources=[source1, source2, source3, ...], as suggested in #1347 (comment) (still leaving out the sources if there are none).

Caveats

Haven't changed the JSON formatter, since I'm not really sure about how to do that. The current format is {.., "fields": {.., "err": "message"}}, disregarding the sources entirely.

We could change that to {.., "fields": {.., "err": "message", "err.sources": ["source1", "source2", "source3", ..]}}, which would keep backwards compatibility but looks pretty ugly.

Another option would be {.., "fields": {.., "err": {"message": "message", "sources": ["source1", "source2", "source3", ..]}}} which leaves room for future expansion.

Then again, that begs the question about why the first error is special, so maybe it ought to be {.., "fields": {.., "err": {"message": "message", "source": {"message": "source1", "source": ..}}}}.

But that style of linked list is pretty annoying to parse, so maybe it ought to be flattened to {.., "fields": {.., "err": [{"message": "message"}, {"message": "source1"}, ..]}}?

According to the format suggested in tokio-rs#1347 (comment) Fixes tokio-rs#1347

nightkr · 2021-07-09T03:23:12Z

examples/examples/fmt/yak_shave.rs

+        return Err(YakError::MissingYak {
+            source: MissingYakError::OutOfSpace {
+                source: OutOfSpaceError::OutOfCash,
+            },
+        }
+        .into());


From<String>: Box<dyn Error> doesn't support specifying an source.

nightkr · 2021-07-09T03:25:13Z

examples/examples/fmt/yak_shave.rs

+#[derive(Debug)]
+enum OutOfSpaceError {
+    OutOfCash,
+}
+
+impl Error for OutOfSpaceError {}
+
+impl Display for OutOfSpaceError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            OutOfSpaceError::OutOfCash => f.write_str("out of cash"),
+        }
+    }
+}
+
+#[derive(Debug)]
+enum MissingYakError {
+    OutOfSpace { source: OutOfSpaceError },
+}
+
+impl Error for MissingYakError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            MissingYakError::OutOfSpace { source } => Some(source),
+        }
+    }
+}
+
+impl Display for MissingYakError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            MissingYakError::OutOfSpace { .. } => f.write_str("out of space"),
+        }
+    }
+}
+
+#[derive(Debug)]
+enum YakError {
+    MissingYak { source: MissingYakError },
+}
+
+impl Error for YakError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            YakError::MissingYak { source } => Some(source),
+        }
+    }
+}
+
+impl Display for YakError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            YakError::MissingYak { .. } => f.write_str("missing yak"),
+        }
+    }
+}


All of these verbose impls kind of get in the way of the meat of the example. Maybe it'd be worth bringing in snafu or thiserror here?

Then again, that could imply that there is some special magic for whatever library is shown...

Hmm...this example was really intended to be a simple demo of the instrumentation-side API (e.g. using the span, #[instrument], and event macros). What if we added a new example demonstrating the use of tracing` with more complex nested error types? Then, the various error impls wouldn't be as much of a distraction from an example that's really not primarily about errors.

If we wanted to demonstrate that multiple error libraries are supported without special-casing, we could also have an error-specific example depend on several error libraries and use all of them in the example --- building an error chain including errors from eyre, thiserror, and snafu, or something, and then recording that error chain with tracing. Mixing and matching several error libraries is unlikely to occur much in real code, but it would demonstrate that tracing works with any library that implements std::error::Error. Also, it might be nice to have a smoke test that the errors generated by multiple libs are, in fact, formatted nicely...

Hmm...this example was really intended to be a simple demo of the instrumentation-side API (e.g. using the span, #[instrument], and event macros). What if we added a new example demonstrating the use of tracing` with more complex nested error types? Then, the various error impls wouldn't be as much of a distraction from an example that's really not primarily about errors.

Makes sense. My main worry there would be about the cartesian explosion of example cases.. :P

Mixing and matching several error libraries is unlikely to occur much in real code

Maybe not in the same project, but it seems fairly likely that a source stack would cross library borders.

Hmm, would it be OK to:

Move the errors to the bottom of the file instead (so they don't take the primary focus when reading)

Migrate them to error crates (snafu/thiserror)

While keeping them in the same example otherwise?

Done in 2377ad0

hawkw

overall, this is definitely the right approach. i noticed a couple things about the formatting that i think could be improved.

i also had some minor nitpicks, including how we could simplify the fmt::Display impl for ErrorSourceList, and a slight preference for moving the complex nested error example into a separate example, but those aren't blockers. however, we should fix the formatting issues.

hawkw · 2021-07-12T18:50:56Z

examples/examples/fmt/yak_shave.rs

+#[derive(Debug)]
+enum OutOfSpaceError {
+    OutOfCash,
+}
+
+impl Error for OutOfSpaceError {}
+
+impl Display for OutOfSpaceError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            OutOfSpaceError::OutOfCash => f.write_str("out of cash"),
+        }
+    }
+}
+
+#[derive(Debug)]
+enum MissingYakError {
+    OutOfSpace { source: OutOfSpaceError },
+}
+
+impl Error for MissingYakError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            MissingYakError::OutOfSpace { source } => Some(source),
+        }
+    }
+}
+
+impl Display for MissingYakError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            MissingYakError::OutOfSpace { .. } => f.write_str("out of space"),
+        }
+    }
+}
+
+#[derive(Debug)]
+enum YakError {
+    MissingYak { source: MissingYakError },
+}
+
+impl Error for YakError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            YakError::MissingYak { source } => Some(source),
+        }
+    }
+}
+
+impl Display for YakError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            YakError::MissingYak { .. } => f.write_str("missing yak"),
+        }
+    }
+}


Hmm...this example was really intended to be a simple demo of the instrumentation-side API (e.g. using the span, #[instrument], and event macros). What if we added a new example demonstrating the use of tracing` with more complex nested error types? Then, the various error impls wouldn't be as much of a distraction from an example that's really not primarily about errors.

If we wanted to demonstrate that multiple error libraries are supported without special-casing, we could also have an error-specific example depend on several error libraries and use all of them in the example --- building an error chain including errors from eyre, thiserror, and snafu, or something, and then recording that error chain with tracing. Mixing and matching several error libraries is unlikely to occur much in real code, but it would demonstrate that tracing works with any library that implements std::error::Error. Also, it might be nice to have a smoke test that the errors generated by multiple libs are, in fact, formatted nicely...

tracing-subscriber/src/fmt/format/mod.rs

hawkw

overall, this is definitely the right approach. i noticed a couple things about the formatting that i think could be improved.

i also had some minor nitpicks, including how we could simplify the fmt::Display impl for ErrorSourceList, and a slight preference for moving the complex nested error example into a separate example, but those aren't blockers. however, we should fix the formatting issues.

hawkw · 2021-07-12T19:16:54Z

tracing-subscriber/src/fmt/format/mod.rs

+struct ErrorSourceList<'a>(&'a (dyn std::error::Error + 'static));
+
+impl<'a> Display for ErrorSourceList<'a> {


another thought i had, but again, take it or leave it: i wonder if a nicer way to factor this would be to have this type just implement Iterator, analogous to the Error::chain unstable API. then, we could have the default formatter use f.debug_list().entries(ErrorSourceList(&error)).finish(), but the Pretty formatter could do something arbitrarily fancy with the iterator, like delimiting the errors with indentation...and eventually, we could also use the iterator in the JSON formatter to serialize the error chain as a JSON list.

what do you think? i'd be fine with merging this as-is, and making that change later if and when it's necessary, but it seems like it might be a somewhat nicer way to factor this?

then, we could have the default formatter

The Display of ErrorSourceList?

hawkw

👍 looks good to me — sorry i missed that this one was ready for another review!

nightkr · 2021-08-06T00:19:58Z

Fixed the compiler warnings in 59578e4, but that should probably be cherry-picked (or split into a separate PR) to make the rest of the PR easier to forwardport to 0.2.x (since it's a treewide change that is more related to the new rustc release than this PR specifically).

nightkr · 2021-08-06T12:01:39Z

@hawkw Sorry to pester you, but what would be the next step here? Anything more I should do to move this along?

hawkw · 2021-08-06T15:54:09Z

@hawkw Sorry to pester you, but what would be the next step here? Anything more I should do to move this along?

I'd really like to get this merged today. Would you mind moving the change that fixes warnings to a separate PR, so that we can merge that and then merge this change?

Split from tokio-rs#1460

nightkr · 2021-08-06T15:58:37Z

Sure: #1495

Split from #1460

nightkr · 2021-08-06T17:14:21Z

Thanks!

Forward-port of tokio-rs#1460 to the 0.2.x (master) branch.

Forward-port of #1460 to the 0.2.x (master) branch. ## Motivation Fixes #1347 ## Solution Change the format from `err=message, err: source1` to `err=message err.sources=[source1, source2, source3, ...]`, as suggested in #1347 (comment) (still leaving out the sources if there are none). ## Caveats Haven't changed the JSON formatter, since I'm not really sure about how to do that. The current format is `{.., "fields": {.., "err": "message"}}`, disregarding the sources entirely. We could change that to `{.., "fields": {.., "err": "message", "err.sources": ["source1", "source2", "source3", ..]}}`, which would keep backwards compatibility but looks pretty ugly. Another option would be `{.., "fields": {.., "err": {"message": "message", "sources": ["source1", "source2", "source3", ..]}}}` which leaves room for future expansion. Then again, that begs the question about why the first error is special, so maybe it ought to be `{.., "fields": {.., "err": {"message": "message", "source": {"message": "source1", "source": ..}}}}`. But that style of linked list is pretty annoying to parse, so maybe it ought to be flattened to `{.., "fields": {.., "err": [{"message": "message"}, {"message": "source1"}, ..]}}`? Co-authored-by: Eliza Weisman <eliza@buoyant.io>

@joshtriplett

# 0.2.20 (August 17, 2021) ### Fixed - **fmt**: Fixed `fmt` printing only the first `source` for errors with a chain of sources ([#1460]) - **fmt**: Fixed missing space between level and event in the `Pretty` formatter ([#1498]) - **json**: Fixed `Json` formatter not honoring `without_time` and `with_level` configurations ([#1463]) ### Added - **registry**: Improved panic message when cloning a span whose ID doesn't exist, to aid in debugging issues with multiple subscribers ([#1483]) - **registry**: Improved documentation on span ID generation ([#1453]) Thanks to new contributors @joshtriplett and @lerouxgd, and returning contributor @teozkr, for contributing to this release! [#1460]: #1460 [#1483]: #1483 [#1463]: #1463 [#1453]: #1453

@joshtriplett

# 0.2.20 (August 17, 2021) ### Fixed - **fmt**: Fixed `fmt` printing only the first `source` for errors with a chain of sources ([#1460]) - **fmt**: Fixed missing space between level and event in the `Pretty` formatter ([#1498]) - **json**: Fixed `Json` formatter not honoring `without_time` and `with_level` configurations ([#1463]) ### Added - **registry**: Improved panic message when cloning a span whose ID doesn't exist, to aid in debugging issues with multiple subscribers ([#1483]) - **registry**: Improved documentation on span ID generation ([#1453]) Thanks to new contributors @joshtriplett and @lerouxgd, and returning contributor @teozkr, for contributing to this release! [#1460]: #1460 [#1483]: #1483 [#1463]: #1463 [#1453]: #1453

@joshtriplett

# 0.2.20 (August 17, 2021) ### Fixed - **fmt**: Fixed `fmt` printing only the first `source` for errors with a chain of sources ([#1460]) - **fmt**: Fixed missing space between level and event in the `Pretty` formatter ([#1498]) - **json**: Fixed `Json` formatter not honoring `without_time` and `with_level` configurations ([#1463]) ### Added - **registry**: Improved panic message when cloning a span whose ID doesn't exist, to aid in debugging issues with multiple subscribers ([#1483]) - **registry**: Improved documentation on span ID generation ([#1453]) Thanks to new contributors @joshtriplett and @lerouxrgd, and returning contributor @teozkr, for contributing to this release! [#1460]: #1460 [#1483]: #1483 [#1463]: #1463 [#1453]: #1453 [#1498]: #1498

Split from tokio-rs#1460

@joshtriplett

# 0.2.20 (August 17, 2021) ### Fixed - **fmt**: Fixed `fmt` printing only the first `source` for errors with a chain of sources ([tokio-rs#1460]) - **fmt**: Fixed missing space between level and event in the `Pretty` formatter ([tokio-rs#1498]) - **json**: Fixed `Json` formatter not honoring `without_time` and `with_level` configurations ([tokio-rs#1463]) ### Added - **registry**: Improved panic message when cloning a span whose ID doesn't exist, to aid in debugging issues with multiple subscribers ([tokio-rs#1483]) - **registry**: Improved documentation on span ID generation ([tokio-rs#1453]) Thanks to new contributors @joshtriplett and @lerouxrgd, and returning contributor @teozkr, for contributing to this release! [tokio-rs#1460]: tokio-rs#1460 [tokio-rs#1483]: tokio-rs#1483 [tokio-rs#1463]: tokio-rs#1463 [tokio-rs#1453]: tokio-rs#1453 [tokio-rs#1498]: tokio-rs#1498

nightkr added 2 commits July 9, 2021 05:05

Update yak shaving example to use error sources

57885b1

Display the whole error stack when formatting errors

b7bea69

According to the format suggested in tokio-rs#1347 (comment) Fixes tokio-rs#1347

nightkr requested review from hawkw and a team as code owners July 9, 2021 03:20

Remove comment about creating a YakError type since we have one now

cb57b54

nightkr commented Jul 9, 2021

View reviewed changes

Change error-sources delimiter to align with other fields

8e0c804

hawkw requested changes Jul 12, 2021

View reviewed changes

nightkr and others added 4 commits July 14, 2021 23:02

Use list helper for formatting ErrorSourceList

2c2a2b1

Rewrite error example to use error derive crates

2377ad0

Merge branch 'v0.1.x' into feature/subscriber-fmt-error-sources

24e25cd

Merge branch 'v0.1.x' into feature/subscriber-fmt-error-sources

024731c

hawkw approved these changes Aug 5, 2021

View reviewed changes

Fix warnings

59578e4

nightkr requested review from jtescher and yaahc as code owners August 6, 2021 00:19

nightkr added a commit to nightkr/tracing that referenced this pull request Aug 6, 2021

Fix warnings introduced by Rustc 1.54

eeed85a

Split from tokio-rs#1460

nightkr mentioned this pull request Aug 6, 2021

Fix warnings introduced by Rustc 1.54 #1495

Merged

hawkw pushed a commit that referenced this pull request Aug 6, 2021

chore: fix warnings introduced by Rustc 1.54 (#1495)

5f0f5cc

Split from #1460

Merge branch 'v0.1.x' into feature/subscriber-fmt-error-sources

8279088

hawkw merged commit aeae4fa into tokio-rs:v0.1.x Aug 6, 2021

nightkr deleted the feature/subscriber-fmt-error-sources branch August 6, 2021 17:14

nightkr added a commit to nightkr/tracing that referenced this pull request Aug 6, 2021

0.2 edition: subscriber::fmt: print all error sources

f397d69

Forward-port of tokio-rs#1460 to the 0.2.x (master) branch.

nightkr mentioned this pull request Aug 6, 2021

0.2 edition: subscriber::fmt: print all error sources #1496

Merged

hawkw mentioned this pull request Aug 17, 2021

subscriber: prepare to release v0.2.20 #1514

Merged

nightkr mentioned this pull request Aug 30, 2021

Don't include sources in their errors' display() kube-rs/kube#515

Closed

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024

chore: fix warnings introduced by Rustc 1.54 (tokio-rs#1495)

3a37838

Split from tokio-rs#1460

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber::fmt: Print all error sources #1460

subscriber::fmt: Print all error sources #1460

nightkr commented Jul 9, 2021 •

edited

Loading

nightkr Jul 9, 2021

nightkr Jul 9, 2021

hawkw Jul 12, 2021

nightkr Jul 12, 2021

nightkr Jul 14, 2021

nightkr Jul 14, 2021

hawkw left a comment

hawkw Jul 12, 2021

hawkw left a comment

hawkw Jul 12, 2021

nightkr Jul 12, 2021

hawkw left a comment

nightkr commented Aug 6, 2021 •

edited

Loading

nightkr commented Aug 6, 2021

hawkw commented Aug 6, 2021

nightkr commented Aug 6, 2021

nightkr commented Aug 6, 2021

		struct ErrorSourceList<'a>(&'a (dyn std::error::Error + 'static));

		impl<'a> Display for ErrorSourceList<'a> {

subscriber::fmt: Print all error sources #1460

subscriber::fmt: Print all error sources #1460

Conversation

nightkr commented Jul 9, 2021 • edited Loading

Motivation

Solution

Caveats

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

nightkr commented Aug 6, 2021 • edited Loading

nightkr commented Aug 6, 2021

hawkw commented Aug 6, 2021

nightkr commented Aug 6, 2021

nightkr commented Aug 6, 2021

nightkr commented Jul 9, 2021 •

edited

Loading

nightkr commented Aug 6, 2021 •

edited

Loading