kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Kreps <...@confluent.io>
Subject Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Date Thu, 05 Jan 2017 19:18:10 GMT
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> 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>
> 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> 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>
> > > 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> 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>
> > >>> 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>
> 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>
> > >>>> 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>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> Thanks for starting the discussion on these KIPs Aarti.
> > >>>>>>>
> > >>>>>>> Eno
> > >>>>>>>
> > >>>>>>> On Sunday, January 1, 2017, Aarti Gupta <aartiguptaa@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.rosenblatt@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
<
> > >> 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/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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message