4 ms·
Being able to wrap the error with context is the key. If done right, you can identify the exactly what caused the error without any additional infrastructure.
by jrockway 3y ago
Being able to wrap the error with context is the key. If done right, you can identify the exactly what caused the error without any additional infrastructure. (For people that say stack traces do this, they don't. They don't tell you which iteration of the for loop you're on. They don't tell you what the code on the other side of the channel you read the error from was doing.)
My rule for errors is to add only information that the caller isn't aware of. So don't do:
func GetFooByID(ctx context.Context, id int) (Foo, error) {
row, err := LookupRow(ctx, "foo", id)
if err != nil {
return nil, errors.Wrapf(err, "GetFooByID id=%v", id)
}
return row, nil
}
The caller already knows the id it's looking up, and that the name of the function is GetFooByID. It might include those in its wrapping of that error, but GetFooByID shouldn't.
Meanwhile, this is good:
func UpgradeFoos(ctx context.Context, tx *Tx) error {
foos, err := GetAll(ctx, tx, "foos")
if err != nil {
return errors.Wrap(err, "GetAll")
}
for _, foo := range foos {
if err := UpgradeFoo(ctx, tx, foo.ID); err != nil {
return errors.Wrapf(err, "UpgradeFoo(%v)", foo.ID)
}
}
return nil
}
Now your logs look something like "server failed to startup error=apply migration: upgrade foos: UpgradeFoo(42): i/o timeout" instead of "server failed to startup error=i/o timeout". This saves you from "hmm, maybe it's too slow to get all the foos in a batch like that, we should change that". But nope, it's actually UpgradeFoo(42) that's broken.
Wrapping applied well is my favorite feature of Go. Most people don't do it. The standard library doesn't follow my rules. But if you do it this way, every error you see is so easy to debug and resolve. Less downtime, more reliability.
(As an aside, that loop where you accumulate errors can easily accumulate into a multi-error as of recent versions of Go. I always prefer to try everything possible and return all the errors, then you can fix multiple problems with your input on one go. It's also good for cases where you are doing a main operation and an ancillary operation, like closing something. People often ignore errors on "Close" and "Sync", but by joining those into a multierror, then you no longer have questions like "there were no errors, but this file isn't on disk". https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functions https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functio... is a really nice approach for the common case of "defer fh.Close()". I have an `errors.Close` wrapper I use: https://github.com/pachyderm/pachyderm/blob/master/src/internal/errors/multi.go#L26 https://github.com/pachyderm/pachyderm/blob/master/src/inter.... Can't live without it!)
- jjnoakes 3y agoI think solving the context problem is key. I've seen a few proposals for go error handling that address the verbosity while also allowing you to provide proper local context in a terse and readable way. I'm hoping something like that makes its way into go eventually.
- CWIZO 3y agoI like this. Do you have any real life examples to share or a blog diving deeper into it? We have no standards for this at our place and I think this might be a nice starting point to introduce something.