proposal: log/slog: add an 'error' field to 'Record'

[veqryn]

This proposal is to discuss adding an error field to Record, and potentially adding associated methods to slog and Logger.

Example adding error to Record:

type Record struct {
	// The time at which the output method (Log, Info, etc.) was called.
	Time time.Time

	// The log message.
	Message string

	// The level of the event.
	Level Level

	// The error associated with the event, if any.
	Err error // <-- this is new.  This could potentially be an `any` instead of an `error`.

// etc...
}

Possible new methods:

func (l *Logger) WithErr(err error) *Logger {
	// Returns a copy of Logger with the error field filled in
}

// Err returns an Attr for an error.
func Err(v error) Attr {
	return Attr{internalErrorKey, ErrValue(v)}
}

// internalErrorKey is private on purpose, and the string could be any unlikely string.
// When an Attr comes in with this key, the value replaces the Record's Err field instead of being added to the array of regular attributes.
const internalErrorKey = "!ERROR!!"

// ErrValue returns a Value for an error.
func ErrValue(v error) Value {
	return Value{any: v} // Could be optimized?
}

Advantages of doing this proposal:

• Handlers can choose how to handle and display errors.
• Handlers can choose whatever key they want, if any, and it will affect all errors.
• Eliminates confusion for what key to use when adding an error.
• Eliminates possibility of libraries choosing different keys. Instead there are no keys, and the handler chooses what if any key to use when displaying.

Other comments:

• Custom error can still implment log.LogValuer, fmt.Stringer, and/or json.Marshaler, and the Handler can still choose how to handle that.
• Deduplication of message keys can be handled in the exact same was as the existing Time, Level, and Message fields on Record.
• Errors are just as important and special as messages and timestamps and levels.
• By doing this proposal, we avoid a situation where 1 single application, that uses multiple libraries, ends up having log output looking like this because each library chose a different key to use for error types:

{
	"msg": "library1",
	"error": "db: bad connection"
}
{
	"msg": "library2",
	"err": "duplicate login detected"
}
{
	"msg": "library3",
	"_ERROR": "divide by zero"
}

With this proposal, the handler would choose what the key is, or if there even is a key.
This puts control back where it should be for something as important as errors.
If the log aggregation system I am using wants messages or errors to come in with a specific key or in a specific format, I can accomodate that with a custom handler.
This lets me have consistent logs, even for libraries and sub-libraries. Like this:

{
	"msg": "library1",
	"err": "db: bad connection"
}
{
	"msg": "library2",
	"err": "duplicate login detected"
}
{
	"msg": "library3",
	"err": "divide by zero"
}

thanks,
Chris

#56345 and @jba

[xgfone]

It may be discussed in #56345, and removed.

[veqryn]

It may be discussed in #56345, and removed.

Record has never had an Error field. What was removed in previous discussions was a public ErrorKey, which this proposal does Not reintroduce.

[croemmich]

This would be really nice. After coming from Logrus and Zap, I find myself missing a WithErr or even just a dedicated error attribute type, both for fewer key presses and the desire to make sure errors are always logged and aggregated the same way.

I have currently have a wrapper like this in every project using slog:

func ErrAttr(err error) slog.Attr {
	return slog.Any("error", err)
}

[jba]

We discussed this on the main proposal and decided against it. Representative comment: #56345 (comment).

[croemmich]

Disappointing. I can see the argument, but I think it fails to take the broader perspective of the importance of error content (as opposed to other structured content) and third-party libraries. I was excited about the prospect of pulling in a library that uses slog and not needing to write a logger or transform entries. While this might still be necessary for some use cases, it sure would have been nice to a least have errors work out of the box.

[veqryn]

We discussed this on the main proposal and decided against it. Representative comment: #56345 (comment).

Record contains Time, Level, and Message, which lets the handler decide what to use for keys for those.
If Record also contained Err, it would let the handler easily decide on the key for that too.

This proposal does not force a key on anyone.

[jba]

True, it doesn't export a key. But another way to look at it is that the Record.Err field isn't really necessary: if the key were exported, then anyone could look for it with Record.Attrs to get the error.

For that matter, anyone could look for a value of type error to find errors. The key doesn't matter. I could imagine a middleware handler or ReplaceAttr function that looks for error values and overrides the keys with err, err2, err3, etc. to make them uniform and unique.

Which brings up a question: how would your proposal handle multiple errors in a log call?

[veqryn]

Which brings up a question: how would your proposal handle multiple errors in a log call?

I would say that the most recent error would be kept (ie: calling WithErr replaces any previous error).
This could depend on the answer to #59365

[rsc]

We just removed the special casing of errors from slog.Err (vs slog.Info etc). It doesn't seem like it makes sense to add a special case for errors back.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[flibustenet]

I can be confusing if we have a record at error level with Err nil or a record not at error level but with an Err value.

[veqryn]

I can be confusing if we have a record at error level with Err nil or a record not at error level but with an Err value.

I don't think that is related to this proposal.

You can have an error level log, without an error currently, and also can do it with this proposal too. An example would be something that is a problem for the application (such as no more messages in the queue, or the user did something bad) that isn't caused by golang. I have such log lines in my applications, usually at only the highest level.

You can have a non-error level log (such as debug level or warn level) that does include an error currently, and also can do that with this proposal too. An example would be an error that is returned but mostly ignored, or otherwise dealt with in a way that doesn't need anyone to be woken up at night for (if you have pagerduty or something set to ping people if Error level logs lines are seen). Such as a context cancellation, which I usually log at info level if it is being handled appropriately. I have such log lines in my applications too.

[jba]

I would say that the most recent error would be kept (ie: calling WithErr replaces any previous error).

But what if I want the last one? Or both?

My point is, I think it's too limiting to have a Record.Err field. There are too many ways to log errors for slog to take a position. That is not the case for the other built-in fields (except for the minor question of what to do with an empty message).

[rsc]

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

[rsc]

No change in consensus, so declined.
— rsc for the proposal review group

[Nikola-Milovic]

My conclusion from reading the original log/slog proposal is that tying the error to slog.Error has been rejected for fair reasons. But I haven't been convinced about declining this proposal.

My point is, I think it's too limiting to have a Record.Err field. There are too many ways to log errors for slog to take a position. That is not the case for the other built-in fields (except for the minor question of what to do with an empty message).

This seems like a valid point, I personally have never had the need to do anything much different than using my own custom ErrAttr function, since usually errors are wrapped and propagated upwards, meaning, it's almost always a very clear case of how we're logging the error. My argument is that, considering that error is a first class citizen, and a primitive of the language, and the fact that so many people tried typing slog.Error() as the attribute and then we're left googling to figure out why this isn't included in the library. It goes against, at least to me, my mental model of the language. Also, considering many third-party libraries ( that we're the go-to standard prior to log/slog introduction, notably Zap and Zerolog ), provide this proposal out of the box.

Which brings up a question: how would your proposal handle multiple errors in a log call?

Since the current proposal already leaves you to implement this yourself/ to write your own error attr in some way or form, why wouldn't the multiple errors be treated the same? I'd argue that the case for multiple errors can be treated like error attribute is treated at the moment, write your own solution for handling them (eg writing your own ErrsAttribute(...error) slog.Attr) if you need special handling, like differentiating them, if not, the default behavior will occur. I haven't had enough of those situations to say what kind of output I'd expect, but error wrapping is something that people should expect from the concentration of errors (and from 1.20 I believe, wrapping errors became even easier)

Error attribute also does not tie the implementation to any logging level, so you're free to mix and match.

Having said all of that, I do agree that the error attribute does not have a clear cut solution like the others, but I cannot help but feel like I am hacking and piecing together what I would expect to be part of the library when writing my own error attributes, especially since I have to put it in some module and reach out for it every time, even though I have log/slog already imported in scope.

Please do correct me, and tell me if I am missing something, but this has been nagging me ever since I tried I tried the library some while ago. Can anyone point me to the current recommendation on how to approach error logging within the current iteration of the log/slog, are there any guidelines available?