-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add an option to disable errorVerbose #1487
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
base: master
Are you sure you want to change the base?
Conversation
zapcore/field.go
Outdated
Integer int64 | ||
String string | ||
Interface interface{} | ||
DisableErrorVerbose bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We avoid adding new struct fields to the Field
type for performance reasons, as the struct is passed as a value.
We could add a new FieldType
to indicate an error that should only have the message included, though that could have backwards compatibility concerns with some encoders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your feedback. I will create a new type with error field and DisableErrorVerbose field.
1200ec6
to
d3399cc
Compare
Sorry for bothering you. Could you review my PR, please? |
Also eager to see this merged ! Many thanks in advance for your consideration :) |
Here's a simple test that shows that this approach does not work in common cases: func TestDisableErrorVerbose(t *testing.T) {
err := errTooManyUsers(4) // custom error with %+v formatting.
logger := zap.NewExample(zap.DisableErrorVerbose())
logger.Error("test error log", zap.Error(err))
logger.Info("test info log", zap.Error(err))
if ce := logger.Check(zap.InfoLevel, "test check"); ce != nil {
ce.Write(zap.Error(err))
}
} This outputs:
Though the This approach also has some performance implications as it iterates over every field and checks the type, though it only impacts the Before a code review, I'd recommend discussing the design on the issue to make sure it will solve the problem fully (and ideally in a performant way). |
This PR adds
disableErrorVerbose
option toLogger
type.This option allows users to disable the output of error verbose when they use pkg/errors.
Closes #650 #1168