-
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 14 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 |
---|---|---|
|
@@ -27,6 +27,7 @@ import ( | |
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"go.uber.org/atomic" | ||
"go.uber.org/zap/zapcore" | ||
) | ||
|
||
|
@@ -144,3 +145,72 @@ func TestConfigWithMissingAttributes(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func makeSamplerCountingHook() (h func(zapcore.Entry, zapcore.SamplingDecision), | ||
dropped, sampled *atomic.Int64) { | ||
dropped = new(atomic.Int64) | ||
sampled = new(atomic.Int64) | ||
h = func(_ zapcore.Entry, dec zapcore.SamplingDecision) { | ||
if dec == zapcore.LogDropped { | ||
dropped.Inc() | ||
} else if dec == zapcore.LogSampled { | ||
sampled.Inc() | ||
} | ||
} | ||
return h, dropped, sampled | ||
} | ||
|
||
func TestConfigWithSamplingHook(t *testing.T) { | ||
shook, dcount, scount := makeSamplerCountingHook() | ||
cfg := Config{ | ||
Level: NewAtomicLevelAt(InfoLevel), | ||
Development: false, | ||
Sampling: &SamplingConfig{ | ||
Initial: 100, | ||
Thereafter: 100, | ||
Hook: shook, | ||
}, | ||
Encoding: "json", | ||
EncoderConfig: NewProductionEncoderConfig(), | ||
OutputPaths: []string{"stderr"}, | ||
ErrorOutputPaths: []string{"stderr"}, | ||
} | ||
expectN := 2 + 101 // out of 203 logs: 3 types (debug, info, warn), 2 (debug + warn) + 101 (100 initial + 200th thereafter) | ||
expectRe := `{"level":"info","caller":"zap/config_test.go:\d+","msg":"info","k":"v","z":"zz"}` + "\n" + | ||
`{"level":"warn","caller":"zap/config_test.go:\d+","msg":"warn","k":"v","z":"zz"}` + "\n" | ||
expectDropped := 99 // 200 - 100 initial - 1 thereafter | ||
expectSampled := 103 // 2 from initial + 100 + 1 thereafter | ||
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. why is this the same as 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.
|
||
|
||
temp, err := ioutil.TempFile("", "zap-prod-config-test") | ||
require.NoError(t, err, "Failed to create temp file.") | ||
defer func() { | ||
err := os.Remove(temp.Name()) | ||
if err != nil { | ||
return | ||
} | ||
}() | ||
|
||
cfg.OutputPaths = []string{temp.Name()} | ||
cfg.EncoderConfig.TimeKey = "" // no timestamps in tests | ||
cfg.InitialFields = map[string]interface{}{"z": "zz", "k": "v"} | ||
|
||
hook, count := makeCountingHook() | ||
logger, err := cfg.Build(Hooks(hook)) | ||
require.NoError(t, err, "Unexpected error constructing logger.") | ||
|
||
logger.Debug("debug") | ||
logger.Info("info") | ||
logger.Warn("warn") | ||
|
||
byteContents, err := ioutil.ReadAll(temp) | ||
require.NoError(t, err, "Couldn't read log contents from temp file.") | ||
logs := string(byteContents) | ||
assert.Regexp(t, expectRe, logs, "Unexpected log output.") | ||
|
||
for i := 0; i < 200; i++ { | ||
logger.Info("sampling") | ||
} | ||
assert.Equal(t, int64(expectN), count.Load(), "Hook called an unexpected number of times.") | ||
assert.Equal(t, int64(expectDropped), dcount.Load()) | ||
assert.Equal(t, int64(expectSampled), scount.Load()) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -81,33 +81,96 @@ func (c *counter) IncCheckReset(t time.Time, tick time.Duration) uint64 { | |||||||||||||||||||||||
return 1 | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
type sampler struct { | ||||||||||||||||||||||||
Core | ||||||||||||||||||||||||
// SamplingDecision is a decision made by sampler. | ||||||||||||||||||||||||
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. we should comment that more decision values may be added in future, so it's not safe to assume these are the only values. |
||||||||||||||||||||||||
type SamplingDecision uint8 | ||||||||||||||||||||||||
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. I'm guessing this is so that it takes one byte instead of 8? 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. yup, as byte is an alias to uint8. |
||||||||||||||||||||||||
|
||||||||||||||||||||||||
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) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// NopSamplingHook is the default hook used by sampler. | ||||||||||||||||||||||||
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. Do we actually need to export this? We'll use this if unset. 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. Maybe if someone wants to test this as NewSampler* constructors are public? 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. NewSampler can be public but I mean the implementation of a no-op hook. Users don't really need access to that; they can write their own empty function, or just not provide the SamplingHook option. 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. Agree, not even sure if we need this in the production code, we could keep it in the test file? 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. It's production. The sampler defaults the hook field to NopSamplingHook. 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. I see, in that case, can we unexport and keep it here? |
||||||||||||||||||||||||
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. | ||||||||||||||||||||||||
func SamplerHook(hook func(entry Entry, dec SamplingDecision)) SamplerOption { | ||||||||||||||||||||||||
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. Worth adding a sample of how to use the sampling decision?
Suggested change
|
||||||||||||||||||||||||
return optionFunc(func(s *sampler) { | ||||||||||||||||||||||||
s.hook = hook | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// 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. | ||||||||||||||||||||||||
// 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 +180,7 @@ func (s *sampler) With(fields []Field) Core { | |||||||||||||||||||||||
counts: s.counts, | ||||||||||||||||||||||||
first: s.first, | ||||||||||||||||||||||||
thereafter: s.thereafter, | ||||||||||||||||||||||||
hook: s.hook, | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -128,7 +192,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 { | ||
droppedCount.Inc() | ||
} else if dec == LogSampled { | ||
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.
I'm confused by these numbers. What exactly is 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.
see #813 (comment)