Discard counters for out-of-bounds levels#975
Merged
prashantv merged 2 commits intouber-go:masterfrom Jul 1, 2021
thockin:master
Merged
Discard counters for out-of-bounds levels#975prashantv merged 2 commits intouber-go:masterfrom thockin:master
prashantv merged 2 commits intouber-go:masterfrom
thockin:master
Conversation
|
|
prashantv
reviewed
Jun 29, 2021
Previously this just crashed.
Contributor
Author
|
Please take a look - is that what you had in mind?
…On Wed, Jun 30, 2021 at 2:50 PM Prashant Varanasi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zapcore/sampler.go
<#975 (comment)>:
> func (cs *counters) get(lvl Level, key string) *counter {
i := lvl - _minLevel
+ if i < 0 {
+ return &nullCounter
My alternative proposal was mostly to localize this change to where we use
the level to convert to an index, but I agree that affecting other levels
may be surprising.
I think the preferred behaviour is for the sampler to ignore unknown
levels. Instead of making any change here, we can instead do the level
check at the caller,
https://github.com/uber-go/zap/blob/v1.18.1/zapcore/sampler.go#L200
If the level is unsupported, then we "fail open" by calling the underlying
core's Check.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVFUG2T4N55BWENNKSTTVOGQ5ANCNFSM47PEQ2YQ>
.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
abhinav
approved these changes
Jul 1, 2021
Contributor
Author
|
Hello. Any chance we can get a new tag, so I can pin to this version? |
nmiculinic
reviewed
Aug 5, 2021
| if n > s.first && (n-s.first)%s.thereafter != 0 { | ||
| s.hook(ent, LogDropped) | ||
| return ce | ||
| if ent.Level >= _minLevel && ent.Level <= _maxLevel { |
There was a problem hiding this comment.
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?
Collaborator
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Contributor
Author
|
My feeling is that super-verbose levels are special and not "normal" and,
as such, sampling is probably not appropriate.
That's how I justify it to myself anyway :)
BTW: How about a new tag ? :)
|
Contributor
Author
|
That was my feeling. logr V(1) == zap.Debug (so sampled). logr V(2) ==
zap.Level(-2). Now I will admit that some k8s components run at V=2 by
default, but that itself seems like a problem :)
…On Thu, Aug 5, 2021 at 11:36 PM Neven Miculinic ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zapcore/sampler.go
<#975 (comment)>:
> @@ -197,12 +197,14 @@ func (s *sampler) Check(ent Entry, ce *CheckedEntry) *CheckedEntry {
return ce
}
- counter := s.counts.get(ent.Level, ent.Message)
- n := counter.IncCheckReset(ent.Time, s.tick)
- if n > s.first && (n-s.first)%s.thereafter != 0 {
- s.hook(ent, LogDropped)
- return ce
+ if ent.Level >= _minLevel && ent.Level <= _maxLevel {
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?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVH2IAIBPZ7HPQNSFPLT3N7FBANCNFSM47PEQ2YQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
Collaborator
Oops, missed this. We'll tag a release next week. (We prefer not to tag new releases of core components on Fridays.) Thanks, @thockin! |
Contributor
Author
|
Thanks!
…On Fri, Aug 6, 2021 at 11:38 AM Abhinav Gupta ***@***.***> wrote:
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
<https://github.com/thockin>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#975 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVFVI5O746LPZEPK5QTT3QT35ANCNFSM47PEQ2YQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously this just crashed.
Fixes #972