Skip to content

Add an option to disable errorVerbose#1487

Open
makotonakai wants to merge 15 commits into
uber-go:masterfrom
makotonakai:turn-off-error-verbose
Open

Add an option to disable errorVerbose#1487
makotonakai wants to merge 15 commits into
uber-go:masterfrom
makotonakai:turn-off-error-verbose

Conversation

@makotonakai

@makotonakai makotonakai commented Jan 30, 2025

Copy link
Copy Markdown

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

Comment thread zapcore/field.go Outdated
Integer int64
String string
Interface interface{}
DisableErrorVerbose bool

Copy link
Copy Markdown
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.

@makotonakai makotonakai Feb 4, 2025

Copy link
Copy Markdown
Author

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
Copy Markdown
Author

@prashantv

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

@Amphaal

Amphaal commented May 20, 2025

Copy link
Copy Markdown

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

@prashantv

Copy link
Copy Markdown
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