RFC: special types: structured error #56
Conversation
pohly
force-pushed
the
special-types
branch
from
4cbab42 to
2d9d936
Compare
pohly
changed the title
pohly
force-pushed
the
special-types
branch
2 times, most recently
from
ed3114f to
1809a60
Compare
thockin
left a comment
thockin
left a comment
There was a problem hiding this comment.
I don't know zap well, but I am fine with this, I think. A few questions.
| return zap.Skip() | ||
| } | ||
| return zap.Inline(zapcore.ObjectMarshalerFunc(func(encoder zapcore.ObjectEncoder) error { | ||
| // Always log as a normal error first. |
There was a problem hiding this comment.
So this logs both? If someone implemented LogValuer, I would expect ONLY that, no?
There was a problem hiding this comment.
If it did that under the key for the structured error value, then we would end up with "err" changing it's content from string (normal errors) to struct (structured error). We decided against that in kubernetes/klog#357 (comment).
There was a problem hiding this comment.
I updated the PR with documentation.
As we now agree on this approach I'll make the same change in the klog text logger. I'm undecided whether this should be documented in the logr docs. It's an implementation detail of some logging backends, but the more backends agree on this as a convention, the better.
There was a problem hiding this comment.
/hold
- extend textlogger
- extend klog with a helper which constructs a structured error (e.g.
ErrWithDetails(err error, details any) error- need to think the details (sic!) and what type they should have).
zapr_slog_test.go
Outdated
| } | ||
|
|
||
| type structuredError struct { | ||
| error |
There was a problem hiding this comment.
from an obviousness POV, I would just write this as an empty struct, with an Error() method and an LogValue() method.
A "structured error" is an error which implements error and slog.LogValuer. When encountering such an error, it gets logged normally and then another field gets added with the result of LogValue(). The name of the extra field is the original field name plus a configurable suffix, with "Details" as default. The extra details are logged as if they had been passed as a value to zapr. slog.Value.Resolve is used to protect against errors and recursion while calling LogValue, but does not protect against recursion that can occur when LogValue returns the original error: then zapIt->zapError->zapIt->... repeats until the program gets killed. A simple guard against this (not expanding error again while formatting an error) is too simplistic and would prevent nice rendering of a wrapped error that might get returned by MarshalLog. zapIt would have to track the exact error instance and detect when it gets the same value again.
| return zap.Skip() | ||
| } | ||
| return zap.Inline(zapcore.ObjectMarshalerFunc(func(encoder zapcore.ObjectEncoder) error { | ||
| // Always log as a normal error first. |
There was a problem hiding this comment.
If it did that under the key for the structured error value, then we would end up with "err" changing it's content from string (normal errors) to struct (structured error). We decided against that in kubernetes/klog#357 (comment).
zapr_slog_test.go
Outdated
| } | ||
|
|
||
| type structuredError struct { | ||
| error |
| return zap.Skip() | ||
| } | ||
| return zap.Inline(zapcore.ObjectMarshalerFunc(func(encoder zapcore.ObjectEncoder) error { | ||
| // Always log as a normal error first. |
There was a problem hiding this comment.
I updated the PR with documentation.
As we now agree on this approach I'll make the same change in the klog text logger. I'm undecided whether this should be documented in the logr docs. It's an implementation detail of some logging backends, but the more backends agree on this as a convention, the better.
2 participants
This is based on #60 and adds support for "structured errors" (see kubernetes/klog#357).