Skip to content

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

makotonakai
Copy link

@makotonakai makotonakai commented Jan 30, 2025

This PR adds disableErrorVerbose option to Logger type.
This option allows users to disable the output of error verbose when they use pkg/errors.

Closes #650 #1168

zapcore/field.go Outdated
Integer int64
String string
Interface interface{}
DisableErrorVerbose bool
Copy link
Collaborator

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.

Copy link
Author

@makotonakai makotonakai Feb 4, 2025

Choose a reason for hiding this comment

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

@prashantv

I appreciate your feedback. I will create a new type with error field and DisableErrorVerbose field.

@makotonakai makotonakai force-pushed the turn-off-error-verbose branch 2 times, most recently from 1200ec6 to d3399cc Compare February 10, 2025 09:29
@makotonakai
Copy link
Author

@prashantv

Sorry for bothering you. Could you review my PR, please?

@Amphaal
Copy link

Amphaal commented May 20, 2025

Also eager to see this merged ! Many thanks in advance for your consideration :)

@prashantv
Copy link
Collaborator

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:

{"level":"error","msg":"test error log","test error log":"4 too many users"}
{"level":"info","msg":"test info log","error":"4 too many users","errorVerbose":"4 too many users verbose"}
{"level":"info","msg":"test check","error":"4 too many users","errorVerbose":"4 too many users verbose"}

Though the Error method is handled, other methods are not, and the path for logger.Check can't be handled with this approach.

This approach also has some performance implications as it iterates over every field and checks the type, though it only impacts the DisableErrorVerbose case.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Expose the ability to disable errorVerbose via logger config
3 participants