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
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.
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.
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.
#[derive(Debug, thiserror::Error)]
enum MyError {
#[error(transparent)]
ThirdPartyError(#[from] third_party::Error)
}
fn some_function() -> Result<(), MyError> {
third_party::do_thing()?;
}
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
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?
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.
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
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.
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.