Guozhang,
I was thinking to do this in Sensor.java, and not touch the MetricsReporter
interface. Basically I'd go for not adding a KafkaMetric at all with this
approach. But perhaps I'm missing something.
Thanks
Eno
> On 5 Jan 2017, at 17:59, 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
