vimeo / graph-explorer

A graphite dashboard powered by structured metrics
http://vimeo.github.io/graph-explorer/
Apache License 2.0
1.06k stars 93 forks source link

Always use nonNegativeDerivative for counters #66

Closed thepaul closed 10 years ago

thepaul commented 10 years ago

Instead of using plain derivative() when the metric has no wraparound set. Refs #65.

Dieterbe commented 10 years ago

we can leave line 165 as is, since inside apply_derivative_to_target we can check if target_type == counter (which by definition is the only case that should get nonnegativederive.).

It seems also possible that for certain targets with target_type == counter, there could be a wraparound tag. (and actually can we name it max_value, i find that a clearer name, and it's also more in line with the graphite argument), so we should also account for the case where we have both t_t == counter and max_value is set

thepaul commented 10 years ago

I don't agree- there will likely be cases when we want nonNegativeDerivative applied to targets that don't have t_t == counter. For example, we have a few nondecreasing metrics with t_t == gauge, since usually the value that we care about is the cumulative total. But sometimes it is useful also to view the rate. With the new auto-deriving code, we can just add "unit=Thing/s" and automatically have the derivative applied. But we want it to be nonNegativeDerivative instead of plain derivative, since (a) the source value sometimes resets and (b) it does wrap around once or twice a month, and we don't want the rate graph to show huge negative spikes. So we can set a wraparound tag on that metric and get both cases cleanly addressed.

So, I think we should say "counter gets nonNegativeDerivative by default", but allow other things to have nonNegativeDerivative under the right conditions.

Also I kind of still prefer "wraparound" to "max_value", to avoid confusing users between the two related concepts. A lot of metric systems define "max value" and "min value" quantities which refer to the possible domain of a metric; any data values outside of that (min, max) domain should be thrown out. That concept is related to wraparound, especially in the case of counters, but it's different. And if that sort of thing ever needs to be added to graph-explorer it'd be nice to have the name available. It does make sense for graphite to call that argument to nonNegativeDerivative "max_value" since the scope of that name only covers the function itself. The scope of a metric tag has to cover pretty much everything in GE.