Skip to content
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

Add ability to print error Result using err via Debug instead of only Display #1630

Open
ciuncan opened this issue Oct 10, 2021 · 8 comments
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request

Comments

@ciuncan
Copy link
Contributor

ciuncan commented Oct 10, 2021

Feature Request

Crates

  • tracing-attributes

Motivation

Currently instrument attribute macro's err meta item makes the expanded code to print the Err result only using its Display implementation. Some Error types could be more beneficial to be printed using their Debug implementation. For example eyre::Result contains an error chain. When printed in via Display, it shows the most recent error message. But if you print it via the Debug implementation it shows the error with the error chain.

Proposal

Being able to write #[insturment(?err, ...)] would be in line with trace! et. al macro's fields, but it appears this isn't compatible with meta item syntax. Maybe two alternatives can be provided as:

  • #[instrument(err, ...)]: print the Error using Display implementation so that existing codes aren't affected.
  • #[instrument(err_dbg, ...)]: print the Error using Debug implementation.

I am willing to work on this and open a PR. Let me know if you can think of a better alternative syntax.

Alternatives

The only alternative I can think of is to remove err meta item from the attribute, catch the error somehow and print manually with "{:?}" format string. But this defeats the purpose of #[instrument] attribute (ergonomy) as it could easily implement this behavior.

@hawkw hawkw added crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request labels Oct 10, 2021
@hawkw
Copy link
Member

hawkw commented Oct 10, 2021

Maybe two alternatives can be provided as:

  • #[instrument(err, ...)]: print the Error using Display implementation so that existing codes aren't affected.
  • #[instrument(err_dbg, ...)]: print the Error using Debug implementation.

Syntactically, what do you think about using #[instrument(err)] and #[instrument(err(Debug)] or something, instead of adding an additional argument? IMO this feels more consistent with other attribute macros. Also, if we wanted to also permit recording the errors as dyn Error values in the future, we could add an #[instrument(err(Error))] option to use the error trait...

@ciuncan
Copy link
Contributor Author

ciuncan commented Oct 10, 2021

Hmm, #[instrument(err(Debug))] sounds good to me, it looks like as in the #[derive(...)] attribute macro. 👍 I'll try and push it as another commit.

But I am not sure if I understood #[instrument(err(Error))] one. How would that change the tracing::error! macro invocation? (Currently it would be invoked as error!(error = ?e); when err(Debug) and as error!(error = %e); when err only). Since Error trait implements both Debug and Display, it may create an ambiguity. I might be misunderstanding though.

@ciuncan
Copy link
Contributor Author

ciuncan commented Oct 10, 2021

hi @hawkw , I have implemented err(Debug) feel free to take a look at my PR https://github.com/tokio-rs/tracing/pull/1631/files#diff-74a47b15914675cd4c2a0c370bbec4a3d2d6e50d939dda795497ec67de050e8fR178-R181.

As an alternative, how would you regard err(?)/err(%)/err instead of err(Debug)/err(Display)/err? That would be in line with how fields are declared (these symbols are prefixed to field names, or their values if they are different, e.g. fields(?field1, %field2, field3 = ?value3, field4 = ?value4)).

@ciuncan
Copy link
Contributor Author

ciuncan commented Oct 10, 2021

Another idea: err = ?/err = %/err. We probably also extend the decision we make here to the proposed return meta in #1281.

@hawkw
Copy link
Member

hawkw commented Oct 12, 2021

But I am not sure if I understood #[instrument(err(Error))] one. How would that change the tracing::error! macro invocation? (Currently it would be invoked as error!(error = ?e); when err(Debug) and as error!(error = %e); when err only). Since Error trait implements both Debug and Display, it may create an ambiguity. I might be misunderstanding though.

The Error trait (in the form of dyn Error + 'static trait objects) is one of tracing's primitive Value types. Visitors can opt in to recording errors as dyn Error values rather than as Display or Debug values. If they implement record_error, a Visit implementation can choose to record a dyn Error using Debug or Display, and can also call Error methods like source and downcast_ref to inspect the error in a more structured way.

I think it would be good to support this as well, but it's only applicable when the error type being returned implements the Error trait, which is not always the case.

@ciuncan
Copy link
Contributor Author

ciuncan commented Oct 14, 2021

Oh, see now. I also understand why we would need to specify Error (or Value maybe?), since the behavior would be changing (using Value implementation rather than the Display implementation as existing behavior).

Is it ok, if we cover this later, in a separate issue/PR? I'd be willing to implement this as well.

@hawkw
Copy link
Member

hawkw commented Oct 14, 2021

Oh, see now. I also understand why we would need to specify Error (or Value maybe?), since the behavior would be changing (using Value implementation rather than the Display implementation as existing behavior).

Yeah, that's right! Sorry if that wasn't clear from my original comment!

Is it ok, if we cover this later, in a separate issue/PR? I'd be willing to implement this as well.

Of course. I think this should definitely be a follow-up change once the Debug PR is merged. :)

hawkw pushed a commit that referenced this issue Oct 15, 2021
## Motivation

This PR attempts to solve #1630 by introducing `err(Debug)` meta to
`intrument` attribute macro. As `err` meta causes the error (`e`)
returned by instrumented function to be passed to `tracing::error!(error
= %e)` i.e. makes it use the `Display` implementation of `e`, the newly
added `err(Debug)` makes expands to `tracing::error!(error = ?e)` which
makes the `error!` macro to use `Debug` implementation for `e`. `err`
and `err(Debug)` are mutually exclusive, adding both will create a
compilation error.

`err(Display)` is also supported to specify `Display` explicitly.

As tried to describe, for some types implementing `Error` it might be
more suitable to use `Debug` implementation as in the case of
`eyre::Result`. This frees us to manually go over the error chain and
print them all, so that `instrument` attribute macro would do it for us.

## Solution

- Added a custom keyword `err(Debug)` similar to `err`,
- Add `err(Debug)` field to `InstrumentArgs`,
- Add parsing for `err(Debug)` arg and check for conflicts with `err`,
- Generate `tracing::error!(error = ?e)` when `err(Debug)` is `true` and
  `tracing::error!(error = %e)` when `err(Display)` or `err` is `true`,
- Interpolate generated `err_block` into `Err` branches in both async
  and sync return positions, if `err` or `err(Debug)` is `true`.
@hawkw
Copy link
Member

hawkw commented Oct 15, 2021

Closed by #1631!

hawkw pushed a commit that referenced this issue Oct 22, 2021
## Motivation

This PR attempts to solve #1630 by introducing `err(Debug)` meta to
`intrument` attribute macro. As `err` meta causes the error (`e`)
returned by instrumented function to be passed to `tracing::error!(error
= %e)` i.e. makes it use the `Display` implementation of `e`, the newly
added `err(Debug)` makes expands to `tracing::error!(error = ?e)` which
makes the `error!` macro to use `Debug` implementation for `e`. `err`
and `err(Debug)` are mutually exclusive, adding both will create a
compilation error.

`err(Display)` is also supported to specify `Display` explicitly.

As tried to describe, for some types implementing `Error` it might be
more suitable to use `Debug` implementation as in the case of
`eyre::Result`. This frees us to manually go over the error chain and
print them all, so that `instrument` attribute macro would do it for us.

## Solution

- Added a custom keyword `err(Debug)` similar to `err`,
- Add `err(Debug)` field to `InstrumentArgs`,
- Add parsing for `err(Debug)` arg and check for conflicts with `err`,
- Generate `tracing::error!(error = ?e)` when `err(Debug)` is `true` and
  `tracing::error!(error = %e)` when `err(Display)` or `err` is `true`,
- Interpolate generated `err_block` into `Err` branches in both async
  and sync return positions, if `err` or `err(Debug)` is `true`.
hawkw pushed a commit that referenced this issue Oct 22, 2021
## Motivation

This PR attempts to solve #1630 by introducing `err(Debug)` meta to
`intrument` attribute macro. As `err` meta causes the error (`e`)
returned by instrumented function to be passed to `tracing::error!(error
= %e)` i.e. makes it use the `Display` implementation of `e`, the newly
added `err(Debug)` makes expands to `tracing::error!(error = ?e)` which
makes the `error!` macro to use `Debug` implementation for `e`. `err`
and `err(Debug)` are mutually exclusive, adding both will create a
compilation error.

`err(Display)` is also supported to specify `Display` explicitly.

As tried to describe, for some types implementing `Error` it might be
more suitable to use `Debug` implementation as in the case of
`eyre::Result`. This frees us to manually go over the error chain and
print them all, so that `instrument` attribute macro would do it for us.

## Solution

- Added a custom keyword `err(Debug)` similar to `err`,
- Add `err(Debug)` field to `InstrumentArgs`,
- Add parsing for `err(Debug)` arg and check for conflicts with `err`,
- Generate `tracing::error!(error = ?e)` when `err(Debug)` is `true` and
  `tracing::error!(error = %e)` when `err(Display)` or `err` is `true`,
- Interpolate generated `err_block` into `Err` branches in both async
  and sync return positions, if `err` or `err(Debug)` is `true`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/attributes Related to the `tracing-attributes` crate kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants