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
