Not mutate global zerolog.LevelFieldName #7
Conversation
example/example_test.go
Outdated
| @@ -45,6 +45,7 @@ func helper2(log logr.Logger, msg string) { | |||
|
|
|||
| func ExampleNew() { | |||
| zerolog.SetGlobalLevel(zerolog.DebugLevel) | |||
| zerolog.LevelFieldName = "" | |||
There was a problem hiding this comment.
You have to add this to get the test to pass, right? That implies that the change in NewLogSink is a breaking change.
We'll need a NewLogSinkWithOptions and leave NewLogSink unchanged.
There was a problem hiding this comment.
Normally I agree that we need to avoid breaking change, but this PR is to fix a bug that users can not see any level or v field in the codepath not using logr/zerologr. And restoring level field is less likely to break code than removing field.
|
@thockin @pohly As I explained in the issue #6, this PR is to restore level key to NOT breaking zerolog API, otherwises there is no level and v field in the codepath using zerolog without zerologr. NewWithOptions may not be necessary since DisableZerologLevelField is just mutating zerolog.LevelFieldName globally, so IMHO we can add NewWithOptions later when we have options to apply on LogSink struct. Restoring level field will make zerologr consistent with zapr. If users who use logr in all parts of application think level field and v field are redundant, they can disable zerolog.LevelFieldName and zapcore.OmitKey (or we can do a better job documenting it) |
|
I see. I had it backwards. |
#6