Didn't get a chance to review this earlier, but this LGTM. Minor comments: - The current name is fine, but I would have preferred calling it RecordingLevel to RecordLevel - first thing that comes to my mind with RecordLevel is Kafka records. - Under future work: it may be useful to allow specifying different recording levels for different hierarchies of sensors (similar to logging levels for different loggers)
Thanks, Joel On Fri, Jan 6, 2017 at 2:27 AM, Ismael Juma <ism...@juma.me.uk> wrote: > 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/conf > luence/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 > > >>>> > > >> > > > > > > > >
