Add slog compatibility by StevenACoffman · Pull Request #97 · bracesdev/errtrace

StevenACoffman · 2024-04-15T00:46:24Z

Go 1.21 adds the log/slog package and this PR adds convenience and compatibility with that package. Merging this PR would require dropping Go 1.20 support, which may not (yet!) be desired. If so, this PR can wait until that is desired.

Each major Go release is supported until there are two newer major releases. Go 1.22 was released on February 6, 2024
so Go 1.20 is no longer supported.

abhinav · 2024-04-15T04:53:40Z

Hey, @StevenACoffman!
We're willing to drop support for Go 1.20 at this point. We tend to follow the upstream support policy for most of our projects (current and last minor Go are supported) with the addendum that we're willing to keep support for older versions around if it's not too difficult. (Sometimes folks lag behind on upgrades.) But we're happy to bump to minimum supported version when the need arises.

abhinav

Prior comment notwithstanding, I think we can do this without bumping the Go version and just moving the two new functions into a Go 1.21 tagged file.

abhinav · 2024-04-15T04:55:33Z

errtrace.go

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


Thinking out loud:
The number of times "err" appears in this expression is quite high:

errtrace.ErrAttr(err)

What if we named this LogAttr?

slog.Default().Error("great sadness", errtrace.LogAttr(err))

abhinav · 2024-04-15T04:56:10Z

errtrace.go


 // Format writes the return trace for given error to the writer.
-// The output takes a fromat similar to the following:
+// The output takes a format similar to the following:


abhinav · 2024-04-15T04:59:42Z

errtrace.go

+//
+//	slog.Default().Error("msg here", errtrace.ErrAttr(err))
+func ErrAttr(err error) slog.Attr {
+	return slog.Any("error", err)


The convention on log/slog seems to be to use "err" as the key for errors, not "error". This matches what Zap and Zerolog do as well.

Actually, Zap uses "error" as the key: https://github.com/uber-go/zap/blob/master/error.go#L34

The documentation also shows that "error" is the key in Zerolog https://github.com/rs/zerolog?tab=readme-ov-file#error-logging

package main import ( "errors" "github.com/rs/zerolog" "github.com/rs/zerolog/log" ) func main() { zerolog.TimeFieldFormat = zerolog.TimeFormatUnix err := errors.New("seems we have an error here") log.Error().Err(err).Msg("") } // Output: {"level":"error","error":"seems we have an error here","time":1609085256}

In slog, the various proposals (and usage by the primary author jba) show using "error" as the key: golang/go#59364

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

Actually, Zap uses "error" as the key: https://github.com/uber-go/zap/blob/master/error.go#L34

Well, that's embarrassing for me. 😆

I don't know why I assumed "err". Will revert that change.

abhinav · 2024-04-15T05:08:14Z

I've renamed the field and the function just to see how it looks.
What do you think? We can revert it if it doesn't look good.

StevenACoffman · 2024-04-15T11:28:01Z

I would rather not use "err" as the key, but I don't feel strongly, and otherwise looks good to me! Thanks for a wonderful library.

Zap uses "error" as the key: https://github.com/uber-go/zap/blob/master/error.go#L34

// Error is shorthand for the common idiom NamedError("error", err).
func Error(err error) Field {
	return NamedError("error", err)
}

Zerolog has documentation that also shows that "error" is the key in Zerolog https://github.com/rs/zerolog?tab=readme-ov-file#error-logging

package main

import (
	"errors"

	"github.com/rs/zerolog"
	"github.com/rs/zerolog/log"
)

func main() {
	zerolog.TimeFieldFormat = zerolog.TimeFormatUnix

	err := errors.New("seems we have an error here")
	log.Error().Err(err).Msg("")
}

// Output: {"level":"error","error":"seems we have an error here","time":1609085256}

In slog, the various proposals (and usage by the primary author jba) show using "error" as the key: golang/go#59364

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

Maybe there is some other examples that you have seen where that is not the case?

abhinav · 2024-04-15T16:05:00Z

Oops. Yeah, "error" is good.
I was just having a bad case of memory corruption.

StevenACoffman · 2024-05-05T21:46:43Z

@abhinav is there anything remaining to get this merged?

abhinav · 2024-05-05T21:54:07Z

Hey @StevenACoffman, apologies, I had an update typed out but I never sent it.
I was discussing this with @prashantv earlier, and the short of it is:
I was having trouble deciding on whether the logged value should be a structured object or a string.

After mulling it over a bit, I don't think that the value should be structured: the majority use case will want to consume a string. For cases where folks want a structured object logged for errors, the logger APIs should provide enough control/overrides for how specific values are encoded—e.g. "encode errors using this function instead."
So I think we should keep it a string—keep this PR as-is.

But I want to confirm my thinking with others—you, @prashantv, @akshayjshah.

Outside of that, if we agree on the approach, what remains before merging this PR is:

dropping the go directive in go.mod back to 1.20 (since we're using a build tag)
adding a test for this functionality

prashantv · 2024-05-06T04:32:19Z

After mulling it over a bit, I don't think that the value should be structured: the majority use case will want to consume a string. For cases where folks want a structured object logged for errors, the logger APIs should provide enough control/overrides for how specific values are encoded—e.g. "encode errors using this function instead." So I think we should keep it a string—keep this PR as-is.

But I want to confirm my thinking with others—you, @prashantv, @akshayjshah.

+1 on using string. Should we do something similar to zap here, errorVerbose for the formatted string? Or is there an existing slog idiom that we want to follow?

abhinav · 2024-05-06T12:46:58Z

Should we do something similar to zap here, errorVerbose for the formatted string?

I think not. In hindsight, that wasn't the most ideal choice we could've made there.
The error should be a single field, and if it has multiple pieces of information, the encoder should be responsible for turning it into a group of values.

prashantv · 2024-05-06T15:50:55Z

I think not. In hindsight, that wasn't the most ideal choice we could've made there. The error should be a single field, and if it has multiple pieces of information, the encoder should be responsible for turning it into a group of values.

Flexibility (at the logger level) is probably the right answer, but I've found the message-only errors useful when dealing with logs in aggregate (e.g., looking at a distribution of errors across log messages in a timeframe) where additional stacks/traces get quite noisy. On the flip side, once I've drilled into a specific log, then I find the additional information useful to understand where it came from.

cbandy · 2024-11-14T22:36:49Z

📝 Go 1.21 since #111.

Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Chris Bandy <bandy.chris@gmail.com> Signed-off-by: Steve Coffman <steve@khanacademy.org>

cbandy · 2025-01-25T22:51:23Z

I rebased and squashed all these down over here: main...cbandy:errtrace:pr-97

StevenACoffman · 2025-06-01T17:47:39Z

@abhinav @prashantv I have force pushed @cbandy rebased squashed commit to my branch.

Writing to a strings.Builder can never fail so we don't need the impossible error path. Use FormatString instead, which already does this.

abhinav

This looks good to me. I've added a couple test cases.

abhinav · 2025-06-01T18:18:54Z

Fixing lint breakage here: #125

abhinav · 2025-06-01T22:28:57Z

Will give it a day to see if @prashantv has any thoughts, but good to merge otherwise

prashantv

Will drop LogAttr and merge.

prashantv · 2025-06-02T00:24:48Z

errtrace.go

+// When serialized with a slog-based logger,
+// this will report an error return trace if the error has one,
+// otherwise the original error message will be logged as-is.
+func LogAttr(err error) slog.Attr {


Since the implementation of LogAttr is completely independent of errtrace, I'm not sure if it's valuable to add it here. I'll drop this for now, and we can always bring it back if we think there's value in this being part of the errtrace interface.

prashantv · 2025-06-02T00:33:44Z

errtrace.go


+// LogValue implements the [slog.LogValuer] interface.
+func (e *errTrace) LogValue() slog.Value {
+	return slog.StringValue(FormatString(e))


no changes needed, but wanted to consider whether this should return a string value, or a group with multiple fields for message, trace, etc.

I think a string makes sense to start, and allows callers to choose how they want the error encoded, e.g., in their equivalent of the LogAttr function.

abhinav · 2025-06-02T02:01:14Z

Merged! Thanks for your patience, @StevenACoffman

Follow-up to #97. We dropped the `LogAttr` function, so the changelog needs to be updated.

StevenACoffman · 2025-06-02T10:44:42Z

Wohoo! Thanks!

bwknight877 · 2025-06-02T13:58:52Z

CHANGELOG.md

  }
  ```

+- Add `LogAttr` function to log any error with log/slog with the key "error".


looks like this changelog entry was left behind after the LogAttr func was removed

We realized this after merging too, and fixed it in #126.
Thanks for catching either way!

abhinav reviewed Apr 15, 2024

View reviewed changes

Add slog compatibility

c729dda

Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Chris Bandy <bandy.chris@gmail.com> Signed-off-by: Steve Coffman <steve@khanacademy.org>

StevenACoffman force-pushed the main branch 2 times, most recently from a857d47 to c729dda Compare June 1, 2025 17:47

abhinav added 3 commits June 1, 2025 11:05

example tests and changelog

b4feed7

fix: local path agnostic output assertions

e398330

Format => FormatString

7332cd0

Writing to a strings.Builder can never fail so we don't need the impossible error path. Use FormatString instead, which already does this.

abhinav changed the title ~~Add slog compatibility, move to Go 1.21~~ Add slog compatibility Jun 1, 2025

abhinav approved these changes Jun 1, 2025

View reviewed changes

Merge branch 'main' into main

253e34c

prashantv approved these changes Jun 2, 2025

View reviewed changes

Drop LogAttr and related tests

99bec3d

abhinav merged commit cc0b57f into bracesdev:main Jun 2, 2025
15 of 16 checks passed

prashantv added a commit that referenced this pull request Jun 2, 2025

Update CHANGELOG for slog support

089880c

Follow-up to #97. We dropped the `LogAttr` function, so the changelog needs to be updated.

prashantv mentioned this pull request Jun 2, 2025

Update CHANGELOG for slog support #126

Merged

prashantv added a commit that referenced this pull request Jun 2, 2025

Update CHANGELOG for slog support (#126)

35f7066

Follow-up to #97. We dropped the `LogAttr` function, so the changelog needs to be updated.

bwknight877 reviewed Jun 2, 2025

View reviewed changes

Comments

Conversation

StevenACoffman commented Apr 15, 2024

Uh oh!

abhinav commented Apr 15, 2024

Uh oh!

abhinav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinav commented Apr 15, 2024

Uh oh!

StevenACoffman commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinav commented Apr 15, 2024

Uh oh!

StevenACoffman commented May 5, 2024

Uh oh!

abhinav commented May 5, 2024

Uh oh!

prashantv commented May 6, 2024

Uh oh!

abhinav commented May 6, 2024

Uh oh!

prashantv commented May 6, 2024

Uh oh!

cbandy commented Nov 14, 2024

Uh oh!

cbandy commented Jan 25, 2025

Uh oh!

StevenACoffman commented Jun 1, 2025

Uh oh!

abhinav left a comment

Choose a reason for hiding this comment

Uh oh!

abhinav commented Jun 1, 2025

Uh oh!

abhinav commented Jun 1, 2025

Uh oh!

prashantv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhinav commented Jun 2, 2025

Uh oh!

StevenACoffman commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

StevenACoffman commented Apr 15, 2024 •

edited

Loading