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

Support SpanTraces #400

Open
joshka opened this issue Dec 19, 2024 · 0 comments
Open

Support SpanTraces #400

joshka opened this issue Dec 19, 2024 · 0 comments

Comments

@joshka
Copy link

joshka commented Dec 19, 2024

I was reading https://greptime.com/blogs/2024-05-07-error-rust just now via https://news.ycombinator.com/item?id=42457515 and wondered what it would take to support SpanTraces from the tracing-error crate directly in derived Error implementations. It turned out to be simpler than I expected (30 mins work to implement).

This is an example of what this makes possible:

fn main() {
    tracing_subscriber::registry()
        .with(tracing_subscriber::fmt::layer())
        .with(tracing_error::ErrorLayer::default())
        .init();

    let result = boom();
    match result {
        Err(err) => {
            eprintln!("error: {}", err);
            match err {
                Error::MyError(source, span_trace) => {
                    eprintln!("source: {}", source);
                    eprintln!("span trace: {:#?}", span_trace);
                }
            }
        }
        _ => unreachable!(),
    }
}

#[tracing::instrument]
fn boom() -> Result<(), Error> {
    inner_boom()?;
    Ok(())
}

#[tracing::instrument]
fn inner_boom() -> Result<(), Error> {
    non_span_trace()?;
    Ok(())
}

#[tracing::instrument]
fn non_span_trace() -> std::io::Result<()> {
    std::fs::read_to_string("nonexistent-file")?;
    Ok(())
}

#[derive(Error, Debug)]
enum Error {
    #[error("I/O error: {0}")]
    MyError(#[from] std::io::Error, SpanTrace),
}

Produces:

error: I/O error: No such file or directory (os error 2)
source: No such file or directory (os error 2)
span trace: SpanTrace [
    { target: "spantrace", name: "inner_boom", file: "examples/spantrace.rs", line: 32 },
    { target: "spantrace", name: "boom", file: "examples/spantrace.rs", line: 26 },
]

This is instead of changing all the various places where traced errors are needed to return TracedErrror and manually calling some_fallible_method().in_current_span()? everywhere.

PR #401 implements this, but I figured it was probably nicer to discuss the idea in an issue rather than in a PR.

The obvious downside to the implementation is that tracing-* is not 1.x yet, so realistically the code would not be able to be merged as yet (except perhaps behind an unstable feature flag / compiler flag).

Would love to hear some thoughts on this, particularly if they can suggest a direction that would make this work properly (perhaps a thiserror-tracing fork which closely tracks thiserror releases would work?)

joshka added a commit to joshka/thiserror that referenced this issue Dec 19, 2024
This was inspired by reading a blog post that showed some shortcomings
of thiserror compared to snafu in capturing backtraces.

The approach here captures a SpanTrace created by the tracing-error
crate. https://docs.rs/tracing-error/

Note: this is a fairly mechanical copy of the relevant backtrace methods
for discussion. I have not yet checked whether theres simplifications
that make sense. It also is problematic due to the unstable version of
the tracing crates, so it's doubtful that this should be merged as-is.

My goal with this PR is to start a conversation about how it might work.

Fixes dtolnay#400
joshka added a commit to joshka/thiserror that referenced this issue Dec 19, 2024
This was inspired by reading a blog post that showed some shortcomings
of thiserror compared to snafu in capturing backtraces.

The approach here captures a SpanTrace created by the tracing-error
crate. https://docs.rs/tracing-error/

Note: this is a fairly mechanical copy of the relevant backtrace methods
for discussion. I have not yet checked whether theres simplifications
that make sense. It also is problematic due to the unstable version of
the tracing crates, so it's doubtful that this should be merged as-is.

My goal with this PR is to start a conversation about how it might work.

Fixes dtolnay#400
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

Successfully merging a pull request may close this issue.

1 participant