kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eno Thereska <eno.there...@gmail.com>
Subject Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Date Thu, 05 Jan 2017 13:34:18 GMT
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> 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>
> 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> 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>
>>> 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> 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>
>>>> 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>
>>>>>> wrote:
>>>>>> 
>>>>>>> Thanks for starting the discussion on these KIPs Aarti.
>>>>>>> 
>>>>>>> Eno
>>>>>>> 
>>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aartiguptaa@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.rosenblatt@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 <
>> 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
>>>>>> 
>>>> 
>>>> 
>> 
>> 


Mime
View raw message