Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

report accurate mean instead of an approximation #744

Merged
merged 1 commit into from
Nov 22, 2017
Merged

Conversation

Dieterbe
Copy link
Contributor

No description provided.

@woodsaj
Copy link
Member

woodsaj commented Nov 22, 2017

I dont think this will work. Your sum will quickly overflow and you won't get meaning values anymore.

@woodsaj
Copy link
Member

woodsaj commented Nov 22, 2017

Actually as you are using microseconds, this will take a while to overflow, but it will still eventually overflow.

@shanson7
Copy link
Collaborator

shanson7 commented Nov 22, 2017

A 64-bit uint has an enormous max value. Even if every microsecond of every day was counted it would take ~3.5 million years to overflow. Even if this were updated from 1000 threads all day every day, it would still not overflow before the hardware rotted :).

@Dieterbe
Copy link
Contributor Author

FTR math.MaxUint64 is 18446744073709551615. should be plenty to track the sum for each report interval (between 10~60s)
i just saw one tweak I can do:

-		sum := atomic.LoadUint64(&l.sum)
-		atomic.StoreUint64(&l.sum, 0)
+		sum := atomic.SwapUint64(&l.sum, 0)

will rebase and merge. thanks guys

@@ -30,8 +33,9 @@ func (l *LatencyHistogram15s32) ReportGraphite(prefix, buf []byte, now time.Time
// for now, only report the summaries :(
r, ok := l.hist.Report(snap)
if ok {
sum := atomic.SwapUint64(&l.sum, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are reseting sum to 0 on every flush, but not count.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r.Count comes from the report which comes from the histogram snapshot. the histogram is reset when snapshot is taken

@Dieterbe Dieterbe deleted the accurate-mean branch September 18, 2018 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants