add reservoir timers by maciuszek · Pull Request #171 · lyft/gostats

maciuszek · 2025-01-08T18:27:16Z

Add a reservoir implementation for timers. This relies on a new timer type, and using this type should allow for a more controlled stat load from timers.

maciuszek · 2025-01-08T18:28:22Z

-	counter = sink.record
-	if !strings.Contains(counter, expected) {
-		t.Error("wanted counter value of test.___f=i:1|c, got", counter)
+	expected = "test.__host=i:1|c"


Note: this test change is out of scope of this work but it was previously volatile with the order of reserved_tag vs test.__host not being deterministic

…imer-reservoir

re-design with the notion of sample rate appended to the metric

improve naming

maciuszek · 2025-01-11T05:06:58Z

 	FlushCounter(name string, value uint64)
 	FlushGauge(name string, value uint64)
 	FlushTimer(name string, value float64)
+	FlushAggregatedTimer(name string, value, sampleRate float64)


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.

charlievieth · 2025-01-14T03:31:10Z

+	if t.count < t.capacity {
+		t.values = append(t.values, value)
+	} else {
+		t.values = append(t.values[1:], value) // discard the oldest value when the reservoir is full, this can probably be smarter


Treating this more as a ring buffer when at capacity would be better since it would provide a more accurate representation of the most recent timings (whereas here we only update the first timing):

diff --git a/stats.go b/stats.go index 70a582b..99a0a0a 100644 --- a/stats.go +++ b/stats.go @@ -346,6 +346,7 @@ type reservoirTimer struct { capacity int values []float64 count int + off int mu sync.Mutex } @@ -364,7 +365,8 @@ func (t *reservoirTimer) AddValue(value float64) { if t.count < t.capacity { t.values = append(t.values, value) } else { - t.values = append(t.values[1:], value) // discard the oldest value when the reservoir is full, this can probably be smarter + t.values[t.off%len(t.values)] = value + t.off++ } t.count++

Also, making capacity a power of two would allow for us to use a mask instead of an expensive modulo operation. This can be cheaply calculated using the bits package:

func nextPowerOfTwo(capacity int) uint { return 1 << bits.Len(uint(capacity)) }

charlievieth · 2025-01-14T03:31:55Z

+	capacity int
+	values   []float64
+	count    int
+	mu       sync.Mutex


super nit, make the Mutex the first field (it's advantageous performance wise to make the most accessed field first)

charlievieth · 2025-01-14T03:36:16Z

 }

 type statStore struct {
+	// todo: no idea how memory is managed here, when are the map entries ever deleted?


The assumption is the that stat names are stable over the lifetime of a process, but this is a known issue for processes that incorrectly use this library and cause a cardinality explosion of stat names. See:

[RFC] replace sync.Map with lru.Cache #158 (original issue)

stats: delete unused Counters and Timers when flushing #160 (my proposed fix)

charlievieth · 2025-01-14T03:39:30Z

+}
+
+func (s *netSink) FlushAggregatedTimer(name string, value, sampleRate float64) {
+	suffix := fmt.Sprintf("|ms|@%.1f\n", sampleRate)


For performance reasons it would be great to avoid fmt.Sprintf here if possible.

charlievieth · 2025-01-14T03:51:36Z

+			for _, value := range timer.CollectedValue() {
+				s.sink.FlushAggregatedTimer(key.(string), value, sampleRate)
+			}
+			s.timers.Delete(key) // delete it from the map so it's not flushed again


Deletion is expensive and since we assume the names of stats are stable will just lead to it being re-added to the map at a later time. For counters we only flush non-zero values and I think we could do something similar with the reservoirTimer type (say not flush if it has zero values and trim the slice to zero elements on flush).

charlievieth · 2025-01-14T03:56:23Z

-	return &statStore{sink: sink}
+	return &statStore{
+		sink: sink,
+		conf: GetSettings(), // todo: right now the environment is being loaded in multiple places and can be made more efficient


The sync.OnceValue function can help eliminate these multiple loads.

optimize reservoirTimer by allocating an reusing memory fix bug with sampleRate calculation optimize FlushAggregatedTimer a bit expand and fix test coverage

maciuszek · 2025-01-14T23:47:50Z

@charlievieth just did a first pass of your comments. Sincerely thank you both for the clarification regarding how data/keys in the maps are reused, and the ring buffer recommendation.

I now have a lot of new todos I need to dive into, but I just pushed a pretty big change (ref: ea5ae6a), if/when you have a chance I would be eager to know if it aligns with your thoughts.

Fwiw I appreciate any and all nits. Very much a go noob.

Edit: had a logical bug which I addressed in e81d603

charlievieth · 2025-01-15T11:52:32Z

@charlievieth just did a first pass of your comments. Sincerely thank you both for the clarification regarding how data/keys in the maps are reused, and the ring buffer recommendation.

np! I'm vacation the next few days but will take a look at your changes when I can.

charlievieth · 2025-01-15T16:17:09Z

+	t.values[t.overflow&t.ringMask] = value
+	t.overflow++
+
+	// todo: can i optimize this with xor?


I don't think you need to reset the overflow since you're masking (taking only the lower bits is equivalent to a modulo operation). Also using an unsigned int here would be a bit safer / clearer (makes the value always positive and unsigned overflow is well defined).

Ah this implementation here is wrong

additionally remove variablility of the reservoir size as we need to ensure this number is a power of two

improve comments

…ewDefaultStore and resevoir timers are enabled implicitly optimize store construction

fix data race in reservoirTimer

sokada1221 · 2025-01-16T20:47:53Z

+}
+
+func (t *reservoirTimer) Reset() {
+	atomic.StoreUint64(&t.count, 0)


Don't we need a lock here?

It's already atomic so the access should be protected iiuc. i did add a test for data races 9762152 , but actually this isn't covered, let me cover this reset too.

I had a data race that displayed how atomic works. Inside a mutex i didn't use atomic access, however there was access (atomic) to the same data from a different method outside of the mutex, the run down is that the access in the mutex also needed to be atomic and that itself protects from datarace

I think this test should cover it 2641924 , this will aggressively flush (and Reset) with interleaving writes (changing the counter), to the same reservoirTimer allocation in the (statStore instance).timers

Nevermind I think you're right! Thank you @sokada1221

@sokada1221 4e9611d

prototype a simple reservoir for timers

72bc838

maciuszek force-pushed the mattkuzminski/add-timer-reservoir branch from 98706b1 to 72bc838 Compare January 8, 2025 18:27

maciuszek commented Jan 8, 2025

View reviewed changes

maciuszek marked this pull request as draft January 8, 2025 18:28

clarify comment

f67aca4

maciuszek changed the title ~~prototype a reservoir for timers~~ add a reservoir for timers Jan 8, 2025

bshramin and others added 8 commits January 8, 2025 14:57

Move s.timerCount inside timer.Range

5cc34d9

fix test messages

f7fc55e

fix spelling

4a0662a

Merge remote-tracking branch 'origin/master' into mattkuzminski/add-t…

f35471d

…imer-reservoir

re-design with timer reservoirs correctly independent per mertic

57ef42b

re-design with the notion of sample rate appended to the metric

add some more todos

4610f55

clean up redundant code

cc908b5

improve naming

some more clean up

b1a2def

maciuszek commented Jan 11, 2025

View reviewed changes

address todos

8eb942d

maciuszek marked this pull request as ready for review January 13, 2025 19:24

maciuszek force-pushed the mattkuzminski/add-timer-reservoir branch 2 times, most recently from 7748bb6 to 4a82e5c Compare January 13, 2025 19:39

fix comment

5dd8757

maciuszek force-pushed the mattkuzminski/add-timer-reservoir branch from 9a8b85d to 5dd8757 Compare January 13, 2025 19:44

ensure memory and flush management for timers

0d3fb45

maciuszek changed the title ~~add a reservoir for timers~~ add reservoir timers Jan 13, 2025

charlievieth reviewed Jan 14, 2025

View reviewed changes

optimize reservoirTimer by utilizing a ring buffer

ea5ae6a

optimize reservoirTimer by allocating an reusing memory fix bug with sampleRate calculation optimize FlushAggregatedTimer a bit expand and fix test coverage

maciuszek added 2 commits January 14, 2025 19:50

correct how we flush reusable timer entries

e81d603

add test for reused timer map after flushing

74a26a1

charlievieth reviewed Jan 15, 2025

View reviewed changes

maciuszek added 8 commits January 15, 2025 16:02

correct the ring buffer implementation to utilize bitwise benefits

6d2687c

additionally remove variablility of the reservoir size as we need to ensure this number is a power of two

improve reservoirTimer property access

d067744

make reservoir tests more dynamic

7e5a451

improve comments

improve comments

a54db1a

optimize reservoir timer flush

18c0e57

block never flush edge cases when stores are constructed outside of N…

bf0ef63

…ewDefaultStore and resevoir timers are enabled implicitly optimize store construction

fix typo in comment

8dad5ed

add test for reservoir automatic flushing

9762152

fix data race in reservoirTimer

sokada1221 reviewed Jan 16, 2025

View reviewed changes

maciuszek added 11 commits January 16, 2025 16:49

add test for concurrent reservoir writes and flushing

2641924

fix typo in comment

858a3fd

protect writes while flushing

4e9611d

dont export controls that can result in a deadlock or datarace

70cc61c

add critical optimization todo

b352a4f

simplify reservoir processing

ef8cf0b

unexport RingSize and document immutability

8cceead

print to stdout for testing

4cae9ac

improve test logging

7fe2893

temporarily make logging a bit better at the sacrifice of performance

c2738fa

remove test logging

236e2cc

maciuszek force-pushed the mattkuzminski/add-timer-reservoir branch from 13cbeb5 to 236e2cc Compare January 20, 2025 20:07

Uh oh!

Conversation

maciuszek commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charlievieth Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciuszek commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charlievieth commented Jan 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciuszek Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciuszek Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maciuszek commented Jan 8, 2025 •

edited

Loading

charlievieth Jan 14, 2025 •

edited

Loading

maciuszek commented Jan 14, 2025 •

edited

Loading

maciuszek Jan 16, 2025 •

edited

Loading

maciuszek Jan 16, 2025 •

edited

Loading