Negative values cause panics
mixalturek opened this issue · 8 comments
Hi there, negative values passed to the API calls cause the code to panic. We are using latest dipstick = "~0.6", but I found similar code also in master.
counter.count(-1);
gauge.value(-1);
timer.interval_us(-1);The library expects only values from unsigned range to be passed in and internally operates with u64 type. This assumption is surely wrong for counter and gauge where the negative values are fully valid. I'm unsure about timer now, but runtime panic in production wouldn't be much pleasant ;-)
impl Counter {
/// Record a value count.
pub fn count<V: ToPrimitive>(&self, count: V) {
self.inner.write(count.to_u64().unwrap(), labels![]) // <<<<< e.g. here
}
}Hello, sorry for the delayed response.
The API should be changed to indicate that only positive values can be passed to counters and timers, as the are strictly increasing. The doc may not be clear on this... "negative time" doesn't make sense, in the same way that "minus one apple" is not a valid quantity of fruit.
For gauges though it is true that negative numbers could be useful, I'll look into it.
Hi, let me disagree with the counters, negative values makes sense there too. Imagine for example size of a queue in producer-consumer schema. If you produce an item, it will be +1, if you consume it, it will be -1. StatsD and Dropwizard metrics also support negative counter increments. Please don't cut off useful features that would be nearly for free for no real reason.
You are right. I envisioned the current Counters for "absolute number of things" being processed, such as bytes copied, etc. I know other systems allow negative numbers but I thought Gauges would fill that need entirely. But since "gauging" absolute values is not always efficient (e.g. size of a linked list) or possible, there is a requirement gap for something like "relative counters".
The statistics to track for each type are different though, as relative counters are closer to gauges (they're "stateful gauges" if you will). Relative counters would track the min and max of the sum of values, as opposed to the existing absolute counters which track the min and max collected values. They're for different use cases. I need also to consider the rates stats, and the stats for a new Relative Marker class.
Added #32 to track this.
Side question, how attached are you to 0.6? The version in master is the unreleased 0.7 which is much improved from 0.6 but has quite a different API. I could backport any counter changes to 0.6 or just publish 0.7 which I've been sitting on for a few months...
Not at all, we use 0.6 because it's the latest, we can simply update. It would be a single file change, the rest of the application use these typedefs that mimics (Java/Scala) API of https://github.com/avast/metrics we are familiar with.
pub(crate) type Monitor = AppMetrics<Arc<Scoreboard>>;
pub(crate) type Counter = AppCounter<Arc<Scoreboard>>;
pub(crate) type Gauge = AppGauge<Arc<Scoreboard>>;
// ...
And we are not working around this bug, I have just noticed the panic when I passed -1 to gauge during development before I had the real value.
Ok, so I've finally released Dipstick 0.7. I feel it is a much better designed than 0.6 and I hope you'll agree. The API has changed a lot, if not completely, but the goals and patterns remain mostly the same just more options and simpler names. Performance should similar to what it was.
In it I've introduced the Level class in , which is a "relative" quantity counter which accepts negative and positive values. Gauge also accept negative values now. The original Counter is still there and now only takes u64 as input. They differ in the aggregated stats that are tracked (if aggregation is used) and possibly in the type of external instrument reported (statsd). Internally, all values are now i64 instead of u64.
Great, thanks. I will try to migrate to the new version sometime next week.