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

Ergonomic Reporting of std::error::Error #1308

Open
theduke opened this issue Mar 17, 2021 · 6 comments
Open

Ergonomic Reporting of std::error::Error #1308

theduke opened this issue Mar 17, 2021 · 6 comments

Comments

@theduke
Copy link

theduke commented Mar 17, 2021

Feature Request

Find some more ergonomic way to report std::error::Error values.

Crates

  • tracing

Motivation

Logging a type that implements std::error::Error is currently quite awkward.
Value is only implemented for dyn std::error::Error.

That means logging a concrete type involves awkward type gymanstics for something that is a very common operation.

(unless I'm missing some trick here...)

EG:

// Inline
info!(error = &err as &dyn std::error::Error, "msg");

// Helper variable
let error: &dyn std::error::Error = &err;
info!(error, "msg");

Proposal

Not sure what would be the best way to implement this.
A generic default impl is probably out of the question without specialization.

The most straight-forward solution would probably be a new formatter sigil, eg !.

warn!( error = !err, "msg");
@lilyball
Copy link
Contributor

lilyball commented Nov 4, 2021

A related issue is this is only implemented for dyn Error + 'static but it should also have implementations for dyn Error + Send + 'static, dyn Error + Sync + 'static, and dyn Error + Send + Sync + 'static, as those are all unique types.

As an example of where this is annoying, if I have an eyre::Report, it derefs to dyn Error + Send + Sync + 'static, but I can't say info!(err = &*err, "foo"), I still have to say info!(err = &*err as &dyn std::error::Error, "foo") just to drop the Send/Sync bounds.

@lilyball
Copy link
Contributor

lilyball commented Mar 8, 2022

record_error should probably also drop the 'static bound, we don't require other recorded types to be 'static so we should be able to record non-static errors too.

@hawkw
Copy link
Member

hawkw commented Mar 8, 2022

record_error should probably also drop the 'static bound, we don't require other recorded types to be 'static so we should be able to record non-static errors too.

The 'static bound is intended to permit downcasting the error, which would not be possible otherwise.

@lilyball
Copy link
Contributor

lilyball commented Mar 8, 2022

Ahh right, downcasting relies on TypeId and that's only available for 'static.

@hawkw
Copy link
Member

hawkw commented Mar 8, 2022

Right, this is also why the standard library deprecated Error::cause in favor of Error::source, which returns an Option<&(dyn Error + 'static)>. In general, I think it's normally a standard practice to avoid non-'static Errors for this reason. We could consider adding support for errors that are not valid for the 'static lifetime, but if we did this, it would probably have to be via a separate method or something, because we definitely don't want to remove the ability to downcast errors in the common case.

lilyball added a commit to lilyball/tracing that referenced this issue Apr 11, 2022
`Value` was already implemented for `dyn Error + 'static`, but rust
doesn't silently coerce trait objects. This means that passing an error
of type `dyn Error + Send + Sync + 'static` would not work. These extra
impls just delegate to the existing `dyn Error + 'static` impl.

Also update one of the examples to use `dyn Error + Send + Sync` to
demonstrate that this works.

Refs: tokio-rs#1308
lilyball added a commit to lilyball/tracing that referenced this issue Apr 11, 2022
`Value` was already implemented for `dyn Error + 'static`, but rust
doesn't silently coerce trait objects. This means that passing an error
of type `dyn Error + Send + Sync + 'static` would not work. These extra
impls just delegate to the existing `dyn Error + 'static` impl.

Also update one of the examples to use `dyn Error + Send + Sync` to
demonstrate that this works.

Refs: tokio-rs#1308
lilyball added a commit to lilyball/tracing that referenced this issue Apr 11, 2022
`Value` was already implemented for `dyn Error + 'static`, but rust
doesn't silently coerce trait objects. This means that passing an error
of type `dyn Error + Send + Sync + 'static` would not work. These extra
impls just delegate to the existing `dyn Error + 'static` impl.

Also update one of the examples to use `dyn Error + Send + Sync` to
demonstrate that this works.

Refs: tokio-rs#1308
hawkw pushed a commit that referenced this issue Apr 11, 2022
## Motivation

`Value` was already implemented for `dyn Error + 'static`, but rust
doesn't silently coerce trait objects. This means that passing an error
of type `dyn Error + Send + Sync + 'static` would not work. This is
related to #1308.

## Solution

Add impls for `dyn Error + …` variants for `Send`, `Sync`, and `Send +
Sync`. These extra impls just delegate to the existing `dyn Error +
'static` impl.

Also update one of the examples to use `dyn Error + Send + Sync` to
demonstrate that this works.

Refs: #1308
hawkw pushed a commit that referenced this issue Apr 12, 2022
## Motivation

`Value` was already implemented for `dyn Error + 'static`, but rust
doesn't silently coerce trait objects. This means that passing an error
of type `dyn Error + Send + Sync + 'static` would not work. This is
related to #1308.

## Solution

Add impls for `dyn Error + …` variants for `Send`, `Sync`, and `Send +
Sync`. These extra impls just delegate to the existing `dyn Error +
'static` impl.

Also update one of the examples to use `dyn Error + Send + Sync` to
demonstrate that this works.

Refs: #1308
hawkw added a commit that referenced this issue Apr 12, 2022
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
#2066, but it may be useful for other boxed values as well.

Refs: #1308
hawkw added a commit that referenced this issue Apr 14, 2022
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
#2066, but it may be useful for other boxed values as well.

Refs: #1308
hawkw added a commit that referenced this issue Apr 14, 2022
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
#2066, but it may be useful for other boxed values as well.

Refs: #1308
hawkw added a commit that referenced this issue Apr 14, 2022
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
#2066, but it may be useful for other boxed values as well.

Refs: #1308
hawkw added a commit that referenced this issue Apr 14, 2022
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
#2066, but it may be useful for other boxed values as well.

Refs: #1308
@Swatinem
Copy link
Contributor

Apart from this not being ergonomic (yet), the problem is also that the Rust compiler errors are not really helpful in this case.
Neither are the main tracing docs.

A more prominent example that shows users how to do it would be a really good idea. We have too many cases in our codebase where we just use the Display/Debug formatting as a workaround because people are not aware that tracing actually supports std::error::Error directly, and how to use that.

It might be a good idea to explicitly call out in the docs, How to capture a std::error::Error.

The only example in the main crate docs is hidden under the For log users heading, and that example conveniently uses Box<dyn Error> which just works.

jtescher pushed a commit to tokio-rs/tracing-opentelemetry that referenced this issue Sep 29, 2023
## Motivation

Currently, the tracing instrumentation macro emits a single event after
the function call, but in the current span, with just a field named
error set.

For example:
```rust
#[instrument(err)]
fn test() -> Result<(), ()> {
...body
}
```
gets roughly expanded to 
```rust
fn test() -> Result<(), ()> {
  let span = span!("test")
  fn inner() -> Result<(), ()> {
  ...body
  }
  match inner() {
    Ok(x) => Ok(x),
    Err(err) => {
    error!(error=%err)
    Err(err)
  }
}
```

In the error case of the result, the macro will emit an error level
event with just an `error` field set to the display (or debug) value of
the returned error.

While there exists support for the Error primitive in tracing, the
primitive only supports 'static Errors. See
tokio-rs/tracing#1308


## Solution

This PR adds support to use this event to fill the span status error
description with the content of the error field of this event.
Additionally, this ass support to emit these events (or manually created
ones that follow the same format) as OTel events following the exception
convention.
The operation is optional and can be configured using the
`ErrorFieldConfig`.

This seems like another hack similar to `otel.*` fields, but should
reduce some boilerplate in existing codebases.
I propose to keep this until `tracing` improves support for Error
fields.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
This commit adds a `Value` implementation for `Box<T> where T: Value`.
This is *primarily* intended to make `Box<dyn Error + ...>` implement
`Value`, building on the `Value` impls for `dyn Error + ...` added in
tokio-rs#2066, but it may be useful for other boxed values as well.

Refs: tokio-rs#1308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants