Hi Kevin, Thanks for providing context.
I've edited the KIP to use `NaN` for all three metric types. Here is the updated link: KIP-386: Standardize on Min/Avg/Max metrics' default value <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652345> On Tue, Oct 23, 2018 at 7:30 PM Kevin Lu <lu.ke...@berkeley.edu> wrote: > Hi Stanislav, > > Thanks for the KIP! > > Standardizing this would be extremely helpful. We have been publishing > client-side metrics to a time-series database using metrics reporter since > 0.8, and we had to do explicit checks like "!Double.isNaN(metricValue)", > "metricValue != Double.NEGATIVE_INFINITY", and such to skip these values. > > I like John's suggestion for using NaN for the same reason that -INF > and +INF are still technically valid values as they are just the extreme > bounds. > > Regards, > Kevin > > On Tue, Oct 23, 2018 at 3:46 AM Stanislav Kozlovski < > stanis...@confluent.io> > wrote: > > > Hey John, > > > > I think NaN would be the better option semantically. If we were to use > > that, maybe it makes sense to change `Avg()`'s default value from 0.0 to > > NaN as well. > > > > I am a bit more concerned with backwards compatibility if we were to use > > NaN since we would change three types of metrics. There were only three > > metrics that use `Min` but `Avg` and `Max` are used everywhere. > > I guess it boils down to how likely it is that such a change can break > > users' tools and if we're okay with it. My assumption is that it won't - > > most tools should be mature enough to handle these values. > > > > Thanks for the suggestion! I will wait out a bit more for other people to > > share their thoughts and update the KIP with `NaN` > > > > > > On Tue, Oct 23, 2018 at 6:12 AM John Roesler <j...@confluent.io> wrote: > > > > > Hi Stanislav, > > > Thanks for this KIP. I coincidentally just noticed these strange > initial > > > values while doing some performance measurements. > > > > > > Since the metric type is a double, could we consider NaN instead? It > > seems > > > like +Inf is somewhat arbitrary for Max, so it might as well be an > > > arbitrary value that actually means "this is not a number". > > > > > > Consider that +- Infinity is technically "in range" for max and min, > > while > > > NaN is not. NaN is "in range" for an average or a rate, but in those > > cases > > > it would mean that the result is over 0 samples or 0 time, > respectively. > > I > > > think this only happens when nothing has been recorded, so it would > still > > > be sound for the situation you're attempting to address. > > > > > > Just to throw it out there, `null` is technically also (maybe moreso) a > > > sound choice, but I'd be concerned about causing a bunch of null > > > dereference errors. > > > > > > Thanks again, > > > -John > > > > > > On Mon, Oct 22, 2018 at 2:27 PM Stanislav Kozlovski < > > > stanis...@confluent.io> > > > wrote: > > > > > > > Hi Suman, > > > > > > > > Thanks for taking a look at this. Yeah, it's very minor but it > changes > > > the > > > > public API (even if this is a very slight change) and as far as I > know > > > this > > > > warrants some discussion > > > > > > > > On Mon, Oct 22, 2018 at 9:28 PM Suman B N <sumannew...@gmail.com> > > wrote: > > > > > > > > > Looks good to me. Maintains uniformity. +1 from me. > > > > > But its more of a bug rather than improvement proposal. Let's see > > what > > > > > contributors got to say. > > > > > > > > > > On Mon, Oct 22, 2018 at 7:23 PM Stanislav Kozlovski < > > > > > stanis...@confluent.io> > > > > > wrote: > > > > > > > > > > > Hey everybody, > > > > > > > > > > > > I've opened up a very short KIP to make the Max and Min metrics' > > > > default > > > > > > values consistent with each other. > > > > > > > > > > > > KIP: > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-386%3A+Make+Min+metrics%27+default+value+consistent+with+Max+metrics > > > > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-7528 > > > > > > > > > > > > This is hopefully a very straightforward change. Please provide > > > > feedback. > > > > > > Thanks > > > > > > > > > > > > -- > > > > > > Best, > > > > > > Stanislav > > > > > > > > > > > > > > > > > > > > > -- > > > > > *Suman* > > > > > *OlaCabs* > > > > > > > > > > > > > > > > > -- > > > > Best, > > > > Stanislav > > > > > > > > > > > > > -- > > Best, > > Stanislav > > > -- Best, Stanislav
