kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eno Thereska <eno.there...@gmail.com>
Subject Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Date Mon, 09 Jan 2017 22:30:47 GMT
Thanks Joel, I'll address these tomorrow, no problem.

Eno
> On 9 Jan 2017, at 22:04, Joel Koshy <jjkoshy.w@gmail.com> 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 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 <ismael@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.thereska@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.thereska@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.thereska@gmail.com
>> <mailto:
>>> eno.thereska@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 <jay@confluent.io <mailto:
>>> jay@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 <wangguoz@gmail.com
>>> <mailto:wangguoz@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.thereska@gmail.com
>>> <mailto:eno.thereska@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 <ismael@juma.me.uk
<mailto:
>>> ismael@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.thereska@gmail.com <mailto:eno.thereska@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 <ismael@juma.me.uk
>> <mailto:
>>> ismael@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.thereska@gmail.com <mailto:eno.thereska@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
<aartiguptaa@gmail.com
>>> <mailto:aartiguptaa@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 <
>>> wangguoz@gmail.com <mailto:wangguoz@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.thereska@gmail.com <mailto:eno.thereska@gmail.com>>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Thanks for starting the discussion
on these KIPs Aarti.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Eno
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> On Sunday, January 1, 2017, Aarti
Gupta <
>>> aartiguptaa@gmail.com <mailto:aartiguptaa@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.rosenblatt@gmail.com <mailto:radai.rosenblatt@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 <
>>>>>>>>>> aartiguptaa@gmail.com <mailto:aartiguptaa@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
>>>>>>> 
>>>>> 
>>>> 
>>> 
>>> 
>> 


Mime
View raw message