[franticgecko3]

I'm worried readers of this article will be horrified and believe this kind of DIY error handling is necessary in Go.

The author has attempted to fix their unidiomatic error handling with an even more unidiomatic error framework.

New Go users: most of the time returning an error without checking its value or adding extra context is the right thing to do

[mattgreenrocks]

> New Go users: most of the time returning an error without checking its value or adding extra context is the right thing to do

Thank you.

Feels like Go is having its Java moment: lots of people started using it, so questions of practice arise despite the language aiming at simplicity, leading to the proliferation of questionable advice by people who can't recognize it as such. The next phase of this is the belief that the std library is somehow inadequate even for tiny prototypes because people have it beaten over their heads that "everybody" uses SuperUltraLogger now, so it becomes orthodox to pull that dependency in without questioning it.

After a bunch of iterations of this cycle, you're now far away from simplicity the language was meant to create. And the users created this situation.

[int_19h]

Go is having a Go moment: lots of people using it are realizing that other programming languages have all that complexity for a reason, and that "aiming at simplicity" by aggressively removing or ignoring well-established language features often results in more complicated code that's easier to get wrong and harder to reason about.

[vultour]

From my experience this is not the case. If you error out 7 functions deep and only return the original error there's no chance you're figuring out where it happened. Adding context on several levels is basically a simplified stack trace which lets you quickly find the source of the error.

[richbell]

I agree; I've wasted countless hours troubleshooting errors returned in complex Go applications. The original error is not sufficient.

[mrj]

I inherited a codebase with the same problem. After a few debugging sessions where it wasn't clear where the error was coming from, I decided the root problem was that we didn't have stack traces.

Fortunately, the code was already using zap and it had a method for doing exactly that:

zap.AddStacktrace(zap.LevelEnablerFunc(func(lvl zapcore.Level) bool { return lvl >= zapcore.InfoLevel }))

Because most of the time if there's an error, you'd likely want to log it out. Much of the code was doing this already, so it made sense to ensure we had good stack traces.

There's overhead to this, but in our codebase there was a dearth of logging so it didn't matter much. Now when things are captured we know exactly where it happened without having to do what the post is doing manually... adding stack info.

[mplanchard]

We actually went through the same realization when we started writing Rust a few years ago. The `thiserror` crate makes it easy to just wrap and return an error from some third-party library, like:

    #[derive(Debug, thiserror::Error)]
    enum MyError {
      #[error(transparent)]
      ThirdPartyError(#[from] third_party::Error)
    }

Since it derives a `From` implementation, you can use it as easily as:

    fn some_function() -> Result<(), MyError> {
      third_party::do_thing()?;
    }

But if that's happening somewhere deep in your application and you call that function from more than one place, good luck figuring out what it is! You wind up with an error log like `third_party thing failed` and that's it.

Generally, we now use structured error types with context fields, which adds some verbosity as specifying a context becomes required, but it's a lot more useful in error logs. Our approach was significantly inspired by this post from Sabrina Jewson: https://sabrinajewson.org/blog/errors

[tetha]

It's not a binary decision though. Just because the article arrives at overkill for most things in my opinion doesn't mean sentinel errors or wrapping errors in custom types should be avoided at all costs in all situations.

In my experience, it's good and healthy to introduce this additional context on the boundaries of more complex systems (like a database, or something accessing an external API and such), especially if other code wants to behave differently based on the errors returned (using errors.Is/errors.As).

But it's completely not necessary for every single plumping function starts inspecting and wrapping all errors it encounters, especially if it cannot make a decision on these errors or provide better context.

[mariusor]

Do you maybe have a constructive advice for people that need to return errors that demand different behaviour from the calling code?

I gave an example higher in the thread: if searching for the entity that owns the creds.json files fails, we want to return a 404 HTTP error, but if creds.json itself is missing, we want a 401 HTTP error. What would be the idiomatic way of achieving this in your opinion?

[tetha]

With some of these examples, I'd change the API of the lower-level methods. Instead of a (Credentials, err) and the err is a NotFound sometimes, I'd rather make it a (*Credentials, bool, err) so you can have a (creds, found, err), and err would be used for actual errors like "File not found"/"File unreadable"/...

But other than that, there is nothing wrong with having sentinel errors or custom error types on your subsystem / module boundaries, like ErrCredentialsNotFetched, ErrUserNotFound, ErrFileInvalid and such. That's just good abstraction.

The main worry is: How many errors do you actually need, and how many functions need to mess about with the errors going around? More error types mean harder maintenance in the future because code will rely on those. Many plumbing or workflow functions probably should just hand the errors upwards because they can't do much about it anyways.

A lot of the details in the errors of the article very much feel like business logic and API design is getting conflated with the error framework.

Is "Cannot edit a whatsapp message template more than 24 hours" or "the users account is locked" really an error like "cannot open creds.json: permission denied" or "cannot query database: connection refused"? You can create working code like that, but I can also use exceptions for control flow. I'd expect these things to come from some OpenAPI spec and some controller-code make this decision in an if statement.

[sethammons]

Use errors.Is and compare to the returned err to mypkg.ErrOwnerNotExists and mypkg.ErrMissingConfig and the handler decides which status code is appropriate

[mariusor]

Cool, but error.Is what? In my case would both come as a os.NotExist errors because both are files on the disk.

I think that the original dismissal I replied to, might not have taken into account some of the complexities that OP most likely has given thought to and made decisions accordingly. Among those there's the need to extract or append the additional information OP seems to require (request id, tracking information, etc). Maybe it can be done all at the top level, but maybe not, maybe some come from deeper in the stack and need to be passed upwards.

[sethammons]

no no no; do not return os.NotExists in both cases. The function needs to handle os.NotExists and then return mypkg.ErrOwnerNotExists or mypkg.ErrMissingConfig (or whatever names) depending on the state in the function.

The os.NotExists error is an implementation detail that is not important to callers. Callers shouldn't care about files on disk as that is leaking abstraction info. What if the function decides to move those configs to s3? Then callers have to update to handle s3 errors? No way. Return errors specific to your function that abstract the underlying implementation.

Edit: here is some sample code https://go.dev/play/p/vFnx_v8NBDf

Second edit: same code, but leveraging my other comment's kverr package to propagate context like kv pairs up the stack for logging: https://go.dev/play/p/pSk3s0Roysm

[mariusor]

Exactly, and that's what OP argues for, albeit in a very complex manner.

Distilling their implementation to the basics, that's what we get: typed errors that wrap the Go standard library's ones with custom logic. Frankly I doubt that the API your library exposes (kv maps) vs OPs typed structs, is better. Maybe their main issue is relying on stuffing all error types in the same module, instead of having each independent app coming up with their own, but probably that's because they need the behaviour for handling those errors at the top of the calling stack is uniform and has only one implementation.

A quick back of the napkin list for what an error needs to contain to be useful in a post execution debugging context would be:

* calling stack

* traceability info like (request id, trace id, etc)

* data for the handling code to make meaningful distinction about how to handle the error

I think your library could be used for the last two, but I don't know how you store calling stack in kv pairs without some serious handwaving. Also kv is unreliable because it's not compile time checked to match at both ends.

[sethammons]

I'm not saying use kverr for explicit error handling (like, you could, but that is non ideal), use kverr as a context bag of data you want to capture in a log. If you programmatically are routing with untyped string data, I agree, unreliable