refactor: reinvestigate error handling and reporting in the system

[BugenZhao]

[!note] Tracking the tasks in the comment: https://github.com/risingwavelabs/risingwave/issues/11443#issuecomment-1790393393

---

As more and more features are introduced in our system, it turns out that error handling and reporting become increasingly messy at the same time. We're struggling with several issues, including...

• Missing context during error propagation. (#10764)
• Error's source chain gets broken when reporting error, so the actual cause for the error is lost. (#10993)
• Capture backtrace for the error everywhere multiple times, which may shadow the deepest backtrace.
• Accidentally report the backtrace to users through the wire, which is not friendly at all.

Besides, developers also find it annoying to deal with errors...

• Find it hard to categorize errors into different types or enum variants.
• Write a lot of boilerplate code just for propagating the error to the caller.
• Really verbose to construct an error. https://github.com/risingwavelabs/risingwave/blob/8d35cb77871c1a9196a66de1d60c5a3ba3c410b1/src/frontend/src/binder/expr/function.rs#L421-L428
• RwError still lives in the frontend and batch executors even though we proposed to clean it up one year ago. (https://github.com/risingwavelabs/risingwave/issues/4077)
• As a result, developers often use anyhow or bail extensively, resulting in a messy categorization of the majority of errors as internal error.

---

Based on this, I propose several guidelines on how we should deal with errors in different modules in RisingWave:

• Use snafu (or thiserror) to define errors, if a considerable usage of this error is to construct new errors, and the rest is to propagate errors attaching contexts optionally.

  • This applies to modules that reside in the lower level of RisingWave or sound like a library, like ExprError, StreamError, and OptimizerError.
  • Define ad-hoc error types as much as possible, instead of passing String everywhere.
  • Use anyhow as the fall-back variant of the error for the errors that cannot be easily categorized or can rarely happen.

• Directly wrap anyhow::Error into a new error type, if a considerable usage of this error is to propagate errors from lower-level crates without strong necessity of attaching contexts, and the rest is to construct new errors.

  • This applies to modules that sound like an application, like StreamExecutorError.
  • Backtraces will be automatically captured in this case.

[xxchan]

strong +1

some points

Error's source chain gets broken when reporting error, so the actual cause for the error is lost.

We might need to revisit this to see whether other places have the same problem. I didn't care much about source before.

Besides, developers also find it annoying to deal with errors...

We might need a developer guide (and/or crate rustdocs) for developers to follow. Even I've read a lot about error handling before, I feel still confused about how to do it. A short and actionable HOWTOs (and some simple explanations) should reduce a lot of confusions. It can be opinionated, but we need some guide.

Find it hard to categorize errors into different types or enum variants.

This is especially confusing. I'm now wondering whether categorization is needed at all.

Directly wrap anyhow::Error into a new error type

I recently noticed wasmtime::Error is like that. I had thought it should be an enum or sth, since it's a library. It turns out they use error.downcast_ref::<Trap> to get the "kind" (Trap). Might worth checking how they do it.

---

Some arbitrary refs (for myself to read later)

• https://awslabs.github.io/smithy-rs/design/rfcs/rfc0022_error_context_and_compatibility.html
• https://github.com/rust-lang/project-error-handling

[xxchan]

BTW, I like this sentence: "Be either actionable or informational" (from https://github.com/awslabs/smithy-rs/issues/1950)

At a high level, errors were refactored to:

• Be either actionable or informational. Actionable errors can be matched upon, leading to different program flow. Informational errors indicate why a failure occurred, but are not intended to be matched upon.
• No longer print error sources in Display impls. A DisplayErrorContext utility has been re-exported in the types module to easily log or print the entire error source chain.
• Reliably return their cause/source in their Error::cause/Error::source impl

P.S., I don't know whether the result of their refactor is good, since it has some learning burden for me. Haven't check its every details yet. AWS SDK is complex beasts though. 🤣

[BugenZhao]

• No longer print error sources in Display impls.

I also found this a good practice we can follow. This is also how anyhow handles Display.

https://github.com/dtolnay/anyhow/blob/7fc0c073c4c31ef8664f699e9e4883b1c92b2fb6/src/fmt.rs#L7

[BugenZhao]

• No longer print error sources in Display impls.

I also found this a good practice we can follow. This is also how anyhow handles Display.

dtolnay/anyhow@7fc0c07/src/fmt.rs#L7

However, not all libraries follows the convention. So eventually we get https://docs.rs/snafu/latest/snafu/struct.CleanedErrorText.html 🤣.

[BugenZhao]

Tracking the task:

• [x] #13215
• [ ] Simplify error construction
  • thiserror way, used for most crates
    • With Construct and ContextInto from thiserror-ext, introduction covered in #13200
    • With Macro, introduction: #13627
    • Further put them to use...
  • anyhow-wrapper way, used for connector crate now

    • 15086

    • 15042

• [x] Correctly maintain the source chain, including RPC

  • 13248

  • 13282

  • But in some cases the error is formatted into the message, check the next task.
• [ ] Format and report the error by visiting the source chain.
  • The most intuitive user-facing parts: #13264
  • Also #13348
  • Then we find it's hard to manually figure out all occurrences. Let's leverage the power of the compiler:
  • Introduction: #13750
  • Follow-ups (really a lot!)

    • 13763

    • 13870

    • ...

    • 14956

  • No need to use DisplayErrorContext anymore: #15225
• [x] #4077
  • Why it's bad: https://github.com/risingwavelabs/risingwave/pull/13588#issuecomment-1831176611
  • Remained: #2366
• [x] Misc refinements:

  • 13455

  • 13547

[xxchan]

There are some good discussions. Note here for future reference.

13215 about boxed error: benefits and limitations

13248 about thiserror: what does its attributes do; when to add #[source] and #[backtrace] (almost always)

[github-actions[bot]]

This issue has been open for 60 days with no activity. Could you please update the status? Feel free to continue discussion or close as not planned.