Thanks Eno, sounds good to me. This is indeed what I was suggesting for the initial release. Extending the `JmxReporter` to use the additional information is something that can be done later, as you said.
Ismael On Fri, Jan 6, 2017 at 10:24 AM, Eno Thereska <eno.there...@gmail.com> wrote: > To clarify an earlier point Ismael made, MetricReporter implementations > have access to the record level via KafkaMetric.config().recordLevel() > and can also retrieve the active config for the record level via the > configure() method. However, the built-in JmxReporter will not use that > information in the initial release. I've updated the KIP to reflect that. > > Thanks > Eno > > > On 6 Jan 2017, at 09:47, Eno Thereska <eno.there...@gmail.com> wrote: > > > > After considering the changes needed for not registering sensors at all, > coupled with the objective that Jay mentioned to leave open the possibility > of changing the recording level at runtime, we decided that the current way > we have approached the solution is the best way to go. The KIP focuses on > the main problem we have, which is the overhead of computing metrics. It > allows for a subsequent JIRA to have a way to change the levels at runtime > via JMX. It also allows for a subsequent JIRA to provide more tags to the > metrics reporter as Ismael had suggested (e.g., "debug", "info"). > > > > I've adjusted the KIP to reflect the above. > > > > Thanks > > Eno > > > >> On 5 Jan 2017, at 22:14, Eno Thereska <eno.there...@gmail.com <mailto: > eno.there...@gmail.com>> wrote: > >> > >> Thanks Jay, will fix the motivation. We have a microbenchmark and perf > graph in the PR: > >> https://github.com/apache/kafka/pull/1446#issuecomment-268106260 < > https://github.com/apache/kafka/pull/1446#issuecomment-268106260> > >> > >> I'll need to think some more about point 3. > >> > >> Thanks > >> Eno > >> > >>> On 5 Jan 2017, at 19:18, Jay Kreps <j...@confluent.io <mailto: > j...@confluent.io>> wrote: > >>> > >>> This is great! A couple of quick comments: > >>> > >>> 1. It'd be good to make the motivation a bit more clear. I think the > >>> motivation is "We want to have lots of partition/task/etc metrics > but we're > >>> concerned about the performance impact so we want to disable them by > >>> default." Currently the motivation section is more about the proposed > >>> change and doesn't really make clear why. > >>> 2. Do we have a microbenchmark that shows that the performance of (1) > >>> enabled metrics, (2) disabled metrics, (3) no metrics? This would > help > >>> build the case for needing this extra knob. Obviously if metrics are > cheap > >>> you would always just leave them enabled and not worry about it. I > think > >>> there should be some cost because we are at least taking a lock > during the > >>> recording but I'm not sure how material that is. > >>> 3. One consideration in how this exposed: we always found the > ability to > >>> dynamically change the logging level in JMX for log4j pretty useful. > I > >>> think if we want to leave the door open to add this ability to enable > >>> metrics at runtime it may have some impact on the decision around how > >>> metrics are registered/reported. > >>> > >>> -Jay > >>> > >>> On Thu, Jan 5, 2017 at 9:59 AM, Guozhang Wang <wangg...@gmail.com > <mailto: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 > <mailto: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 <mailto: > 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 <mailto: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 <mailto: > 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 <mailto: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 > <mailto: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 <mailto: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 <mailto: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 <mailto:aartigup...@gmail.com>> > >>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> Thanks Radai, > >>>>>>>>>>>>> > >>>>>>>>>>>>> Yes that is the correct link, my bad > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- < > 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 <mailto: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- < > 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 <mailto: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 > >>>> > >> > > > >
