Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
From: Eno Thereska <eno.thereska@gmail.com>
Content-Type: multipart/alternative;
 boundary="Apple-Mail=_FCD9C9A1-2A17-4A86-94F9-33F1F68F51EC"
Mime-Version: 1.0 (Mac OS X Mail 10.2 \(3259\))
Subject: Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Date: Fri, 6 Jan 2017 10:24:33 +0000
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>
To: dev@kafka.apache.org
In-Reply-To: <1F56AF75-70F8-4332-A4CD-443B6B226FD9@gmail.com>
Message-Id: <B82C4782-FF96-431B-B8F3-A66DAF3C5AF0@gmail.com>
archived-at: Fri, 06 Jan 2017 10:24:52 -0000

--Apple-Mail=_FCD9C9A1-2A17-4A86-94F9-33F1F68F51EC
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

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:
>=20
> 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").
>=20
> I've adjusted the KIP to reflect the above.
>=20
> Thanks
> Eno
>=20
>> On 5 Jan 2017, at 22:14, Eno Thereska <eno.thereska@gmail.com =
<mailto:eno.thereska@gmail.com>> wrote:
>>=20
>> 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>
>>=20
>> I'll need to think some more about point 3.
>>=20
>> Thanks
>> Eno
>>=20
>>> On 5 Jan 2017, at 19:18, Jay Kreps <jay@confluent.io =
<mailto:jay@confluent.io>> wrote:
>>>=20
>>> This is great! A couple of quick comments:
>>>=20
>>>   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.
>>>=20
>>> -Jay
>>>=20
>>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangguoz@gmail.com =
<mailto:wangguoz@gmail.com>> wrote:
>>>=20
>>>> 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.
>>>>=20
>>>>=20
>>>> Guozhang
>>>>=20
>>>> On Thu, Jan 5, 2017 at 5:34 AM, Eno Thereska =
<eno.thereska@gmail.com <mailto:eno.thereska@gmail.com>>
>>>> wrote:
>>>>=20
>>>>> Updated KIP for 1. Waiting to hear from Guozhang on 2 and then we =
can
>>>>> proceed.
>>>>>=20
>>>>> Thanks
>>>>> Eno
>>>>>> On 5 Jan 2017, at 12:27, Ismael Juma <ismael@juma.me.uk =
<mailto:ismael@juma.me.uk>> wrote:
>>>>>>=20
>>>>>> Thanks Eno. It would be good to update the KIP as well with =
regards to
>>>> 1.
>>>>>>=20
>>>>>> 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.
>>>>>>=20
>>>>>> Ismael
>>>>>>=20
>>>>>> On Thu, Jan 5, 2017 at 12:07 PM, Eno Thereska =
<eno.thereska@gmail.com <mailto:eno.thereska@gmail.com>>
>>>>>> wrote:
>>>>>>=20
>>>>>>> Thanks Ismael. Already addressed 1. in the PR.
>>>>>>>=20
>>>>>>> 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.
>>>>>>>=20
>>>>>>> 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.
>>>>>>>=20
>>>>>>> Eno
>>>>>>>=20
>>>>>>>=20
>>>>>>>> On 5 Jan 2017, at 11:23, Ismael Juma <ismael@juma.me.uk =
<mailto:ismael@juma.me.uk>> wrote:
>>>>>>>>=20
>>>>>>>> Thanks for the KIP, it seems like a good improvement. A couple =
of
>>>>>>> comments:
>>>>>>>>=20
>>>>>>>> 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.
>>>>>>>>=20
>>>>>>>> 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).
>>>>>>>>=20
>>>>>>>> Ismael
>>>>>>>>=20
>>>>>>>> On Thu, Jan 5, 2017 at 10:37 AM, Eno Thereska <
>>>> eno.thereska@gmail.com <mailto:eno.thereska@gmail.com>>
>>>>>>>> wrote:
>>>>>>>>=20
>>>>>>>>> Correct on 2. Guozhang: the sensor will be registered and =
polled by
>>>> a
>>>>>>>>> reporter, but the metrics associated with it will not be =
updated.
>>>>>>>>>=20
>>>>>>>>> That would allow a user to have, for example, a debug =
dashboard and
>>>> an
>>>>>>>>> info dashboard.
>>>>>>>>>=20
>>>>>>>>> Updated KIP to make this clear.
>>>>>>>>>=20
>>>>>>>>> Thanks
>>>>>>>>> Eno
>>>>>>>>>=20
>>>>>>>>>=20
>>>>>>>>>> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com =
<mailto:aartiguptaa@gmail.com>>
>>>> wrote:
>>>>>>>>>>=20
>>>>>>>>>> Thanks for the review, Guozhang,
>>>>>>>>>>=20
>>>>>>>>>> Addressed 2 out of the three comments,
>>>>>>>>>>=20
>>>>>>>>>> 1. Fixed and updated KIP (swapped code variable name
>>>>>>>>>> METRICS_RECORD_LEVEL_CONFIG with config name =
metrics.record.level)
>>>>>>>>>>=20
>>>>>>>>>> 3. >>Could you elaborate on the "shouldRecord()" function, =
e.g.
>>>> which
>>>>>>>>> class
>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>=20
>>>>>>>>>> Added more details on shouldRecord()  on the KIP
>>>>>>>>>>=20
>>>>>>>>>> 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.
>>>>>>>>>>=20
>>>>>>>>>> =46rom Sensor.java
>>>>>>>>>>=20
>>>>>>>>>> /**
>>>>>>>>>> * @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());
>>>>>>>>>> }
>>>>>>>>>>=20
>>>>>>>>>> =46rom nested enum, Sensor.RecordLevel
>>>>>>>>>>=20
>>>>>>>>>> public boolean shouldRecord(final RecordLevel configLevel) {
>>>>>>>>>> if (configLevel.equals(DEBUG)) {
>>>>>>>>>>     return true;
>>>>>>>>>> } else {
>>>>>>>>>>     return configLevel.equals(this);
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>> 2. Need to discuss with Eno.
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>> Thanks!
>>>>>>>>>>=20
>>>>>>>>>> aarti
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>>=20
>>>>>>>>>> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang =
<wangguoz@gmail.com <mailto:wangguoz@gmail.com>>
>>>>>>>>> wrote:
>>>>>>>>>>=20
>>>>>>>>>>> LGTM overall. A few comments below:
>>>>>>>>>>>=20
>>>>>>>>>>> 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?
>>>>>>>>>>>=20
>>>>>>>>>>> 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."
>>>>>>>>>>>=20
>>>>>>>>>>> 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?
>>>>>>>>>>>=20
>>>>>>>>>>> =46rom 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.
>>>>>>>>>>>=20
>>>>>>>>>>> 3. Could you elaborate on the "shouldRecord()" function, =
e.g.
>>>> which
>>>>>>>>> class
>>>>>>>>>>> it will be added to? Does it contain any parameters?
>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>> Guozhang
>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <
>>>>> eno.thereska@gmail.com <mailto:eno.thereska@gmail.com>>
>>>>>>>>>>> wrote:
>>>>>>>>>>>=20
>>>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>>>=20
>>>>>>>>>>>> Eno
>>>>>>>>>>>>=20
>>>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta =
<aartiguptaa@gmail.com <mailto:aartiguptaa@gmail.com>>
>>>>>>> wrote:
>>>>>>>>>>>>=20
>>>>>>>>>>>>> Thanks Radai,
>>>>>>>>>>>>>=20
>>>>>>>>>>>>> Yes that is the correct link, my bad
>>>>>>>>>>>>>=20
>>>>>>>>>>>>>=20
>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- =
<https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>=20
>>>>>>>>>>>>>=20
>>>>>>>>>>>>>=20
>>>>>>>>>>>>> Aarti
>>>>>>>>>>>>>=20
>>>>>>>>>>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <
>>>> radai.rosenblatt@gmail.com <mailto:radai.rosenblatt@gmail.com>
>>>>>>>>>>>>> <javascript:;>> wrote:
>>>>>>>>>>>>>=20
>>>>>>>>>>>>>> link leads to 104. i think this is the correct one -
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- =
<https://cwiki.apache.org/confluence/display/KAFKA/KIP->
>>>>>>>>>>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>> ?
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta <
>>>>>>> aartiguptaa@gmail.com <mailto:aartiguptaa@gmail.com>
>>>>>>>>>>>>> <javascript:;>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> I would like to start the discussion on KIP-105: =
Addition of
>>>>>>> Record
>>>>>>>>>>>>> Level
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> for Sensors
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> =
*https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>>> action?pageId=3D67636480
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> *pageId=3D67636483*
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>>=20
>>>>>>>>>>>>>=20
>>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>>=20
>>>>>>>>>>> --
>>>>>>>>>>> -- Guozhang
>>>>>>>>>>>=20
>>>>>>>>>=20
>>>>>>>>>=20
>>>>>>>=20
>>>>>>>=20
>>>>>=20
>>>>>=20
>>>>=20
>>>>=20
>>>> --
>>>> -- Guozhang
>>>>=20
>>=20
>=20


--Apple-Mail=_FCD9C9A1-2A17-4A86-94F9-33F1F68F51EC--