Skip to content
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

fix(slog): disable automatic logging of stacktraces for errors #464

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

peanball
Copy link
Contributor

@peanball peanball commented Jan 31, 2025

The slog handler for Zap is configured to automatically emit stack traces at "Error" level.

In pure Zap, stack traces are added via option AddStacktrace, which was not used before the move to slog.

The Zap slog handler has a mapping from Zap levels to slog levels, and does not emit values above slog.LevelError. By setting the limit for automatically added stack traces via slog to ErrorLevel + 1, this effectively disables automatic stack traces.

The panic-check handler that catches and logs panics in handlers remains unaffected, as it explicitly logs a field called "stacktrace", as it did with pure Zap.

Related: #435

Breaking change: No. This is restoring the behavior we had with the pure zap logger / logger wrappers.

@peanball peanball requested a review from a team as a code owner January 31, 2025 16:00
Copy link
Member

@maxmoehl maxmoehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious because you mentioned the panic-check handler, seems like there's a bug in there:

logger.Error("panic-check", slog.String("host", r.Host), log.ErrAttr(err), slog.Any("stacktrace", runtime.StartTrace()))

From runtime.StartTrace:

StartTrace enables tracing for the current process.

that doesn't sound right. It should probably call runtime/debug.Stack. Without your change this is not noticed because the stack-trace is emitted either way, but by merging this there will be no stack-trace anymore. And if you don't mind, a test to ensure the panic-check handler emits the stack trace would probably be a good idea as well 😅

Edit: An alternative approach could be to log at ErrorLevel+1 to trigger the stack-trace again and use it as a replacement for the previous PanicLevel.

@peanball
Copy link
Contributor Author

peanball commented Feb 3, 2025

+100. Will add the check for the panic check handler outputting a proper stack trace, not just the field named stacktrace, as it does now.

The slog handler for Zap is configured to automatically emit stack traces at "Error" level.

In pure Zap, stack traces are added via option AddStacktrace, which was not used before the move to slog.

The Zap slog handler has a mapping from Zap levels to slog levels, and does not emit values above slog.LevelError. By setting the limit for automatically added stack traces via slog to ErrorLevel + 1, this effectively disables automatic stack traces.

The `panic-check` handler that catches and logs panics in handlers remains unaffected, as it explicitly logs a field called "stacktrace", as it did with pure Zap.

Related: #435
@peanball peanball force-pushed the zap-slog-disable-default-stacktrace branch from 5a11bb0 to e3aa65d Compare February 3, 2025 11:20
@peanball
Copy link
Contributor Author

peanball commented Feb 3, 2025

@maxmoehl, I've applied the change you mention and ensure in the paniccheck_test.go that the stack trace is actually there. I'm checking that an expected fragment that should show up in the stacktrace is really there.

@peanball peanball requested a review from maxmoehl February 3, 2025 11:21
@peanball peanball merged commit 1e14fcf into main Feb 3, 2025
1 check passed
@peanball peanball deleted the zap-slog-disable-default-stacktrace branch February 3, 2025 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants