Skip to content
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 sum stat to basicstats aggregator #3797

Merged
merged 2 commits into from
Mar 5, 2018

Conversation

cpacey
Copy link
Contributor

@cpacey cpacey commented Feb 16, 2018

Implements #3467. sum is not included by default to maintain backwards compatibility.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Is not included by default to maintain backwards compatibility.
@@ -146,6 +147,9 @@ func (m *BasicStats) Push(acc telegraf.Accumulator) {
if config.mean {
fields[k+"_mean"] = v.mean
}
if config.sum {
fields[k+"_sum"] = (v.mean * v.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.

This seemed the simplest way to implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

And cheapest for backwards compatibility case

// stdev, and s2. We purposely exclude sum for backwards compatability,
// otherwise user's working systems will suddenly (and surprisingly) start
// capturing sum without their input.
func TestBasicStatsWithDefaultStats(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be overkill, but I like the idea of verifying "default configuration" separately from verifying "aggregations are working".

@JeffAshton
Copy link
Contributor

Hey @danielnelson , I would love to use this feature to help simplify my continuous queries. Do you have time to take a look?

@danielnelson
Copy link
Contributor

One thing I'm worried about is that we might see some weird floating point artifacts that wouldn't appear if we kept a running sum:

cpu value=1 1519251291871635057
cpu value=1 1519251291871803032
cpu value=1 1519251291871857634
cpu value=1 1519251291871884218
cpu value=2 1519251291871907024
cpu value=1 1519251291871931726
cpu value_sum=6.999999999999999 1519251294000000000

@cpacey
Copy link
Contributor Author

cpacey commented Feb 21, 2018

@danielnelson I'll switch to tracking sum as we go. Would you like to keep tracking mean as it's done now, or would you want to switch to calculating the mean based on sum and count? The former simplifies the code change and reduces the likelihood of changing previous behaviour; the latter simplifies the code and reduces calculation cost.

@danielnelson
Copy link
Contributor

Let's switch to calculating the mean based on sum and count.

@cpacey
Copy link
Contributor Author

cpacey commented Feb 22, 2018

@danielnelson I've switched to keeping a running sum (and added a test for the floating-point problem you were concerned about). However, when I tried to remove tracking the mean, the naive approach added another floating-point divide when calculating variance (example here). Do we care about a potential performance impact of this? I haven't measured anything yet - I wanted to get a sense of whether you'd be concerned before investing time.

If you are concerned, options I see are: continue tracking the mean; or rearranging the variance equations to avoid the extra floating-point divide, but at the cost of an extra floating-point multiplication (example here).

@danielnelson
Copy link
Contributor

In the first example, doesn't it equal out since we can remove this divide?

mean = mean + delta/n

@cpacey
Copy link
Contributor Author

cpacey commented Feb 26, 2018

Not without changing the stdev calculation, which I'm not excited to do.

The online variance algorithm produces its estimate by using both the current mean and the previous mean. In the current code, the previous mean is tracked, and the new mean is calculated with a single divide (mean + delta/n). If we stop tracking the mean, then we need to compute both the previous mean and the current mean, which (naively) requires 2 divides: either oldSum / oldCount and newSum / newCount; or oldSum / oldCount and oldMean + delta/n.

(Note that I'd be very happy to be wrong about this.)

@cpacey
Copy link
Contributor Author

cpacey commented Mar 1, 2018

@danielnelson, could we keep tracking mean and merge as-is?

@danielnelson danielnelson added this to the 1.6.0 milestone Mar 5, 2018
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 5, 2018
@danielnelson danielnelson merged commit 0a37386 into influxdata:master Mar 5, 2018
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants