-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
core/sampler: Support decision hook #813
Changes from all commits
14b1350
c59b269
e268359
cefe15e
1f5cf2c
44fe037
25478e6
5cc987e
fd20684
4ae250b
d48791a
3c00734
0bc5a78
8b91fbb
09045e2
6d960ec
ae3e230
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,33 +81,104 @@ func (c *counter) IncCheckReset(t time.Time, tick time.Duration) uint64 { | |
return 1 | ||
} | ||
|
||
type sampler struct { | ||
Core | ||
// SamplingDecision is a decision represented as a bit field made by sampler. | ||
// More decisions may be added in the future. | ||
type SamplingDecision uint32 | ||
|
||
counts *counters | ||
tick time.Duration | ||
first, thereafter uint64 | ||
const ( | ||
// LogDropped indicates that the Sampler dropped a log entry. | ||
LogDropped SamplingDecision = 1 << iota | ||
// LogSampled indicates that the Sampler sampled a log entry. | ||
LogSampled | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something to discuss
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just an idea: If we make SamplingDecision a bitfield, we could do that separately and re-use sampler logic
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good idea, I like the idea of a bit field, so users can just check we might want to make that clear in the comments that users should check this like a bit field (or maybe we provide a helper on the type to do that). We could expand this to have:
|
||
) | ||
|
||
// optionFunc wraps a func so it satisfies the SamplerOption interface. | ||
type optionFunc func(*sampler) | ||
|
||
func (f optionFunc) apply(s *sampler) { | ||
f(s) | ||
} | ||
|
||
// SamplerOption configures a Sampler. | ||
type SamplerOption interface { | ||
apply(*sampler) | ||
} | ||
|
||
// NewSampler creates a Core that samples incoming entries, which caps the CPU | ||
// and I/O load of logging while attempting to preserve a representative subset | ||
// of your logs. | ||
// nopSamplingHook is the default hook used by sampler. | ||
func nopSamplingHook(Entry, SamplingDecision) {} | ||
|
||
// SamplerHook registers a function which will be called when Sampler makes a | ||
// decision. | ||
// | ||
// This hook may be used to get visibility into the performance of the sampler. | ||
// For example, use it to track metrics of dropped versus sampled logs. | ||
// | ||
// var dropped atomic.Int64 | ||
// zapcore.SamplerHook(func(ent zapcore.Entry, dec zapcore.SamplingDecision) { | ||
// if dec&zapcore.LogDropped > 0 { | ||
// dropped.Inc() | ||
// } | ||
// }) | ||
func SamplerHook(hook func(entry Entry, dec SamplingDecision)) SamplerOption { | ||
return optionFunc(func(s *sampler) { | ||
s.hook = hook | ||
}) | ||
} | ||
|
||
// NewSamplerWithOptions creates a Core that samples incoming entries, which | ||
// caps the CPU and I/O load of logging while attempting to preserve a | ||
// representative subset of your logs. | ||
// | ||
// Zap samples by logging the first N entries with a given level and message | ||
// each tick. If more Entries with the same level and message are seen during | ||
// the same interval, every Mth message is logged and the rest are dropped. | ||
// | ||
// Sampler can be configured to report sampling decisions with the SamplerHook | ||
// option. | ||
// | ||
// Keep in mind that zap's sampling implementation is optimized for speed over | ||
// absolute precision; under load, each tick may be slightly over- or | ||
// under-sampled. | ||
func NewSampler(core Core, tick time.Duration, first, thereafter int) Core { | ||
return &sampler{ | ||
func NewSamplerWithOptions(core Core, tick time.Duration, first, thereafter int, opts ...SamplerOption) Core { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to reduce the added API surface (E.g., new function that looks similar + new types), we could add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't we still want a means of building the sampler directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. true, I'm not sure how valuable the direct structs have been vs configs, but don't have to make that change here. let's keep this as-is |
||
s := &sampler{ | ||
Core: core, | ||
tick: tick, | ||
counts: newCounters(), | ||
first: uint64(first), | ||
thereafter: uint64(thereafter), | ||
hook: nopSamplingHook, | ||
} | ||
for _, opt := range opts { | ||
opt.apply(s) | ||
} | ||
|
||
return s | ||
} | ||
|
||
type sampler struct { | ||
Core | ||
|
||
counts *counters | ||
tick time.Duration | ||
first, thereafter uint64 | ||
hook func(Entry, SamplingDecision) | ||
} | ||
|
||
// NewSampler creates a Core that samples incoming entries, which | ||
// caps the CPU and I/O load of logging while attempting to preserve a | ||
// representative subset of your logs. | ||
// | ||
// Zap samples by logging the first N entries with a given level and message | ||
// each tick. If more Entries with the same level and message are seen during | ||
// the same interval, every Mth message is logged and the rest are dropped. | ||
// | ||
// Keep in mind that zap's sampling implementation is optimized for speed over | ||
// absolute precision; under load, each tick may be slightly over- or | ||
// under-sampled. | ||
// | ||
// Deprecated: use NewSamplerWithOptions. | ||
shirchen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func NewSampler(core Core, tick time.Duration, first, thereafter int) Core { | ||
return NewSamplerWithOptions(core, tick, first, thereafter) | ||
} | ||
|
||
func (s *sampler) With(fields []Field) Core { | ||
|
@@ -117,6 +188,7 @@ func (s *sampler) With(fields []Field) Core { | |
counts: s.counts, | ||
first: s.first, | ||
thereafter: s.thereafter, | ||
hook: s.hook, | ||
} | ||
} | ||
|
||
|
@@ -128,7 +200,9 @@ func (s *sampler) Check(ent Entry, ce *CheckedEntry) *CheckedEntry { | |
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) | ||
return s.Core.Check(ent, ce) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,8 @@ import ( | |
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"go.uber.org/atomic" | ||
"go.uber.org/zap/internal/ztest" | ||
. "go.uber.org/zap/zapcore" | ||
) | ||
|
@@ -203,7 +205,7 @@ var counterTestCases = [][]string{ | |
func BenchmarkSampler_Check(b *testing.B) { | ||
for _, keys := range counterTestCases { | ||
b.Run(fmt.Sprintf("%v keys", len(keys)), func(b *testing.B) { | ||
fac := NewSampler( | ||
fac := NewSamplerWithOptions( | ||
NewCore( | ||
NewJSONEncoder(testEncoderConfig()), | ||
&ztest.Discarder{}, | ||
|
@@ -228,3 +230,54 @@ func BenchmarkSampler_Check(b *testing.B) { | |
}) | ||
} | ||
} | ||
|
||
func makeSamplerCountingHook() (func(_ Entry, dec SamplingDecision), *atomic.Int64, *atomic.Int64) { | ||
droppedCount := new(atomic.Int64) | ||
sampledCount := new(atomic.Int64) | ||
h := func(_ Entry, dec SamplingDecision) { | ||
if dec&LogDropped > 0 { | ||
droppedCount.Inc() | ||
} else if dec&LogSampled > 0 { | ||
sampledCount.Inc() | ||
} | ||
} | ||
return h, droppedCount, sampledCount | ||
} | ||
|
||
func BenchmarkSampler_CheckWithHook(b *testing.B) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. optional: count dropped messages only to more accurately measure the use case we're trying to satisfy here. |
||
hook, dropped, sampled := makeSamplerCountingHook() | ||
for _, keys := range counterTestCases { | ||
b.Run(fmt.Sprintf("%v keys", len(keys)), func(b *testing.B) { | ||
fac := NewSamplerWithOptions( | ||
NewCore( | ||
NewJSONEncoder(testEncoderConfig()), | ||
&ztest.Discarder{}, | ||
DebugLevel, | ||
), | ||
time.Millisecond, | ||
1, | ||
1000, | ||
SamplerHook(hook), | ||
) | ||
b.ResetTimer() | ||
b.RunParallel(func(pb *testing.PB) { | ||
i := 0 | ||
for pb.Next() { | ||
ent := Entry{ | ||
Level: DebugLevel + Level(i%4), | ||
Message: keys[i], | ||
} | ||
_ = fac.Check(ent, nil) | ||
i++ | ||
if n := len(keys); i >= n { | ||
i -= n | ||
} | ||
} | ||
}) | ||
}) | ||
} | ||
// We expect to see 1000 dropped messages for every sampled per settings, | ||
// with a delta due to less 1000 messages getting dropped after initial one | ||
// is sampled. | ||
assert.Greater(b, dropped.Load()/1000, sampled.Load()-1000) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ import ( | |
|
||
func fakeSampler(lvl LevelEnabler, tick time.Duration, first, thereafter int) (Core, *observer.ObservedLogs) { | ||
core, logs := observer.New(lvl) | ||
// Keep using deprecated constructor for cc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code coverage |
||
core = NewSampler(core, tick, first, thereafter) | ||
return core, logs | ||
} | ||
|
@@ -162,7 +163,7 @@ func TestSamplerConcurrent(t *testing.T) { | |
|
||
tick := ztest.Timeout(10 * time.Millisecond) | ||
cc := &countingCore{} | ||
sampler := NewSampler(cc, tick, logsPerTick, 100000) | ||
sampler := NewSamplerWithOptions(cc, tick, logsPerTick, 100000) | ||
|
||
var ( | ||
done atomic.Bool | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this the same as
expectN
? we expect the hook to be called on every log, and thenexpectDropped + expectSampled == expectN
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectN
is not a count from samplerHook, but from a zap hook which only measures what is sent. i regret carrying over a hook from another test now as it seems to cause confusion. :)