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