4 ms·
The implementation of WithMeta() is flawed. Not only is it not concurrency-safe, every nested call will be modifying the parent map. The way to do this in a sa
by atombender 1y ago
The implementation of WithMeta() is flawed. Not only is it not concurrency-safe, every nested call will be modifying the parent map.
The way to do this in a safe and performant manner is to structure the metadata as a tree, with a parent pointing to the previous metadata. You'd probably want to do some pooling and other optimizations to avoid allocating a map every time. Then all the maps can be immutable and therefore not require any locks. To construct the final map at error time, you simply traverse the map depth-first, building a merged map.
I'm not sure I agree with the approach, however. This system will incur a performance and memory penalty every time you descend into a new metadata context, even when no errors are occurring. Building up this contextual data (which presumably already exists on the call stack in the form of local variables) will be constantly going on and causing trouble in hot paths.
A better approach is to return a structured error describing the failed action that includes data known to the returner, which should have enough data to be meaningful. Then, every time you pass an error up the stack, you augment it with additional data so that everything can be gleaned from it. Rather than:
val, err := GetStuff()
if err != nil {
return err
}
You do:
val, err := GetStuff()
if err != nil {
return fmt.Errorf("getting stuff: %w")
}
Or maybe:
val, err := GetStuff()
if err != nil {
return wrapWithMetadata(err, meta.KV("database", db.Name))
}
Here, wrapWithMetadata() can construct an efficient error value that implements Unwrap().
This pays the performance cost only at error time, and the contextual information travels up the stack with a tree of error causes that can be gotten with `errors.Unwrap()`. The point is that Go errors already are a tree of causes.
Sometimes tracking contextual information in a context is useful, of course. But I think the benefit of my approach is that a function returning an error only needs to provide what it knows about the failing error. Any "ambient" contextual information can be added by the caller at no extra cost when following the happy path.
- j1elo 1y agoI think this is the way to bubble up error messages that I like the most. Simple, not needing any additional tools, and very practical (sometimes even better than a stack trace). The idea is to only add information that the caller isn't already aware of. Error messages shouldn't include the function name or any of its arguments, because the caller will include those in its own wrapping of that error. This is done with fmt.Errorf(): userId := "A0101" err := database.Store(userId); if err != nil { return fmt.Errorf("database.Store({userId: %q}): %w", userId, err) } If this is done consistently across all layers, and finally logged in the outermost layer, the end result will be nice error messages with all the context needed to understand the exact call chain that failed: fmt.Printf("ERROR %v\n", err) Output: ERROR app.run(): room.start({name: "participant5"}): UseStorage({type: "sqlite"}): Store({userId: "A0101"}): the transaction was interrupted This message shows at a quick glance which participant, which database selection, and which integer value where used when the call failed. Much more useful than Stack Traces, which don't show argument values. Of course, longer error messages could be written, but it seems optimal to just convey a minimal expression of what function call and argument was being called when the error happened. Adding to this, the Go code linter forbids writing error messages that start with Upper Case, precisely because it assumes that all this will be done and error messages are just parts of a longer sentence: https://staticcheck.dev/docs/checks/#ST1005 https://staticcheck.dev/docs/checks/#ST1005
- masklinn 1y ago> This message shows at a quick glance which participant, which database selection, and which integer value where used when the call failed. And it's completely useless for looking up the errors linked to a participant in an aggregator, which is pretty much the first issue the article talks about, unless you add an entire parsing and extraction layer overtop. > Much more useful than Stack Traces, which don't show argument values. No idea how universal it is, but there are at least some languages where you can get full stackframes out of the stacktrace. That's how pytest can show the locals of the leaf function out of the box in case of traceback: def test_a(): > a(0) test.py:11: test.py:2: in a b(f) test.py:5: in b c(f, 5/g) f = 0, x = 5.0 def c(f, x): > assert f E assert 0 test.py:8: AssertionError and can do so in every function of the trace if requested: def test_a(): > a(0) test.py:11: f = 0 def a(f): > b(f) test.py:2: f = 0, g = 1 def b(f, g=1): > c(f, 5/g) test.py:5: f = 0, x = 5.0 def c(f, x): > assert f E assert 0 test.py:8: AssertionError So this is just a matter of formatting.
- deleted 1y ago[deleted]
- atombender 1y agoI agree except for the part about using strings, as you lose structure they way. You should instead return structured errors: return CouldNotStoreUser{ UserID: userId, } and now this struct is available to anyone looking up the chain of wrapped errors.
- masklinn 1y agoThis is covered by the “ergonomics” section of TFA: > With custom error structs however, it's a lot of writing to create your own error type and thus it becomes more of a burden to encourage your team members to do this. Because you need a type per layer, and that type needs to implement both error and unwrap.
- 9dev 1y agoJust a little more work, and you will reinvent exceptions with stack traces! Proper error handling in Go is now tantalisingly close!
- asp_hornet 1y agoerrors aren’t exception
- candiddevmike 1y agoGo already has exceptions with stack traces...
- dlock17 1y agoI like this, as it also maps well to using defer at the top of the function to wrap any returned errors with metadata. I've implemented something similar in my errors library relying on log/slog.Attr.