-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 8 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 |
---|---|---|
|
@@ -32,10 +32,15 @@ import ( | |
// global CPU and I/O load that logging puts on your process while attempting | ||
// to preserve a representative subset of your logs. | ||
// | ||
// Values configured here are per-second. See zapcore.NewSampler for details. | ||
// Hook is called whenever a Sampler makes a decision. Currently, whenever a | ||
// log is dropped. | ||
// | ||
// Values configured here are per-second. See zapcore.NewSamplerWithOptions for | ||
// details. | ||
type SamplingConfig struct { | ||
Initial int `json:"initial" yaml:"initial"` | ||
Thereafter int `json:"thereafter" yaml:"thereafter"` | ||
Hook func(zapcore.Entry, zapcore.SamplingDecision) error | ||
} | ||
|
||
// Config offers a declarative way to construct a logger. It doesn't do | ||
|
@@ -207,10 +212,20 @@ func (cfg Config) buildOptions(errSink zapcore.WriteSyncer) []Option { | |
if !cfg.DisableStacktrace { | ||
opts = append(opts, AddStacktrace(stackLevel)) | ||
} | ||
if cfg.Sampling != nil && cfg.Sampling.Hook == nil { | ||
// Assign a default nop sampling hook. | ||
cfg.Sampling.Hook = zapcore.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. we should apply the default in NewSamplerWithOptions and just not pass the SamplerHook argument if nil here.
|
||
} | ||
|
||
if cfg.Sampling != nil { | ||
opts = append(opts, WrapCore(func(core zapcore.Core) zapcore.Core { | ||
return zapcore.NewSampler(core, time.Second, int(cfg.Sampling.Initial), int(cfg.Sampling.Thereafter)) | ||
return zapcore.NewSamplerWithOptions( | ||
core, | ||
time.Second, | ||
cfg.Sampling.Initial, | ||
cfg.Sampling.Thereafter, | ||
zapcore.SamplerHook(cfg.Sampling.Hook), | ||
) | ||
})) | ||
} | ||
|
||
|
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,65 @@ func TestConfigWithMissingAttributes(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func makeSamplerCountingHook() (func(zapcore.Entry, zapcore.SamplingDecision) error, *atomic.Int64) { | ||
count := &atomic.Int64{} | ||
h := func(zapcore.Entry, zapcore.SamplingDecision) error { | ||
count.Inc() | ||
return nil | ||
} | ||
return h, count | ||
} | ||
|
||
func TestConfigWithSamplingHook(t *testing.T) { | ||
shook, 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 + 100 + 1 // 2 from initial logs, 100 initial sampled logs, 1 from off-by-one in 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. can we fix the off-by-one, seems a little odd, haven't looked into where it's coming from 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. Yea, I didn't verify the comment and copied from another test. Fixed explanation. |
||
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 | ||
|
||
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), scount.Load()) | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -81,17 +81,87 @@ func (c *counter) IncCheckReset(t time.Time, tick time.Duration) uint64 { | |||||||||||||||||||
return 1 | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// SamplingDecision represents 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. nit: all types represent something. can do:
Suggested change
|
||||||||||||||||||||
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. |
||||||||||||||||||||
|
||||||||||||||||||||
const ( | ||||||||||||||||||||
// Dropped means that a log was dropped. | ||||||||||||||||||||
Dropped SamplingDecision = iota | ||||||||||||||||||||
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 like the idea behind this but the presence of this type suggests that there are at least two values: dropped and sampled, and I would then expect my hook to be called for all cases: sampled and dropped. So question for you and @prashantv is: Should we call the hook for both, sampled and dropped cases? If not, then the hook is just a "drop hook" or similar. (Name TBD) Naming-wise: Sampled and Dropped works for me. To be more specific, we can also use LogSampled and LogDropped. |
||||||||||||||||||||
) | ||||||||||||||||||||
|
||||||||||||||||||||
// 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 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.
Suggested change
|
||||||||||||||||||||
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) error { | ||||||||||||||||||||
return nil | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// SamplerHook registers a which will be called when Sampler makes a decision. | ||||||||||||||||||||
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. typo: missing "a function" |
||||||||||||||||||||
// Currently a hook is called when a log is dropped and zapcore.Dropped decision | ||||||||||||||||||||
// is emitted. | ||||||||||||||||||||
// | ||||||||||||||||||||
// This hook is useful for side effects, for example emitting number of dropped | ||||||||||||||||||||
// logs. Note, there is no access to Fields in this hook. In the future, this | ||||||||||||||||||||
// hook can be expanded to emit whether this is first entry that was dropped, | ||||||||||||||||||||
// first after a period, etc. | ||||||||||||||||||||
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 would avoid mentioning future plans in documentation. That's a comment-level thing, possibly referencing tickets.
Suggested change
|
||||||||||||||||||||
func SamplerHook(hook func(entry Entry, dec SamplingDecision) error) 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. I don't know if hooks should be able to return errors. |
||||||||||||||||||||
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 also accepts an optional hook that can be used to count number of | ||||||||||||||||||||
// dropped logs. | ||||||||||||||||||||
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.
Suggested change
|
||||||||||||||||||||
// | ||||||||||||||||||||
// 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 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) error | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// 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. | ||||||||||||||||||||
// 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 | ||||||||||||||||||||
|
@@ -100,13 +170,15 @@ type sampler struct { | |||||||||||||||||||
// 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 &sampler{ | ||||||||||||||||||||
Core: core, | ||||||||||||||||||||
tick: tick, | ||||||||||||||||||||
counts: newCounters(), | ||||||||||||||||||||
first: uint64(first), | ||||||||||||||||||||
thereafter: uint64(thereafter), | ||||||||||||||||||||
hook: 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. We should make it so that the old implementation calls the new one so that if we make changes to
Suggested change
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. Good call. |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -117,6 +189,7 @@ func (s *sampler) With(fields []Field) Core { | |||||||||||||||||||
counts: s.counts, | ||||||||||||||||||||
first: s.first, | ||||||||||||||||||||
thereafter: s.thereafter, | ||||||||||||||||||||
hook: s.hook, | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
|
@@ -128,7 +201,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, Dropped) | ||||||||||||||||||||
return ce | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
return s.Core.Check(ent, ce) | ||||||||||||||||||||
} |
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.
Second part no longer true. How about,