5 ms·

This approach is so bad, I don't even know where to start. But it's all symptoms of their, sorry, incompetence. Take the loadCredentials example on top. If os.R

This approach is so bad, I don't even know where to start. But it's all symptoms of their, sorry, incompetence. Take the loadCredentials example on top. If os.ReadFile cannot find the file, it returns an error with string representation: "open cred.json: no such file or directory". This comes straight from the std lib as it is, a great error. What does the errors.Is(err, os.ErrNotExist) do: prepend "file not found" to it, rendering: "file not found: open cred.json: no such file or directory". So this adds exactly nothing. The next if will prepend "failed to read file" to it, again, adding nothing as well. The two errors checks should be replaced by one if statement, optionally wrapping it with a context string but I cannot think of any use. Then the next step, error handling of verifyCredentials. I can only guess what it does, but assume that it returns an "username 'foo bar' cannot contain spaces" error. Does prepending "invalid credentials" help anything? Nope, so the whole if can be removed as well. No surprise your errors get clunky if you make them clunky. I have more pressing things to do than dissect this article line by line, but let me suffice that I feel sorry for newcomers to the language that an article like this is so high on HN. Back in the days there was just Dave Cheney's material to read [1], and it was excellent. It's unfortunately outdated in certain regards (e.g. with new Is/As functionality in the errors package for inspection and the %w formatting directive in fmt.Errorf) but it's still an excellent article. [1]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully https://dave.cheney.net/2016/04/27/dont-just-check-errors-ha...

franticgecko3 2y ago
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.
vl 2y ago
>it returns an error with string representation: "open cred.json: no such file or directory". This comes straight from the std lib as it is, a great error. It’s a terrible error. It’s not structured, so you can’t aggregate it effectively in logs, on top of that it leaks potential secret, so you can’t return it from RPC handler.
- rollulus 2y ago
  The string representation is obviously not structured, because it's a string representation and strings are scalars. The typed representation is structured, which you can put into your structured logs as you'd like, omitting sensitive information where needed.
  - zaptheimpaler 2y ago
    The "typed" representation is an `error` type that a million other methods use with a single method that returns the string: type error interface { Error() string } It's the same unstructured goslop.
    - philosopher1234 2y ago
      This error is backed by a strict with error codes and so on.
mariusor 2y ago
As someone that ended up implementing something very similar to TFA, I'd like to ask in which way can you pass errors from 3 layers deep in your stack to the top layer and maintain context? Ie, when I can't find cred.json I want to return a 401 error, but when I can't find the entity cred.json is supposed to be owned by I want to return 404. How can one "not incompetent" Go developer solve this and distinguish between the two errors?
tetha 2y ago
> No surprise your errors get clunky if you make them clunky. From a user perspective, good errors in go make me think or Perls croak/carp. Croak and carp gave you a stacktrace of your error, but it cut out all the module-internal calls and left you with the function calls across module boundaries. Very useful - enough so that Java discovered it again later on. Personally, I wouldn't wrap the errors in loadCredentials at all. I'd just wrap the result of this method into an fmt.Errorf("failed to load credentials: %w"). This way the user knows the context the error happened in, and then we have to cross our fingers the error returned by this is good enough. But something like "application startup failed: failed to load credentials: open cred.json: no such file or directory" is a very nice error message from an application. Just enough context to know what's going on, but no 1200 line stacktrace to sift through.

franticgecko3 2y ago

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.

vl 2y ago

>it returns an error with string representation: "open cred.json: no such file or directory". This comes straight from the std lib as it is, a great error. It’s a terrible error. It’s not structured, so you can’t aggregate it effectively in logs, on top of that it leaks potential secret, so you can’t return it from RPC handler.

rollulus 2y ago
The string representation is obviously not structured, because it's a string representation and strings are scalars. The typed representation is structured, which you can put into your structured logs as you'd like, omitting sensitive information where needed.
- zaptheimpaler 2y ago
  The "typed" representation is an `error` type that a million other methods use with a single method that returns the string: type error interface { Error() string } It's the same unstructured goslop.
  - philosopher1234 2y ago
    This error is backed by a strict with error codes and so on.

mariusor 2y ago

As someone that ended up implementing something very similar to TFA, I'd like to ask in which way can you pass errors from 3 layers deep in your stack to the top layer and maintain context? Ie, when I can't find cred.json I want to return a 401 error, but when I can't find the entity cred.json is supposed to be owned by I want to return 404. How can one "not incompetent" Go developer solve this and distinguish between the two errors?

tetha 2y ago

> No surprise your errors get clunky if you make them clunky. From a user perspective, good errors in go make me think or Perls croak/carp. Croak and carp gave you a stacktrace of your error, but it cut out all the module-internal calls and left you with the function calls across module boundaries. Very useful - enough so that Java discovered it again later on. Personally, I wouldn't wrap the errors in loadCredentials at all. I'd just wrap the result of this method into an fmt.Errorf("failed to load credentials: %w"). This way the user knows the context the error happened in, and then we have to cross our fingers the error returned by this is good enough. But something like "application startup failed: failed to load credentials: open cred.json: no such file or directory" is a very nice error message from an application. Just enough context to know what's going on, but no 1200 line stacktrace to sift through.