Allow configurable stacktrace encoding #1371
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #1371 +/- ##
==========================================
- Coverage 98.28% 98.15% -0.13%
==========================================
Files 53 53
Lines 3493 3527 +34
==========================================
+ Hits 3433 3462 +29
- Misses 50 55 +5
Partials 10 10
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks for the PR, @arwineap! This change looks largely in the right direction.
However:
The EncodeStacktrace field has been inserted as a required field [..] it can cause a panic if a user is manually building their config without these helpers
That's a breaking change and we can't merge that. The new field must be optional.
The console and JSON encoders already have handling for optional overrides like this one; this should be another such optional field. For example:
Lines 381 to 387 in 87577d8
|
Thank you for the clarification, I'm unsure how I missed the default catch for the nil cases. This has been added. One other concern I had was with the console encoder; I'm not sure I can avoid calling |
There was a problem hiding this comment.
This is great, thanks. I've left some minor comments.
I have a couple open questions that I maybe need input from other maintainers on.
- Should the encoder be primitives-based like the other encoder overrides or a full one capable of posting objects -- e.g. if someone parses the stack trace?
- Should the fallback behavior be to encode the full stack trace, or should we take that as an indication that they don't want to encode the stack trace?
| if cur == final.buf.Len() { | ||
| // User-supplied EncodeStacktrace was a no-op. Fall back to strings to | ||
| // keep output JSON valid. | ||
| final.AppendString("") |
There was a problem hiding this comment.
Did you mean to pass ent.Stack here?
| final.AppendString("") | |
| final.AppendString(ent.Stack) |
There was a problem hiding this comment.
Good catch, this was actually intentional.
The idea being that a user can disable stacktrace key / values in json output by configuring StacktraceKey to be blank
However, if the user created the custom encoder which emptied out the stack trace, it seems like we should respect their key / value, and append their StacktraceKey with an empty string
The issue is that at this part of the code the StacktraceKey has already been added to the json output, so we need a string value to make the json valid, so an empty string makes sense. Perfectly reasonable to only add the Key if a value exists, just need some rearranging
It's a matter of design, and it's more important that it's consistent across all of the options, so I will defer to the zap team here 👍
| for i := range arr.elems { | ||
| if i > 0 { | ||
| line.AppendString(c.ConsoleSeparator) | ||
| } | ||
| fmt.Fprint(line, arr.elems[i]) | ||
| } |
There was a problem hiding this comment.
The contract for stacktraceEncoder is that it makes at most one Append* call.
So we don't need the full arr.elems handling here. It can just be:
| for i := range arr.elems { | |
| if i > 0 { | |
| line.AppendString(c.ConsoleSeparator) | |
| } | |
| fmt.Fprint(line, arr.elems[i]) | |
| } | |
| if len(arr.elems) > 0 { | |
| fmt.Fprint(line, arr.elems[0]) | |
| } else { | |
| // User-provided encoder was a no-op. | |
| // Fall back to full encoder. | |
| line.AppendString(ent.Stack) | |
| } |
There was a problem hiding this comment.
The first AppendString in this call is actually a way to add the ConsoleSeparator between each line; it could probably be integrated into the Fprint to reduce branching here
|
Related: We'll want to make a decision on #1373 before we release this because choices there affect the design for this. |
|
I will work to rebase this with modern changes; but depending on any decision in #1373 we may try to send the entire stack object into the function instead of the string |
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.
This is now unneeded because `EncodeStacktrace` will not be nil
improve test cases
Co-authored-by: Abhinav Gupta <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>
arwineap
force-pushed
the
configurable-stacktrace-encoding
branch
from
d45920a
to
a7fe65f
Compare
This PR adds a
StacktraceEncodertype to theEncoderConfigstructUsers can override the
EncodeStacktracefield to configure how stacktraces are outputed This PR aims to resolve #514 by mirroring the behavior of theEncodeCallerfieldThe
EncodeStacktracefield has been inserted as a required field, and has been added toNewDevelopmentConfigandNewProductionConfigas sane defaults however, as a required field it can cause a panic if a user is manually building their config without these helpers.