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.

Sign up for GitHub

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

Jump to bottom

Add an option to override stacktrace collection logic #514

Open
zllak opened this issue Oct 12, 2017 · 5 comments · May be fixed by #1371
Open

Add an option to override stacktrace collection logic #514

zllak opened this issue Oct 12, 2017 · 5 comments · May be fixed by #1371
Labels
help wanted

Comments

@zllak
Copy link

zllak commented Oct 12, 2017

Stacktrace collection

While trying to implement an encoder that is compatible with GCP Stackdriver logging and also for GCP Stackdriver Error Reporting, I suspected that it's not working properly because the generated stacktrace is not compatible with what Error Reporting is expected.

The documentation says (https://cloud.google.com/error-reporting/docs/formatting-error-messages):
Go: Must be the return value of runtime.Stack()

The code in go sources (cloud.google.com/go/errors/errors.go) shows that cloud.google.com/go/errors.(*Client).logInternal indeed calls runtime.Stack.

Unfortunately, the stack is grabbed by trackStacktrace in stracktrace.go, and there is apparently no other way around to provide the stracktrace.

Unless I'm missing something, we cannot provide the way the stacktrace is grabbed by zap
@akshayjshah
Copy link
Contributor

There's no way to customize this now, but I don't mind adding an option for this. We could make a zap.Option that takes a func() string and overrides the current stacktrace-snapping functionality.

However, this wouldn't affect the output of the zap.Stack field if it's used explicitly - the user would have to use some alternate field for stackdriver compatibility.

@akshayjshah akshayjshah changed the title stacktrace collection should be overridable Add an option to override stacktrace collection logic Apr 11, 2018
@djui
Copy link
Contributor

djui commented Sep 6, 2018

(I don't want to hijack this issue but would rather like to add a similar question and ask for a recommendation) Besides adding an option for the stacktrace-snapping functionality, is it currently already possible to modify the stack trace output? Would it work by implementing a zapcore.Encoder?

We would like to trim off some stacks that are not adding much to the signal and are rather noise.

I know trimming stacks is a inherently difficult task, but I would at least like to go give our heuristic a try.

@evanj
Copy link

evanj commented Feb 24, 2019

FWIW: It is possible to wrap the existing JSON encoder to hack stack traces. I've done this to format stacks so they get picked up by Google Cloud's Stackdriver Error Report (where it needs to look like a "real" panic). It would be much nicer to have a customization hook similar to EncoderConfig.EncodeTime, but this is possible today. See: https://github.com/evanj/gcplogs/blob/master/gcpzap/encoder.go#L48

@FlorianLoch
Copy link

FlorianLoch commented Jan 10, 2023 •
edited
Loading

Any progress/change of mind regarding adding support for the mentioned zap.Option allowing a custom function to provide the stack trace? That would be very handy to have ;)

@sywhang
Copy link
Contributor

sywhang commented Jan 10, 2023

@FlorianLoch nope, the issue is still a valid ask and we welcome PRs!

arwineap added a commit to arwineap/zap that referenced this issue Oct 9, 2023
@arwineap
Allow configurable stacktrace encoding
456073a 
This PR adds a `StacktraceEncoder` type to the `EncoderConfig` struct

Users can override the `EncodeStacktrace` field to configure how stacktraces are outputed
This PR aims to resolve uber-go#514 by mirroring the behavior of the `EncodeCaller` field

The `EncodeStacktrace` field has been inserted as a required field, and has been added to `NewDevelopmentConfig` and `NewProductionConfig`
as sane defaults however, it is currently a required field and can cause a panic if a user is manually building their config without these helpers.
@arwineap arwineap linked a pull request Oct 9, 2023 that will close this issue
Open
arwineap added a commit to arwineap/zap that referenced this issue Oct 19, 2024
@arwineap
Allow configurable stacktrace encoding
fab0c00 
This PR adds a `StacktraceEncoder` type to the `EncoderConfig` struct

Users can override the `EncodeStacktrace` field to configure how stacktraces are outputed
This PR aims to resolve uber-go#514 by mirroring the behavior of the `EncodeCaller` field

The `EncodeStacktrace` field has been inserted as a required field, and has been added to `NewDevelopmentConfig` and `NewProductionConfig`
as sane defaults however, it is currently a required field and can cause a panic if a user is manually building their config without these helpers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow configurable stacktrace encoding arwineap/zap
6 participants
@djui @zllak @evanj @akshayjshah @FlorianLoch @sywhang