You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Stanislav Kozlovski <st...@confluent.io> on 2018/10/22 13:53:40 UTC
[DISCUSS] KIP-386: Make Min metrics' default value consistent with
Max metrics
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
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by 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
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by Stanislav Kozlovski <st...@confluent.io>.
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
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by Kevin Lu <lu...@berkeley.edu>.
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 <st...@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
>
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by Stanislav Kozlovski <st...@confluent.io>.
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
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by John Roesler <jo...@confluent.io>.
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 <st...@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
>
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by Stanislav Kozlovski <st...@confluent.io>.
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
Re: [DISCUSS] KIP-386: Make Min metrics' default value consistent
with Max metrics
Posted by Suman B N <su...@gmail.com>.
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 <st...@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*