Skip to content

log/slog: logger discards errors from handler #66579

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

Closed
shaj13 opened this issue Mar 28, 2024 · 3 comments
Closed

log/slog: logger discards errors from handler #66579

shaj13 opened this issue Mar 28, 2024 · 3 comments
Assignees
Labels
Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.

Comments

@shaj13
Copy link

shaj13 commented Mar 28, 2024

Go version

go version go1.21.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/s.hajyahya/Library/Caches/go-build'
GOENV='/Users/s.hajyahya/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/s.hajyahya/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/s.hajyahya/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/s.hajyahya/dev/stuff/ssh/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/_w/y7hb861d42z_drrgmkk7pb440000gp/T/go-build182075280=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

func TestXxx(t *testing.T) {
	t.Error("log")
	h := new(remoteHandler)
	lg := slog.New(h)
	lg.Info("example")
}

type remoteHandler struct {
}

func (h *remoteHandler) Enabled(_ context.Context, level slog.Level) bool {
	return true
}

// WithAttrs returns a new [TextHandler] whose attributes consists
// of h's attributes followed by attrs.
func (h *remoteHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
	return nil
}

func (h *remoteHandler) WithGroup(name string) slog.Handler {
	return nil
}

func (h *remoteHandler) Handle(_ context.Context, r slog.Record) error {
	err := errors.New("any remote error")
	fmt.Println(err)
	return err
}

What did you see happen?

The logger disregards errors from the handler, and there is a method to explicitly set an error handling mechanism.

See https://github.com/golang/go/blob/master/src/log/slog/logger.go#L257

What did you expect to see?

Slog has introduced a philosophy by adopting a frontend and backend design, where the logger serves as the frontend and the handler as the backend. However, the backend/handler may need to write logs to a remote logger, so handling those errors is essential for users to understand why some logs are not being sent or logged, regardless of what the handler is doing. While it's acceptable for the frontend/logger to ignore these errors by default, users should at least be allowed to set their error handler.

When using the stdlib log, there is no way to set an error handler, but that is acceptable as that log package only adopted a different design. If a user intended to handle errors, they could wrap the io writer with a custom error handler. However, wrapping the slog handler and passing it to slog is considered overkill due to the interface and conflicts with the slog design and philosophy.

Looking at other packages like flag, which offers error handling options like continue or panic, and httputil, where the proxy allows handling errors by setting a function explicitly, it would be beneficial to update the docs to clarify that the slog logger ignores errors from the handler (if this information is not already present). Additionally, exposing an error handling function to allow users to set it explicitly would be beneficial.

This may need to be converted from a bug report to a proposal, but starting the discussion here before moving it to the proposal boards could be helpful.

func TestXxx(t *testing.T) {
	t.Error("log")
	h := new(remoteHandler)
	lg := slog.New(h).WithErrorHandler(func(slog.Record, error){
               // handle error. 
        })
	lg.Info("example")
}

type remoteHandler struct {
}

func (h *remoteHandler) Enabled(_ context.Context, level slog.Level) bool {
	return true
}

// WithAttrs returns a new [TextHandler] whose attributes consists
// of h's attributes followed by attrs.
func (h *remoteHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
	return nil
}

func (h *remoteHandler) WithGroup(name string) slog.Handler {
	return nil
}

func (h *remoteHandler) Handle(_ context.Context, r slog.Record) error {
	err := errors.New("any remote error")
	fmt.Println(err)
	return err
}
@shaj13
Copy link
Author

shaj13 commented Mar 28, 2024

CC @jba

@jba jba self-assigned this Mar 28, 2024
@jba
Copy link
Contributor

jba commented Mar 28, 2024

However, wrapping the slog handler and passing it to slog is considered overkill due to the interface and conflicts with the slog design and philosophy.

On the contrary, a wrapping Handler that reports errors is exactly the intent.
We should explain this better in the docs, and maybe have an example.
I think once we do that, it will easy enough to write such a hander and we don't need to add any APi to the package.

@seankhliao seankhliao added Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. labels Mar 28, 2024
@seankhliao seankhliao changed the title log/slog: logger discards errors. log/slog: logger discards errors from handler Mar 29, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/658515 mentions this issue: log/slog: document Logger ignores Handler.Handle errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants