You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Aarti Gupta <aa...@gmail.com> on 2016/12/31 04:31:40 UTC
[DISCUSS] KIP-105: Addition of Record Level for Sensors
Hi all,
I would like to start the discussion on KIP-105: Addition of Record Level
for Sensors
*https://cwiki.apache.org/confluence/pages/viewpage.action?
<https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=67636480>*
*pageId=67636483*
Looking forward to your feedback.
Thanks,
Aarti and Eno
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Thanks Joel, I'll address these tomorrow, no problem.
Eno
> On 9 Jan 2017, at 22:04, Joel Koshy <jj...@gmail.com> wrote:
>
> Didn't get a chance to review this earlier, but this LGTM. Minor comments:
> - The current name is fine, but I would have preferred calling it
> RecordingLevel to RecordLevel - first thing that comes to my mind with
> RecordLevel is Kafka records.
> - Under future work: it may be useful to allow specifying different
> recording levels for different hierarchies of sensors (similar to logging
> levels for different loggers)
>
> Thanks,
>
> Joel
>
> On Fri, Jan 6, 2017 at 2:27 AM, Ismael Juma <is...@juma.me.uk> wrote:
>
>> Thanks Eno, sounds good to me. This is indeed what I was suggesting for the
>> initial release. Extending the `JmxReporter` to use the additional
>> information is something that can be done later, as you said.
>>
>> Ismael
>>
>> On Fri, Jan 6, 2017 at 10:24 AM, Eno Thereska <en...@gmail.com>
>> wrote:
>>
>>> To clarify an earlier point Ismael made, MetricReporter implementations
>>> have access to the record level via KafkaMetric.config().recordLevel()
>>> and can also retrieve the active config for the record level via the
>>> configure() method. However, the built-in JmxReporter will not use that
>>> information in the initial release. I've updated the KIP to reflect that.
>>>
>>> Thanks
>>> Eno
>>>
>>>> On 6 Jan 2017, at 09:47, Eno Thereska <en...@gmail.com> wrote:
>>>>
>>>> After considering the changes needed for not registering sensors at
>> all,
>>> coupled with the objective that Jay mentioned to leave open the
>> possibility
>>> of changing the recording level at runtime, we decided that the current
>> way
>>> we have approached the solution is the best way to go. The KIP focuses on
>>> the main problem we have, which is the overhead of computing metrics. It
>>> allows for a subsequent JIRA to have a way to change the levels at
>> runtime
>>> via JMX. It also allows for a subsequent JIRA to provide more tags to the
>>> metrics reporter as Ismael had suggested (e.g., "debug", "info").
>>>>
>>>> I've adjusted the KIP to reflect the above.
>>>>
>>>> Thanks
>>>> Eno
>>>>
>>>>> On 5 Jan 2017, at 22:14, Eno Thereska <eno.thereska@gmail.com
>> <mailto:
>>> eno.thereska@gmail.com>> wrote:
>>>>>
>>>>> Thanks Jay, will fix the motivation. We have a microbenchmark and perf
>>> graph in the PR:
>>>>> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <
>>> https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
>>>>>
>>>>> I'll need to think some more about point 3.
>>>>>
>>>>> Thanks
>>>>> Eno
>>>>>
>>>>>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io <mailto:
>>> jay@confluent.io>> wrote:
>>>>>>
>>>>>> This is great! A couple of quick comments:
>>>>>>
>>>>>> 1. It'd be good to make the motivation a bit more clear. I think
>> the
>>>>>> motivation is "We want to have lots of partition/task/etc metrics
>>> but we're
>>>>>> concerned about the performance impact so we want to disable them
>> by
>>>>>> default." Currently the motivation section is more about the
>> proposed
>>>>>> change and doesn't really make clear why.
>>>>>> 2. Do we have a microbenchmark that shows that the performance of
>> (1)
>>>>>> enabled metrics, (2) disabled metrics, (3) no metrics? This would
>>> help
>>>>>> build the case for needing this extra knob. Obviously if metrics
>> are
>>> cheap
>>>>>> you would always just leave them enabled and not worry about it. I
>>> think
>>>>>> there should be some cost because we are at least taking a lock
>>> during the
>>>>>> recording but I'm not sure how material that is.
>>>>>> 3. One consideration in how this exposed: we always found the
>>> ability to
>>>>>> dynamically change the logging level in JMX for log4j pretty
>> useful.
>>> I
>>>>>> think if we want to leave the door open to add this ability to
>> enable
>>>>>> metrics at runtime it may have some impact on the decision around
>> how
>>>>>> metrics are registered/reported.
>>>>>>
>>>>>> -Jay
>>>>>>
>>>>>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com
>>> <ma...@gmail.com>> wrote:
>>>>>>
>>>>>>> I thought about "not registering at all" and left a comment on the
>> PR
>>> as
>>>>>>> well regarding this idea. My concern is that it may be not very
>>>>>>> straight-forward to implement though via the MetricsReporter
>>> interface, if
>>>>>>> Eno and Aarti has a good approach to tackle it I would love it.
>>>>>>>
>>>>>>>
>>>>>>> Guozhang
>>>>>>>
>>>>>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <
>> eno.thereska@gmail.com
>>> <ma...@gmail.com>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we
>> can
>>>>>>>> proceed.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Eno
>>>>>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk <mailto:
>>> ismael@juma.me.uk>> wrote:
>>>>>>>>>
>>>>>>>>> Thanks Eno. It would be good to update the KIP as well with
>> regards
>>> to
>>>>>>> 1.
>>>>>>>>>
>>>>>>>>> About 2, I am not sure how adding a field could break existing
>>> tools.
>>>>>>>>> Having said that, your suggestion not to register sensors at all
>> if
>>>>>>> their
>>>>>>>>> record level is below what is configured works for me.
>>>>>>>>>
>>>>>>>>> Ismael
>>>>>>>>>
>>>>>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <
>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>>>>>>>
>>>>>>>>>> As for 2. I'd prefer not adding extra info to the metrics
>>> reporters at
>>>>>>>>>> this point, since it might break existing tools out there (e.g.,
>>> if we
>>>>>>>> add
>>>>>>>>>> things like configured level). Existing tools might be expecting
>>> the
>>>>>>>> info
>>>>>>>>>> to be reported in a particular format.
>>>>>>>>>>
>>>>>>>>>> If the current way is confusing, I think the next best
>> alternative
>>> is
>>>>>>> to
>>>>>>>>>> not register sensors at all if their record level is below what
>> is
>>>>>>>>>> configured. That way they don't show up at all. This will require
>>> some
>>>>>>>> more
>>>>>>>>>> code in Sensors.java to check at every step, but I think it's
>> clean
>>>>>>> from
>>>>>>>>>> the user's point of view.
>>>>>>>>>>
>>>>>>>>>> Eno
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk
>> <mailto:
>>> ismael@juma.me.uk>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple
>> of
>>>>>>>>>> comments:
>>>>>>>>>>>
>>>>>>>>>>> 1. As Jun asked in the PR, do we need a broker config as well?
>> The
>>>>>>>> broker
>>>>>>>>>>> uses Kafka Metrics for some metrics, but we probably don't have
>>> any
>>>>>>>> debug
>>>>>>>>>>> sensors at the broker yet. Either way, it would be good to
>>> describe
>>>>>>> the
>>>>>>>>>>> thinking around this.
>>>>>>>>>>>
>>>>>>>>>>> 2. The behaviour with regards to the metrics reporter could be
>>>>>>>>>> surprising.
>>>>>>>>>>> It would be good to elaborate a little more on this aspect. For
>>>>>>>> example,
>>>>>>>>>>> maybe we want to expose the sensor level and the current
>>> configured
>>>>>>>> level
>>>>>>>>>>> to metric reporters. That could then be used to build the
>>> debug/info
>>>>>>>>>>> dashboard that Eno mentioned (while making it clear that some
>>> metrics
>>>>>>>>>>> exist, but are not currently being recorded).
>>>>>>>>>>>
>>>>>>>>>>> Ismael
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
>>>>>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and
>> polled
>>> by
>>>>>>> a
>>>>>>>>>>>> reporter, but the metrics associated with it will not be
>> updated.
>>>>>>>>>>>>
>>>>>>>>>>>> That would allow a user to have, for example, a debug dashboard
>>> and
>>>>>>> an
>>>>>>>>>>>> info dashboard.
>>>>>>>>>>>>
>>>>>>>>>>>> Updated KIP to make this clear.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Eno
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com
>>> <ma...@gmail.com>>
>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for the review, Guozhang,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Addressed 2 out of the three comments,
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name
>>> metrics.record.level)
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function,
>> e.g.
>>>>>>> which
>>>>>>>>>>>> class
>>>>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Added more details on shouldRecord() on the KIP
>>>>>>>>>>>>>
>>>>>>>>>>>>> In Sensor.java the shouldRecord() method is used to compare
>> the
>>>>>>> value
>>>>>>>>>> of
>>>>>>>>>>>>> metric record level in the consumer config and the RecordLevel
>>>>>>>>>> associated
>>>>>>>>>>>>> with the sensor, to determine if metrics should recorded.
>>>>>>>>>>>>>
>>>>>>>>>>>>> From Sensor.java
>>>>>>>>>>>>>
>>>>>>>>>>>>> /**
>>>>>>>>>>>>> * @return true if the sensor's record level indicates that the
>>>>>>> metric
>>>>>>>>>>>>> will be recorded, false otherwise
>>>>>>>>>>>>> */
>>>>>>>>>>>>> public boolean shouldRecord() {
>>>>>>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> From nested enum, Sensor.RecordLevel
>>>>>>>>>>>>>
>>>>>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>>>>>>>> return true;
>>>>>>>>>>>>> } else {
>>>>>>>>>>>>> return configLevel.equals(this);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2. Need to discuss with Eno.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>
>>>>>>>>>>>>> aarti
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <
>>> wangguoz@gmail.com <ma...@gmail.com>>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's
>>> variable
>>>>>>>> name,
>>>>>>>>>>>> but
>>>>>>>>>>>>>> not the real config string, I think it is
>>> `metrics.record.level`
>>>>>>>>>>>> instead?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2. In the Motivation section, as in "This associates each
>>> sensor
>>>>>>>> with
>>>>>>>>>> a
>>>>>>>>>>>>>> record level ... only if the metric config of the client
>>> requires
>>>>>>>>>> these
>>>>>>>>>>>>>> metrics to be recorded."
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you elaborate this a bit more, for example, will the
>>> sensor
>>>>>>>> ever
>>>>>>>>>>>> be
>>>>>>>>>>>>>> registered in the reporter if its level is not allowed in the
>>>>>>> client
>>>>>>>>>>>>>> config? Or it will be registered but never polled? Or it will
>>> be
>>>>>>>>>>>> registered
>>>>>>>>>>>>>> and polled, but recorded?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor
>> will
>>>>>>> have
>>>>>>>>>> the
>>>>>>>>>>>>>> default value based on its type, and it will still be polled
>> by
>>>>>>> the
>>>>>>>>>>>>>> reporter but not recorded. To an end-user's experience it
>> will
>>>>>>> mean
>>>>>>>>>> that
>>>>>>>>>>>>>> for example the monitoring UI that displays all polled
>> metrics
>>>>>>> will
>>>>>>>>>>>> still
>>>>>>>>>>>>>> show the metrics graph, with the value consistently shown as
>>> the
>>>>>>>>>> default
>>>>>>>>>>>>>> value, instead of not showing the graphs at all.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
>>>>>>> which
>>>>>>>>>>>> class
>>>>>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Guozhang
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>>>>>>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Eno
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <
>>> aartiguptaa@gmail.com <ma...@gmail.com>>
>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks Radai,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Aarti
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
>>>>>>> radai.rosenblatt@gmail.com <ma...@gmail.com>
>>>>>>>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>>>>>>>> aartiguptaa@gmail.com <ma...@gmail.com>
>>>>>>>>>>>>>>>> <javascript:;>>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition
>>> of
>>>>>>>>>> Record
>>>>>>>>>>>>>>>> Level
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> for Sensors
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> *https://cwiki.apache.org/conf
>> luence/pages/viewpage.action
>>> ?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> *pageId=67636483*
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> -- Guozhang
>>>>>>>
>>>>>
>>>>
>>>
>>>
>>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Joel Koshy <jj...@gmail.com>.
Didn't get a chance to review this earlier, but this LGTM. Minor comments:
- The current name is fine, but I would have preferred calling it
RecordingLevel to RecordLevel - first thing that comes to my mind with
RecordLevel is Kafka records.
- Under future work: it may be useful to allow specifying different
recording levels for different hierarchies of sensors (similar to logging
levels for different loggers)
Thanks,
Joel
On Fri, Jan 6, 2017 at 2:27 AM, Ismael Juma <is...@juma.me.uk> wrote:
> Thanks Eno, sounds good to me. This is indeed what I was suggesting for the
> initial release. Extending the `JmxReporter` to use the additional
> information is something that can be done later, as you said.
>
> Ismael
>
> On Fri, Jan 6, 2017 at 10:24 AM, Eno Thereska <en...@gmail.com>
> wrote:
>
> > To clarify an earlier point Ismael made, MetricReporter implementations
> > have access to the record level via KafkaMetric.config().recordLevel()
> > and can also retrieve the active config for the record level via the
> > configure() method. However, the built-in JmxReporter will not use that
> > information in the initial release. I've updated the KIP to reflect that.
> >
> > Thanks
> > Eno
> >
> > > On 6 Jan 2017, at 09:47, Eno Thereska <en...@gmail.com> wrote:
> > >
> > > After considering the changes needed for not registering sensors at
> all,
> > coupled with the objective that Jay mentioned to leave open the
> possibility
> > of changing the recording level at runtime, we decided that the current
> way
> > we have approached the solution is the best way to go. The KIP focuses on
> > the main problem we have, which is the overhead of computing metrics. It
> > allows for a subsequent JIRA to have a way to change the levels at
> runtime
> > via JMX. It also allows for a subsequent JIRA to provide more tags to the
> > metrics reporter as Ismael had suggested (e.g., "debug", "info").
> > >
> > > I've adjusted the KIP to reflect the above.
> > >
> > > Thanks
> > > Eno
> > >
> > >> On 5 Jan 2017, at 22:14, Eno Thereska <eno.thereska@gmail.com
> <mailto:
> > eno.thereska@gmail.com>> wrote:
> > >>
> > >> Thanks Jay, will fix the motivation. We have a microbenchmark and perf
> > graph in the PR:
> > >> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <
> > https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
> > >>
> > >> I'll need to think some more about point 3.
> > >>
> > >> Thanks
> > >> Eno
> > >>
> > >>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io <mailto:
> > jay@confluent.io>> wrote:
> > >>>
> > >>> This is great! A couple of quick comments:
> > >>>
> > >>> 1. It'd be good to make the motivation a bit more clear. I think
> the
> > >>> motivation is "We want to have lots of partition/task/etc metrics
> > but we're
> > >>> concerned about the performance impact so we want to disable them
> by
> > >>> default." Currently the motivation section is more about the
> proposed
> > >>> change and doesn't really make clear why.
> > >>> 2. Do we have a microbenchmark that shows that the performance of
> (1)
> > >>> enabled metrics, (2) disabled metrics, (3) no metrics? This would
> > help
> > >>> build the case for needing this extra knob. Obviously if metrics
> are
> > cheap
> > >>> you would always just leave them enabled and not worry about it. I
> > think
> > >>> there should be some cost because we are at least taking a lock
> > during the
> > >>> recording but I'm not sure how material that is.
> > >>> 3. One consideration in how this exposed: we always found the
> > ability to
> > >>> dynamically change the logging level in JMX for log4j pretty
> useful.
> > I
> > >>> think if we want to leave the door open to add this ability to
> enable
> > >>> metrics at runtime it may have some impact on the decision around
> how
> > >>> metrics are registered/reported.
> > >>>
> > >>> -Jay
> > >>>
> > >>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com
> > <ma...@gmail.com>> wrote:
> > >>>
> > >>>> I thought about "not registering at all" and left a comment on the
> PR
> > as
> > >>>> well regarding this idea. My concern is that it may be not very
> > >>>> straight-forward to implement though via the MetricsReporter
> > interface, if
> > >>>> Eno and Aarti has a good approach to tackle it I would love it.
> > >>>>
> > >>>>
> > >>>> Guozhang
> > >>>>
> > >>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <
> eno.thereska@gmail.com
> > <ma...@gmail.com>>
> > >>>> wrote:
> > >>>>
> > >>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we
> can
> > >>>>> proceed.
> > >>>>>
> > >>>>> Thanks
> > >>>>> Eno
> > >>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk <mailto:
> > ismael@juma.me.uk>> wrote:
> > >>>>>>
> > >>>>>> Thanks Eno. It would be good to update the KIP as well with
> regards
> > to
> > >>>> 1.
> > >>>>>>
> > >>>>>> About 2, I am not sure how adding a field could break existing
> > tools.
> > >>>>>> Having said that, your suggestion not to register sensors at all
> if
> > >>>> their
> > >>>>>> record level is below what is configured works for me.
> > >>>>>>
> > >>>>>> Ismael
> > >>>>>>
> > >>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <
> > eno.thereska@gmail.com <ma...@gmail.com>>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> Thanks Ismael. Already addressed 1. in the PR.
> > >>>>>>>
> > >>>>>>> As for 2. I'd prefer not adding extra info to the metrics
> > reporters at
> > >>>>>>> this point, since it might break existing tools out there (e.g.,
> > if we
> > >>>>> add
> > >>>>>>> things like configured level). Existing tools might be expecting
> > the
> > >>>>> info
> > >>>>>>> to be reported in a particular format.
> > >>>>>>>
> > >>>>>>> If the current way is confusing, I think the next best
> alternative
> > is
> > >>>> to
> > >>>>>>> not register sensors at all if their record level is below what
> is
> > >>>>>>> configured. That way they don't show up at all. This will require
> > some
> > >>>>> more
> > >>>>>>> code in Sensors.java to check at every step, but I think it's
> clean
> > >>>> from
> > >>>>>>> the user's point of view.
> > >>>>>>>
> > >>>>>>> Eno
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk
> <mailto:
> > ismael@juma.me.uk>> wrote:
> > >>>>>>>>
> > >>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple
> of
> > >>>>>>> comments:
> > >>>>>>>>
> > >>>>>>>> 1. As Jun asked in the PR, do we need a broker config as well?
> The
> > >>>>> broker
> > >>>>>>>> uses Kafka Metrics for some metrics, but we probably don't have
> > any
> > >>>>> debug
> > >>>>>>>> sensors at the broker yet. Either way, it would be good to
> > describe
> > >>>> the
> > >>>>>>>> thinking around this.
> > >>>>>>>>
> > >>>>>>>> 2. The behaviour with regards to the metrics reporter could be
> > >>>>>>> surprising.
> > >>>>>>>> It would be good to elaborate a little more on this aspect. For
> > >>>>> example,
> > >>>>>>>> maybe we want to expose the sensor level and the current
> > configured
> > >>>>> level
> > >>>>>>>> to metric reporters. That could then be used to build the
> > debug/info
> > >>>>>>>> dashboard that Eno mentioned (while making it clear that some
> > metrics
> > >>>>>>>> exist, but are not currently being recorded).
> > >>>>>>>>
> > >>>>>>>> Ismael
> > >>>>>>>>
> > >>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
> > >>>> eno.thereska@gmail.com <ma...@gmail.com>>
> > >>>>>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and
> polled
> > by
> > >>>> a
> > >>>>>>>>> reporter, but the metrics associated with it will not be
> updated.
> > >>>>>>>>>
> > >>>>>>>>> That would allow a user to have, for example, a debug dashboard
> > and
> > >>>> an
> > >>>>>>>>> info dashboard.
> > >>>>>>>>>
> > >>>>>>>>> Updated KIP to make this clear.
> > >>>>>>>>>
> > >>>>>>>>> Thanks
> > >>>>>>>>> Eno
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com
> > <ma...@gmail.com>>
> > >>>> wrote:
> > >>>>>>>>>>
> > >>>>>>>>>> Thanks for the review, Guozhang,
> > >>>>>>>>>>
> > >>>>>>>>>> Addressed 2 out of the three comments,
> > >>>>>>>>>>
> > >>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
> > >>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name
> > metrics.record.level)
> > >>>>>>>>>>
> > >>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function,
> e.g.
> > >>>> which
> > >>>>>>>>> class
> > >>>>>>>>>> it will be added to? Does it contain any parameters?
> > >>>>>>>>>>
> > >>>>>>>>>> Added more details on shouldRecord() on the KIP
> > >>>>>>>>>>
> > >>>>>>>>>> In Sensor.java the shouldRecord() method is used to compare
> the
> > >>>> value
> > >>>>>>> of
> > >>>>>>>>>> metric record level in the consumer config and the RecordLevel
> > >>>>>>> associated
> > >>>>>>>>>> with the sensor, to determine if metrics should recorded.
> > >>>>>>>>>>
> > >>>>>>>>>> From Sensor.java
> > >>>>>>>>>>
> > >>>>>>>>>> /**
> > >>>>>>>>>> * @return true if the sensor's record level indicates that the
> > >>>> metric
> > >>>>>>>>>> will be recorded, false otherwise
> > >>>>>>>>>> */
> > >>>>>>>>>> public boolean shouldRecord() {
> > >>>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
> > >>>>>>>>>> }
> > >>>>>>>>>>
> > >>>>>>>>>> From nested enum, Sensor.RecordLevel
> > >>>>>>>>>>
> > >>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
> > >>>>>>>>>> if (configLevel.equals(DEBUG)) {
> > >>>>>>>>>> return true;
> > >>>>>>>>>> } else {
> > >>>>>>>>>> return configLevel.equals(this);
> > >>>>>>>>>> }
> > >>>>>>>>>> }
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> 2. Need to discuss with Eno.
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> Thanks!
> > >>>>>>>>>>
> > >>>>>>>>>> aarti
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <
> > wangguoz@gmail.com <ma...@gmail.com>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>>
> > >>>>>>>>>>> LGTM overall. A few comments below:
> > >>>>>>>>>>>
> > >>>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's
> > variable
> > >>>>> name,
> > >>>>>>>>> but
> > >>>>>>>>>>> not the real config string, I think it is
> > `metrics.record.level`
> > >>>>>>>>> instead?
> > >>>>>>>>>>>
> > >>>>>>>>>>> 2. In the Motivation section, as in "This associates each
> > sensor
> > >>>>> with
> > >>>>>>> a
> > >>>>>>>>>>> record level ... only if the metric config of the client
> > requires
> > >>>>>>> these
> > >>>>>>>>>>> metrics to be recorded."
> > >>>>>>>>>>>
> > >>>>>>>>>>> Could you elaborate this a bit more, for example, will the
> > sensor
> > >>>>> ever
> > >>>>>>>>> be
> > >>>>>>>>>>> registered in the reporter if its level is not allowed in the
> > >>>> client
> > >>>>>>>>>>> config? Or it will be registered but never polled? Or it will
> > be
> > >>>>>>>>> registered
> > >>>>>>>>>>> and polled, but recorded?
> > >>>>>>>>>>>
> > >>>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor
> will
> > >>>> have
> > >>>>>>> the
> > >>>>>>>>>>> default value based on its type, and it will still be polled
> by
> > >>>> the
> > >>>>>>>>>>> reporter but not recorded. To an end-user's experience it
> will
> > >>>> mean
> > >>>>>>> that
> > >>>>>>>>>>> for example the monitoring UI that displays all polled
> metrics
> > >>>> will
> > >>>>>>>>> still
> > >>>>>>>>>>> show the metrics graph, with the value consistently shown as
> > the
> > >>>>>>> default
> > >>>>>>>>>>> value, instead of not showing the graphs at all.
> > >>>>>>>>>>>
> > >>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
> > >>>> which
> > >>>>>>>>> class
> > >>>>>>>>>>> it will be added to? Does it contain any parameters?
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> Guozhang
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
> > >>>>> eno.thereska@gmail.com <ma...@gmail.com>>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Eno
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <
> > aartiguptaa@gmail.com <ma...@gmail.com>>
> > >>>>>>> wrote:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>> Thanks Radai,
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Yes that is the correct link, my bad
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP->
> > >>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Aarti
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
> > >>>> radai.rosenblatt@gmail.com <ma...@gmail.com>
> > >>>>>>>>>>>>> <javascript:;>> wrote:
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP->
> > >>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> ?
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
> > >>>>>>> aartiguptaa@gmail.com <ma...@gmail.com>
> > >>>>>>>>>>>>> <javascript:;>>
> > >>>>>>>>>>>>>> wrote:
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Hi all,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition
> > of
> > >>>>>>> Record
> > >>>>>>>>>>>>> Level
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> for Sensors
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> *https://cwiki.apache.org/conf
> luence/pages/viewpage.action
> > ?
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> <
> > >>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> > >>>>>>>>>>>>> action?pageId=67636480
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>> *
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> *pageId=67636483*
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Looking forward to your feedback.
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Thanks,
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>> Aarti and Eno
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> --
> > >>>>>>>>>>> -- Guozhang
> > >>>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>> --
> > >>>> -- Guozhang
> > >>>>
> > >>
> > >
> >
> >
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Ismael Juma <is...@juma.me.uk>.
Thanks Eno, sounds good to me. This is indeed what I was suggesting for the
initial release. Extending the `JmxReporter` to use the additional
information is something that can be done later, as you said.
Ismael
On Fri, Jan 6, 2017 at 10:24 AM, Eno Thereska <en...@gmail.com>
wrote:
> To clarify an earlier point Ismael made, MetricReporter implementations
> have access to the record level via KafkaMetric.config().recordLevel()
> and can also retrieve the active config for the record level via the
> configure() method. However, the built-in JmxReporter will not use that
> information in the initial release. I've updated the KIP to reflect that.
>
> Thanks
> Eno
>
> > On 6 Jan 2017, at 09:47, Eno Thereska <en...@gmail.com> wrote:
> >
> > After considering the changes needed for not registering sensors at all,
> coupled with the objective that Jay mentioned to leave open the possibility
> of changing the recording level at runtime, we decided that the current way
> we have approached the solution is the best way to go. The KIP focuses on
> the main problem we have, which is the overhead of computing metrics. It
> allows for a subsequent JIRA to have a way to change the levels at runtime
> via JMX. It also allows for a subsequent JIRA to provide more tags to the
> metrics reporter as Ismael had suggested (e.g., "debug", "info").
> >
> > I've adjusted the KIP to reflect the above.
> >
> > Thanks
> > Eno
> >
> >> On 5 Jan 2017, at 22:14, Eno Thereska <eno.thereska@gmail.com <mailto:
> eno.thereska@gmail.com>> wrote:
> >>
> >> Thanks Jay, will fix the motivation. We have a microbenchmark and perf
> graph in the PR:
> >> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <
> https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
> >>
> >> I'll need to think some more about point 3.
> >>
> >> Thanks
> >> Eno
> >>
> >>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io <mailto:
> jay@confluent.io>> wrote:
> >>>
> >>> This is great! A couple of quick comments:
> >>>
> >>> 1. It'd be good to make the motivation a bit more clear. I think the
> >>> motivation is "We want to have lots of partition/task/etc metrics
> but we're
> >>> concerned about the performance impact so we want to disable them by
> >>> default." Currently the motivation section is more about the proposed
> >>> change and doesn't really make clear why.
> >>> 2. Do we have a microbenchmark that shows that the performance of (1)
> >>> enabled metrics, (2) disabled metrics, (3) no metrics? This would
> help
> >>> build the case for needing this extra knob. Obviously if metrics are
> cheap
> >>> you would always just leave them enabled and not worry about it. I
> think
> >>> there should be some cost because we are at least taking a lock
> during the
> >>> recording but I'm not sure how material that is.
> >>> 3. One consideration in how this exposed: we always found the
> ability to
> >>> dynamically change the logging level in JMX for log4j pretty useful.
> I
> >>> think if we want to leave the door open to add this ability to enable
> >>> metrics at runtime it may have some impact on the decision around how
> >>> metrics are registered/reported.
> >>>
> >>> -Jay
> >>>
> >>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com
> <ma...@gmail.com>> wrote:
> >>>
> >>>> I thought about "not registering at all" and left a comment on the PR
> as
> >>>> well regarding this idea. My concern is that it may be not very
> >>>> straight-forward to implement though via the MetricsReporter
> interface, if
> >>>> Eno and Aarti has a good approach to tackle it I would love it.
> >>>>
> >>>>
> >>>> Guozhang
> >>>>
> >>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <eno.thereska@gmail.com
> <ma...@gmail.com>>
> >>>> wrote:
> >>>>
> >>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
> >>>>> proceed.
> >>>>>
> >>>>> Thanks
> >>>>> Eno
> >>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk <mailto:
> ismael@juma.me.uk>> wrote:
> >>>>>>
> >>>>>> Thanks Eno. It would be good to update the KIP as well with regards
> to
> >>>> 1.
> >>>>>>
> >>>>>> About 2, I am not sure how adding a field could break existing
> tools.
> >>>>>> Having said that, your suggestion not to register sensors at all if
> >>>> their
> >>>>>> record level is below what is configured works for me.
> >>>>>>
> >>>>>> Ismael
> >>>>>>
> >>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <
> eno.thereska@gmail.com <ma...@gmail.com>>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Thanks Ismael. Already addressed 1. in the PR.
> >>>>>>>
> >>>>>>> As for 2. I'd prefer not adding extra info to the metrics
> reporters at
> >>>>>>> this point, since it might break existing tools out there (e.g.,
> if we
> >>>>> add
> >>>>>>> things like configured level). Existing tools might be expecting
> the
> >>>>> info
> >>>>>>> to be reported in a particular format.
> >>>>>>>
> >>>>>>> If the current way is confusing, I think the next best alternative
> is
> >>>> to
> >>>>>>> not register sensors at all if their record level is below what is
> >>>>>>> configured. That way they don't show up at all. This will require
> some
> >>>>> more
> >>>>>>> code in Sensors.java to check at every step, but I think it's clean
> >>>> from
> >>>>>>> the user's point of view.
> >>>>>>>
> >>>>>>> Eno
> >>>>>>>
> >>>>>>>
> >>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk <mailto:
> ismael@juma.me.uk>> wrote:
> >>>>>>>>
> >>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple of
> >>>>>>> comments:
> >>>>>>>>
> >>>>>>>> 1. As Jun asked in the PR, do we need a broker config as well? The
> >>>>> broker
> >>>>>>>> uses Kafka Metrics for some metrics, but we probably don't have
> any
> >>>>> debug
> >>>>>>>> sensors at the broker yet. Either way, it would be good to
> describe
> >>>> the
> >>>>>>>> thinking around this.
> >>>>>>>>
> >>>>>>>> 2. The behaviour with regards to the metrics reporter could be
> >>>>>>> surprising.
> >>>>>>>> It would be good to elaborate a little more on this aspect. For
> >>>>> example,
> >>>>>>>> maybe we want to expose the sensor level and the current
> configured
> >>>>> level
> >>>>>>>> to metric reporters. That could then be used to build the
> debug/info
> >>>>>>>> dashboard that Eno mentioned (while making it clear that some
> metrics
> >>>>>>>> exist, but are not currently being recorded).
> >>>>>>>>
> >>>>>>>> Ismael
> >>>>>>>>
> >>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
> >>>> eno.thereska@gmail.com <ma...@gmail.com>>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and polled
> by
> >>>> a
> >>>>>>>>> reporter, but the metrics associated with it will not be updated.
> >>>>>>>>>
> >>>>>>>>> That would allow a user to have, for example, a debug dashboard
> and
> >>>> an
> >>>>>>>>> info dashboard.
> >>>>>>>>>
> >>>>>>>>> Updated KIP to make this clear.
> >>>>>>>>>
> >>>>>>>>> Thanks
> >>>>>>>>> Eno
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com
> <ma...@gmail.com>>
> >>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Thanks for the review, Guozhang,
> >>>>>>>>>>
> >>>>>>>>>> Addressed 2 out of the three comments,
> >>>>>>>>>>
> >>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
> >>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name
> metrics.record.level)
> >>>>>>>>>>
> >>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g.
> >>>> which
> >>>>>>>>> class
> >>>>>>>>>> it will be added to? Does it contain any parameters?
> >>>>>>>>>>
> >>>>>>>>>> Added more details on shouldRecord() on the KIP
> >>>>>>>>>>
> >>>>>>>>>> In Sensor.java the shouldRecord() method is used to compare the
> >>>> value
> >>>>>>> of
> >>>>>>>>>> metric record level in the consumer config and the RecordLevel
> >>>>>>> associated
> >>>>>>>>>> with the sensor, to determine if metrics should recorded.
> >>>>>>>>>>
> >>>>>>>>>> From Sensor.java
> >>>>>>>>>>
> >>>>>>>>>> /**
> >>>>>>>>>> * @return true if the sensor's record level indicates that the
> >>>> metric
> >>>>>>>>>> will be recorded, false otherwise
> >>>>>>>>>> */
> >>>>>>>>>> public boolean shouldRecord() {
> >>>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> From nested enum, Sensor.RecordLevel
> >>>>>>>>>>
> >>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
> >>>>>>>>>> if (configLevel.equals(DEBUG)) {
> >>>>>>>>>> return true;
> >>>>>>>>>> } else {
> >>>>>>>>>> return configLevel.equals(this);
> >>>>>>>>>> }
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> 2. Need to discuss with Eno.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Thanks!
> >>>>>>>>>>
> >>>>>>>>>> aarti
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <
> wangguoz@gmail.com <ma...@gmail.com>>
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> LGTM overall. A few comments below:
> >>>>>>>>>>>
> >>>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's
> variable
> >>>>> name,
> >>>>>>>>> but
> >>>>>>>>>>> not the real config string, I think it is
> `metrics.record.level`
> >>>>>>>>> instead?
> >>>>>>>>>>>
> >>>>>>>>>>> 2. In the Motivation section, as in "This associates each
> sensor
> >>>>> with
> >>>>>>> a
> >>>>>>>>>>> record level ... only if the metric config of the client
> requires
> >>>>>>> these
> >>>>>>>>>>> metrics to be recorded."
> >>>>>>>>>>>
> >>>>>>>>>>> Could you elaborate this a bit more, for example, will the
> sensor
> >>>>> ever
> >>>>>>>>> be
> >>>>>>>>>>> registered in the reporter if its level is not allowed in the
> >>>> client
> >>>>>>>>>>> config? Or it will be registered but never polled? Or it will
> be
> >>>>>>>>> registered
> >>>>>>>>>>> and polled, but recorded?
> >>>>>>>>>>>
> >>>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will
> >>>> have
> >>>>>>> the
> >>>>>>>>>>> default value based on its type, and it will still be polled by
> >>>> the
> >>>>>>>>>>> reporter but not recorded. To an end-user's experience it will
> >>>> mean
> >>>>>>> that
> >>>>>>>>>>> for example the monitoring UI that displays all polled metrics
> >>>> will
> >>>>>>>>> still
> >>>>>>>>>>> show the metrics graph, with the value consistently shown as
> the
> >>>>>>> default
> >>>>>>>>>>> value, instead of not showing the graphs at all.
> >>>>>>>>>>>
> >>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
> >>>> which
> >>>>>>>>> class
> >>>>>>>>>>> it will be added to? Does it contain any parameters?
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Guozhang
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
> >>>>> eno.thereska@gmail.com <ma...@gmail.com>>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Eno
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <
> aartiguptaa@gmail.com <ma...@gmail.com>>
> >>>>>>> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks Radai,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Yes that is the correct link, my bad
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP->
> >>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Aarti
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
> >>>> radai.rosenblatt@gmail.com <ma...@gmail.com>
> >>>>>>>>>>>>> <javascript:;>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP->
> >>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
> >>>>>>> aartiguptaa@gmail.com <ma...@gmail.com>
> >>>>>>>>>>>>> <javascript:;>>
> >>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition
> of
> >>>>>>> Record
> >>>>>>>>>>>>> Level
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> for Sensors
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action
> ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> <
> >>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> >>>>>>>>>>>>> action?pageId=67636480
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> *
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> *pageId=67636483*
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Looking forward to your feedback.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Aarti and Eno
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> --
> >>>>>>>>>>> -- Guozhang
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> --
> >>>> -- Guozhang
> >>>>
> >>
> >
>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
To clarify an earlier point Ismael made, MetricReporter implementations have access to the record level via KafkaMetric.config().recordLevel() and can also retrieve the active config for the record level via the configure() method. However, the built-in JmxReporter will not use that information in the initial release. I've updated the KIP to reflect that.
Thanks
Eno
> On 6 Jan 2017, at 09:47, Eno Thereska <en...@gmail.com> wrote:
>
> After considering the changes needed for not registering sensors at all, coupled with the objective that Jay mentioned to leave open the possibility of changing the recording level at runtime, we decided that the current way we have approached the solution is the best way to go. The KIP focuses on the main problem we have, which is the overhead of computing metrics. It allows for a subsequent JIRA to have a way to change the levels at runtime via JMX. It also allows for a subsequent JIRA to provide more tags to the metrics reporter as Ismael had suggested (e.g., "debug", "info").
>
> I've adjusted the KIP to reflect the above.
>
> Thanks
> Eno
>
>> On 5 Jan 2017, at 22:14, Eno Thereska <eno.thereska@gmail.com <ma...@gmail.com>> wrote:
>>
>> Thanks Jay, will fix the motivation. We have a microbenchmark and perf graph in the PR:
>> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
>>
>> I'll need to think some more about point 3.
>>
>> Thanks
>> Eno
>>
>>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io <ma...@confluent.io>> wrote:
>>>
>>> This is great! A couple of quick comments:
>>>
>>> 1. It'd be good to make the motivation a bit more clear. I think the
>>> motivation is "We want to have lots of partition/task/etc metrics but we're
>>> concerned about the performance impact so we want to disable them by
>>> default." Currently the motivation section is more about the proposed
>>> change and doesn't really make clear why.
>>> 2. Do we have a microbenchmark that shows that the performance of (1)
>>> enabled metrics, (2) disabled metrics, (3) no metrics? This would help
>>> build the case for needing this extra knob. Obviously if metrics are cheap
>>> you would always just leave them enabled and not worry about it. I think
>>> there should be some cost because we are at least taking a lock during the
>>> recording but I'm not sure how material that is.
>>> 3. One consideration in how this exposed: we always found the ability to
>>> dynamically change the logging level in JMX for log4j pretty useful. I
>>> think if we want to leave the door open to add this ability to enable
>>> metrics at runtime it may have some impact on the decision around how
>>> metrics are registered/reported.
>>>
>>> -Jay
>>>
>>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com <ma...@gmail.com>> wrote:
>>>
>>>> I thought about "not registering at all" and left a comment on the PR as
>>>> well regarding this idea. My concern is that it may be not very
>>>> straight-forward to implement though via the MetricsReporter interface, if
>>>> Eno and Aarti has a good approach to tackle it I would love it.
>>>>
>>>>
>>>> Guozhang
>>>>
>>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <eno.thereska@gmail.com <ma...@gmail.com>>
>>>> wrote:
>>>>
>>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
>>>>> proceed.
>>>>>
>>>>> Thanks
>>>>> Eno
>>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk <ma...@juma.me.uk>> wrote:
>>>>>>
>>>>>> Thanks Eno. It would be good to update the KIP as well with regards to
>>>> 1.
>>>>>>
>>>>>> About 2, I am not sure how adding a field could break existing tools.
>>>>>> Having said that, your suggestion not to register sensors at all if
>>>> their
>>>>>> record level is below what is configured works for me.
>>>>>>
>>>>>> Ismael
>>>>>>
>>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>> wrote:
>>>>>>
>>>>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>>>>
>>>>>>> As for 2. I'd prefer not adding extra info to the metrics reporters at
>>>>>>> this point, since it might break existing tools out there (e.g., if we
>>>>> add
>>>>>>> things like configured level). Existing tools might be expecting the
>>>>> info
>>>>>>> to be reported in a particular format.
>>>>>>>
>>>>>>> If the current way is confusing, I think the next best alternative is
>>>> to
>>>>>>> not register sensors at all if their record level is below what is
>>>>>>> configured. That way they don't show up at all. This will require some
>>>>> more
>>>>>>> code in Sensors.java to check at every step, but I think it's clean
>>>> from
>>>>>>> the user's point of view.
>>>>>>>
>>>>>>> Eno
>>>>>>>
>>>>>>>
>>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk <ma...@juma.me.uk>> wrote:
>>>>>>>>
>>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple of
>>>>>>> comments:
>>>>>>>>
>>>>>>>> 1. As Jun asked in the PR, do we need a broker config as well? The
>>>>> broker
>>>>>>>> uses Kafka Metrics for some metrics, but we probably don't have any
>>>>> debug
>>>>>>>> sensors at the broker yet. Either way, it would be good to describe
>>>> the
>>>>>>>> thinking around this.
>>>>>>>>
>>>>>>>> 2. The behaviour with regards to the metrics reporter could be
>>>>>>> surprising.
>>>>>>>> It would be good to elaborate a little more on this aspect. For
>>>>> example,
>>>>>>>> maybe we want to expose the sensor level and the current configured
>>>>> level
>>>>>>>> to metric reporters. That could then be used to build the debug/info
>>>>>>>> dashboard that Eno mentioned (while making it clear that some metrics
>>>>>>>> exist, but are not currently being recorded).
>>>>>>>>
>>>>>>>> Ismael
>>>>>>>>
>>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
>>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and polled by
>>>> a
>>>>>>>>> reporter, but the metrics associated with it will not be updated.
>>>>>>>>>
>>>>>>>>> That would allow a user to have, for example, a debug dashboard and
>>>> an
>>>>>>>>> info dashboard.
>>>>>>>>>
>>>>>>>>> Updated KIP to make this clear.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Eno
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com <ma...@gmail.com>>
>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Thanks for the review, Guozhang,
>>>>>>>>>>
>>>>>>>>>> Addressed 2 out of the three comments,
>>>>>>>>>>
>>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>>>>>>>>
>>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g.
>>>> which
>>>>>>>>> class
>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>
>>>>>>>>>> Added more details on shouldRecord() on the KIP
>>>>>>>>>>
>>>>>>>>>> In Sensor.java the shouldRecord() method is used to compare the
>>>> value
>>>>>>> of
>>>>>>>>>> metric record level in the consumer config and the RecordLevel
>>>>>>> associated
>>>>>>>>>> with the sensor, to determine if metrics should recorded.
>>>>>>>>>>
>>>>>>>>>> From Sensor.java
>>>>>>>>>>
>>>>>>>>>> /**
>>>>>>>>>> * @return true if the sensor's record level indicates that the
>>>> metric
>>>>>>>>>> will be recorded, false otherwise
>>>>>>>>>> */
>>>>>>>>>> public boolean shouldRecord() {
>>>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> From nested enum, Sensor.RecordLevel
>>>>>>>>>>
>>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>>>>> return true;
>>>>>>>>>> } else {
>>>>>>>>>> return configLevel.equals(this);
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 2. Need to discuss with Eno.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks!
>>>>>>>>>>
>>>>>>>>>> aarti
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wangguoz@gmail.com <ma...@gmail.com>>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>>>>
>>>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
>>>>> name,
>>>>>>>>> but
>>>>>>>>>>> not the real config string, I think it is `metrics.record.level`
>>>>>>>>> instead?
>>>>>>>>>>>
>>>>>>>>>>> 2. In the Motivation section, as in "This associates each sensor
>>>>> with
>>>>>>> a
>>>>>>>>>>> record level ... only if the metric config of the client requires
>>>>>>> these
>>>>>>>>>>> metrics to be recorded."
>>>>>>>>>>>
>>>>>>>>>>> Could you elaborate this a bit more, for example, will the sensor
>>>>> ever
>>>>>>>>> be
>>>>>>>>>>> registered in the reporter if its level is not allowed in the
>>>> client
>>>>>>>>>>> config? Or it will be registered but never polled? Or it will be
>>>>>>>>> registered
>>>>>>>>>>> and polled, but recorded?
>>>>>>>>>>>
>>>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will
>>>> have
>>>>>>> the
>>>>>>>>>>> default value based on its type, and it will still be polled by
>>>> the
>>>>>>>>>>> reporter but not recorded. To an end-user's experience it will
>>>> mean
>>>>>>> that
>>>>>>>>>>> for example the monitoring UI that displays all polled metrics
>>>> will
>>>>>>>>> still
>>>>>>>>>>> show the metrics graph, with the value consistently shown as the
>>>>>>> default
>>>>>>>>>>> value, instead of not showing the graphs at all.
>>>>>>>>>>>
>>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
>>>> which
>>>>>>>>> class
>>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Guozhang
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>>>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>>>
>>>>>>>>>>>> Eno
>>>>>>>>>>>>
>>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aartiguptaa@gmail.com <ma...@gmail.com>>
>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks Radai,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Aarti
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
>>>> radai.rosenblatt@gmail.com <ma...@gmail.com>
>>>>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>>>>> aartiguptaa@gmail.com <ma...@gmail.com>
>>>>>>>>>>>>> <javascript:;>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
>>>>>>> Record
>>>>>>>>>>>>> Level
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> for Sensors
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *pageId=67636483*
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> -- Guozhang
>>>>
>>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
After considering the changes needed for not registering sensors at all, coupled with the objective that Jay mentioned to leave open the possibility of changing the recording level at runtime, we decided that the current way we have approached the solution is the best way to go. The KIP focuses on the main problem we have, which is the overhead of computing metrics. It allows for a subsequent JIRA to have a way to change the levels at runtime via JMX. It also allows for a subsequent JIRA to provide more tags to the metrics reporter as Ismael had suggested (e.g., "debug", "info").
I've adjusted the KIP to reflect the above.
Thanks
Eno
> On 5 Jan 2017, at 22:14, Eno Thereska <en...@gmail.com> wrote:
>
> Thanks Jay, will fix the motivation. We have a microbenchmark and perf graph in the PR:
> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
>
> I'll need to think some more about point 3.
>
> Thanks
> Eno
>
>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io <ma...@confluent.io>> wrote:
>>
>> This is great! A couple of quick comments:
>>
>> 1. It'd be good to make the motivation a bit more clear. I think the
>> motivation is "We want to have lots of partition/task/etc metrics but we're
>> concerned about the performance impact so we want to disable them by
>> default." Currently the motivation section is more about the proposed
>> change and doesn't really make clear why.
>> 2. Do we have a microbenchmark that shows that the performance of (1)
>> enabled metrics, (2) disabled metrics, (3) no metrics? This would help
>> build the case for needing this extra knob. Obviously if metrics are cheap
>> you would always just leave them enabled and not worry about it. I think
>> there should be some cost because we are at least taking a lock during the
>> recording but I'm not sure how material that is.
>> 3. One consideration in how this exposed: we always found the ability to
>> dynamically change the logging level in JMX for log4j pretty useful. I
>> think if we want to leave the door open to add this ability to enable
>> metrics at runtime it may have some impact on the decision around how
>> metrics are registered/reported.
>>
>> -Jay
>>
>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com <ma...@gmail.com>> wrote:
>>
>>> I thought about "not registering at all" and left a comment on the PR as
>>> well regarding this idea. My concern is that it may be not very
>>> straight-forward to implement though via the MetricsReporter interface, if
>>> Eno and Aarti has a good approach to tackle it I would love it.
>>>
>>>
>>> Guozhang
>>>
>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <eno.thereska@gmail.com <ma...@gmail.com>>
>>> wrote:
>>>
>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
>>>> proceed.
>>>>
>>>> Thanks
>>>> Eno
>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk <ma...@juma.me.uk>> wrote:
>>>>>
>>>>> Thanks Eno. It would be good to update the KIP as well with regards to
>>> 1.
>>>>>
>>>>> About 2, I am not sure how adding a field could break existing tools.
>>>>> Having said that, your suggestion not to register sensors at all if
>>> their
>>>>> record level is below what is configured works for me.
>>>>>
>>>>> Ismael
>>>>>
>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <eno.thereska@gmail.com <ma...@gmail.com>>
>>>>> wrote:
>>>>>
>>>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>>>
>>>>>> As for 2. I'd prefer not adding extra info to the metrics reporters at
>>>>>> this point, since it might break existing tools out there (e.g., if we
>>>> add
>>>>>> things like configured level). Existing tools might be expecting the
>>>> info
>>>>>> to be reported in a particular format.
>>>>>>
>>>>>> If the current way is confusing, I think the next best alternative is
>>> to
>>>>>> not register sensors at all if their record level is below what is
>>>>>> configured. That way they don't show up at all. This will require some
>>>> more
>>>>>> code in Sensors.java to check at every step, but I think it's clean
>>> from
>>>>>> the user's point of view.
>>>>>>
>>>>>> Eno
>>>>>>
>>>>>>
>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk <ma...@juma.me.uk>> wrote:
>>>>>>>
>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple of
>>>>>> comments:
>>>>>>>
>>>>>>> 1. As Jun asked in the PR, do we need a broker config as well? The
>>>> broker
>>>>>>> uses Kafka Metrics for some metrics, but we probably don't have any
>>>> debug
>>>>>>> sensors at the broker yet. Either way, it would be good to describe
>>> the
>>>>>>> thinking around this.
>>>>>>>
>>>>>>> 2. The behaviour with regards to the metrics reporter could be
>>>>>> surprising.
>>>>>>> It would be good to elaborate a little more on this aspect. For
>>>> example,
>>>>>>> maybe we want to expose the sensor level and the current configured
>>>> level
>>>>>>> to metric reporters. That could then be used to build the debug/info
>>>>>>> dashboard that Eno mentioned (while making it clear that some metrics
>>>>>>> exist, but are not currently being recorded).
>>>>>>>
>>>>>>> Ismael
>>>>>>>
>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and polled by
>>> a
>>>>>>>> reporter, but the metrics associated with it will not be updated.
>>>>>>>>
>>>>>>>> That would allow a user to have, for example, a debug dashboard and
>>> an
>>>>>>>> info dashboard.
>>>>>>>>
>>>>>>>> Updated KIP to make this clear.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Eno
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com <ma...@gmail.com>>
>>> wrote:
>>>>>>>>>
>>>>>>>>> Thanks for the review, Guozhang,
>>>>>>>>>
>>>>>>>>> Addressed 2 out of the three comments,
>>>>>>>>>
>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>>>>>>>
>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g.
>>> which
>>>>>>>> class
>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>
>>>>>>>>> Added more details on shouldRecord() on the KIP
>>>>>>>>>
>>>>>>>>> In Sensor.java the shouldRecord() method is used to compare the
>>> value
>>>>>> of
>>>>>>>>> metric record level in the consumer config and the RecordLevel
>>>>>> associated
>>>>>>>>> with the sensor, to determine if metrics should recorded.
>>>>>>>>>
>>>>>>>>> From Sensor.java
>>>>>>>>>
>>>>>>>>> /**
>>>>>>>>> * @return true if the sensor's record level indicates that the
>>> metric
>>>>>>>>> will be recorded, false otherwise
>>>>>>>>> */
>>>>>>>>> public boolean shouldRecord() {
>>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> From nested enum, Sensor.RecordLevel
>>>>>>>>>
>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>>>> return true;
>>>>>>>>> } else {
>>>>>>>>> return configLevel.equals(this);
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2. Need to discuss with Eno.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> aarti
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wangguoz@gmail.com <ma...@gmail.com>>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>>>
>>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
>>>> name,
>>>>>>>> but
>>>>>>>>>> not the real config string, I think it is `metrics.record.level`
>>>>>>>> instead?
>>>>>>>>>>
>>>>>>>>>> 2. In the Motivation section, as in "This associates each sensor
>>>> with
>>>>>> a
>>>>>>>>>> record level ... only if the metric config of the client requires
>>>>>> these
>>>>>>>>>> metrics to be recorded."
>>>>>>>>>>
>>>>>>>>>> Could you elaborate this a bit more, for example, will the sensor
>>>> ever
>>>>>>>> be
>>>>>>>>>> registered in the reporter if its level is not allowed in the
>>> client
>>>>>>>>>> config? Or it will be registered but never polled? Or it will be
>>>>>>>> registered
>>>>>>>>>> and polled, but recorded?
>>>>>>>>>>
>>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will
>>> have
>>>>>> the
>>>>>>>>>> default value based on its type, and it will still be polled by
>>> the
>>>>>>>>>> reporter but not recorded. To an end-user's experience it will
>>> mean
>>>>>> that
>>>>>>>>>> for example the monitoring UI that displays all polled metrics
>>> will
>>>>>>>> still
>>>>>>>>>> show the metrics graph, with the value consistently shown as the
>>>>>> default
>>>>>>>>>> value, instead of not showing the graphs at all.
>>>>>>>>>>
>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
>>> which
>>>>>>>> class
>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Guozhang
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>>>> eno.thereska@gmail.com <ma...@gmail.com>>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>>
>>>>>>>>>>> Eno
>>>>>>>>>>>
>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aartiguptaa@gmail.com <ma...@gmail.com>>
>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Thanks Radai,
>>>>>>>>>>>>
>>>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- <https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Aarti
>>>>>>>>>>>>
>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
>>> radai.rosenblatt@gmail.com <ma...@gmail.com>
>>>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>
>>>>>>>>>>>>> ?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>>>> aartiguptaa@gmail.com
>>>>>>>>>>>> <javascript:;>>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
>>>>>> Record
>>>>>>>>>>>> Level
>>>>>>>>>>>>>
>>>>>>>>>>>>>> for Sensors
>>>>>>>>>>>>>
>>>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>>>>>
>>>>>>>>>>>>>> <
>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>
>>>>>>>>>>>>>> *pageId=67636483*
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> -- Guozhang
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> -- Guozhang
>>>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Thanks Jay, will fix the motivation. We have a microbenchmark and perf graph in the PR:
https://github.com/apache/kafka/pull/1446#issuecomment-268106260 <https://github.com/apache/kafka/pull/1446#issuecomment-268106260>
I'll need to think some more about point 3.
Thanks
Eno
> On 5 Jan 2017, at 19:18, Jay Kreps <ja...@confluent.io> wrote:
>
> This is great! A couple of quick comments:
>
> 1. It'd be good to make the motivation a bit more clear. I think the
> motivation is "We want to have lots of partition/task/etc metrics but we're
> concerned about the performance impact so we want to disable them by
> default." Currently the motivation section is more about the proposed
> change and doesn't really make clear why.
> 2. Do we have a microbenchmark that shows that the performance of (1)
> enabled metrics, (2) disabled metrics, (3) no metrics? This would help
> build the case for needing this extra knob. Obviously if metrics are cheap
> you would always just leave them enabled and not worry about it. I think
> there should be some cost because we are at least taking a lock during the
> recording but I'm not sure how material that is.
> 3. One consideration in how this exposed: we always found the ability to
> dynamically change the logging level in JMX for log4j pretty useful. I
> think if we want to leave the door open to add this ability to enable
> metrics at runtime it may have some impact on the decision around how
> metrics are registered/reported.
>
> -Jay
>
> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wa...@gmail.com> wrote:
>
>> I thought about "not registering at all" and left a comment on the PR as
>> well regarding this idea. My concern is that it may be not very
>> straight-forward to implement though via the MetricsReporter interface, if
>> Eno and Aarti has a good approach to tackle it I would love it.
>>
>>
>> Guozhang
>>
>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <en...@gmail.com>
>> wrote:
>>
>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
>>> proceed.
>>>
>>> Thanks
>>> Eno
>>>> On 5 Jan 2017, at 12:27, Ismael Juma <is...@juma.me.uk> wrote:
>>>>
>>>> Thanks Eno. It would be good to update the KIP as well with regards to
>> 1.
>>>>
>>>> About 2, I am not sure how adding a field could break existing tools.
>>>> Having said that, your suggestion not to register sensors at all if
>> their
>>>> record level is below what is configured works for me.
>>>>
>>>> Ismael
>>>>
>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
>>>> wrote:
>>>>
>>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>>
>>>>> As for 2. I'd prefer not adding extra info to the metrics reporters at
>>>>> this point, since it might break existing tools out there (e.g., if we
>>> add
>>>>> things like configured level). Existing tools might be expecting the
>>> info
>>>>> to be reported in a particular format.
>>>>>
>>>>> If the current way is confusing, I think the next best alternative is
>> to
>>>>> not register sensors at all if their record level is below what is
>>>>> configured. That way they don't show up at all. This will require some
>>> more
>>>>> code in Sensors.java to check at every step, but I think it's clean
>> from
>>>>> the user's point of view.
>>>>>
>>>>> Eno
>>>>>
>>>>>
>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
>>>>>>
>>>>>> Thanks for the KIP, it seems like a good improvement. A couple of
>>>>> comments:
>>>>>>
>>>>>> 1. As Jun asked in the PR, do we need a broker config as well? The
>>> broker
>>>>>> uses Kafka Metrics for some metrics, but we probably don't have any
>>> debug
>>>>>> sensors at the broker yet. Either way, it would be good to describe
>> the
>>>>>> thinking around this.
>>>>>>
>>>>>> 2. The behaviour with regards to the metrics reporter could be
>>>>> surprising.
>>>>>> It would be good to elaborate a little more on this aspect. For
>>> example,
>>>>>> maybe we want to expose the sensor level and the current configured
>>> level
>>>>>> to metric reporters. That could then be used to build the debug/info
>>>>>> dashboard that Eno mentioned (while making it clear that some metrics
>>>>>> exist, but are not currently being recorded).
>>>>>>
>>>>>> Ismael
>>>>>>
>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
>> eno.thereska@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Correct on 2. Guozhang: the sensor will be registered and polled by
>> a
>>>>>>> reporter, but the metrics associated with it will not be updated.
>>>>>>>
>>>>>>> That would allow a user to have, for example, a debug dashboard and
>> an
>>>>>>> info dashboard.
>>>>>>>
>>>>>>> Updated KIP to make this clear.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Eno
>>>>>>>
>>>>>>>
>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com>
>> wrote:
>>>>>>>>
>>>>>>>> Thanks for the review, Guozhang,
>>>>>>>>
>>>>>>>> Addressed 2 out of the three comments,
>>>>>>>>
>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>>>>>>
>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g.
>> which
>>>>>>> class
>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>
>>>>>>>> Added more details on shouldRecord() on the KIP
>>>>>>>>
>>>>>>>> In Sensor.java the shouldRecord() method is used to compare the
>> value
>>>>> of
>>>>>>>> metric record level in the consumer config and the RecordLevel
>>>>> associated
>>>>>>>> with the sensor, to determine if metrics should recorded.
>>>>>>>>
>>>>>>>> From Sensor.java
>>>>>>>>
>>>>>>>> /**
>>>>>>>> * @return true if the sensor's record level indicates that the
>> metric
>>>>>>>> will be recorded, false otherwise
>>>>>>>> */
>>>>>>>> public boolean shouldRecord() {
>>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>>>>> }
>>>>>>>>
>>>>>>>> From nested enum, Sensor.RecordLevel
>>>>>>>>
>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>>> return true;
>>>>>>>> } else {
>>>>>>>> return configLevel.equals(this);
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> 2. Need to discuss with Eno.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> aarti
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>>
>>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
>>> name,
>>>>>>> but
>>>>>>>>> not the real config string, I think it is `metrics.record.level`
>>>>>>> instead?
>>>>>>>>>
>>>>>>>>> 2. In the Motivation section, as in "This associates each sensor
>>> with
>>>>> a
>>>>>>>>> record level ... only if the metric config of the client requires
>>>>> these
>>>>>>>>> metrics to be recorded."
>>>>>>>>>
>>>>>>>>> Could you elaborate this a bit more, for example, will the sensor
>>> ever
>>>>>>> be
>>>>>>>>> registered in the reporter if its level is not allowed in the
>> client
>>>>>>>>> config? Or it will be registered but never polled? Or it will be
>>>>>>> registered
>>>>>>>>> and polled, but recorded?
>>>>>>>>>
>>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will
>> have
>>>>> the
>>>>>>>>> default value based on its type, and it will still be polled by
>> the
>>>>>>>>> reporter but not recorded. To an end-user's experience it will
>> mean
>>>>> that
>>>>>>>>> for example the monitoring UI that displays all polled metrics
>> will
>>>>>>> still
>>>>>>>>> show the metrics graph, with the value consistently shown as the
>>>>> default
>>>>>>>>> value, instead of not showing the graphs at all.
>>>>>>>>>
>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
>> which
>>>>>>> class
>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Guozhang
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>>> eno.thereska@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>
>>>>>>>>>> Eno
>>>>>>>>>>
>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Thanks Radai,
>>>>>>>>>>>
>>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Aarti
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
>> radai.rosenblatt@gmail.com
>>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>
>>>>>>>>>>>> ?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>>> aartiguptaa@gmail.com
>>>>>>>>>>> <javascript:;>>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
>>>>> Record
>>>>>>>>>>> Level
>>>>>>>>>>>>
>>>>>>>>>>>>> for Sensors
>>>>>>>>>>>>
>>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>>>>
>>>>>>>>>>>>> <
>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>
>>>>>>>>>>>>>> *
>>>>>>>>>>>>
>>>>>>>>>>>>> *pageId=67636483*
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> -- Guozhang
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
>> --
>> -- Guozhang
>>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Jay Kreps <ja...@confluent.io>.
This is great! A couple of quick comments:
1. It'd be good to make the motivation a bit more clear. I think the
motivation is "We want to have lots of partition/task/etc metrics but we're
concerned about the performance impact so we want to disable them by
default." Currently the motivation section is more about the proposed
change and doesn't really make clear why.
2. Do we have a microbenchmark that shows that the performance of (1)
enabled metrics, (2) disabled metrics, (3) no metrics? This would help
build the case for needing this extra knob. Obviously if metrics are cheap
you would always just leave them enabled and not worry about it. I think
there should be some cost because we are at least taking a lock during the
recording but I'm not sure how material that is.
3. One consideration in how this exposed: we always found the ability to
dynamically change the logging level in JMX for log4j pretty useful. I
think if we want to leave the door open to add this ability to enable
metrics at runtime it may have some impact on the decision around how
metrics are registered/reported.
-Jay
On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wa...@gmail.com> wrote:
> I thought about "not registering at all" and left a comment on the PR as
> well regarding this idea. My concern is that it may be not very
> straight-forward to implement though via the MetricsReporter interface, if
> Eno and Aarti has a good approach to tackle it I would love it.
>
>
> Guozhang
>
> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <en...@gmail.com>
> wrote:
>
> > Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
> > proceed.
> >
> > Thanks
> > Eno
> > > On 5 Jan 2017, at 12:27, Ismael Juma <is...@juma.me.uk> wrote:
> > >
> > > Thanks Eno. It would be good to update the KIP as well with regards to
> 1.
> > >
> > > About 2, I am not sure how adding a field could break existing tools.
> > > Having said that, your suggestion not to register sensors at all if
> their
> > > record level is below what is configured works for me.
> > >
> > > Ismael
> > >
> > > On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
> > > wrote:
> > >
> > >> Thanks Ismael. Already addressed 1. in the PR.
> > >>
> > >> As for 2. I'd prefer not adding extra info to the metrics reporters at
> > >> this point, since it might break existing tools out there (e.g., if we
> > add
> > >> things like configured level). Existing tools might be expecting the
> > info
> > >> to be reported in a particular format.
> > >>
> > >> If the current way is confusing, I think the next best alternative is
> to
> > >> not register sensors at all if their record level is below what is
> > >> configured. That way they don't show up at all. This will require some
> > more
> > >> code in Sensors.java to check at every step, but I think it's clean
> from
> > >> the user's point of view.
> > >>
> > >> Eno
> > >>
> > >>
> > >>> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
> > >>>
> > >>> Thanks for the KIP, it seems like a good improvement. A couple of
> > >> comments:
> > >>>
> > >>> 1. As Jun asked in the PR, do we need a broker config as well? The
> > broker
> > >>> uses Kafka Metrics for some metrics, but we probably don't have any
> > debug
> > >>> sensors at the broker yet. Either way, it would be good to describe
> the
> > >>> thinking around this.
> > >>>
> > >>> 2. The behaviour with regards to the metrics reporter could be
> > >> surprising.
> > >>> It would be good to elaborate a little more on this aspect. For
> > example,
> > >>> maybe we want to expose the sensor level and the current configured
> > level
> > >>> to metric reporters. That could then be used to build the debug/info
> > >>> dashboard that Eno mentioned (while making it clear that some metrics
> > >>> exist, but are not currently being recorded).
> > >>>
> > >>> Ismael
> > >>>
> > >>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
> eno.thereska@gmail.com>
> > >>> wrote:
> > >>>
> > >>>> Correct on 2. Guozhang: the sensor will be registered and polled by
> a
> > >>>> reporter, but the metrics associated with it will not be updated.
> > >>>>
> > >>>> That would allow a user to have, for example, a debug dashboard and
> an
> > >>>> info dashboard.
> > >>>>
> > >>>> Updated KIP to make this clear.
> > >>>>
> > >>>> Thanks
> > >>>> Eno
> > >>>>
> > >>>>
> > >>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com>
> wrote:
> > >>>>>
> > >>>>> Thanks for the review, Guozhang,
> > >>>>>
> > >>>>> Addressed 2 out of the three comments,
> > >>>>>
> > >>>>> 1. Fixed and updated KIP (swapped code variable name
> > >>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
> > >>>>>
> > >>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g.
> which
> > >>>> class
> > >>>>> it will be added to? Does it contain any parameters?
> > >>>>>
> > >>>>> Added more details on shouldRecord() on the KIP
> > >>>>>
> > >>>>> In Sensor.java the shouldRecord() method is used to compare the
> value
> > >> of
> > >>>>> metric record level in the consumer config and the RecordLevel
> > >> associated
> > >>>>> with the sensor, to determine if metrics should recorded.
> > >>>>>
> > >>>>> From Sensor.java
> > >>>>>
> > >>>>> /**
> > >>>>> * @return true if the sensor's record level indicates that the
> metric
> > >>>>> will be recorded, false otherwise
> > >>>>> */
> > >>>>> public boolean shouldRecord() {
> > >>>>> return this.recordLevel.shouldRecord(config.recordLevel());
> > >>>>> }
> > >>>>>
> > >>>>> From nested enum, Sensor.RecordLevel
> > >>>>>
> > >>>>> public boolean shouldRecord(final RecordLevel configLevel) {
> > >>>>> if (configLevel.equals(DEBUG)) {
> > >>>>> return true;
> > >>>>> } else {
> > >>>>> return configLevel.equals(this);
> > >>>>> }
> > >>>>> }
> > >>>>>
> > >>>>>
> > >>>>> 2. Need to discuss with Eno.
> > >>>>>
> > >>>>>
> > >>>>> Thanks!
> > >>>>>
> > >>>>> aarti
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
> > >>>> wrote:
> > >>>>>
> > >>>>>> LGTM overall. A few comments below:
> > >>>>>>
> > >>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
> > name,
> > >>>> but
> > >>>>>> not the real config string, I think it is `metrics.record.level`
> > >>>> instead?
> > >>>>>>
> > >>>>>> 2. In the Motivation section, as in "This associates each sensor
> > with
> > >> a
> > >>>>>> record level ... only if the metric config of the client requires
> > >> these
> > >>>>>> metrics to be recorded."
> > >>>>>>
> > >>>>>> Could you elaborate this a bit more, for example, will the sensor
> > ever
> > >>>> be
> > >>>>>> registered in the reporter if its level is not allowed in the
> client
> > >>>>>> config? Or it will be registered but never polled? Or it will be
> > >>>> registered
> > >>>>>> and polled, but recorded?
> > >>>>>>
> > >>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will
> have
> > >> the
> > >>>>>> default value based on its type, and it will still be polled by
> the
> > >>>>>> reporter but not recorded. To an end-user's experience it will
> mean
> > >> that
> > >>>>>> for example the monitoring UI that displays all polled metrics
> will
> > >>>> still
> > >>>>>> show the metrics graph, with the value consistently shown as the
> > >> default
> > >>>>>> value, instead of not showing the graphs at all.
> > >>>>>>
> > >>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g.
> which
> > >>>> class
> > >>>>>> it will be added to? Does it contain any parameters?
> > >>>>>>
> > >>>>>>
> > >>>>>> Guozhang
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
> > eno.thereska@gmail.com>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> > >>>>>>>
> > >>>>>>> Eno
> > >>>>>>>
> > >>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
> > >> wrote:
> > >>>>>>>
> > >>>>>>>> Thanks Radai,
> > >>>>>>>>
> > >>>>>>>> Yes that is the correct link, my bad
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Aarti
> > >>>>>>>>
> > >>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
> radai.rosenblatt@gmail.com
> > >>>>>>>> <javascript:;>> wrote:
> > >>>>>>>>
> > >>>>>>>>> link leads to 104. i think this is the correct one -
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> > >>>>>>>>>
> > >>>>>>>>> ?
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
> > >> aartiguptaa@gmail.com
> > >>>>>>>> <javascript:;>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> Hi all,
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
> > >> Record
> > >>>>>>>> Level
> > >>>>>>>>>
> > >>>>>>>>>> for Sensors
> > >>>>>>>>>
> > >>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
> > >>>>>>>>>
> > >>>>>>>>>> <
> > >>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> > >>>>>>>> action?pageId=67636480
> > >>>>>>>>>
> > >>>>>>>>>>> *
> > >>>>>>>>>
> > >>>>>>>>>> *pageId=67636483*
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> Looking forward to your feedback.
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>> Thanks,
> > >>>>>>>>>
> > >>>>>>>>>> Aarti and Eno
> > >>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> --
> > >>>>>> -- Guozhang
> > >>>>>>
> > >>>>
> > >>>>
> > >>
> > >>
> >
> >
>
>
> --
> -- Guozhang
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Guozhang,
I was thinking to do this in Sensor.java, and not touch the MetricsReporter interface. Basically I'd go for not adding a KafkaMetric at all with this approach. But perhaps I'm missing something.
Thanks
Eno
> On 5 Jan 2017, at 17:59, Guozhang Wang <wa...@gmail.com> wrote:
>
> I thought about "not registering at all" and left a comment on the PR as
> well regarding this idea. My concern is that it may be not very
> straight-forward to implement though via the MetricsReporter interface, if
> Eno and Aarti has a good approach to tackle it I would love it.
>
>
> Guozhang
>
> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <en...@gmail.com> wrote:
>
>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
>> proceed.
>>
>> Thanks
>> Eno
>>> On 5 Jan 2017, at 12:27, Ismael Juma <is...@juma.me.uk> wrote:
>>>
>>> Thanks Eno. It would be good to update the KIP as well with regards to 1.
>>>
>>> About 2, I am not sure how adding a field could break existing tools.
>>> Having said that, your suggestion not to register sensors at all if their
>>> record level is below what is configured works for me.
>>>
>>> Ismael
>>>
>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
>>> wrote:
>>>
>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>
>>>> As for 2. I'd prefer not adding extra info to the metrics reporters at
>>>> this point, since it might break existing tools out there (e.g., if we
>> add
>>>> things like configured level). Existing tools might be expecting the
>> info
>>>> to be reported in a particular format.
>>>>
>>>> If the current way is confusing, I think the next best alternative is to
>>>> not register sensors at all if their record level is below what is
>>>> configured. That way they don't show up at all. This will require some
>> more
>>>> code in Sensors.java to check at every step, but I think it's clean from
>>>> the user's point of view.
>>>>
>>>> Eno
>>>>
>>>>
>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
>>>>>
>>>>> Thanks for the KIP, it seems like a good improvement. A couple of
>>>> comments:
>>>>>
>>>>> 1. As Jun asked in the PR, do we need a broker config as well? The
>> broker
>>>>> uses Kafka Metrics for some metrics, but we probably don't have any
>> debug
>>>>> sensors at the broker yet. Either way, it would be good to describe the
>>>>> thinking around this.
>>>>>
>>>>> 2. The behaviour with regards to the metrics reporter could be
>>>> surprising.
>>>>> It would be good to elaborate a little more on this aspect. For
>> example,
>>>>> maybe we want to expose the sensor level and the current configured
>> level
>>>>> to metric reporters. That could then be used to build the debug/info
>>>>> dashboard that Eno mentioned (while making it clear that some metrics
>>>>> exist, but are not currently being recorded).
>>>>>
>>>>> Ismael
>>>>>
>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Correct on 2. Guozhang: the sensor will be registered and polled by a
>>>>>> reporter, but the metrics associated with it will not be updated.
>>>>>>
>>>>>> That would allow a user to have, for example, a debug dashboard and an
>>>>>> info dashboard.
>>>>>>
>>>>>> Updated KIP to make this clear.
>>>>>>
>>>>>> Thanks
>>>>>> Eno
>>>>>>
>>>>>>
>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
>>>>>>>
>>>>>>> Thanks for the review, Guozhang,
>>>>>>>
>>>>>>> Addressed 2 out of the three comments,
>>>>>>>
>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>>>>>
>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
>>>>>> class
>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>
>>>>>>> Added more details on shouldRecord() on the KIP
>>>>>>>
>>>>>>> In Sensor.java the shouldRecord() method is used to compare the value
>>>> of
>>>>>>> metric record level in the consumer config and the RecordLevel
>>>> associated
>>>>>>> with the sensor, to determine if metrics should recorded.
>>>>>>>
>>>>>>> From Sensor.java
>>>>>>>
>>>>>>> /**
>>>>>>> * @return true if the sensor's record level indicates that the metric
>>>>>>> will be recorded, false otherwise
>>>>>>> */
>>>>>>> public boolean shouldRecord() {
>>>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>>>> }
>>>>>>>
>>>>>>> From nested enum, Sensor.RecordLevel
>>>>>>>
>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>> return true;
>>>>>>> } else {
>>>>>>> return configLevel.equals(this);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> 2. Need to discuss with Eno.
>>>>>>>
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> aarti
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>
>>>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
>> name,
>>>>>> but
>>>>>>>> not the real config string, I think it is `metrics.record.level`
>>>>>> instead?
>>>>>>>>
>>>>>>>> 2. In the Motivation section, as in "This associates each sensor
>> with
>>>> a
>>>>>>>> record level ... only if the metric config of the client requires
>>>> these
>>>>>>>> metrics to be recorded."
>>>>>>>>
>>>>>>>> Could you elaborate this a bit more, for example, will the sensor
>> ever
>>>>>> be
>>>>>>>> registered in the reporter if its level is not allowed in the client
>>>>>>>> config? Or it will be registered but never polled? Or it will be
>>>>>> registered
>>>>>>>> and polled, but recorded?
>>>>>>>>
>>>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will have
>>>> the
>>>>>>>> default value based on its type, and it will still be polled by the
>>>>>>>> reporter but not recorded. To an end-user's experience it will mean
>>>> that
>>>>>>>> for example the monitoring UI that displays all polled metrics will
>>>>>> still
>>>>>>>> show the metrics graph, with the value consistently shown as the
>>>> default
>>>>>>>> value, instead of not showing the graphs at all.
>>>>>>>>
>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
>>>>>> class
>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>
>>>>>>>>
>>>>>>>> Guozhang
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>> eno.thereska@gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>
>>>>>>>>> Eno
>>>>>>>>>
>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Thanks Radai,
>>>>>>>>>>
>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Aarti
>>>>>>>>>>
>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>
>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>
>>>>>>>>>>> ?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>> aartiguptaa@gmail.com
>>>>>>>>>> <javascript:;>>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
>>>> Record
>>>>>>>>>> Level
>>>>>>>>>>>
>>>>>>>>>>>> for Sensors
>>>>>>>>>>>
>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>>>
>>>>>>>>>>>> <
>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>
>>>>>>>>>>>>> *
>>>>>>>>>>>
>>>>>>>>>>>> *pageId=67636483*
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> -- Guozhang
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
>
>
> --
> -- Guozhang
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Guozhang Wang <wa...@gmail.com>.
I thought about "not registering at all" and left a comment on the PR as
well regarding this idea. My concern is that it may be not very
straight-forward to implement though via the MetricsReporter interface, if
Eno and Aarti has a good approach to tackle it I would love it.
Guozhang
On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska <en...@gmail.com> wrote:
> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can
> proceed.
>
> Thanks
> Eno
> > On 5 Jan 2017, at 12:27, Ismael Juma <is...@juma.me.uk> wrote:
> >
> > Thanks Eno. It would be good to update the KIP as well with regards to 1.
> >
> > About 2, I am not sure how adding a field could break existing tools.
> > Having said that, your suggestion not to register sensors at all if their
> > record level is below what is configured works for me.
> >
> > Ismael
> >
> > On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
> > wrote:
> >
> >> Thanks Ismael. Already addressed 1. in the PR.
> >>
> >> As for 2. I'd prefer not adding extra info to the metrics reporters at
> >> this point, since it might break existing tools out there (e.g., if we
> add
> >> things like configured level). Existing tools might be expecting the
> info
> >> to be reported in a particular format.
> >>
> >> If the current way is confusing, I think the next best alternative is to
> >> not register sensors at all if their record level is below what is
> >> configured. That way they don't show up at all. This will require some
> more
> >> code in Sensors.java to check at every step, but I think it's clean from
> >> the user's point of view.
> >>
> >> Eno
> >>
> >>
> >>> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
> >>>
> >>> Thanks for the KIP, it seems like a good improvement. A couple of
> >> comments:
> >>>
> >>> 1. As Jun asked in the PR, do we need a broker config as well? The
> broker
> >>> uses Kafka Metrics for some metrics, but we probably don't have any
> debug
> >>> sensors at the broker yet. Either way, it would be good to describe the
> >>> thinking around this.
> >>>
> >>> 2. The behaviour with regards to the metrics reporter could be
> >> surprising.
> >>> It would be good to elaborate a little more on this aspect. For
> example,
> >>> maybe we want to expose the sensor level and the current configured
> level
> >>> to metric reporters. That could then be used to build the debug/info
> >>> dashboard that Eno mentioned (while making it clear that some metrics
> >>> exist, but are not currently being recorded).
> >>>
> >>> Ismael
> >>>
> >>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
> >>> wrote:
> >>>
> >>>> Correct on 2. Guozhang: the sensor will be registered and polled by a
> >>>> reporter, but the metrics associated with it will not be updated.
> >>>>
> >>>> That would allow a user to have, for example, a debug dashboard and an
> >>>> info dashboard.
> >>>>
> >>>> Updated KIP to make this clear.
> >>>>
> >>>> Thanks
> >>>> Eno
> >>>>
> >>>>
> >>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
> >>>>>
> >>>>> Thanks for the review, Guozhang,
> >>>>>
> >>>>> Addressed 2 out of the three comments,
> >>>>>
> >>>>> 1. Fixed and updated KIP (swapped code variable name
> >>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
> >>>>>
> >>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
> >>>> class
> >>>>> it will be added to? Does it contain any parameters?
> >>>>>
> >>>>> Added more details on shouldRecord() on the KIP
> >>>>>
> >>>>> In Sensor.java the shouldRecord() method is used to compare the value
> >> of
> >>>>> metric record level in the consumer config and the RecordLevel
> >> associated
> >>>>> with the sensor, to determine if metrics should recorded.
> >>>>>
> >>>>> From Sensor.java
> >>>>>
> >>>>> /**
> >>>>> * @return true if the sensor's record level indicates that the metric
> >>>>> will be recorded, false otherwise
> >>>>> */
> >>>>> public boolean shouldRecord() {
> >>>>> return this.recordLevel.shouldRecord(config.recordLevel());
> >>>>> }
> >>>>>
> >>>>> From nested enum, Sensor.RecordLevel
> >>>>>
> >>>>> public boolean shouldRecord(final RecordLevel configLevel) {
> >>>>> if (configLevel.equals(DEBUG)) {
> >>>>> return true;
> >>>>> } else {
> >>>>> return configLevel.equals(this);
> >>>>> }
> >>>>> }
> >>>>>
> >>>>>
> >>>>> 2. Need to discuss with Eno.
> >>>>>
> >>>>>
> >>>>> Thanks!
> >>>>>
> >>>>> aarti
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
> >>>> wrote:
> >>>>>
> >>>>>> LGTM overall. A few comments below:
> >>>>>>
> >>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable
> name,
> >>>> but
> >>>>>> not the real config string, I think it is `metrics.record.level`
> >>>> instead?
> >>>>>>
> >>>>>> 2. In the Motivation section, as in "This associates each sensor
> with
> >> a
> >>>>>> record level ... only if the metric config of the client requires
> >> these
> >>>>>> metrics to be recorded."
> >>>>>>
> >>>>>> Could you elaborate this a bit more, for example, will the sensor
> ever
> >>>> be
> >>>>>> registered in the reporter if its level is not allowed in the client
> >>>>>> config? Or it will be registered but never polled? Or it will be
> >>>> registered
> >>>>>> and polled, but recorded?
> >>>>>>
> >>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will have
> >> the
> >>>>>> default value based on its type, and it will still be polled by the
> >>>>>> reporter but not recorded. To an end-user's experience it will mean
> >> that
> >>>>>> for example the monitoring UI that displays all polled metrics will
> >>>> still
> >>>>>> show the metrics graph, with the value consistently shown as the
> >> default
> >>>>>> value, instead of not showing the graphs at all.
> >>>>>>
> >>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
> >>>> class
> >>>>>> it will be added to? Does it contain any parameters?
> >>>>>>
> >>>>>>
> >>>>>> Guozhang
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
> eno.thereska@gmail.com>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> >>>>>>>
> >>>>>>> Eno
> >>>>>>>
> >>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
> >> wrote:
> >>>>>>>
> >>>>>>>> Thanks Radai,
> >>>>>>>>
> >>>>>>>> Yes that is the correct link, my bad
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Aarti
> >>>>>>>>
> >>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> >>>>>>>> <javascript:;>> wrote:
> >>>>>>>>
> >>>>>>>>> link leads to 104. i think this is the correct one -
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>>>>
> >>>>>>>>> ?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
> >> aartiguptaa@gmail.com
> >>>>>>>> <javascript:;>>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Hi all,
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
> >> Record
> >>>>>>>> Level
> >>>>>>>>>
> >>>>>>>>>> for Sensors
> >>>>>>>>>
> >>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
> >>>>>>>>>
> >>>>>>>>>> <
> >>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> >>>>>>>> action?pageId=67636480
> >>>>>>>>>
> >>>>>>>>>>> *
> >>>>>>>>>
> >>>>>>>>>> *pageId=67636483*
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Looking forward to your feedback.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> Thanks,
> >>>>>>>>>
> >>>>>>>>>> Aarti and Eno
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> -- Guozhang
> >>>>>>
> >>>>
> >>>>
> >>
> >>
>
>
--
-- Guozhang
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we can proceed.
Thanks
Eno
> On 5 Jan 2017, at 12:27, Ismael Juma <is...@juma.me.uk> wrote:
>
> Thanks Eno. It would be good to update the KIP as well with regards to 1.
>
> About 2, I am not sure how adding a field could break existing tools.
> Having said that, your suggestion not to register sensors at all if their
> record level is below what is configured works for me.
>
> Ismael
>
> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
> wrote:
>
>> Thanks Ismael. Already addressed 1. in the PR.
>>
>> As for 2. I'd prefer not adding extra info to the metrics reporters at
>> this point, since it might break existing tools out there (e.g., if we add
>> things like configured level). Existing tools might be expecting the info
>> to be reported in a particular format.
>>
>> If the current way is confusing, I think the next best alternative is to
>> not register sensors at all if their record level is below what is
>> configured. That way they don't show up at all. This will require some more
>> code in Sensors.java to check at every step, but I think it's clean from
>> the user's point of view.
>>
>> Eno
>>
>>
>>> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
>>>
>>> Thanks for the KIP, it seems like a good improvement. A couple of
>> comments:
>>>
>>> 1. As Jun asked in the PR, do we need a broker config as well? The broker
>>> uses Kafka Metrics for some metrics, but we probably don't have any debug
>>> sensors at the broker yet. Either way, it would be good to describe the
>>> thinking around this.
>>>
>>> 2. The behaviour with regards to the metrics reporter could be
>> surprising.
>>> It would be good to elaborate a little more on this aspect. For example,
>>> maybe we want to expose the sensor level and the current configured level
>>> to metric reporters. That could then be used to build the debug/info
>>> dashboard that Eno mentioned (while making it clear that some metrics
>>> exist, but are not currently being recorded).
>>>
>>> Ismael
>>>
>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
>>> wrote:
>>>
>>>> Correct on 2. Guozhang: the sensor will be registered and polled by a
>>>> reporter, but the metrics associated with it will not be updated.
>>>>
>>>> That would allow a user to have, for example, a debug dashboard and an
>>>> info dashboard.
>>>>
>>>> Updated KIP to make this clear.
>>>>
>>>> Thanks
>>>> Eno
>>>>
>>>>
>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
>>>>>
>>>>> Thanks for the review, Guozhang,
>>>>>
>>>>> Addressed 2 out of the three comments,
>>>>>
>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>>>
>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
>>>> class
>>>>> it will be added to? Does it contain any parameters?
>>>>>
>>>>> Added more details on shouldRecord() on the KIP
>>>>>
>>>>> In Sensor.java the shouldRecord() method is used to compare the value
>> of
>>>>> metric record level in the consumer config and the RecordLevel
>> associated
>>>>> with the sensor, to determine if metrics should recorded.
>>>>>
>>>>> From Sensor.java
>>>>>
>>>>> /**
>>>>> * @return true if the sensor's record level indicates that the metric
>>>>> will be recorded, false otherwise
>>>>> */
>>>>> public boolean shouldRecord() {
>>>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>>>> }
>>>>>
>>>>> From nested enum, Sensor.RecordLevel
>>>>>
>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>> if (configLevel.equals(DEBUG)) {
>>>>> return true;
>>>>> } else {
>>>>> return configLevel.equals(this);
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> 2. Need to discuss with Eno.
>>>>>
>>>>>
>>>>> Thanks!
>>>>>
>>>>> aarti
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
>>>> wrote:
>>>>>
>>>>>> LGTM overall. A few comments below:
>>>>>>
>>>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name,
>>>> but
>>>>>> not the real config string, I think it is `metrics.record.level`
>>>> instead?
>>>>>>
>>>>>> 2. In the Motivation section, as in "This associates each sensor with
>> a
>>>>>> record level ... only if the metric config of the client requires
>> these
>>>>>> metrics to be recorded."
>>>>>>
>>>>>> Could you elaborate this a bit more, for example, will the sensor ever
>>>> be
>>>>>> registered in the reporter if its level is not allowed in the client
>>>>>> config? Or it will be registered but never polled? Or it will be
>>>> registered
>>>>>> and polled, but recorded?
>>>>>>
>>>>>> From PR 1446 it seems to be the last case, i.e. the sensor will have
>> the
>>>>>> default value based on its type, and it will still be polled by the
>>>>>> reporter but not recorded. To an end-user's experience it will mean
>> that
>>>>>> for example the monitoring UI that displays all polled metrics will
>>>> still
>>>>>> show the metrics graph, with the value consistently shown as the
>> default
>>>>>> value, instead of not showing the graphs at all.
>>>>>>
>>>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
>>>> class
>>>>>> it will be added to? Does it contain any parameters?
>>>>>>
>>>>>>
>>>>>> Guozhang
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>
>>>>>>> Eno
>>>>>>>
>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
>> wrote:
>>>>>>>
>>>>>>>> Thanks Radai,
>>>>>>>>
>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>
>>>>>>>>
>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Aarti
>>>>>>>>
>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
>>>>>>>> <javascript:;>> wrote:
>>>>>>>>
>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>
>>>>>>>>> ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>> aartiguptaa@gmail.com
>>>>>>>> <javascript:;>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> I would like to start the discussion on KIP-105: Addition of
>> Record
>>>>>>>> Level
>>>>>>>>>
>>>>>>>>>> for Sensors
>>>>>>>>>
>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>
>>>>>>>>>> <
>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>> action?pageId=67636480
>>>>>>>>>
>>>>>>>>>>> *
>>>>>>>>>
>>>>>>>>>> *pageId=67636483*
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>>> Aarti and Eno
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> -- Guozhang
>>>>>>
>>>>
>>>>
>>
>>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Ismael Juma <is...@juma.me.uk>.
Thanks Eno. It would be good to update the KIP as well with regards to 1.
About 2, I am not sure how adding a field could break existing tools.
Having said that, your suggestion not to register sensors at all if their
record level is below what is configured works for me.
Ismael
On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska <en...@gmail.com>
wrote:
> Thanks Ismael. Already addressed 1. in the PR.
>
> As for 2. I'd prefer not adding extra info to the metrics reporters at
> this point, since it might break existing tools out there (e.g., if we add
> things like configured level). Existing tools might be expecting the info
> to be reported in a particular format.
>
> If the current way is confusing, I think the next best alternative is to
> not register sensors at all if their record level is below what is
> configured. That way they don't show up at all. This will require some more
> code in Sensors.java to check at every step, but I think it's clean from
> the user's point of view.
>
> Eno
>
>
> > On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
> >
> > Thanks for the KIP, it seems like a good improvement. A couple of
> comments:
> >
> > 1. As Jun asked in the PR, do we need a broker config as well? The broker
> > uses Kafka Metrics for some metrics, but we probably don't have any debug
> > sensors at the broker yet. Either way, it would be good to describe the
> > thinking around this.
> >
> > 2. The behaviour with regards to the metrics reporter could be
> surprising.
> > It would be good to elaborate a little more on this aspect. For example,
> > maybe we want to expose the sensor level and the current configured level
> > to metric reporters. That could then be used to build the debug/info
> > dashboard that Eno mentioned (while making it clear that some metrics
> > exist, but are not currently being recorded).
> >
> > Ismael
> >
> > On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
> > wrote:
> >
> >> Correct on 2. Guozhang: the sensor will be registered and polled by a
> >> reporter, but the metrics associated with it will not be updated.
> >>
> >> That would allow a user to have, for example, a debug dashboard and an
> >> info dashboard.
> >>
> >> Updated KIP to make this clear.
> >>
> >> Thanks
> >> Eno
> >>
> >>
> >>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
> >>>
> >>> Thanks for the review, Guozhang,
> >>>
> >>> Addressed 2 out of the three comments,
> >>>
> >>> 1. Fixed and updated KIP (swapped code variable name
> >>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
> >>>
> >>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
> >> class
> >>> it will be added to? Does it contain any parameters?
> >>>
> >>> Added more details on shouldRecord() on the KIP
> >>>
> >>> In Sensor.java the shouldRecord() method is used to compare the value
> of
> >>> metric record level in the consumer config and the RecordLevel
> associated
> >>> with the sensor, to determine if metrics should recorded.
> >>>
> >>> From Sensor.java
> >>>
> >>> /**
> >>> * @return true if the sensor's record level indicates that the metric
> >>> will be recorded, false otherwise
> >>> */
> >>> public boolean shouldRecord() {
> >>> return this.recordLevel.shouldRecord(config.recordLevel());
> >>> }
> >>>
> >>> From nested enum, Sensor.RecordLevel
> >>>
> >>> public boolean shouldRecord(final RecordLevel configLevel) {
> >>> if (configLevel.equals(DEBUG)) {
> >>> return true;
> >>> } else {
> >>> return configLevel.equals(this);
> >>> }
> >>> }
> >>>
> >>>
> >>> 2. Need to discuss with Eno.
> >>>
> >>>
> >>> Thanks!
> >>>
> >>> aarti
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
> >> wrote:
> >>>
> >>>> LGTM overall. A few comments below:
> >>>>
> >>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name,
> >> but
> >>>> not the real config string, I think it is `metrics.record.level`
> >> instead?
> >>>>
> >>>> 2. In the Motivation section, as in "This associates each sensor with
> a
> >>>> record level ... only if the metric config of the client requires
> these
> >>>> metrics to be recorded."
> >>>>
> >>>> Could you elaborate this a bit more, for example, will the sensor ever
> >> be
> >>>> registered in the reporter if its level is not allowed in the client
> >>>> config? Or it will be registered but never polled? Or it will be
> >> registered
> >>>> and polled, but recorded?
> >>>>
> >>>> From PR 1446 it seems to be the last case, i.e. the sensor will have
> the
> >>>> default value based on its type, and it will still be polled by the
> >>>> reporter but not recorded. To an end-user's experience it will mean
> that
> >>>> for example the monitoring UI that displays all polled metrics will
> >> still
> >>>> show the metrics graph, with the value consistently shown as the
> default
> >>>> value, instead of not showing the graphs at all.
> >>>>
> >>>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
> >> class
> >>>> it will be added to? Does it contain any parameters?
> >>>>
> >>>>
> >>>> Guozhang
> >>>>
> >>>>
> >>>>
> >>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> Thanks for starting the discussion on these KIPs Aarti.
> >>>>>
> >>>>> Eno
> >>>>>
> >>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com>
> wrote:
> >>>>>
> >>>>>> Thanks Radai,
> >>>>>>
> >>>>>> Yes that is the correct link, my bad
> >>>>>>
> >>>>>>
> >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Aarti
> >>>>>>
> >>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> >>>>>> <javascript:;>> wrote:
> >>>>>>
> >>>>>>> link leads to 104. i think this is the correct one -
> >>>>>>>
> >>>>>>>
> >>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>>>
> >>>>>>> ?
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
> aartiguptaa@gmail.com
> >>>>>> <javascript:;>>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> Hi all,
> >>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>> I would like to start the discussion on KIP-105: Addition of
> Record
> >>>>>> Level
> >>>>>>>
> >>>>>>>> for Sensors
> >>>>>>>
> >>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
> >>>>>>>
> >>>>>>>> <
> >>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> >>>>>> action?pageId=67636480
> >>>>>>>
> >>>>>>>>> *
> >>>>>>>
> >>>>>>>> *pageId=67636483*
> >>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>> Looking forward to your feedback.
> >>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>
> >>>>>>>> Aarti and Eno
> >>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> -- Guozhang
> >>>>
> >>
> >>
>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Thanks Ismael. Already addressed 1. in the PR.
As for 2. I'd prefer not adding extra info to the metrics reporters at this point, since it might break existing tools out there (e.g., if we add things like configured level). Existing tools might be expecting the info to be reported in a particular format.
If the current way is confusing, I think the next best alternative is to not register sensors at all if their record level is below what is configured. That way they don't show up at all. This will require some more code in Sensors.java to check at every step, but I think it's clean from the user's point of view.
Eno
> On 5 Jan 2017, at 11:23, Ismael Juma <is...@juma.me.uk> wrote:
>
> Thanks for the KIP, it seems like a good improvement. A couple of comments:
>
> 1. As Jun asked in the PR, do we need a broker config as well? The broker
> uses Kafka Metrics for some metrics, but we probably don't have any debug
> sensors at the broker yet. Either way, it would be good to describe the
> thinking around this.
>
> 2. The behaviour with regards to the metrics reporter could be surprising.
> It would be good to elaborate a little more on this aspect. For example,
> maybe we want to expose the sensor level and the current configured level
> to metric reporters. That could then be used to build the debug/info
> dashboard that Eno mentioned (while making it clear that some metrics
> exist, but are not currently being recorded).
>
> Ismael
>
> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
> wrote:
>
>> Correct on 2. Guozhang: the sensor will be registered and polled by a
>> reporter, but the metrics associated with it will not be updated.
>>
>> That would allow a user to have, for example, a debug dashboard and an
>> info dashboard.
>>
>> Updated KIP to make this clear.
>>
>> Thanks
>> Eno
>>
>>
>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
>>>
>>> Thanks for the review, Guozhang,
>>>
>>> Addressed 2 out of the three comments,
>>>
>>> 1. Fixed and updated KIP (swapped code variable name
>>> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>>>
>>> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
>> class
>>> it will be added to? Does it contain any parameters?
>>>
>>> Added more details on shouldRecord() on the KIP
>>>
>>> In Sensor.java the shouldRecord() method is used to compare the value of
>>> metric record level in the consumer config and the RecordLevel associated
>>> with the sensor, to determine if metrics should recorded.
>>>
>>> From Sensor.java
>>>
>>> /**
>>> * @return true if the sensor's record level indicates that the metric
>>> will be recorded, false otherwise
>>> */
>>> public boolean shouldRecord() {
>>> return this.recordLevel.shouldRecord(config.recordLevel());
>>> }
>>>
>>> From nested enum, Sensor.RecordLevel
>>>
>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>> if (configLevel.equals(DEBUG)) {
>>> return true;
>>> } else {
>>> return configLevel.equals(this);
>>> }
>>> }
>>>
>>>
>>> 2. Need to discuss with Eno.
>>>
>>>
>>> Thanks!
>>>
>>> aarti
>>>
>>>
>>>
>>>
>>>
>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
>> wrote:
>>>
>>>> LGTM overall. A few comments below:
>>>>
>>>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name,
>> but
>>>> not the real config string, I think it is `metrics.record.level`
>> instead?
>>>>
>>>> 2. In the Motivation section, as in "This associates each sensor with a
>>>> record level ... only if the metric config of the client requires these
>>>> metrics to be recorded."
>>>>
>>>> Could you elaborate this a bit more, for example, will the sensor ever
>> be
>>>> registered in the reporter if its level is not allowed in the client
>>>> config? Or it will be registered but never polled? Or it will be
>> registered
>>>> and polled, but recorded?
>>>>
>>>> From PR 1446 it seems to be the last case, i.e. the sensor will have the
>>>> default value based on its type, and it will still be polled by the
>>>> reporter but not recorded. To an end-user's experience it will mean that
>>>> for example the monitoring UI that displays all polled metrics will
>> still
>>>> show the metrics graph, with the value consistently shown as the default
>>>> value, instead of not showing the graphs at all.
>>>>
>>>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
>> class
>>>> it will be added to? Does it contain any parameters?
>>>>
>>>>
>>>> Guozhang
>>>>
>>>>
>>>>
>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
>>>> wrote:
>>>>
>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>
>>>>> Eno
>>>>>
>>>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
>>>>>
>>>>>> Thanks Radai,
>>>>>>
>>>>>> Yes that is the correct link, my bad
>>>>>>
>>>>>>
>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>
>>>>>>
>>>>>>
>>>>>> Aarti
>>>>>>
>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
>>>>>> <javascript:;>> wrote:
>>>>>>
>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>
>>>>>>>
>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>
>>>>>>> ?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
>>>>>> <javascript:;>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Hi all,
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> I would like to start the discussion on KIP-105: Addition of Record
>>>>>> Level
>>>>>>>
>>>>>>>> for Sensors
>>>>>>>
>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>
>>>>>>>> <
>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>> action?pageId=67636480
>>>>>>>
>>>>>>>>> *
>>>>>>>
>>>>>>>> *pageId=67636483*
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> Looking forward to your feedback.
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>
>>>>>>>> Aarti and Eno
>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> -- Guozhang
>>>>
>>
>>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Ismael Juma <is...@juma.me.uk>.
Thanks for the KIP, it seems like a good improvement. A couple of comments:
1. As Jun asked in the PR, do we need a broker config as well? The broker
uses Kafka Metrics for some metrics, but we probably don't have any debug
sensors at the broker yet. Either way, it would be good to describe the
thinking around this.
2. The behaviour with regards to the metrics reporter could be surprising.
It would be good to elaborate a little more on this aspect. For example,
maybe we want to expose the sensor level and the current configured level
to metric reporters. That could then be used to build the debug/info
dashboard that Eno mentioned (while making it clear that some metrics
exist, but are not currently being recorded).
Ismael
On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <en...@gmail.com>
wrote:
> Correct on 2. Guozhang: the sensor will be registered and polled by a
> reporter, but the metrics associated with it will not be updated.
>
> That would allow a user to have, for example, a debug dashboard and an
> info dashboard.
>
> Updated KIP to make this clear.
>
> Thanks
> Eno
>
>
> > On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
> >
> > Thanks for the review, Guozhang,
> >
> > Addressed 2 out of the three comments,
> >
> > 1. Fixed and updated KIP (swapped code variable name
> > METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
> >
> > 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which
> class
> > it will be added to? Does it contain any parameters?
> >
> > Added more details on shouldRecord() on the KIP
> >
> > In Sensor.java the shouldRecord() method is used to compare the value of
> > metric record level in the consumer config and the RecordLevel associated
> > with the sensor, to determine if metrics should recorded.
> >
> > From Sensor.java
> >
> > /**
> > * @return true if the sensor's record level indicates that the metric
> > will be recorded, false otherwise
> > */
> > public boolean shouldRecord() {
> > return this.recordLevel.shouldRecord(config.recordLevel());
> > }
> >
> > From nested enum, Sensor.RecordLevel
> >
> > public boolean shouldRecord(final RecordLevel configLevel) {
> > if (configLevel.equals(DEBUG)) {
> > return true;
> > } else {
> > return configLevel.equals(this);
> > }
> > }
> >
> >
> > 2. Need to discuss with Eno.
> >
> >
> > Thanks!
> >
> > aarti
> >
> >
> >
> >
> >
> > On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com>
> wrote:
> >
> >> LGTM overall. A few comments below:
> >>
> >> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name,
> but
> >> not the real config string, I think it is `metrics.record.level`
> instead?
> >>
> >> 2. In the Motivation section, as in "This associates each sensor with a
> >> record level ... only if the metric config of the client requires these
> >> metrics to be recorded."
> >>
> >> Could you elaborate this a bit more, for example, will the sensor ever
> be
> >> registered in the reporter if its level is not allowed in the client
> >> config? Or it will be registered but never polled? Or it will be
> registered
> >> and polled, but recorded?
> >>
> >> From PR 1446 it seems to be the last case, i.e. the sensor will have the
> >> default value based on its type, and it will still be polled by the
> >> reporter but not recorded. To an end-user's experience it will mean that
> >> for example the monitoring UI that displays all polled metrics will
> still
> >> show the metrics graph, with the value consistently shown as the default
> >> value, instead of not showing the graphs at all.
> >>
> >> 3. Could you elaborate on the "shouldRecord()" function, e.g. which
> class
> >> it will be added to? Does it contain any parameters?
> >>
> >>
> >> Guozhang
> >>
> >>
> >>
> >> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
> >> wrote:
> >>
> >>> Thanks for starting the discussion on these KIPs Aarti.
> >>>
> >>> Eno
> >>>
> >>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
> >>>
> >>>> Thanks Radai,
> >>>>
> >>>> Yes that is the correct link, my bad
> >>>>
> >>>>
> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>
> >>>>
> >>>>
> >>>> Aarti
> >>>>
> >>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> >>>> <javascript:;>> wrote:
> >>>>
> >>>>> link leads to 104. i think this is the correct one -
> >>>>>
> >>>>>
> >>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>> 105%3A+Addition+of+Record+Level+for+Sensors
> >>>>>
> >>>>> ?
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
> >>>> <javascript:;>>
> >>>>> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>>> Hi all,
> >>>>>
> >>>>>>
> >>>>>
> >>>>>> I would like to start the discussion on KIP-105: Addition of Record
> >>>> Level
> >>>>>
> >>>>>> for Sensors
> >>>>>
> >>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
> >>>>>
> >>>>>> <
> >>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> >>>> action?pageId=67636480
> >>>>>
> >>>>>>> *
> >>>>>
> >>>>>> *pageId=67636483*
> >>>>>
> >>>>>>
> >>>>>
> >>>>>> Looking forward to your feedback.
> >>>>>
> >>>>>>
> >>>>>
> >>>>>> Thanks,
> >>>>>
> >>>>>> Aarti and Eno
> >>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
> >>
> >>
> >> --
> >> -- Guozhang
> >>
>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Correct on 2. Guozhang: the sensor will be registered and polled by a reporter, but the metrics associated with it will not be updated.
That would allow a user to have, for example, a debug dashboard and an info dashboard.
Updated KIP to make this clear.
Thanks
Eno
> On 4 Jan 2017, at 18:00, Aarti Gupta <aa...@gmail.com> wrote:
>
> Thanks for the review, Guozhang,
>
> Addressed 2 out of the three comments,
>
> 1. Fixed and updated KIP (swapped code variable name
> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>
> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which class
> it will be added to? Does it contain any parameters?
>
> Added more details on shouldRecord() on the KIP
>
> In Sensor.java the shouldRecord() method is used to compare the value of
> metric record level in the consumer config and the RecordLevel associated
> with the sensor, to determine if metrics should recorded.
>
> From Sensor.java
>
> /**
> * @return true if the sensor's record level indicates that the metric
> will be recorded, false otherwise
> */
> public boolean shouldRecord() {
> return this.recordLevel.shouldRecord(config.recordLevel());
> }
>
> From nested enum, Sensor.RecordLevel
>
> public boolean shouldRecord(final RecordLevel configLevel) {
> if (configLevel.equals(DEBUG)) {
> return true;
> } else {
> return configLevel.equals(this);
> }
> }
>
>
> 2. Need to discuss with Eno.
>
>
> Thanks!
>
> aarti
>
>
>
>
>
> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com> wrote:
>
>> LGTM overall. A few comments below:
>>
>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name, but
>> not the real config string, I think it is `metrics.record.level` instead?
>>
>> 2. In the Motivation section, as in "This associates each sensor with a
>> record level ... only if the metric config of the client requires these
>> metrics to be recorded."
>>
>> Could you elaborate this a bit more, for example, will the sensor ever be
>> registered in the reporter if its level is not allowed in the client
>> config? Or it will be registered but never polled? Or it will be registered
>> and polled, but recorded?
>>
>> From PR 1446 it seems to be the last case, i.e. the sensor will have the
>> default value based on its type, and it will still be polled by the
>> reporter but not recorded. To an end-user's experience it will mean that
>> for example the monitoring UI that displays all polled metrics will still
>> show the metrics graph, with the value consistently shown as the default
>> value, instead of not showing the graphs at all.
>>
>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which class
>> it will be added to? Does it contain any parameters?
>>
>>
>> Guozhang
>>
>>
>>
>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
>> wrote:
>>
>>> Thanks for starting the discussion on these KIPs Aarti.
>>>
>>> Eno
>>>
>>> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
>>>
>>>> Thanks Radai,
>>>>
>>>> Yes that is the correct link, my bad
>>>>
>>>>
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>
>>>>
>>>>
>>>> Aarti
>>>>
>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
>>>> <javascript:;>> wrote:
>>>>
>>>>> link leads to 104. i think this is the correct one -
>>>>>
>>>>>
>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>
>>>>> ?
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
>>>> <javascript:;>>
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Hi all,
>>>>>
>>>>>>
>>>>>
>>>>>> I would like to start the discussion on KIP-105: Addition of Record
>>>> Level
>>>>>
>>>>>> for Sensors
>>>>>
>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>
>>>>>> <
>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>> action?pageId=67636480
>>>>>
>>>>>>> *
>>>>>
>>>>>> *pageId=67636483*
>>>>>
>>>>>>
>>>>>
>>>>>> Looking forward to your feedback.
>>>>>
>>>>>>
>>>>>
>>>>>> Thanks,
>>>>>
>>>>>> Aarti and Eno
>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>> --
>> -- Guozhang
>>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Aarti Gupta <aa...@gmail.com>.
Thanks for the review, Guozhang,
Addressed 2 out of the three comments,
1. Fixed and updated KIP (swapped code variable name
METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
3. >>Could you elaborate on the "shouldRecord()" function, e.g. which class
it will be added to? Does it contain any parameters?
Added more details on shouldRecord() on the KIP
In Sensor.java the shouldRecord() method is used to compare the value of
metric record level in the consumer config and the RecordLevel associated
with the sensor, to determine if metrics should recorded.
From Sensor.java
/**
* @return true if the sensor's record level indicates that the metric
will be recorded, false otherwise
*/
public boolean shouldRecord() {
return this.recordLevel.shouldRecord(config.recordLevel());
}
From nested enum, Sensor.RecordLevel
public boolean shouldRecord(final RecordLevel configLevel) {
if (configLevel.equals(DEBUG)) {
return true;
} else {
return configLevel.equals(this);
}
}
2. Need to discuss with Eno.
Thanks!
aarti
On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wa...@gmail.com> wrote:
> LGTM overall. A few comments below:
>
> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name, but
> not the real config string, I think it is `metrics.record.level` instead?
>
> 2. In the Motivation section, as in "This associates each sensor with a
> record level ... only if the metric config of the client requires these
> metrics to be recorded."
>
> Could you elaborate this a bit more, for example, will the sensor ever be
> registered in the reporter if its level is not allowed in the client
> config? Or it will be registered but never polled? Or it will be registered
> and polled, but recorded?
>
> From PR 1446 it seems to be the last case, i.e. the sensor will have the
> default value based on its type, and it will still be polled by the
> reporter but not recorded. To an end-user's experience it will mean that
> for example the monitoring UI that displays all polled metrics will still
> show the metrics graph, with the value consistently shown as the default
> value, instead of not showing the graphs at all.
>
> 3. Could you elaborate on the "shouldRecord()" function, e.g. which class
> it will be added to? Does it contain any parameters?
>
>
> Guozhang
>
>
>
> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com>
> wrote:
>
> > Thanks for starting the discussion on these KIPs Aarti.
> >
> > Eno
> >
> > On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
> >
> > > Thanks Radai,
> > >
> > > Yes that is the correct link, my bad
> > >
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 105%3A+Addition+of+Record+Level+for+Sensors
> > >
> > >
> > >
> > > Aarti
> > >
> > > On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> > > <javascript:;>> wrote:
> > >
> > > > link leads to 104. i think this is the correct one -
> > > >
> > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 105%3A+Addition+of+Record+Level+for+Sensors
> > > >
> > > > ?
> > > >
> > > >
> > > >
> > > > On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
> > > <javascript:;>>
> > > > wrote:
> > > >
> > > >
> > > >
> > > > > Hi all,
> > > >
> > > > >
> > > >
> > > > > I would like to start the discussion on KIP-105: Addition of Record
> > > Level
> > > >
> > > > > for Sensors
> > > >
> > > > > *https://cwiki.apache.org/confluence/pages/viewpage.action?
> > > >
> > > > > <
> > > > https://cwiki.apache.org/confluence/pages/viewpage.
> > > action?pageId=67636480
> > > >
> > > > > >*
> > > >
> > > > > *pageId=67636483*
> > > >
> > > > >
> > > >
> > > > > Looking forward to your feedback.
> > > >
> > > > >
> > > >
> > > > > Thanks,
> > > >
> > > > > Aarti and Eno
> > > >
> > > > >
> > > >
> > > >
> > >
> >
>
>
>
> --
> -- Guozhang
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Guozhang Wang <wa...@gmail.com>.
LGTM overall. A few comments below:
1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable name, but
not the real config string, I think it is `metrics.record.level` instead?
2. In the Motivation section, as in "This associates each sensor with a
record level ... only if the metric config of the client requires these
metrics to be recorded."
Could you elaborate this a bit more, for example, will the sensor ever be
registered in the reporter if its level is not allowed in the client
config? Or it will be registered but never polled? Or it will be registered
and polled, but recorded?
From PR 1446 it seems to be the last case, i.e. the sensor will have the
default value based on its type, and it will still be polled by the
reporter but not recorded. To an end-user's experience it will mean that
for example the monitoring UI that displays all polled metrics will still
show the metrics graph, with the value consistently shown as the default
value, instead of not showing the graphs at all.
3. Could you elaborate on the "shouldRecord()" function, e.g. which class
it will be added to? Does it contain any parameters?
Guozhang
On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <en...@gmail.com> wrote:
> Thanks for starting the discussion on these KIPs Aarti.
>
> Eno
>
> On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
>
> > Thanks Radai,
> >
> > Yes that is the correct link, my bad
> >
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 105%3A+Addition+of+Record+Level+for+Sensors
> >
> >
> >
> > Aarti
> >
> > On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> > <javascript:;>> wrote:
> >
> > > link leads to 104. i think this is the correct one -
> > >
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 105%3A+Addition+of+Record+Level+for+Sensors
> > >
> > > ?
> > >
> > >
> > >
> > > On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
> > <javascript:;>>
> > > wrote:
> > >
> > >
> > >
> > > > Hi all,
> > >
> > > >
> > >
> > > > I would like to start the discussion on KIP-105: Addition of Record
> > Level
> > >
> > > > for Sensors
> > >
> > > > *https://cwiki.apache.org/confluence/pages/viewpage.action?
> > >
> > > > <
> > > https://cwiki.apache.org/confluence/pages/viewpage.
> > action?pageId=67636480
> > >
> > > > >*
> > >
> > > > *pageId=67636483*
> > >
> > > >
> > >
> > > > Looking forward to your feedback.
> > >
> > > >
> > >
> > > > Thanks,
> > >
> > > > Aarti and Eno
> > >
> > > >
> > >
> > >
> >
>
--
-- Guozhang
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Eno Thereska <en...@gmail.com>.
Thanks for starting the discussion on these KIPs Aarti.
Eno
On Sunday, January 1, 2017, Aarti Gupta <aa...@gmail.com> wrote:
> Thanks Radai,
>
> Yes that is the correct link, my bad
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 105%3A+Addition+of+Record+Level+for+Sensors
>
>
>
> Aarti
>
> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
> <javascript:;>> wrote:
>
> > link leads to 104. i think this is the correct one -
> >
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 105%3A+Addition+of+Record+Level+for+Sensors
> >
> > ?
> >
> >
> >
> > On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aartiguptaa@gmail.com
> <javascript:;>>
> > wrote:
> >
> >
> >
> > > Hi all,
> >
> > >
> >
> > > I would like to start the discussion on KIP-105: Addition of Record
> Level
> >
> > > for Sensors
> >
> > > *https://cwiki.apache.org/confluence/pages/viewpage.action?
> >
> > > <
> > https://cwiki.apache.org/confluence/pages/viewpage.
> action?pageId=67636480
> >
> > > >*
> >
> > > *pageId=67636483*
> >
> > >
> >
> > > Looking forward to your feedback.
> >
> > >
> >
> > > Thanks,
> >
> > > Aarti and Eno
> >
> > >
> >
> >
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by Aarti Gupta <aa...@gmail.com>.
Thanks Radai,
Yes that is the correct link, my bad
https://cwiki.apache.org/confluence/display/KAFKA/KIP-105%3A+Addition+of+Record+Level+for+Sensors
Aarti
On Sat, Dec 31, 2016 at 9:32 PM radai <ra...@gmail.com> wrote:
> link leads to 104. i think this is the correct one -
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-105%3A+Addition+of+Record+Level+for+Sensors
>
> ?
>
>
>
> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aa...@gmail.com>
> wrote:
>
>
>
> > Hi all,
>
> >
>
> > I would like to start the discussion on KIP-105: Addition of Record Level
>
> > for Sensors
>
> > *https://cwiki.apache.org/confluence/pages/viewpage.action?
>
> > <
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=67636480
>
> > >*
>
> > *pageId=67636483*
>
> >
>
> > Looking forward to your feedback.
>
> >
>
> > Thanks,
>
> > Aarti and Eno
>
> >
>
>
Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Posted by radai <ra...@gmail.com>.
link leads to 104. i think this is the correct one -
https://cwiki.apache.org/confluence/display/KAFKA/KIP-105%3A+Addition+of+Record+Level+for+Sensors
?
On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <aa...@gmail.com> wrote:
> Hi all,
>
> I would like to start the discussion on KIP-105: Addition of Record Level
> for Sensors
> *https://cwiki.apache.org/confluence/pages/viewpage.action?
> <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=67636480
> >*
> *pageId=67636483*
>
> Looking forward to your feedback.
>
> Thanks,
> Aarti and Eno
>