New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Structured Error type #357
Comments
logr supports types that define a |
The klog text output currently checks for diff --git a/internal/serialize/keyvalues.go b/internal/serialize/keyvalues.go
index ad6bf11..33813b4 100644
--- a/internal/serialize/keyvalues.go
+++ b/internal/serialize/keyvalues.go
@@ -127,8 +127,6 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
writeStringValue(b, true, StringerToString(v))
case string:
writeStringValue(b, true, v)
- case error:
- writeStringValue(b, true, ErrorToString(v))
case logr.Marshaler:
value := MarshalerToValue(v)
// A marshaler that returns a string is useful for
@@ -147,6 +145,8 @@ func KVListFormat(b *bytes.Buffer, keysAndValues ...interface{}) {
default:
writeStringValue(b, false, fmt.Sprintf("%+v", value))
}
+ case error:
+ writeStringValue(b, true, ErrorToString(v))
case []byte:
// In https://github.com/kubernetes/klog/pull/237 it was decided
// to format byte slices with "%+q". The advantages of that are: However, that then only has the effect that the structured error gets formatted with
The extra information is there, but it's not getting formatted well. Improving that would be more complicated. zapr also uses diff --git a/zapr.go b/zapr.go
index 8bb7fce..eb9d44b 100644
--- a/zapr.go
+++ b/zapr.go
@@ -213,7 +213,18 @@ func (zl *zapLogger) Info(lvl int, msg string, keysAndVals ...interface{}) {
func (zl *zapLogger) Error(err error, msg string, keysAndVals ...interface{}) {
if checkedEntry := zl.l.Check(zap.ErrorLevel, msg); checkedEntry != nil {
- checkedEntry.Write(zl.handleFields(noLevel, keysAndVals, zap.NamedError(zl.errorKey, err))...)
+ field := zap.NamedError(zl.errorKey, err)
+ if logMarshaler, ok := err.(logr.Marshaler); ok {
+ func() {
+ defer func() {
+ // Ignore internal error in MarshalLog call.
+ // err will get logged as normal error field.
+ recover()
+ }()
+ field = zap.Any(zl.errorKey, logMarshaler.MarshalLog())
+ }()
+ }
+ checkedEntry.Write(zl.handleFields(noLevel, keysAndVals, field)...)
}
} The output then becomes:
@tallclair: is that good enough? I'm a bit worried about "err" changing its schema depending on the type of error. I've been told that some log processing tools get confused when that happens. We don't guarantee it for other keys either, but "err" is a pretty common one. |
It's a good point that perhaps the funcr, for example, goes out of its way to call Error() and then log the result, rather than treat it as any other key-value |
Perhaps we can log both with separate keys:
To achieve what @tallclair originally asked for (providing additional key/value pairs) we could add a new type to logr:
If, and only if, I'm undecided whether that special support should be limited to Key name conflicts could become a problem:
I think I would instead concatenate the keys while expanding:
While conceptually simple, using a map makes the output non-deterministic unless the logger sorts by key. A better type might be a slice:
I'm choosing to limit keys to strings because it simplifies the logging. We are not forced to use |
funcr has `PseudoStruct` which is similar to what you are describing.
I did it in funcr because it felt kind of dirty to try to push that
into every sink.
Maybe a construction-time flag to klog/etc like `AlsoErrorMarshalLog`
meaning, to log both err=string and errRaw=? when er supports
MarshalLog?
|
Not every sink has to support it. If they don't, they'll just log the
Are there situations where we would want such a |
I implemented logging a "structured error" as "err" + "errDetails":
This is work in progress (lacks some unit tests, depends on release of logr with those changes first), but should be complete enough to discuss whether this makes sense conceptually. The name of "errDetails" is configurable in zapr. Conceptually this is similar to how zap handles an error that implements |
Thanks for following up on this @pohly ! I want to make sure I understand how all these pieces connect. My understanding is that with your changes above, I could do something like this: type StructuredError struct {
err error
detail logr.KeysAndValues
}
func (e *StructuredError) Error() string {
return e.err.Error()
}
func (e *StructuredError) MarshalLog() interface{} {
return detail
} And then something like
Does that look right? If I understood correctly, I think this works for my use case. |
That's correct. One caveat is that there's no consensus yet on the |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Update: waiting for log/slog in Go 1.21 IMHO was worthwhile. Instead of adding a new special I've updated go-logr/zapr#56 and could do the same for klog. @tallclair: is depending on slog and thus Go 1.21 okay? |
/unassign thockin |
The following PRs are ready for merging, if we agree on the approach:
TODO: klog text format |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten ping @tallclair: still interested? |
Yes. Nothing (that I'm aware of) is blocked on this, but I've run into a few places where this would be useful, either enabling more detailed error logging, or reducing the amount of context that needs to be plumbed through. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/kind feature
Describe the solution you'd like
Sometimes I want to be able to add structured logging key-value pairs where an error is generated, but it isn't logged until way up the stack. It would be nice if structured logging information could be attached to the error itself, and logged with the error.
Rough proposal:
Open Question: should the message returned by
structuredErr.Error()
include the keyvalue pairs? In that case they should be omitted from the message when logged with klog, but that gets complicated when you have a non-structured error wrapping a structured error wrapping a non-structured error. My inclination is to not include them in the defaultError()
message for this reason.Open Question: should the
StructuredError
type be hoisted tologr
? cc @thockinThe text was updated successfully, but these errors were encountered: