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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
605a517
Discard counters for out-of-bounds levels
thockin Jun 29, 2021
dabfb23
Update test to verify no sampling & fatal+1 level
prashantv Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions zapcore/sampler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link

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.

Copy link

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?

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
}
s.hook(ent, LogSampled)
}
s.hook(ent, LogSampled)
return s.Core.Check(ent, ce)
}
20 changes: 20 additions & 0 deletions zapcore/sampler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,23 @@ func TestSamplerRaces(t *testing.T) {
close(start)
wg.Wait()
}

func TestSamplerUnknownLevels(t *testing.T) {
// Prove that out-of-bounds levels don't panic.
unknownLevels := []Level{
DebugLevel - 1,
FatalLevel + 1,
}

for _, lvl := range unknownLevels {
t.Run(lvl.String(), func(t *testing.T) {
sampler, logs := fakeSampler(lvl, time.Minute, 2, 3)
for i := 1; i < 10; i++ {
writeSequence(sampler, i, lvl)
}

// Expect no sampling for unknown levels.
assertSequence(t, logs.TakeAll(), lvl, 1, 2, 3, 4, 5, 6, 7, 8, 9)
})
}
}