doc/dev: document our approach to error handling/printing

[aljoscha]

Motivation

Socialize the changes implemented in #18583 and #18632, and make sure that we have a more coherent strategy going forward.

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered.
• This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
• This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
• If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
• This PR includes the following user-facing behavior changes:

[aljoscha]

I added all (currently known to me) TLs, to let you voice concerns and/or learn about this.

[maddyblue]

Does the Display trait from thiserror follow this guideline? If so, this paragraph should be more obviously linked to the previous one. As a non-expert in rust errors, it's unclear to me what I need to do exactly, even if I use thiserror. I think these 3 paragraphs could be made more clear with a one sentence happy path: "use thiserror" and then have the other paragraphs maybe in a separate section documenting what to do if you can't use thiserror. That'd make it super clear to a reader what they should probably do.

[aljoscha]

That's very good feedback! I'll try that

[maddyblue]

I'm confused, because afaik, we only ever trace/log errors or surface them to users. Are there other ways we print errors? When surfacing to users they all get converted into a hopefully structured SQL error, and (almost?) never have their chain included. I've read this paragraph a few times and don't understand when I'd want to use those functions still.

[aljoscha]

I should have included more context here or in the PR description! This whole thing got started by #18490 (comment).

As to where we print errors and where we use the _with_causes variants, I'll try and list the different "flavours" we have, with an example. The only real user facing errors should be (as you pointed out) the errors we report back via the SQL client/pgwire and the errors that we report in the error/status history collections.

Errors that we report in mz_source_status_history (and related friends and versions for sinks):

materialize/src/storage/src/sink/kafka.rs

Line 907 in 2191ed5

error: format!("{}", error.display_with_causes()),

Structured SQL errors which include a chain of errors in the details, or in the error itself:

materialize/src/adapter/src/error.rs

Line 266 in acc39ea

storage_error.source().map(|source_error| source_error.to_string_with_causes())

and

materialize/src/sql/src/plan/error.rs

Line 180 in 86cc780

Self::FetchingCsrSchemaFailed { cause, .. } => Some(cause.to_string_with_causes()),

A lot of persist errors, which we mostly log internally, or when panick'ing:

materialize/src/persist-client/src/internal/compact.rs

Line 378 in 1804102

err.display_with_causes()

Top-level error handlers for the processes:

materialize/src/environmentd/src/bin/environmentd/main.rs

Line 532 in 22d35d5

eprintln!("environmentd: {}", err.display_with_causes());

Some that might be pathological cases, where we already have the chain of errors in the unstructured error itself:

materialize/src/adapter/src/error.rs

Line 459 in acc39ea

AdapterError::Unstructured(e) => write!(f, "{}", e.display_with_causes()),

materialize/src/sql/src/plan/error.rs

Line 466 in 86cc780

sql_err!("{}", e.display_with_causes())

[teskje]

This seems reasonable to me.

In the compute controller, we put some effort into defining separate thiserror-based error types to reflect that different controller methods can return different sets of errors. The idea is that if a method cannot return a given error variant that variant should not appear in the method's error type. The benefit is that callers know exactly what types of errors to expect, which isn't the case when every method returns the same large error enum. The drawback is that Rust demands a bunch of boilerplate to define all the different error enums (roughly one per API method), so I'm not sure if this is something we should make a general recommendation.

[aljoscha]

I changed the first paragraph to make it more obvious what you should do in the common case. Please take another look 🙏