-
Notifications
You must be signed in to change notification settings - Fork 18
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
add reservoir timers #171
base: master
Are you sure you want to change the base?
add reservoir timers #171
Changes from 10 commits
72bc838
f67aca4
5cc34d9
f7fc55e
4a0662a
f35471d
57ef42b
4610f55
cc908b5
b1a2def
8eb942d
5dd8757
0d3fb45
ea5ae6a
e81d603
74a26a1
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 |
---|---|---|
|
@@ -6,6 +6,7 @@ type Sink interface { | |
FlushCounter(name string, value uint64) | ||
FlushGauge(name string, value uint64) | ||
FlushTimer(name string, value float64) | ||
FlushAggregatedTimer(name string, value, sampleRate float64) | ||
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. Because of this we'll need a new major release. The library usage should be backwards compatible, and additionally there is a feature flag to control the new behaviour, however if anything is implementing this interface it'll break. |
||
} | ||
|
||
// FlushableSink is an extension of Sink that provides a Flush() function that | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -298,30 +298,100 @@ func (c *gauge) Value() uint64 { | |
return atomic.LoadUint64(&c.value) | ||
} | ||
|
||
type timer struct { | ||
type timer interface { | ||
time(time.Duration) | ||
AddDuration(time.Duration) | ||
AddValue(float64) | ||
AllocateSpan() Timespan | ||
CollectedValue() []float64 | ||
SampleRate() float64 | ||
} | ||
|
||
type standardTimer struct { | ||
base time.Duration | ||
name string | ||
sink Sink | ||
} | ||
|
||
func (t *timer) time(dur time.Duration) { | ||
func (t *standardTimer) time(dur time.Duration) { | ||
t.AddDuration(dur) | ||
} | ||
|
||
func (t *timer) AddDuration(dur time.Duration) { | ||
func (t *standardTimer) AddDuration(dur time.Duration) { | ||
t.AddValue(float64(dur / t.base)) | ||
} | ||
|
||
func (t *timer) AddValue(value float64) { | ||
func (t *standardTimer) AddValue(value float64) { | ||
t.sink.FlushTimer(t.name, value) | ||
} | ||
|
||
func (t *timer) AllocateSpan() Timespan { | ||
func (t *standardTimer) AllocateSpan() Timespan { | ||
return ×pan{timer: t, start: time.Now()} | ||
} | ||
|
||
func (t *standardTimer) CollectedValue() []float64 { | ||
return nil | ||
} | ||
|
||
func (t *standardTimer) SampleRate() float64 { | ||
return 0.0 // todo: using zero value of float64. the correct value would be 1.0 given 1 stat, hwoever that 1 stat is never stored, just flushed right away | ||
} | ||
|
||
type reservoirTimer struct { | ||
base time.Duration | ||
name string | ||
capacity int | ||
values []float64 | ||
fill int // todo: the only purpose of this is to be faster than calculating len(values), is it worht it? | ||
count int | ||
mu sync.Mutex | ||
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. super nit, make the Mutex the first field (it's advantageous performance wise to make the most accessed field first) |
||
} | ||
|
||
func (t *reservoirTimer) time(dur time.Duration) { | ||
t.AddDuration(dur) | ||
} | ||
|
||
func (t *reservoirTimer) AddDuration(dur time.Duration) { | ||
t.AddValue(float64(dur / t.base)) | ||
} | ||
|
||
func (t *reservoirTimer) AddValue(value float64) { | ||
t.mu.Lock() | ||
defer t.mu.Unlock() | ||
|
||
// todo: consider edge cases for < | ||
if t.fill < t.capacity { | ||
t.values = append(t.values, value) | ||
} else { | ||
// todo: discarding the oldest value when the reference is full, this can probably be smarter | ||
t.values = append(t.values[1:], value) | ||
t.fill-- | ||
} | ||
|
||
t.fill++ | ||
t.count++ | ||
} | ||
|
||
func (t *reservoirTimer) AllocateSpan() Timespan { | ||
return ×pan{timer: t, start: time.Now()} | ||
} | ||
|
||
func (t *reservoirTimer) CollectedValue() []float64 { | ||
t.mu.Lock() | ||
defer t.mu.Unlock() | ||
|
||
// todo: Return a copy of the values slice to avoid data races | ||
valuesCopy := make([]float64, len(t.values)) | ||
copy(valuesCopy, t.values) | ||
return valuesCopy | ||
} | ||
|
||
func (t *reservoirTimer) SampleRate() float64 { | ||
return float64(t.fill) / float64(t.count) // todo: is it faster to store these values as float64 instead of converting here | ||
} | ||
|
||
type timespan struct { | ||
timer *timer | ||
timer timer | ||
start time.Time | ||
} | ||
|
||
|
@@ -338,7 +408,7 @@ func (ts *timespan) CompleteWithDuration(value time.Duration) { | |
type statStore struct { | ||
counters sync.Map | ||
gauges sync.Map | ||
timers sync.Map | ||
timers sync.Map // todo: should be control this count, especially for reservoirs we will be storing a lot of these in memory before flushing them | ||
|
||
mu sync.RWMutex | ||
statGenerators []StatGenerator | ||
|
@@ -393,6 +463,23 @@ func (s *statStore) Flush() { | |
return true | ||
}) | ||
|
||
settings := GetSettings() // todo: move this to some shared memory | ||
// todo: i'm not sure not sure if we need a condition here or there's another way to assume this implicitly but since to my understanding s.timers | ||
// will retain/store data even if it's unused in the case of standardTimer. in any case this should provide some optimization | ||
if settings.isTimerReservoirEnabled() { | ||
s.timers.Range(func(key, v interface{}) bool { | ||
timer := v.(timer) | ||
sampleRate := timer.SampleRate() | ||
// CollectedValue() should be nil unless reservoirTimer | ||
for _, value := range timer.CollectedValue() { | ||
s.sink.FlushAggregatedTimer(key.(string), value, sampleRate) | ||
} | ||
|
||
s.timers.Delete(key) // todo: not sure if this cleanup is necessary | ||
return true | ||
}) | ||
} | ||
|
||
flushableSink, ok := s.sink.(FlushableSink) | ||
if ok { | ||
flushableSink.Flush() | ||
|
@@ -490,14 +577,39 @@ func (s *statStore) NewPerInstanceGauge(name string, tags map[string]string) Gau | |
return s.newGaugeWithTagSet(name, tagspkg.TagSet(nil).MergePerInstanceTags(tags)) | ||
} | ||
|
||
func (s *statStore) newTimer(serializedName string, base time.Duration) *timer { | ||
func (s *statStore) newTimer(serializedName string, base time.Duration) timer { | ||
if v, ok := s.timers.Load(serializedName); ok { | ||
return v.(*timer) | ||
return v.(timer) | ||
} | ||
|
||
var t timer | ||
settings := GetSettings() // todo: move this to some shared memory | ||
if settings.isTimerReservoirEnabled() { | ||
// todo: if s.timers gets to a certain size, we can flush all timers and delete them from the map | ||
// todo: no idea how memory was managed here before did we just expect the map of s.timers to just be replaced after it's filled? | ||
|
||
// todo: have defaults defined in a shared location | ||
t = &reservoirTimer{ | ||
name: serializedName, | ||
base: base, | ||
capacity: 100, | ||
values: make([]float64, 0, 100), | ||
fill: 0, | ||
count: 0, | ||
} | ||
} else { | ||
t = &standardTimer{ | ||
name: serializedName, | ||
sink: s.sink, | ||
base: base, | ||
} | ||
} | ||
t := &timer{name: serializedName, sink: s.sink, base: base} | ||
|
||
// todo: why would the timer ever be replaced, will this hurt reservoirs or benefit them? or is it just redundant since we load above? | ||
if v, loaded := s.timers.LoadOrStore(serializedName, t); loaded { | ||
return v.(*timer) | ||
return v.(timer) | ||
} | ||
|
||
return t | ||
} | ||
|
||
|
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.
For performance reasons it would be great to avoid
fmt.Sprintf
here if possible.