Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Optionally omit stats_counts metrics #148

Merged
merged 2 commits into from Aug 2, 2013

Conversation

Projects
None yet
8 participants
Contributor

ktheory commented Aug 23, 2012

Adds the 'flush_counts' config. Defaults to true, so existing configs
aren't affected.

In my (and others?) use case, the stats_counts metrics are unused/redundant, and double the storage requirements for whisper files.

@ktheory ktheory Optionally omit stats_counts metrics
Adds the 'flush_counts' config. Defaults to true, so existing configs
aren't affected.
393eb8f

This pull request passes (merged 393eb8f into 4bd7597).

Contributor

kastner commented Aug 23, 2012

+1 I like making this optional.
@mrtazz is stats_counts useful anymore now that gauges are in? I'm reluctant to remove it altogether (as people might be relying on it) but for the next major release maybe?

Owner

mrtazz commented Aug 23, 2012

In the namespace pull request there is a proposal to change the counter metrics to end in rate or count. This would remove the stats_counts namespace and move it under the proper counter metric instead. I think it is still useful as it fulfills a different need than gauges. And it can also be valuable from a statistics point of view, since you record real counts and not averaged ones. But I like the idea of making this optional, since most of the time, the averaged rate should be precise enough.

I second this proposal, this is effectively useless to us, and as @ktheory said, it doubles our storage requirements.

Contributor

Dieterbe commented Dec 9, 2012

@mrtazz

And it can also be valuable from a statistics point of view, since you record real counts and not averaged ones.

I don't understand this. We're talking about this, right?:

    var value = counters[key];          <----- "count"
    var valuePerSecond = value / (flushInterval / 1000); // calculate "per second" rate

the 'count' value only becomes meaningful when you know how long the flushInterval is (because it gets reset at every flush -- which btw means it's also a rate), and when you know the value of flushInterval then you can calculate the value back from the rate. so basically they hold the same information, other than a factor which may or not be known when interpreting the data (composing/rendering a graph). valuePerSecond (rate) is more useful because it abstracts away that potentially unknown factor. the only argument left against rate is loss of information due to float rounding errors, but that effect should be negligible. the per-second rate is not any more "averaged" than the "count" already is. they are both an average over the flushInterval period.

Contributor

Dieterbe commented Aug 2, 2013

let's merge this. see my comments above.

Contributor

ktheory commented Aug 2, 2013

🤘 Yes, please. 🤘

Contributor

draco2003 commented Aug 2, 2013

@ktheory if you can update against current HEAD we'll get this pulled in.

@ktheory ktheory Merge branch 'master' into optional_counts
Fixed merge conflict for new & legacy namespaces

Conflicts:
	backends/graphite.js
ca3ab5c
Contributor

ktheory commented Aug 2, 2013

@draco2003 Updated!

@draco2003 draco2003 added a commit that referenced this pull request Aug 2, 2013

@draco2003 draco2003 Merge pull request #148 from ktheory/optional_counts
Optionally omit stats_counts metrics
ce6ec09

@draco2003 draco2003 merged commit ce6ec09 into etsy:master Aug 2, 2013

1 check passed

default The Travis CI build passed
Details
Contributor

draco2003 commented Aug 2, 2013

Thanks for the patch!

Contributor

Dieterbe commented Aug 2, 2013

today is a beautiful day. thanks Dan.

Snakecase 😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment