WrapError: wrap an error with fields to be logged by zap.Error

[prashantv]

Related to https://github.com/uber-go/guide/pull/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.

[codecov[bot]]

Codecov Report

Merging #1271 (baf6027) into master (845ca51) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1271      +/-   ##
==========================================
+ Coverage   98.08%   98.11%   +0.02%     
==========================================
  Files          50       51       +1     
  Lines        3240     3287      +47     
==========================================
+ Hits         3178     3225      +47     
  Misses         53       53              
  Partials        9        9              

Impacted Files	Coverage Δ
wrap_error.go	100.00% <100.00%> (ø)
zapcore/error.go	91.35% <100.00%> (+5.94%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[alxn]

is the receiver able to unpack the args?

[prashantv]

is the receiver able to unpack the args?

I didn't have use-cases that required unpacking, but open to it if there's a strong use-case for it.

It's also something that can be added in the future if required, so I'd rather start without.

[rabbbit]

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.

These are frequently nested. Irrespective of whether it's supported or not, perhaps it's worth calling out in the documentation and adding a test?

I also wondered if a syntax like:

err := errors.New("file doesn't exist")
...
return zap.WrapError("load config: %w", err, zap.String("fileName": "config.yaml"))

wouldn't be more natural since you can both wrap and extend the error message? I do typically extend the error message in each layer.

[prashantv]

These are frequently nested. Irrespective of whether it's supported or not, perhaps it's worth calling out in the documentation and adding a test?

Good call on nesting, I've added nesting support -- supporting multiple zap.WrapError calls, and fmt.Errorf("...: %w", err) wrapping.

I also wondered if a syntax like:

err := errors.New("file doesn't exist")
...
return zap.WrapError("load config: %w", err, zap.String("fileName": "config.yaml"))

wouldn't be more natural since you can both wrap and extend the error message? I do typically extend the error message in each layer.

We end up with 2 varargs (args to the format string, and fields). Having a single varargs where a subset is format args, and others are field hides the expectations from the type system which doesn't seem worth it for saving a few characters.

[rabbbit]

We end up with 2 varargs (args to the format string, and fields). Having a single varargs where a subset is format args, and others are field hides the expectations from the type system which doesn't seem worth it for saving a few characters.

Oh fair, I imagined we'd only let people do a single string, without the "error vararg".

I guess return zap.WrapError(error.New("load config: %w", err), zap.String("fileName": "config.yaml")) is indeed very close (but potentially slightly slower, right? Guess we don't care about errors)

Two more comments (I apparently cannot comment on non-touched lines in github?). Wanna:

• update the docstring on encodeError - it's out of date now
• clarify the behavior with errorGroup - perhaps add a test to capture this too?

[mway]

LGTM modulo returning nil if the input error is also nil.

I'm mixed on this change because I think it can:

• violate function semantics (does the function that returns an error imply that that error will "log additional fields"?)
• violate abstractions (is the user logging this error with knowledge of the logging behavior? how is this error different from other errors the user logs?)
• cause surprise ("where did these unexpected fields come from?")

but I can appreciate the value of enabling errors to be annotated without requiring users to define discrete error types IFF all they need to do with error metadata is log it. The above concerns are why I had suggested producing a Field instead of an error, but that would restrict its viability.

My only practical concern right now is if this becomes "a common way to return errors from a function", that it might get expensive. Can you paste benchmark output (unless I missed it somewhere)?