Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent with Max metrics

[Stanislav Kozlovski <st...@confluent.io>]

Hey everybody,

Just wanted to say that I plan on starting a voting thread tomorrow if
there aren't any further comments.

Thanks for the discussion so far!

On Wed, Oct 24, 2018 at 11:27 AM Stanislav Kozlovski <st...@confluent.io>
wrote:

> 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...@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 <
>> stanislav@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 <jo...@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 <
>> > > stanislav@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 <su...@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 <
>> > > > > stanislav@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
>


-- 
Best,
Stanislav