zapslog.Handler: Add stack traces

[zekth]

First i'm really happy to see the fast work on making zap compliant on go 1.21 with slog !

Is your feature request related to a problem? Please describe.

This describes just the miss of stack trace serialization via the slog handler.

Describe the solution you'd like

Taking this example:

package main

import (
	"errors"
	"log/slog"
	"os"

	"go.uber.org/zap"
	"go.uber.org/zap/exp/zapslog"
	"go.uber.org/zap/zapcore"
)

func main() {
	config := zapcore.EncoderConfig{
		MessageKey:    "msg",
		LevelKey:      "level",
		EncodeLevel:   zapcore.CapitalLevelEncoder,
		TimeKey:       "ts",
		EncodeTime:    zapcore.ISO8601TimeEncoder,
		CallerKey:     "caller",
		EncodeCaller:  zapcore.ShortCallerEncoder,
		StacktraceKey: "stack_trace",
	}
	core := zapcore.NewCore(
		zapcore.NewJSONEncoder(config),
		zapcore.Lock(os.Stdout),
		zap.DebugLevel,
	)
	zapInstance := zap.New(core,
		zap.AddCaller(),
		zap.AddStacktrace(zapcore.ErrorLevel),
	)
	slogInstance := slog.New(zapslog.NewHandler(zapInstance.Core(), &zapslog.HandlerOptions{AddSource: true}))
	err := errors.New("failure")
	zapInstance.Error("zap", zap.Error(err))
	slogInstance.Error("slog", slog.Any("error", err))
}

It outputs:

{"level":"ERROR","ts":"2023-08-19T12:10:36.636+0200","caller":"go/main.go:35","msg":"zap","error":"failure","stack_trace":"main.main\n\t/Users/vincent/kong/go/main.go:35\nruntime.main\n\t/usr/local/Cellar/go/1.21.0/libexec/src/runtime/proc.go:267"}
{"level":"ERROR","ts":"2023-08-19T12:10:36.636+0200","caller":"go/main.go:36","msg":"slog","error":"failure"}

I'd hope to find a solution to be able to have the stack_trace also via the slog handler.

Describe alternatives you've considered

Because we only pass the zapcore.Core to the zapslog handler we have access to StacktraceKey, however we don't have access to the AddStacktrace options which are on the zap logger itself. What we can do is like the AddSource option in zapslog.HandlerOptions we can also pass the arguments to enable the stacktrace on a certain level. That would mean then we need to export the stacktraceformatter and other required pieces to be able to serialize the stack trace: https://github.com/uber-go/zap/blob/b454e1885dd30fc65bf5d5b40827d1af2d2c57db/stacktrace.go#L147C10-L147C10

Wdyt?

Is this a breaking change?

No

[abhinav]

This is a valid request and probably a prerequisite for the slog handler before it's moved to the main module and considered stable. Currently, there's only a TODO in its place:

zap/exp/zapslog/slog_go121.go

Lines 139 to 140 in b454e18

	// TODO: do we need to set the following fields?
	// Stack:

RE: implementation: We wouldn't necessarily export the stacktrace formatter from the public Zap package. We would move it to an internal package and access it from there in both places.

[zekth]

I indeed mentioned export the stack formatter if slog would stay as a separate package.

[zekth]

@abhinav i managed to make it work locally by exporting zap.takeStacktrace and modifying the handle function like so:

func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
	zapLevel := convertSlogLevel(record.Level)
	ent := zapcore.Entry{
		Level:      zapLevel,
		Time:       record.Time,
		Message:    record.Message,
		LoggerName: h.name,
	}
	ce := h.core.Check(ent, nil)
	if ce == nil {
		return nil
	}

	if h.addSource && record.PC != 0 {
		frame, _ := runtime.CallersFrames([]uintptr{record.PC}).Next()
		if frame.PC != 0 {
			ce.Caller = zapcore.EntryCaller{
				Defined:  true,
				PC:       frame.PC,
				File:     frame.File,
				Line:     frame.Line,
				Function: frame.Function,
			}
		}
	}

	if zapcore.LevelEnabler.Enabled(h.addStack, zapLevel) {
		ce.Stack = zap.TakeStacktrace(2 + h.skipStack)
	}

	fields := make([]zapcore.Field, 0, record.NumAttrs())
	record.Attrs(func(attr slog.Attr) bool {
		fields = append(fields, convertAttrToField(attr))
		return true
	})
	ce.Write(fields...)
	return nil
}

Handler options have been updated like so:

type HandlerOptions struct {
	// LoggerName is used for log entries received from slog.
	//
	// Defaults to empty.
	LoggerName string

	// AddSource configures the handler to annotate each message with the filename,
	// line number, and function name.
	// AddSource is false by default to skip the cost of computing
	// this information.
	AddSource bool

	AddStack *zapcore.Level

	SkipStack int
}

However i realized we might want to use a similar functionnal options pattern like we have for the zap configuration, Would also makes easier to use parameters from zap to slog handler.

wdyt?

[abhinav]

@zekth That's great! When you feel it's ready, feel free to turn that into a PR.

We originally went with an Options struct to match the constructor signatures for log/slog's own handlers, but there's no hard requirement to match that.
I think it would be okay for this to be powered by functional options, especially because the default no options case is quite likely, so NewHandler(core) is nicer than NewHandler(core, nil /* options */).

One caveat: we will not be able to re-use zap.Options from the main Zap package here; it'll be its own thing.

[zekth]

Ok, consider i'm writing the implementation of stacktrace with the functional options then. Should be ready within few days.