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 >
