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