feat(zapslog): implement stack trace handling #1339

zekth · 2023-08-23T13:27:15Z

As discussed in the issue, replacing the HandlersOptions with functional options, i also renamed addSource to addCaller to properly match the zap semantic.

CLAassistant · 2023-08-23T13:27:21Z

All committers have signed the CLA.

zekth · 2023-08-23T13:27:45Z

stacktrace.go

@@ -125,6 +125,12 @@ func (st *stacktrace) Next() (_ runtime.Frame, more bool) {
 	return st.frames.Next()
 }

+// TakeStacktrace returns the stack trace,
+// and skips the provided number of frames.
+func TakeStacktrace(skip int) string {


Should we deprecate this and add a comment that it's just a temporary export? Although we might want to implement this first because /exp/slog relies on zap 1.24.0 :

zap/exp/go.mod

Line 7 in fd37f1f

go.uber.org/zap v1.24.0

to make it work i need to modify the go.mod:

module go.uber.org/zap/exp go 1.19 require ( github.com/stretchr/testify v1.8.1 go.uber.org/zap v1.24.0 golang.org/x/exp v0.0.0-20230811145659-89c5cff77bcb ) require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.uber.org/atomic v1.10.0 // indirect go.uber.org/multierr v1.10.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) replace go.uber.org/zap => ../

I don't like the idea of exporting this functionality from the main Zap package even as a temporary unsupported thing.
Instead, I've moved the relevant bits into an internal package in #1341.

That makes it easy to use them here.

Thanks for this i agree on this approach too

zekth · 2023-08-23T13:28:21Z

exp/zapslog/slog_pre_go121.go

 		Time:       record.Time,
 		Message:    record.Message,
 		LoggerName: h.name,
-		// TODO: do we need to set the following fields?


Stack was not required for the implementation

Moves the functionality to capture and format stack traces into an internal stacktrace package and exports the relevant bits out for the logger and field.go to use. This will be used by zapslog.Handler to capture stack traces so it needs to be in a shared location. Refs #1329, #1339

abhinav

Thanks for doing this, and thanks for waiting for the review. It's a bit busy these days.

I'll try to get #1341 landed soon to capture the stack traces without exporting them from the main Zap package.

And yeah, exp/ will need a replace directive. We'll have to be careful about updating exp/ to the latest Zap release when we tag it.

abhinav · 2023-08-23T17:00:53Z

exp/zapslog/slog_go121.go

@@ -157,6 +144,10 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 		}
 	}

+	if h.addStack.Enabled(zapLevel) {
+		ce.Stack = zap.TakeStacktrace(4 + h.callerSkip)


I would add a comment here for each of the 4 skips we're adding, e.g. skip Handle, slog.LogAttr, [..], etc.
I suspect this might not be as straightforward.

Sure I'll add revelant comments

abhinav · 2023-08-25T13:02:36Z

stacktrace.go

@@ -125,6 +125,12 @@ func (st *stacktrace) Next() (_ runtime.Frame, more bool) {
 	return st.frames.Next()
 }

+// TakeStacktrace returns the stack trace,
+// and skips the provided number of frames.
+func TakeStacktrace(skip int) string {


I don't like the idea of exporting this functionality from the main Zap package even as a temporary unsupported thing.
Instead, I've moved the relevant bits into an internal package in #1341.

That makes it easy to use them here.

abhinav · 2023-08-25T13:07:06Z

I've pushed some minor fixups to your branch. Please be sure not to lose them if you git push -f.

Moves the functionality to capture and format stack traces into an internal stacktrace package and exports the relevant bits out for the logger and field.go to use. This will be used by zapslog.Handler to capture stack traces so it needs to be in a shared location. Refs #1329, #1339

We don't need AddCaller and WithCaller both. WithCaller was added because AddCaller doesn't take a parameter.

zekth · 2023-08-25T16:21:36Z

@abhinav I rebased and applied all the comments

codecov · 2023-08-25T16:47:13Z

Codecov Report

Merging #1339 (573bc29) into master (e9f4bda) will decrease coverage by 0.23%.
The diff coverage is 75.75%.

@@            Coverage Diff             @@
##           master    #1339      +/-   ##
==========================================
- Coverage   97.95%   97.72%   -0.23%     
==========================================
  Files          51       52       +1     
  Lines        3369     3392      +23     
==========================================
+ Hits         3300     3315      +15     
- Misses         59       67       +8     
  Partials       10       10

Files Changed	Coverage Δ
exp/zapslog/options.go	`55.55% <55.55%> (ø)`
exp/zapslog/handler.go	`93.33% <100.00%> (+0.33%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zekth · 2023-08-26T18:34:28Z

@abhinav should i add tests for the options or we add them in another PR? I'm happy to write a follow up one if needed

abhinav

Hey @zekth, apologies; I haven't forgotten about this, but I've had (and continue to have) a busy few days. I've taken a pass; this mostly looks good to go with minor comments. I'll address a few myself in a few minutes.

Tagging other Zap maintainers as well. CC @sywhang @mway @prashantv
(Context: This PR turns HandlerOptions into functional options per prior discussion, and adds support for stack traces. Stack traces are captured for Error and above -- same as Zap's production logger.)

RE: tests: There's no need to backfill tests for existing options at this time--tests for the new option you're adding for stack traces should be sufficient. We do intend to fill in the rest of the tests before moving this to the main Zap package, though, but that's out of scope for this PR.

exp/go.mod

abhinav · 2023-08-27T04:07:17Z

exp/zapslog/handler.go

+		// Skipping 3:
+		// zapslog/handler log/slog.(*Logger).log
+		// slog/logger log/slog.(*Logger).log
+		// slog/logger log/slog.(*Logger).<level>
+		ce.Stack = stacktrace.Take(3 + h.callerSkip)


Thanks for adding this. Much more obvious what's going on now.

exp/zapslog/options.go

abhinav · 2023-08-27T04:14:07Z

exp/zapslog/options.go

+// AddStacktrace configures the Logger to record a stack trace for all messages at
+// or above a given level.
+func AddStacktrace(lvl zapcore.LevelEnabler) Option {


Just thinking out loud (would love your opinion on this, @zekth):
I wonder if this should be a slog Level instead. Or a new zapslog.LevelEnabler interface based on slog.Level.

type LevelEnabler interface{ Enabled(context.Context, Level) bool }

The reasoning is this: the result of zapslog.NewHandler is a slog Handler. It talks in the language of slog.

And update documentation.

Changes AddStacktrace to use a slog.Level instead of a Zap Level, and renames it to AddStacktraceAt. This leaves room for a LevelEnabler (as suggested in the PR) to be specified with an AddStacktrace in the future.

abhinav · 2023-08-27T04:26:36Z

On LevelEnabler and whether this should use a Zap level or slog level.
The more I think about it, the slog level makes sense.
The slog level has more information; conversion from slog to Zap level is a lossy operation.
I've renamed the option to AddStacktraceAt.
That way, if we want to add a LevelEnabler in the future, we can still use an AddStacktrace(LevelEnabler) option.
(The proposed LevelEnabler's Enabled signature matches slog.Handler.Enabled, so such a user-provided function
would have all the important information about the record to determine whether it should capture a stacktrace.)

We avoid use of assert.New and require.New in our tests.

Now that zaplevel isn't used to determine if we need to capture the stack trace, it can be inlined back into the entry construction.

abhinav

LGTM!

@mway @sywhang would appreciate a second look.
FYI, the coverage drop is because some of the existing functionality is untested; we will want to fix that before considering this stable anyway.

sywhang

LGTM.

zekth commented Aug 23, 2023

View reviewed changes

zekth force-pushed the feat/slog-stacktrace branch 3 times, most recently from 3b30b92 to 964a1cc Compare August 23, 2023 17:15

abhinav mentioned this pull request Aug 25, 2023

chore: Move stacktrace capturing into internal/ #1341

Merged

abhinav reviewed Aug 25, 2023

View reviewed changes

zekth and others added 6 commits August 25, 2023 18:04

feat(zapslog): implement stack trace handling

3e34ef1

options.go: Fix year in copyright header

855ea4b

option: Drop AddCaller

46f5228

We don't need AddCaller and WithCaller both. WithCaller was added because AddCaller doesn't take a parameter.

fix(example): NewHandler shouldn't be called with nil

f6bf507

go.mod: Add replace zap => ../

b8c0132

feat(slog): use internal stacktrace package

397c305

zekth force-pushed the feat/slog-stacktrace branch from c639ac6 to 3be64f5 Compare August 25, 2023 16:16

feat(slog): replace to latest

b66a3d3

zekth force-pushed the feat/slog-stacktrace branch from 3be64f5 to b66a3d3 Compare August 25, 2023 16:19

fix(slog): test

b0bb45f

zekth added 2 commits August 25, 2023 18:47

fix build tag

34f37b7

lint

3a38323

abhinav reviewed Aug 27, 2023

View reviewed changes

abhinav added 3 commits August 26, 2023 21:16

go.mod: replace Zap to ../

6fe8c05

Rename AddCallerSkip to WithCallerSkip

e63605e

And update documentation.

AddStacktrace: Use slog.Level, rename to AddStacktraceAt

d5ebf65

Changes AddStacktrace to use a slog.Level instead of a Zap Level, and renames it to AddStacktraceAt. This leaves room for a LevelEnabler (as suggested in the PR) to be specified with an AddStacktrace in the future.

AddStacktraceAt: Fix test

b68ffcf

abhinav added 3 commits August 26, 2023 21:27

test: Don't use require.New

41948fb

We avoid use of assert.New and require.New in our tests.

Handler.Handle: Inline zapLevel back

31b7f03

Now that zaplevel isn't used to determine if we need to capture the stack trace, it can be inlined back into the entry construction.

doc(WithCaller): remove Zap logger reference

fd6cc64

abhinav approved these changes Aug 27, 2023

View reviewed changes

Merge branch 'master' into feat/slog-stacktrace

573bc29

sywhang approved these changes Sep 1, 2023

View reviewed changes

sywhang merged commit b900128 into uber-go:master Sep 1, 2023

mattrobenolt mentioned this pull request Dec 2, 2024

roll zap/exp forward again with fix planetscale/cli#941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(zapslog): implement stack trace handling #1339

feat(zapslog): implement stack trace handling #1339

zekth commented Aug 23, 2023

CLAassistant commented Aug 23, 2023 •

edited

Loading

zekth Aug 23, 2023 •

edited

Loading

abhinav Aug 25, 2023

zekth Aug 25, 2023

zekth Aug 23, 2023

abhinav left a comment

abhinav Aug 23, 2023

zekth Aug 25, 2023

abhinav Aug 25, 2023

abhinav commented Aug 25, 2023

zekth commented Aug 25, 2023

codecov bot commented Aug 25, 2023 •

edited

Loading

zekth commented Aug 26, 2023

abhinav left a comment

abhinav Aug 27, 2023

abhinav Aug 27, 2023

abhinav commented Aug 27, 2023

abhinav left a comment

sywhang left a comment

feat(zapslog): implement stack trace handling #1339

feat(zapslog): implement stack trace handling #1339

Conversation

zekth commented Aug 23, 2023

CLAassistant commented Aug 23, 2023 • edited Loading

zekth Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Aug 25, 2023

zekth commented Aug 25, 2023

codecov bot commented Aug 25, 2023 • edited Loading

Codecov Report

zekth commented Aug 26, 2023

abhinav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Aug 27, 2023

abhinav left a comment

Choose a reason for hiding this comment

sywhang left a comment

Choose a reason for hiding this comment

CLAassistant commented Aug 23, 2023 •

edited

Loading

zekth Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 25, 2023 •

edited

Loading