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

Added support for providing custom logger name encoders #465

Merged
merged 5 commits into from
Jul 22, 2017

Conversation

pavius
Copy link
Contributor

@pavius pavius commented Jul 3, 2017

Users can now provide a LoggerNameEncoder to 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 and CapitalLoggerNameEncoder which 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 it NameEncoder seeing how LoggerName's key name is referred to as Name, but opted to stick to the member name.

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jul 3, 2017

Codecov Report

Merging #465 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
zapcore/encoder.go 81.6% <100%> (+2.12%) ⬆️
zapcore/console_encoder.go 97.56% <100%> (+0.15%) ⬆️
zapcore/json_encoder.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dadca01...10be78c. Read the comment docs.

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

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.

if ent.LoggerName != "" && c.NameKey != "" {
arr.AppendString(ent.LoggerName)
if ent.LoggerName != "" && c.NameKey != "" && c.EncodeLoggerName != nil {
c.EncodeLoggerName(ent.LoggerName, arr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this isn't backward-compatible. If c.EncodeLoggerName is nil, we should use FullLoggerNameEncoder.

// CapitalLoggerNameEncoder serializes a logger name in all caps
func CapitalLoggerNameEncoder(loggerName string, enc PrimitiveArrayEncoder) {
enc.AppendString(strings.ToUpper(loggerName))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

EncodeDuration: base.EncodeDuration,
EncodeLevel: base.EncodeLevel,
EncodeCaller: base.EncodeCaller,
EncodeLoggerName: base.EncodeLoggerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here - if the user-supplied logger name encoder is nil, we should fall back to the current behavior.

EncodeTime TimeEncoder `json:"timeEncoder" yaml:"timeEncoder"`
EncodeDuration DurationEncoder `json:"durationEncoder" yaml:"durationEncoder"`
EncodeCaller CallerEncoder `json:"callerEncoder" yaml:"callerEncoder"`
EncodeLoggerName LoggerNameEncoder `json:"nameEncoder" yaml:"nameEncoder"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's document that EncoderLoggerName is optional, and that the fallback behavior is to include the name as-is.

@pavius pavius force-pushed the support_encode_logger_name branch from 751edf2 to 3cbbe1a Compare July 10, 2017 13:34
@pavius
Copy link
Contributor Author

pavius commented Jul 10, 2017

Thanks for the comments @akshayjshah. Was too busy following suite to think about backwards compatibility. Addressed your comments and renamed to EncodeName (and consequently FullLoggerNameEncoder became FullNameEncoder).

Copy link
Contributor

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

This looks great - thanks for the contribution, @pavius!

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