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

Discard counters for out-of-bounds levels #975

Merged
merged 2 commits into from
Jul 1, 2021
Merged

Discard counters for out-of-bounds levels #975

merged 2 commits into from
Jul 1, 2021

Conversation

thockin
Copy link
Contributor

@thockin thockin commented Jun 29, 2021

Previously this just crashed.

Fixes #972

@CLAassistant
Copy link

CLAassistant commented Jun 29, 2021

CLA assistant check
All committers have signed the CLA.

zapcore/sampler.go Outdated Show resolved Hide resolved
zapcore/sampler.go Outdated Show resolved Hide resolved
Previously this just crashed.
@thockin
Copy link
Contributor Author

thockin commented Jun 30, 2021 via email

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #975 (dabfb23) into master (120b08c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #975   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files          46       46           
  Lines        2056     2057    +1     
=======================================
+ Hits         2017     2018    +1     
  Misses         30       30           
  Partials        9        9           
Impacted Files Coverage Δ
zapcore/sampler.go 96.22% <100.00%> (+0.07%) ⬆️

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 120b08c...dabfb23. Read the comment docs.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Yep, this is what I had in mind, thanks!

Change looks good to me, I've pushed a couple of small test changes (more levels + validate no sampling).

Tagging @abhinav to take a look as well.

@prashantv prashantv requested a review from abhinav June 30, 2021 23:57
@prashantv prashantv merged commit 007a55e into uber-go:master Jul 1, 2021
@thockin
Copy link
Contributor Author

thockin commented Aug 3, 2021

Hello. Any chance we can get a new tag, so I can pin to this version?

if n > s.first && (n-s.first)%s.thereafter != 0 {
s.hook(ent, LogDropped)
return ce
if ent.Level >= _minLevel && ent.Level <= _maxLevel {

Choose a reason for hiding this comment

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

Maybe I'm dropping here like a martian, but wouldn't it make more sense to clip the ent.Level to [_minLevel, _maxLevel] range?

Will this behavior cause log entries outside valid range to bypass sampling, thus always be emitted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was one of the options considered, see #975 (comment) but there was concern that it could confuse users.

Yes, the current approach causes log entries with unknown levels to bypass sampling.

Choose a reason for hiding this comment

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

In typical usecase for this feature would be integration with klog/logr. Those out-of-bounds entries are going to be the most common ones actually, and ones which should be sampled...probably....maybe when user actually uses those levels they are interested in all logs and will bypass sampling anyway?

@thockin
Copy link
Contributor Author

thockin commented Aug 6, 2021 via email

@thockin
Copy link
Contributor Author

thockin commented Aug 6, 2021 via email

@abhinav
Copy link
Collaborator

abhinav commented Aug 6, 2021

BTW: How about a new tag ? :)

Oops, missed this. We'll tag a release next week. (We prefer not to tag new releases of core components on Fridays.) Thanks, @thockin!

@thockin
Copy link
Contributor Author

thockin commented Aug 9, 2021 via email

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.

Zap + logr == not very expressive, open to fixes?
5 participants