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 <j...@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 <wangg...@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.there...@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 <ism...@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.there...@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 <ism...@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.there...@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 <aartigup...@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 <wangg...@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.there...@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>>>>>
>>>>>>>>>> Eno
>>>>>>>>>>
>>>>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aartigup...@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.rosenbl...@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 <
>>>>> aartigup...@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
>>
