Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

Thanks Joel, I'll address these tomorrow, no problem. Eno > On 9 Jan 2017, at 22:04, Joel Koshy wrote: > > 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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 wrote: > To clarify an earlier point

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 I'll need to think some more about point 3. Thanks Eno > On 5 Jan 2017, at

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 wrote: > > I thought about "not regi

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 motiv

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 particul

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 th

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 conf

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

Thanks for starting the discussion on these KIPs Aarti. Eno On Sunday, January 1, 2017, Aarti Gupta 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

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 wrote: > link leads to 104. i think this is the correct one - > > > https://cwiki.apache.org/conf

Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors

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 wrote: > Hi all, > > I would like to start the discussion on KIP-105: Addition of Record Level > f

[DISCUSS] KIP-105: Addition of Record Level for Sensors

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? * *pageId=67636483* Looking forward to your feedback. Thanks, Aarti