Thanks Joel, I'll address these tomorrow, no problem.
Eno
> On 9 Jan 2017, at 22:04, Joel Koshy <jjkosh...@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 <ism...@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.there...@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.there...@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.there...@gmail.com
>> <mailto:
>>> eno.there...@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 <j...@confluent.io <mailto:
>>> 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
>>> <mailto: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
>>> <mailto: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 <mailto:
>>> 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 <mailto: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
>> <mailto:
>>> 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 <mailto: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
>>> <mailto: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 <mailto: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 <mailto: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 <mailto:aartigup...@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.rosenbl...@gmail.com <mailto: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- <
>>> 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 <mailto: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/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
>>>>>>>
>>>>>
>>>>
>>>
>>>
>>
