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

EncoderConfig and AtomicLevel validation added #791

Merged
merged 9 commits into from Mar 4, 2020

Conversation

YashishDua
Copy link
Contributor

@YashishDua YashishDua commented Feb 21, 2020

Resolves #777

@codecov
Copy link

@codecov codecov bot commented Feb 21, 2020

Codecov Report

Merging #791 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   98.22%   98.31%   +0.08%     
==========================================
  Files          43       43              
  Lines        2306     2310       +4     
==========================================
+ Hits         2265     2271       +6     
+ Misses         33       32       -1     
+ Partials        8        7       -1
Impacted Files Coverage Δ
encoder.go 100% <100%> (ø) ⬆️
config.go 100% <100%> (+2.08%) ⬆️

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 c89bd3c...dffa12d. Read the comment docs.

Copy link
Contributor

@abhinav abhinav left a comment

Hello! Thanks for the PR. I've left a couple comments. In particular, I'm not sure whether we should fall back to a default behavior or return an error. Although the default for log levels is not very controversial, the default for the time encoder is. Since different users have different requirements, we may want to err on the side of requiring that they be explicitly specified.

config.go Show resolved Hide resolved
config.go Outdated
cfg.Level = NewAtomicLevelAt(InfoLevel)
if cfg.Development {
cfg.Level = NewAtomicLevelAt(DebugLevel)
Copy link
Contributor

@abhinav abhinav Feb 22, 2020

Choose a reason for hiding this comment

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

Should we return an error instead?

If we prefer to add a default behavior for Level when unspecified, we'd probably want to document it on Config.Level.

Copy link
Contributor Author

@YashishDua YashishDua Mar 1, 2020

Choose a reason for hiding this comment

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

I could see InfoLevel as default in ProductionConfig and DebugLevel in DevelopmentConfig, that's why thought of adding the same defaults here.

Let's return an error then!

config.go Outdated
@@ -239,5 +246,12 @@ func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error)
}

func (cfg Config) buildEncoder() (zapcore.Encoder, error) {
if cfg.EncoderConfig.TimeKey != "" && cfg.EncoderConfig.EncodeTime == nil {
cfg.EncoderConfig.EncodeTime = zapcore.EpochTimeEncoder
Copy link
Contributor

@abhinav abhinav Feb 22, 2020

Choose a reason for hiding this comment

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

Same here. It's not obvious why we pick epoch for production and ISO8601 for development. Perhaps an error would be preferable?

Copy link
Contributor Author

@YashishDua YashishDua Mar 1, 2020

Choose a reason for hiding this comment

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

NewDevelopmentEncoderConfig specifies ISO8601TimeEncoder by default. I have not made any explicit choices here. But, let's return an error.

@abhinav abhinav requested review from mway and prashantv and removed request for prashantv Feb 22, 2020
@prashantv
Copy link
Contributor

@prashantv prashantv commented Feb 24, 2020

Agree with @abhinav -- I think Development changing time behaviour might be surprising, since the current docs for Development is documented only to put the logger in development mode.

// Development puts the logger in development mode, which changes the
// behavior of DPanicLevel and takes stacktraces more liberally.

To avoid any surprises, returning an error seems best. We should also add unit tests to validate the new behaviour. Thanks!

Copy link
Contributor

@abhinav abhinav left a comment

Thanks! Looks mostly fine. Requesting minor changes.

config_test.go Outdated Show resolved Hide resolved
config_test.go Show resolved Hide resolved
config_test.go Outdated Show resolved Hide resolved
config_test.go Outdated Show resolved Hide resolved
config.go Outdated
@@ -239,5 +244,9 @@ func (cfg Config) openSinks() (zapcore.WriteSyncer, zapcore.WriteSyncer, error)
}

func (cfg Config) buildEncoder() (zapcore.Encoder, error) {
if cfg.EncoderConfig.TimeKey != "" && cfg.EncoderConfig.EncodeTime == nil {
Copy link
Contributor

@abhinav abhinav Mar 4, 2020

Choose a reason for hiding this comment

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

It seems that newEncoder is the ultimate constructor for zapcore.Encoder given
an EncoderConfig. We should maybe move this check to the newEncoder function,
leaving buildEncoder unchanged.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Mar 4, 2020

CLA assistant check
All committers have signed the CLA.

abhinav added 2 commits Mar 4, 2020
`if a == b{} { ... }` is syntactically invalid Go.
@abhinav abhinav requested a review from prashantv Mar 4, 2020
@abhinav abhinav merged commit 2c14fe6 into uber-go:master Mar 4, 2020
4 checks passed
cgxxv pushed a commit to cgxxv/zap that referenced this issue Mar 25, 2022
This adds validation for Level and EncoderConfig.EncoderTime when
building a logger from a zap.Config.

Resolves uber-go#777

Co-authored-by: Abhinav Gupta <abg@uber.com>
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.

4 participants