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.

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

zap: always return a new child logger from *Logger.With #1342

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liuzengh
Copy link

even if fields parameters is empty to make *Logger.With's behavior consistent with *Logger.WithOptions(Fields())'s behavior.

even if fields parameters is empty to make `*Logger.With`'s behavior consistent with `*Logger.WithOptions(Fields())`'s behavior.
@CLAassistant
Copy link

CLAassistant commented Aug 28, 2023

CLA assistant check
All committers have signed the CLA.

@liuzengh
Copy link
Author

liuzengh commented Aug 28, 2023

previous merged refactor PR #201 “altered the external behavior of With method.

// https://github.com/uber-go/zap/pull/201/files#diff-ff87b7c4777a35588053a509583d66c9f404ccbea9e1c71d2a5f224d7ad1323eL75
func (log *logger) With(fields ...Field) Logger {
	clone := &logger{
		Meta: log.Meta.Clone(),
	}
	addFields(clone.Encoder, fields)
	return clone
}

And, I would be happy to see the behavior of *Logger.With and *Logger.WithOptions(Fields()) is same.

@rabbbit
Copy link
Contributor

rabbbit commented Aug 29, 2023

The PR you linked was from 2016 so likely the current state is not upsetting a lot of people.

Is this PR solving an actual issue you encountered? If so, could you elaborate on why you need the logger to be a new object?
Partially related, we were discussing this in #1340.

@liuzengh
Copy link
Author

I guess almost no one would use With with an empty 'fields', and they don't care whether the two Loggers before and after are the same. So people haven't been upset about current state.

  1. If people With an non-empty fields', it is not necessary to check whether the 'fields' is empty in the With method. This PR remove the empty 'fields' check logic, it looks good.

  2. If people With with an empty 'fields'. Let's discuss two scenarios. In scenario 1, he actually doesn't want to deal with empty fields. His code implementation is flawed. Instead of using zap package to check for empty fields within the With method, he should handle the empty field checks within his business logic code. In scenario 2, he intentionally uses empty fields in the With method to get a new logger, and performs some operations on it, such as setting the log level for the new logger( implementing the SetLevel method on the wrapped zap.Logger).

Maybe, I should reword the CL "zap: always return a new child logger from *Logger.With" to "zap: Remove the empty 'fields' check logic in *Logger.With" after I see #1340.

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.

3 participants