Added support for providing custom logger name encoders #465
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #465 +/- ##
=========================================
+ Coverage 96.86% 96.9% +0.03%
=========================================
Files 36 36
Lines 2171 2197 +26
=========================================
+ Hits 2103 2129 +26
Misses 59 59
Partials 9 9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR, @pavius! It looks really good, and the test coverage is excellent.
The main problem is that it's not backward-compatible - I've left some comments that suggest how we can keep compatibility. Also, as you mentioned in the PR summary, EncodeLoggerName is a bit wordy. I'd prefer EncodeName.
If you can make a few small changes, I'm happy to merge this.
zapcore/console_encoder.go
Outdated
| if ent.LoggerName != "" && c.NameKey != "" { | ||
| arr.AppendString(ent.LoggerName) | ||
| if ent.LoggerName != "" && c.NameKey != "" && c.EncodeLoggerName != nil { | ||
| c.EncodeLoggerName(ent.LoggerName, arr) |
There was a problem hiding this comment.
Unfortunately, this isn't backward-compatible. If c.EncodeLoggerName is nil, we should use FullLoggerNameEncoder.
zapcore/encoder.go
Outdated
| // CapitalLoggerNameEncoder serializes a logger name in all caps | ||
| func CapitalLoggerNameEncoder(loggerName string, enc PrimitiveArrayEncoder) { | ||
| enc.AppendString(strings.ToUpper(loggerName)) | ||
| } |
There was a problem hiding this comment.
Rather than exporting this, let's put it into the test code. That'll give unit tests a way to exercise this functionality without adding this unusual option to the public API.
zapcore/encoder_test.go
Outdated
| EncodeDuration: base.EncodeDuration, | ||
| EncodeLevel: base.EncodeLevel, | ||
| EncodeCaller: base.EncodeCaller, | ||
| EncodeLoggerName: base.EncodeLoggerName, |
There was a problem hiding this comment.
We should only need to add two tests here: one that uses the capitalized encoder, and one that uses a no-op encoder. The other tests should pass as-is, and keeping them that way ensures that we don't break backward-compatibility later.
zapcore/json_encoder.go
Outdated
| @@ -302,7 +302,13 @@ func (enc *jsonEncoder) EncodeEntry(ent Entry, fields []Field) (*buffer.Buffer, | |||
| } | |||
| if ent.LoggerName != "" && final.NameKey != "" { | |||
| final.addKey(final.NameKey) | |||
| final.AppendString(ent.LoggerName) | |||
| cur := final.buf.Len() | |||
| final.EncodeLoggerName(ent.LoggerName, final) | |||
There was a problem hiding this comment.
Same here - if the user-supplied logger name encoder is nil, we should fall back to the current behavior.
zapcore/encoder.go
Outdated
| EncodeTime TimeEncoder `json:"timeEncoder" yaml:"timeEncoder"` | ||
| EncodeDuration DurationEncoder `json:"durationEncoder" yaml:"durationEncoder"` | ||
| EncodeCaller CallerEncoder `json:"callerEncoder" yaml:"callerEncoder"` | ||
| EncodeLoggerName LoggerNameEncoder `json:"nameEncoder" yaml:"nameEncoder"` |
There was a problem hiding this comment.
Let's document that EncoderLoggerName is optional, and that the fallback behavior is to include the name as-is.
pavius
force-pushed
the
support_encode_logger_name
branch
from
751edf2
to
3cbbe1a
Compare
|
Thanks for the comments @akshayjshah. Was too busy following suite to think about backwards compatibility. Addressed your comments and renamed to |
Users can now provide a
LoggerNameEncoderto override encoding of the logger name, which currently defaults to simply outputting the string.Two
LoggerNameEncoders are provided -FullLoggerNameEncoder, the default, which dumps the string as before to maintain backwards compatibility andCapitalLoggerNameEncoderwhich is there mostly to give the tests another option to use.Quite a bit of the PR is slight formatting to accommodate the longish name
LoggerNameEncoder. I considered calling itNameEncoderseeing howLoggerName's key name is referred to asName, but opted to stick to the member name.