Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
Sender: ismaelj@gmail.com
In-Reply-To: <B82C4782-FF96-431B-B8F3-A66DAF3C5AF0@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>
From: Ismael Juma <ismael@juma.me.uk>
Date: Fri, 6 Jan 2017 10:27:56 +0000
Message-ID: <CAD5tkZZvy72Uf=4sxiAYfE5k78Sh+einsUx3n1bSFxpCpqP_gA@mail.gmail.com>
Subject: Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
To: dev@kafka.apache.org
Content-Type: multipart/alternative; boundary=001a114b45bc12d00e05456a7bc8
archived-at: Fri, 06 Jan 2017 10:28:50 -0000

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

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/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
> >>>>
> >>
> >
>
>

--001a114b45bc12d00e05456a7bc8--