Handle errors once #179

abhinav · 2023-04-10T16:35:18Z

Adds new guidance on how handling errors once
rather than double handling, e.g. log-and-return.

Adds new guidance on how handling errors once rather than double handling, e.g. log-and-return. Resolves uber-go#65

alxn · 2023-04-10T18:44:13Z

The other note I like, by wrapping errors you are able to specifically test you are hitting that wrapped case. If you have a module that just passes through errors, future changes may cause those paths to change, and you to lose test coverage.

mway

Looks good so far. I'm split on whether it's worth discussing error boundaries (at a high level), maybe through examples (specifically thinking of your degradation examples).

The general idea being that some confusion around error handling may stem from not really understanding what should really "care" about an error (i.e., what should that error's terminal handler be), and examples that help folks think about how to model that (through the lens of error boundaries/SOC) may help as well.

Thoughts?

abhinav · 2023-04-10T20:05:37Z

Could you clarify what you're imagining here, @mway?
I'm having trouble coming up with how to communicate the idea of error boundaries succinctly.
"Handle errors at abstraction boundaries" or variations of it seem a bit too high-level.

sywhang

Overall I like it. Though it may get a little blurry in terms of what errors are ok to handle and what aren't in the Match the error and degrade gracefully section without some kind of guidance on error boundaries as @mway suggested.

For instance suppose someone has code similar to the example, except getUser is an external pkg:

u, err := someClient.getUser(id)

Then does one match for canonical error types defined by someClient and nothing else, like:

if errors.Is(err, someClient.ErrUserInactive) {
   // ...
}

, or does one check for any kind of commonly-occurring errors returned from someClient.getUser regardless of whether the error is wrapped or not

if errors.Is(err, fs.PathError) {
   // ...
}

abhinav · 2023-04-10T22:32:49Z

does one match for canonical error types defined by someClient and nothing else, [..]
or does one check for any kind of commonly-occurring errors returned from someClient.getUser regardless of whether the error is wrapped or not

Okay, yeah, this is where the confusion might be coming from.
I assumed it was obvious that you would check for whatever errors someClient.GetUser is documented as returning that you want to have special behaviors for. You will not check for fs.FooError if GetUser doesn't say that's a thing it can return, even if it uses that internally and possibly wraps it.

I'll see if there's a way to expand the match errors section.

Reorders the list of ways an error can be handled, going from most specific to least specific, and makes the examples more realistic for those cases.

abhinav · 2023-04-12T03:21:56Z

@mway @sywhang Updated to try to clarify error matching more.

I also want to clarify my intent here to help future discussion.
This is not intended to be a comprehensive list of different ways of handling errors.
It's a statement that when you handle errors (of which there are many ways, here are a few), you should do it only once.

If you think that that intent is getting lost because we enumerate some ways of handling errors, I'm happy to pare down the entry and drop those extraneous details.

alxn · 2023-04-12T03:47:12Z

src/error-once.md

@@ -0,0 +1,112 @@
+# Handle Errors Once
+
+When a function receives an error from a caller,


how does a function receive an error from a caller or am I being stupid here?

🤦 oops. yeah, callee.

alxn · 2023-04-12T03:50:49Z

src/error-once.md

+Regardless of how the error is handled, generally,
+a function should handle an error only once.
+It should not, for example, log the error and then return it again
+because its callers will likely handle the error too.


oh, by handle you mean "take an action", where action 1 is log, and action 2 is return.

or you mean "a function should touch a received error only once."

I meant "take an action". Let me reword a tad bit.

Related to uber-go/guide#179 Callsites that receive an error should either log, or return an error. However, if the callsite has additioanl context, the simplest option is to add it to the error, but it's then flattened into a string, losing the benefit of structured logging. This often results in callsites logging with additional fields, and returning an error that is likely to be logged again. `WrapError` provides a way for callsites to return an error that includes fields to be logged, which will be added to an `errorFields` key.

prashantv

+1 on the guidance

prashantv · 2023-04-12T04:28:23Z

src/error-once.md

+  log.Printf("Could not get user %q: %v", id, err)
+  return err


(tangentially related)

I often see this with structured logging when you want id as a separate field, but returning an error forces you to flatten the id into the error string, or you have to add an additional return of what fields to log (making it much more complex)

I've been meaning to solve it for a while, and think it can be solved relatively simply,
uber-go/zap#1271

prashantv · 2023-04-12T04:29:06Z

src/error-once.md

+
+These include, but not are limited to:
+
+- if the function contract defines specific errors,


nit: function can refer to the caller or callee, what do you think of explicitly using those terms throughout? "if the callee function defines specific errors"

oh good call. will change.

src/error-once.md

Related to uber-go/guide#179 Callsites that receive an error should either log, or return an error. However, if the callsite has additioanl context, the simplest option is to add it to the error, but it's then flattened into a string, losing the benefit of structured logging. This often results in callsites logging with additional fields, and returning an error that is likely to be logged again. `WrapError` provides a way for callsites to return an error that includes fields to be logged, which will be added to an `errorFields` key.

Instead of "the function" everywhere, clarify caller/callee relationships.

mway

Nice, thanks @abhinav! LGTM modulo a couple quick suggestions.

src/error-once.md

Co-authored-by: Matt Way <mway@users.noreply.github.com>

abhinav · 2023-04-13T17:00:22Z

Thanks for the reviews, folks. Will add a changelog entry before merging.

feat: Handle errors once

6f4c53e

Adds new guidance on how handling errors once rather than double handling, e.g. log-and-return. Resolves uber-go#65

alxn self-assigned this Apr 10, 2023

mway reviewed Apr 10, 2023

View reviewed changes

abhinav marked this pull request as ready for review April 10, 2023 19:50

sywhang reviewed Apr 10, 2023

View reviewed changes

Clarify when errors should be matched

aeb880e

Reorders the list of ways an error can be handled, going from most specific to least specific, and makes the examples more realistic for those cases.

abhinav force-pushed the error-once branch from 7b8bb71 to aeb880e Compare April 12, 2023 03:18

alxn reviewed Apr 12, 2023

View reviewed changes

prashantv mentioned this pull request Apr 12, 2023

WrapError: wrap an error with fields to be logged by zap.Error uber-go/zap#1271

Open

prashantv reviewed Apr 12, 2023

View reviewed changes

abhinav added 4 commits April 12, 2023 05:17

caller => callee

9114524

clarify handle once guidance

d269146

Use caller, callee

0cbb0e6

Instead of "the function" everywhere, clarify caller/callee relationships.

make style.md

9ffd194

mway approved these changes Apr 13, 2023

View reviewed changes

src/error-once.md Outdated Show resolved Hide resolved

src/error-once.md Outdated Show resolved Hide resolved

abhinav and others added 3 commits April 13, 2023 09:55

return and wrap in one item

bf126d6

Co-authored-by: Matt Way <mway@users.noreply.github.com>

callers may handle

e5a06e4

Co-authored-by: Matt Way <mway@users.noreply.github.com>

make style.md

bde62c8

sywhang approved these changes Apr 13, 2023

View reviewed changes

CHANGELOG

0aca677

abhinav merged commit 42f94b6 into uber-go:master Apr 13, 2023
1 check passed

abhinav deleted the error-once branch April 13, 2023 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle errors once #179

Handle errors once #179

abhinav commented Apr 10, 2023 •

edited

alxn commented Apr 10, 2023

mway left a comment

abhinav commented Apr 10, 2023

sywhang left a comment

abhinav commented Apr 10, 2023

abhinav commented Apr 12, 2023

alxn Apr 12, 2023

abhinav Apr 12, 2023

alxn Apr 12, 2023

abhinav Apr 12, 2023

prashantv left a comment

prashantv Apr 12, 2023

prashantv Apr 12, 2023

abhinav Apr 12, 2023

mway left a comment

abhinav commented Apr 13, 2023

		@@ -0,0 +1,112 @@
		# Handle Errors Once

		When a function receives an error from a caller,


		These include, but not are limited to:

		- if the function contract defines specific errors,

Handle errors once #179

Handle errors once #179

Conversation

abhinav commented Apr 10, 2023 • edited

alxn commented Apr 10, 2023

mway left a comment

Choose a reason for hiding this comment

abhinav commented Apr 10, 2023

sywhang left a comment

Choose a reason for hiding this comment

abhinav commented Apr 10, 2023

abhinav commented Apr 12, 2023

alxn Apr 12, 2023

Choose a reason for hiding this comment

abhinav Apr 12, 2023

Choose a reason for hiding this comment

alxn Apr 12, 2023

Choose a reason for hiding this comment

abhinav Apr 12, 2023

Choose a reason for hiding this comment

prashantv left a comment

Choose a reason for hiding this comment

prashantv Apr 12, 2023

Choose a reason for hiding this comment

prashantv Apr 12, 2023

Choose a reason for hiding this comment

abhinav Apr 12, 2023

Choose a reason for hiding this comment

mway left a comment

Choose a reason for hiding this comment

abhinav commented Apr 13, 2023

abhinav commented Apr 10, 2023 •

edited