4 ms·

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 uni

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

vultour 2y ago
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 2y ago
  I agree; I've wasted countless hours troubleshooting errors returned in complex Go applications. The original error is not sufficient.
- tetha 2y ago
  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.
- mrj 2y ago
  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 2y ago
  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 https://sabrinajewson.org/blog/errors
mariusor 2y ago
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?
- sethammons 2y ago
  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 2y ago
    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 2y ago
      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 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 https://go.dev/play/p/pSk3s0Roysm
mattgreenrocks 2y ago
> 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 2y ago
  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 2y ago

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 2y ago
I agree; I've wasted countless hours troubleshooting errors returned in complex Go applications. The original error is not sufficient.
tetha 2y ago
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.
mrj 2y ago
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 2y ago
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 https://sabrinajewson.org/blog/errors

mariusor 2y ago

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?

sethammons 2y ago
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 2y ago
  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 2y ago
    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 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 https://go.dev/play/p/pSk3s0Roysm

mattgreenrocks 2y ago

> 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 2y ago
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.