Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
In-Reply-To: <CAD5tkZZvy72Uf=4sxiAYfE5k78Sh+einsUx3n1bSFxpCpqP_gA@mail.gmail.com>
References: <CAEhVOwHkpxkotbT72Yuc9pZM61ctEZ_sZx33YXqRY4bc8xhx3A@mail.gmail.com>
 <CANTWn9NJ8G8PamyWV8+RTYT-L_kBcCcD7y4=B8zSXk-9ai6WvQ@mail.gmail.com>
 <CAEhVOwF+u-1z+Z61r9SF4j3_Nm-_fkrj9ErDfLzSAi4DRQ=Uew@mail.gmail.com>
 <CAFX5QBqRQeBv-wGf5WhHHGtn9WRvxj8vcEWAbYY+tRFQ-kSKhA@mail.gmail.com>
 <CAHwHRrWxvn0HW34NfpBu2u4_LVeej+xFxG28YTssdiUjbcr0cQ@mail.gmail.com>
 <CAEhVOwFiQXv09NLNwziG1HQY62DHY5BKWsH37r274eQnyTVk7Q@mail.gmail.com>
 <0EE46D5C-9E68-4202-A135-4A0F25FD3DD9@gmail.com> <CAD5tkZaGURR8LZkUiogUR1q27m0NANFXEksumw1Z1f-Wxv8C5g@mail.gmail.com>
 <9E4A3C86-646D-4C86-A12B-4DB9B24BC85F@gmail.com> <CAD5tkZYiabc79zzajZ3m+3s1-3fpQyqmNubMTRB19XarPW1O5g@mail.gmail.com>
 <6305CC98-A851-4C48-B7A8-63ADD3E2E23A@gmail.com> <CAHwHRrW2UC7iWLdrBvabTNfwHdszJuw=y2vqomC-ps73r=fPdw@mail.gmail.com>
 <CAOeJiJjAZPHPOdfYBANfx35L7zXhY_euR2MYb5Cz5A1QQ=B9Og@mail.gmail.com>
 <E85442AC-D703-4FE8-8E92-19A706CC1A55@gmail.com> <1F56AF75-70F8-4332-A4CD-443B6B226FD9@gmail.com>
 <B82C4782-FF96-431B-B8F3-A66DAF3C5AF0@gmail.com> <CAD5tkZZvy72Uf=4sxiAYfE5k78Sh+einsUx3n1bSFxpCpqP_gA@mail.gmail.com>
From: Joel Koshy <jjkoshy.w@gmail.com>
Date: Mon, 9 Jan 2017 14:04:26 -0800
Message-ID: <CAAOfhrBKnajCzVPD8WL3Ldu_jhKHJnN-NPbVbHbhNKaGV7sB+w@mail.gmail.com>
Subject: Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
To: "dev@kafka.apache.org" <dev@kafka.apache.org>
Content-Type: multipart/alternative; boundary=001a1143f9b01689e30545b08d07
archived-at: Mon, 09 Jan 2017 22:04:39 -0000

--001a1143f9b01689e30545b08d07
Content-Type: text/plain; charset=UTF-8

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 <ismael@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 <eno.thereska@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 <eno.thereska@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
> > <mailto:wangguoz@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
> > <mailto:eno.thereska@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 <mailto:eno.thereska@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 <mailto: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 <aartiguptaa@gmail.com
> > <mailto:aartiguptaa@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 <mailto:wangguoz@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 <mailto:eno.thereska@gmail.com>>
> > >>>>>>>>>>> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Eno
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <
> > aartiguptaa@gmail.com <mailto:aartiguptaa@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 <mailto: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- <
> > 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 <mailto: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/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
> > >>>>
> > >>
> > >
> >
> >
>

--001a1143f9b01689e30545b08d07--