kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Guozhang Wang <wangg...@gmail.com>
Subject Re: [DISCUSS] KIP-274: Kafka Streams Skipped Records Metrics
Date Tue, 03 Apr 2018 21:45:05 GMT
Thanks John, your proposal looks fine to me.

I'll go ahead and look into the PR for more details myself.


Guozhang

On Tue, Apr 3, 2018 at 1:35 PM, Bill Bejeck <bbejeck@gmail.com> wrote:

> Hi John,
>
> Thanks for making the updates.
>
> I agree with the information you've included in the logs as described
> above, as log statements without enough context/information can be
> frustrating.
>
> -Bill
>
> On Tue, Apr 3, 2018 at 3:29 PM, John Roesler <john@confluent.io> wrote:
>
> > Allrighty, how about this, then...
> >
> > I'll move the metric back to the StreamThread and maintain the existing
> tag
> > (client-id=...(per-thread client-id)). It won't be present in the
> > TopologyTestDriver's metrics.
> >
> > As a side note, I'm not sure that the location of the log messages has
> > visibility into the name of the thread or the task, or the processor
> node,
> > for that matter. But at the end of the day, I don't think it really
> > matters.
> >
> > None of those identifiers are in the public interface or user-controlled.
> > For them to be useful for debugging, users would have to gain a very deep
> > understanding of how their DSL program gets executed. From my
> perspective,
> > they are all included in metric tags only to prevent collisions between
> the
> > same metrics in different (e.g.) threads.
> >
> > I think what's important is to provide the right information in the logs
> > that users will be able to debug their issues. This is why the logs in my
> > pr include the topic/partition/offset of the offending data, as well as
> the
> > stacktrace of the exception from the deserializer (or for timestamps, the
> > extracted timestamp and the class name of their extractor). This
> > information alone should let them pinpoint the offending data and fix it.
> >
> > (I am aware that that topic name might be a repartition topic, and
> > therefore also esoteric from the user's perspective, but I think it's the
> > best we can do right now. It might be nice to explicitly take on a
> > debugging ergonomics task in the future and give all processor nodes
> > human-friendly names. Then, we could surface these names in any logs or
> > exceptions. But I'm inclined to call this out-of-scope for now.)
> >
> > Thanks again,
> > -John
> >
> > On Tue, Apr 3, 2018 at 1:40 PM, Guozhang Wang <wangguoz@gmail.com>
> wrote:
> >
> > > 1. If we can indeed gather all the context information from the log4j
> > > entries I'd suggest we change to thread-level (I'm not sure if that is
> > > doable, so if John have already some WIP PR that can help us decide).
> > >
> > > 2. We can consider adding the API in TopologyTestDriver for general
> > testing
> > > purposes; that being said, I think Matthias has a good point that this
> > > alone should not be a driving motivation for us to keep this metric as
> > > task-level if 1) is true.
> > >
> > >
> > >
> > > Guozhang
> > >
> > >
> > > On Tue, Apr 3, 2018 at 11:36 AM, Matthias J. Sax <
> matthias@confluent.io>
> > > wrote:
> > >
> > > > Thanks Guozhang, that was my intent.
> > > >
> > > > @John: yes, we should not nail down the exact log message. It's just
> to
> > > > point out the trade-off. If we can get the required information in
> the
> > > > logs, we might not need task level metrics.
> > > >
> > > >
> > > > -Matthias
> > > >
> > > > On 4/3/18 11:26 AM, Guozhang Wang wrote:
> > > > > I think Matthias' comment is that, we can still record the metrics
> on
> > > the
> > > > > thread-level, while having the WARN log entry to include sufficient
> > > > context
> > > > > information so that users can still easily narrow down the
> > > investigation
> > > > > scope.
> > > > >
> > > > >
> > > > > Guozhang
> > > > >
> > > > > On Tue, Apr 3, 2018 at 11:22 AM, John Roesler <john@confluent.io>
> > > wrote:
> > > > >
> > > > >> I agree we should add as much information as is reasonable to the
> > log.
> > > > For
> > > > >> example, see this WIP PR I started for this KIP:
> > > > >>
> > > > >> https://github.com/apache/kafka/pull/4812/files#diff-
> > > > >> 88d129f048bc842c7db5b2566a45fce8R80
> > > > >>
> > > > >> and
> > > > >>
> > > > >> https://github.com/apache/kafka/pull/4812/files#diff-
> > > > >> 69e6789eb675ec978a1abd24fed96eb1R111
> > > > >>
> > > > >> I'm not sure if we should nail down the log messages in the KIP or
> > in
> > > > the
> > > > >> PR discussion. What say you?
> > > > >>
> > > > >> Thanks,
> > > > >> -John
> > > > >>
> > > > >> On Tue, Apr 3, 2018 at 12:20 AM, Matthias J. Sax <
> > > matthias@confluent.io
> > > > >
> > > > >> wrote:
> > > > >>
> > > > >>> Thanks for sharing your thoughts. As I mentioned originally, I am
> > not
> > > > >>> sure about the right log level either. Your arguments are
> > convincing
> > > --
> > > > >>> thus, I am fine with keeping WARN level.
> > > > >>>
> > > > >>> The task vs thread level argument is an interesting one.
> However, I
> > > am
> > > > >>> wondering if we should add this information into the
> corresponding
> > > WARN
> > > > >>> logs that we write anyway? For this case, we can also log the
> > > > >>> corresponding operator (and other information like topic name etc
> > if
> > > > >>> needed). WDYT about this?
> > > > >>>
> > > > >>>
> > > > >>> -Matthias
> > > > >>>
> > > > >>> On 4/2/18 8:31 PM, Guozhang Wang wrote:
> > > > >>>> Regarding logging: I'm inclined to keep logging at WARN level
> > since
> > > > >>> skipped
> > > > >>>> records are not expected in normal execution (for all reasons
> that
> > > we
> > > > >> are
> > > > >>>> aware of), and hence when error happens users should be alerted
> > from
> > > > >>>> metrics and looked into the log files, so to me if it is really
> > > > >> spamming
> > > > >>>> the log files it is also a good alert for users. Besides for
> > > > >> deserialize
> > > > >>>> errors we already log at WARN level for this reason.
> > > > >>>>
> > > > >>>> Regarding the metrics-levels: I was pondering on that as well.
> > What
> > > > >> made
> > > > >>> me
> > > > >>>> to think and agree on task-level than thread-level is that for
> > some
> > > > >>> reasons
> > > > >>>> like window retention, they may possibly be happening on a
> subset
> > of
> > > > >>> input
> > > > >>>> partitions, and tasks are correlated with partitions the
> > task-level
> > > > >>> metrics
> > > > >>>> can help users to narrow down on the specific input data
> > partitions.
> > > > >>>>
> > > > >>>>
> > > > >>>> Guozhang
> > > > >>>>
> > > > >>>>
> > > > >>>> On Mon, Apr 2, 2018 at 6:43 PM, John Roesler <john@confluent.io
> >
> > > > >> wrote:
> > > > >>>>
> > > > >>>>> Hi Matthias,
> > > > >>>>>
> > > > >>>>> No worries! Thanks for the reply.
> > > > >>>>>
> > > > >>>>> 1) There isn't a connection. I tried using the
> TopologyTestDriver
> > > to
> > > > >>> write
> > > > >>>>> a quick test exercising the current behavior and discovered
> that
> > > the
> > > > >>>>> metrics weren't available. It seemed like they should be, so I
> > > tacked
> > > > >>> it on
> > > > >>>>> to this KIP. If you feel it's inappropriate, I can pull it back
> > > out.
> > > > >>>>>
> > > > >>>>> 2) I was also concerned about that, but I figured it would come
> > up
> > > in
> > > > >>>>> discussion if I just went ahead and proposed it. And here we
> are!
> > > > >>>>>
> > > > >>>>> Here's my thought: maybe there are two classes of skips:
> > > "controlled"
> > > > >>> and
> > > > >>>>> "uncontrolled", where "controlled" means, as an app author, I
> > > > >>> deliberately
> > > > >>>>> filter out some events, and "uncontrolled" means that I simply
> > > don't
> > > > >>>>> account for some feature of the data, and the framework skips
> > them
> > > > (as
> > > > >>>>> opposed to crashing).
> > > > >>>>>
> > > > >>>>> In this breakdowns, the skips I'm adding metrics for are all
> > > > >>> uncontrolled
> > > > >>>>> skips (and we hope to measure all the uncontrolled skips). Our
> > > skips
> > > > >> are
> > > > >>>>> well documented, so it wouldn't be terrible to have an
> > application
> > > in
> > > > >>> which
> > > > >>>>> you know you expect to have tons of uncontrolled skips, but
> it's
> > > not
> > > > >>> great
> > > > >>>>> either, since you may also have some *unexpected* uncontrolled
> > > skips.
> > > > >>> It'll
> > > > >>>>> be difficult to notice, since you're probably not alerting on
> the
> > > > >> metric
> > > > >>>>> and filtering out the logs (whatever their level).
> > > > >>>>>
> > > > >>>>> I'd recommend any app author, as an alternative, to convert all
> > > > >> expected
> > > > >>>>> skips to controlled ones, by updating the topology to filter
> > those
> > > > >>> records
> > > > >>>>> out.
> > > > >>>>>
> > > > >>>>> Following from my recommendation, as a library author, I'm
> > inclined
> > > > to
> > > > >>> mark
> > > > >>>>> those logs WARN, since in my opinion, they should be concerning
> > to
> > > > the
> > > > >>> app
> > > > >>>>> authors. I'd definitely want to show, rather than hide, them by
> > > > >>> default, so
> > > > >>>>> I would pick INFO at least.
> > > > >>>>>
> > > > >>>>> That said, logging is always a tricky issue for lower-level
> > > libraries
> > > > >>> that
> > > > >>>>> run inside user code, since we don't have all the information
> we
> > > need
> > > > >> to
> > > > >>>>> make the right call.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> On your last note, yeah, I got that impression from Guozhang as
> > > well.
> > > > >>>>> Thanks for the clarification.
> > > > >>>>>
> > > > >>>>> -John
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> On Mon, Apr 2, 2018 at 4:03 PM, Matthias J. Sax <
> > > > >> matthias@confluent.io>
> > > > >>>>> wrote:
> > > > >>>>>
> > > > >>>>>> John,
> > > > >>>>>>
> > > > >>>>>> sorry for my late reply and thanks for updating the KIP.
> > > > >>>>>>
> > > > >>>>>> I like your approach about "metrics are for monitoring, logs
> are
> > > for
> > > > >>>>>> debugging" -- however:
> > > > >>>>>>
> > > > >>>>>> 1) I don't see a connection between this and the task-level
> > > metrics
> > > > >>> that
> > > > >>>>>> you propose to get the metrics in `TopologyTestDriver`. I
> don't
> > > > think
> > > > >>>>>> people would monitor the `TopologyTestDriver` an thus
> wondering
> > > why
> > > > >> it
> > > > >>>>>> is important to include the metrics there? Thread-level metric
> > > might
> > > > >> be
> > > > >>>>>> easier to monitor though (ie, less different metric to
> monitor).
> > > > >>>>>>
> > > > >>>>>> 2) I am a little worried about WARN level logging and that it
> > > might
> > > > >> be
> > > > >>>>>> too chatty -- as you pointed out, it's about debugging, thus
> > DEBUG
> > > > >>> level
> > > > >>>>>> might be better. Not 100% sure about this to be honest. What
> is
> > > the
> > > > >>>>>> general assumption about the frequency for skipped records? I
> > > could
> > > > >>>>>> imagine cases for which skipped records are quite frequent and
> > > thus,
> > > > >>>>>> WARN level logs might "flood" the logs
> > > > >>>>>>
> > > > >>>>>> One final remark:
> > > > >>>>>>
> > > > >>>>>>> More
> > > > >>>>>>> generally, I would like to establish a pattern in which we
> > could
> > > > add
> > > > >>>>> new
> > > > >>>>>>> values for the "reason" tags without needing a KIP to do so.
> > > > >>>>>>
> > > > >>>>>> From my understanding, this is not feasible. Changing metrics
> is
> > > > >> always
> > > > >>>>>> considered a public API change, and we need a KIP for any
> > change.
> > > As
> > > > >> we
> > > > >>>>>> moved away from tagging, it doesn't matter for the KIP anymore
> > --
> > > > >> just
> > > > >>>>>> wanted to point it out.
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>> -Matthias
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>> On 3/30/18 2:47 PM, John Roesler wrote:
> > > > >>>>>>> Allrighty! The KIP is updated.
> > > > >>>>>>>
> > > > >>>>>>> Thanks again, all, for the feedback.
> > > > >>>>>>> -John
> > > > >>>>>>>
> > > > >>>>>>> On Fri, Mar 30, 2018 at 3:35 PM, John Roesler <
> > john@confluent.io
> > > >
> > > > >>>>> wrote:
> > > > >>>>>>>
> > > > >>>>>>>> Hey Guozhang and Bill,
> > > > >>>>>>>>
> > > > >>>>>>>> Ok, I'll update the KIP. At the risk of disturbing
> consensus,
> > > I'd
> > > > >>> like
> > > > >>>>>> to
> > > > >>>>>>>> put it in the task instead of the thread so that it'll show
> up
> > > in
> > > > >> the
> > > > >>>>>>>> TopologyTestDriver metrics as well.
> > > > >>>>>>>>
> > > > >>>>>>>> I'm leaning toward keeping the scope where it is right now,
> > but
> > > if
> > > > >>>>>> others
> > > > >>>>>>>> want to advocate for tossing in some more metrics, we can go
> > > that
> > > > >>>>> route.
> > > > >>>>>>>>
> > > > >>>>>>>> Thanks all,
> > > > >>>>>>>> -John
> > > > >>>>>>>>
> > > > >>>>>>>> On Fri, Mar 30, 2018 at 2:37 PM, Bill Bejeck <
> > bbejeck@gmail.com
> > > >
> > > > >>>>> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>>> Thanks for the KIP John, and sorry for the late comments.
> > > > >>>>>>>>>
> > > > >>>>>>>>> I'm on the fence with providing a single level metrics,
> but I
> > > > >> think
> > > > >>>>>> we'll
> > > > >>>>>>>>> have that discussion outside of this KIP.
> > > > >>>>>>>>>
> > > > >>>>>>>>>> * maintain one skipped-record metric (could be per-thread,
> > > > >>> per-task,
> > > > >>>>>> or
> > > > >>>>>>>>>> per-processor-node) with no "reason"
> > > > >>>>>>>>>> * introduce a warn-level log detailing the
> > > > topic/partition/offset
> > > > >>>>> and
> > > > >>>>>>>>>> reason of the skipped record
> > > > >>>>>>>>>
> > > > >>>>>>>>> I'm +1 on both of these suggestions.
> > > > >>>>>>>>>
> > > > >>>>>>>>> Finally, we have had requests in the past for some metrics
> > > around
> > > > >>>>> when
> > > > >>>>>>>>> persistent store removes an expired window.  Would adding
> > that
> > > to
> > > > >>> our
> > > > >>>>>>>>> metrics stretch the scope of this KIP too much?
> > > > >>>>>>>>>
> > > > >>>>>>>>> Thanks again and overall I'm +1 on this KIP
> > > > >>>>>>>>>
> > > > >>>>>>>>> Bill
> > > > >>>>>>>>>
> > > > >>>>>>>>> On Fri, Mar 30, 2018 at 2:00 PM, Guozhang Wang <
> > > > >> wangguoz@gmail.com>
> > > > >>>>>>>>> wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>>> The proposal sounds good to me. About "maintain only one
> > level
> > > > of
> > > > >>>>>>>>> metrics"
> > > > >>>>>>>>>> maybe we can discuss about that separately from this KIP
> > since
> > > > >> that
> > > > >>>>>>>>> would
> > > > >>>>>>>>>> be a larger scope of discussion. I agree that if we are
> > going
> > > to
> > > > >>>>>>>>> maintain
> > > > >>>>>>>>>> only one-level metrics it should be lowest level and we
> > would
> > > > let
> > > > >>>>>> users
> > > > >>>>>>>>> to
> > > > >>>>>>>>>> do the roll-ups themselves, but I'm still not fully
> > convinced
> > > > >> that
> > > > >>>>> we
> > > > >>>>>>>>>> should just provide single-level metrics, because 1) I
> think
> > > for
> > > > >>>>>>>>> different
> > > > >>>>>>>>>> metrics people may be interested to investigate into
> > different
> > > > >>>>>>>>>> granularities, e.g. for poll / commit rate these are at
> the
> > > > >> lowest
> > > > >>>>>>>>>> task-level metrics, while for process-rate / skip-rate
> they
> > > can
> > > > >> be
> > > > >>>>> as
> > > > >>>>>>>>> low
> > > > >>>>>>>>>> as processor-node metrics, and 2) user-side rolling ups
> may
> > > not
> > > > >> be
> > > > >>>>>> very
> > > > >>>>>>>>>> straight-forward. But for 2) if someone can provide an
> > > efficient
> > > > >>> and
> > > > >>>>>>>>> easy
> > > > >>>>>>>>>> implementation of that I can be persuaded :)
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> For now I'm thinking we can add the metric on
> thread-level,
> > > > >> either
> > > > >>>>>> with
> > > > >>>>>>>>>> finer grained ones with "reason" tag plus an aggregated
> one
> > > > >> without
> > > > >>>>>> the
> > > > >>>>>>>>>> tag, or just having a single aggregated metric without the
> > tag
> > > > >>> looks
> > > > >>>>>>>>> good
> > > > >>>>>>>>>> to me.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Guozhang
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> On Fri, Mar 30, 2018 at 8:05 AM, John Roesler <
> > > > john@confluent.io
> > > > >>>
> > > > >>>>>>>>> wrote:
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>> Hey Guozhang,
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Thanks for the reply. Regarding JMX, I can dig it. I'll
> > > provide
> > > > >> a
> > > > >>>>>>>>> list in
> > > > >>>>>>>>>>> the KIP. I was also thinking we'd better start a
> > > documentation
> > > > >>> page
> > > > >>>>>>>>> with
> > > > >>>>>>>>>>> the metrics listed.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> I'd have no problem logging a warning when we skip
> records.
> > > On
> > > > >> the
> > > > >>>>>>>>> metric
> > > > >>>>>>>>>>> front, really I'm just pushing for us to maintain only
> one
> > > > level
> > > > >>> of
> > > > >>>>>>>>>>> metrics. If that's more or less granular (i.e., maybe we
> > > don't
> > > > >>>>> have a
> > > > >>>>>>>>>>> metric per reason and log the reason instead), that's
> fine
> > by
> > > > >> me.
> > > > >>> I
> > > > >>>>>>>>> just
> > > > >>>>>>>>>>> don't think it provides a lot of extra value per
> complexity
> > > > >>>>>> (interface
> > > > >>>>>>>>>> and
> > > > >>>>>>>>>>> implementation) to maintain roll-ups at the thread level
> in
> > > > >>>>> addition
> > > > >>>>>>>>> to
> > > > >>>>>>>>>>> lower-level metrics.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> How about this instead:
> > > > >>>>>>>>>>> * maintain one skipped-record metric (could be
> per-thread,
> > > > >>>>> per-task,
> > > > >>>>>>>>> or
> > > > >>>>>>>>>>> per-processor-node) with no "reason"
> > > > >>>>>>>>>>> * introduce a warn-level log detailing the
> > > > >> topic/partition/offset
> > > > >>>>> and
> > > > >>>>>>>>>>> reason of the skipped record
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> If you like that, I can update the KIP.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Thanks,
> > > > >>>>>>>>>>> -John
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> On Thu, Mar 29, 2018 at 6:22 PM, Guozhang Wang <
> > > > >>> wangguoz@gmail.com
> > > > >>>>>>
> > > > >>>>>>>>>> wrote:
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>>>> One thing you mention is the notion of setting alerts
> on
> > > > >> coarser
> > > > >>>>>>>>>>> metrics
> > > > >>>>>>>>>>>> being easier than finer ones. All the metric alerting
> > > systems
> > > > I
> > > > >>>>> have
> > > > >>>>>>>>>> used
> > > > >>>>>>>>>>>> make it equally easy to alert on metrics by-tag or over
> > > tags.
> > > > >> So
> > > > >>>>> my
> > > > >>>>>>>>>>>> experience doesn't say that this is a use case. Were you
> > > > >> thinking
> > > > >>>>>>>>> of an
> > > > >>>>>>>>>>>> alerting system that makes such a pre-aggregation
> > valuable?
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> For the commonly used JMX reporter tags will be encoded
> > > > >> directly
> > > > >>>>> as
> > > > >>>>>>>>>> part
> > > > >>>>>>>>>>> of
> > > > >>>>>>>>>>>> the object name, and if users wants to monitor them they
> > > need
> > > > >> to
> > > > >>>>>>>>> know
> > > > >>>>>>>>>>> these
> > > > >>>>>>>>>>>> values before hand. That is also why I think we do want
> to
> > > > list
> > > > >>>>> all
> > > > >>>>>>>>> the
> > > > >>>>>>>>>>>> possible values of the reason tags in the KIP, since
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>> In my email in response to Matthias, I gave an example
> of
> > > the
> > > > >>>>>>>>> kind of
> > > > >>>>>>>>>>>> scenario that would lead me as an operator to run with
> > DEBUG
> > > > on
> > > > >>>>> all
> > > > >>>>>>>>> the
> > > > >>>>>>>>>>>> time, since I wouldn't be sure, having seen a skipped
> > record
> > > > >>> once,
> > > > >>>>>>>>> that
> > > > >>>>>>>>>>> it
> > > > >>>>>>>>>>>> would ever happen again. The solution is to capture all
> > the
> > > > >>>>>>>>> available
> > > > >>>>>>>>>>>> information about the reason and location of skips all
> the
> > > > >> time.
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> That is a good point. I think we can either expose all
> > > levels
> > > > >>>>>>>>> metrics
> > > > >>>>>>>>>> as
> > > > >>>>>>>>>>> by
> > > > >>>>>>>>>>>> default, or only expose the most lower-level metrics and
> > get
> > > > >> rid
> > > > >>>>> of
> > > > >>>>>>>>>> other
> > > > >>>>>>>>>>>> levels to let users do roll-ups themselves (which will
> be
> > a
> > > > >> much
> > > > >>>>>>>>> larger
> > > > >>>>>>>>>>>> scope for discussion), or we can encourage users to not
> > > purely
> > > > >>>>>>>>> depend
> > > > >>>>>>>>>> on
> > > > >>>>>>>>>>>> metrics for such trouble shooting: that is to say, users
> > > only
> > > > >> be
> > > > >>>>>>>>>> alerted
> > > > >>>>>>>>>>>> based on metrics, and we can log a info / warn log4j
> entry
> > > > each
> > > > >>>>>>>>> time we
> > > > >>>>>>>>>>> are
> > > > >>>>>>>>>>>> about to skip a record all over the places, so that upon
> > > being
> > > > >>>>>>>>> notified
> > > > >>>>>>>>>>>> users can look into the logs to find the details on
> where
> > /
> > > > >> when
> > > > >>>>> it
> > > > >>>>>>>>>>>> happens. WDYT?
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> Guozhang
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> On Thu, Mar 29, 2018 at 3:57 PM, John Roesler <
> > > > >> john@confluent.io
> > > > >>>>
> > > > >>>>>>>>>> wrote:
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>> Hey Guozhang,
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> Thanks for the review.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> 1.
> > > > >>>>>>>>>>>>> Matthias raised the same question about the "reason"
> tag
> > > > >> values.
> > > > >>>>> I
> > > > >>>>>>>>>> can
> > > > >>>>>>>>>>>> list
> > > > >>>>>>>>>>>>> all possible values of the "reason" tag, but I'm
> thinking
> > > > this
> > > > >>>>>>>>> level
> > > > >>>>>>>>>> of
> > > > >>>>>>>>>>>>> detail may not be KIP-worthy, maybe the code and
> > > > documentation
> > > > >>>>>>>>> review
> > > > >>>>>>>>>>>> would
> > > > >>>>>>>>>>>>> be sufficient. If you all disagree and would like it
> > > included
> > > > >> in
> > > > >>>>>>>>> the
> > > > >>>>>>>>>>>> KIP, I
> > > > >>>>>>>>>>>>> can certainly do that.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> If we do provide roll-up metrics, I agree with the
> > pattern
> > > of
> > > > >>>>>>>>> keeping
> > > > >>>>>>>>>>> the
> > > > >>>>>>>>>>>>> same name but eliminating the tags for the dimensions
> > that
> > > > >> were
> > > > >>>>>>>>>>>> rolled-up.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> 2.
> > > > >>>>>>>>>>>>> I'm not too sure that implementation efficiency really
> > > > >> becomes a
> > > > >>>>>>>>>> factor
> > > > >>>>>>>>>>>> in
> > > > >>>>>>>>>>>>> choosing whether to (by default) update one coarse
> metric
> > > at
> > > > >> the
> > > > >>>>>>>>>> thread
> > > > >>>>>>>>>>>>> level or one granular metric at the processor-node
> level,
> > > > >> since
> > > > >>>>>>>>> it's
> > > > >>>>>>>>>>> just
> > > > >>>>>>>>>>>>> one metric being updated either way. I do agree that if
> > we
> > > > >> were
> > > > >>>>> to
> > > > >>>>>>>>>>> update
> > > > >>>>>>>>>>>>> the granular metrics and multiple roll-ups, then we
> > should
> > > > >>>>>>>>> consider
> > > > >>>>>>>>>> the
> > > > >>>>>>>>>>>>> efficiency.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> I agree it's probably not necessary to surface the
> > metrics
> > > > for
> > > > >>>>> all
> > > > >>>>>>>>>>> nodes
> > > > >>>>>>>>>>>>> regardless of whether they can or do skip records.
> > Perhaps
> > > we
> > > > >>> can
> > > > >>>>>>>>>>> lazily
> > > > >>>>>>>>>>>>> register the metrics.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> In my email in response to Matthias, I gave an example
> of
> > > the
> > > > >>>>>>>>> kind of
> > > > >>>>>>>>>>>>> scenario that would lead me as an operator to run with
> > > DEBUG
> > > > >> on
> > > > >>>>>>>>> all
> > > > >>>>>>>>>> the
> > > > >>>>>>>>>>>>> time, since I wouldn't be sure, having seen a skipped
> > > record
> > > > >>>>> once,
> > > > >>>>>>>>>> that
> > > > >>>>>>>>>>>> it
> > > > >>>>>>>>>>>>> would ever happen again. The solution is to capture all
> > the
> > > > >>>>>>>>> available
> > > > >>>>>>>>>>>>> information about the reason and location of skips all
> > the
> > > > >> time.
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> One thing you mention is the notion of setting alerts
> on
> > > > >> coarser
> > > > >>>>>>>>>>> metrics
> > > > >>>>>>>>>>>>> being easier than finer ones. All the metric alerting
> > > systems
> > > > >> I
> > > > >>>>>>>>> have
> > > > >>>>>>>>>>> used
> > > > >>>>>>>>>>>>> make it equally easy to alert on metrics by-tag or over
> > > tags.
> > > > >> So
> > > > >>>>>>>>> my
> > > > >>>>>>>>>>>>> experience doesn't say that this is a use case. Were
> you
> > > > >>> thinking
> > > > >>>>>>>>> of
> > > > >>>>>>>>>> an
> > > > >>>>>>>>>>>>> alerting system that makes such a pre-aggregation
> > valuable?
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> Thanks again,
> > > > >>>>>>>>>>>>> -John
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>> On Thu, Mar 29, 2018 at 5:24 PM, Guozhang Wang <
> > > > >>>>>>>>> wangguoz@gmail.com>
> > > > >>>>>>>>>>>> wrote:
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Hello John,
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Thanks for the KIP. Some comments:
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> 1. Could you list all the possible values of the
> > "reason"
> > > > >> tag?
> > > > >>>>>>>>> In
> > > > >>>>>>>>>> the
> > > > >>>>>>>>>>>>> JIRA
> > > > >>>>>>>>>>>>>> ticket I left some potential reasons but I'm not clear
> > if
> > > > >>> you're
> > > > >>>>>>>>>>> going
> > > > >>>>>>>>>>>> to
> > > > >>>>>>>>>>>>>> categorize each of them as a separate reason, or is
> > there
> > > > any
> > > > >>>>>>>>>>>> additional
> > > > >>>>>>>>>>>>>> ones you have in mind.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Also I'm wondering if we should add another metric
> that
> > do
> > > > >> not
> > > > >>>>>>>>> have
> > > > >>>>>>>>>>> the
> > > > >>>>>>>>>>>>>> reason tag but aggregates among all possible reasons?
> > This
> > > > is
> > > > >>>>>>>>> for
> > > > >>>>>>>>>>> users
> > > > >>>>>>>>>>>>> to
> > > > >>>>>>>>>>>>>> easily set their alerting notifications (otherwise
> they
> > > have
> > > > >> to
> > > > >>>>>>>>>> write
> > > > >>>>>>>>>>>> on
> > > > >>>>>>>>>>>>>> notification rule per reason) in their monitoring
> > systems.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> 2. Note that the processor-node metrics is actually
> > > > >>> "per-thread,
> > > > >>>>>>>>>>>>> per-task,
> > > > >>>>>>>>>>>>>> per-processor-node", and today we only set the
> > per-thread
> > > > >>>>>>>>> metrics
> > > > >>>>>>>>>> as
> > > > >>>>>>>>>>>> INFO
> > > > >>>>>>>>>>>>>> while leaving the lower two layers as DEBUG. I agree
> > with
> > > > >> your
> > > > >>>>>>>>>>> argument
> > > > >>>>>>>>>>>>>> that we are missing the per-client roll-up metrics
> > today,
> > > > but
> > > > >>>>>>>>> I'm
> > > > >>>>>>>>>>>>> convinced
> > > > >>>>>>>>>>>>>> that the right way to approach it would be
> > > > >>>>>>>>>>> "just-providing-the-lowest-
> > > > >>>>>>>>>>>>>> level
> > > > >>>>>>>>>>>>>> metrics only".
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Note the recoding implementation of these three levels
> > are
> > > > >>>>>>>>>> different
> > > > >>>>>>>>>>>>>> internally today: we did not just do the rolling up to
> > > > >> generate
> > > > >>>>>>>>> the
> > > > >>>>>>>>>>>>>> higher-level metrics from the lower level ones, but we
> > > just
> > > > >>>>>>>>> record
> > > > >>>>>>>>>>> them
> > > > >>>>>>>>>>>>>> separately, which means that, if we turn on multiple
> > > levels
> > > > >> of
> > > > >>>>>>>>>>> metrics,
> > > > >>>>>>>>>>>>> we
> > > > >>>>>>>>>>>>>> maybe duplicate collecting some metrics. One can argue
> > > that
> > > > >> is
> > > > >>>>>>>>> not
> > > > >>>>>>>>>>> the
> > > > >>>>>>>>>>>>> best
> > > > >>>>>>>>>>>>>> way to represent multi-level metrics collecting and
> > > > >> reporting,
> > > > >>>>>>>>> but
> > > > >>>>>>>>>> by
> > > > >>>>>>>>>>>>> only
> > > > >>>>>>>>>>>>>> enabling thread-level metrics as INFO today, that
> > > > >>> implementation
> > > > >>>>>>>>>>> could
> > > > >>>>>>>>>>>> be
> > > > >>>>>>>>>>>>>> more efficient than only collecting the metrics at the
> > > > lowest
> > > > >>>>>>>>>> level,
> > > > >>>>>>>>>>>> and
> > > > >>>>>>>>>>>>>> then do the roll-up calculations outside of the
> metrics
> > > > >>> classes.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Plus, today not all processor-nodes may possibly skip
> > > > >> records,
> > > > >>>>>>>>>> AFAIK
> > > > >>>>>>>>>>> we
> > > > >>>>>>>>>>>>>> will only skip records at the source, sink, window and
> > > > >>>>>>>>> aggregation
> > > > >>>>>>>>>>>>>> processor nodes, so adding a metric per processor
> looks
> > > like
> > > > >> an
> > > > >>>>>>>>>>>> overkill
> > > > >>>>>>>>>>>>> to
> > > > >>>>>>>>>>>>>> me as well. On the other hand, from user's perspective
> > the
> > > > >>>>>>>>> "reason"
> > > > >>>>>>>>>>> tag
> > > > >>>>>>>>>>>>> may
> > > > >>>>>>>>>>>>>> be sufficient for them to narrow down where inside the
> > > > >> topology
> > > > >>>>>>>>> is
> > > > >>>>>>>>>>>>> causing
> > > > >>>>>>>>>>>>>> records to be dropped on the floor. So I think the
> > > > >> "per-thread,
> > > > >>>>>>>>>>>> per-task"
> > > > >>>>>>>>>>>>>> level metrics should be sufficient for them in trouble
> > > shoot
> > > > >> in
> > > > >>>>>>>>>> DEBUG
> > > > >>>>>>>>>>>>> mode,
> > > > >>>>>>>>>>>>>> and we can add another "per-thread" level metrics as
> > INFO
> > > > >> which
> > > > >>>>>>>>> is
> > > > >>>>>>>>>>>> turned
> > > > >>>>>>>>>>>>>> on by default. So under normal execution users still
> > only
> > > > >> need
> > > > >>>>>>>>> INFO
> > > > >>>>>>>>>>>> level
> > > > >>>>>>>>>>>>>> metrics for alerting (e.g. set alerts on all
> > > skipped-records
> > > > >>>>>>>>>> metrics
> > > > >>>>>>>>>>> as
> > > > >>>>>>>>>>>>>> non-zero), and then upon trouble shooting they can
> turn
> > on
> > > > >>> DEBUG
> > > > >>>>>>>>>>>> metrics
> > > > >>>>>>>>>>>>> to
> > > > >>>>>>>>>>>>>> look into which task is actually causing the skipped
> > > > records.
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> Guozhang
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> On Thu, Mar 29, 2018 at 2:03 PM, Matthias J. Sax <
> > > > >>>>>>>>>>>> matthias@confluent.io>
> > > > >>>>>>>>>>>>>> wrote:
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> Thanks for the KIP John.
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> Reading the material on the related Jiras, I am
> > wondering
> > > > >> what
> > > > >>>>>>>>>>>> `reason`
> > > > >>>>>>>>>>>>>>> tags you want to introduce? Can you elaborate? The
> KIP
> > > > >> should
> > > > >>>>>>>>>> list
> > > > >>>>>>>>>>>>> those
> > > > >>>>>>>>>>>>>>> IMHO.
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> About the fine grained metrics vs the roll-up: you
> say
> > > that
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>> the coarse metric aggregates across two dimensions
> > > > >>>>>>>>>> simultaneously
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> Can you elaborate why this is an issue? I am not
> > > convinced
> > > > >> atm
> > > > >>>>>>>>>> that
> > > > >>>>>>>>>>>> we
> > > > >>>>>>>>>>>>>>> should put the fine grained metrics into INFO level
> and
> > > > >> remove
> > > > >>>>>>>>>> the
> > > > >>>>>>>>>>>>>>> roll-up at thread level.
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>> Given that they have to do this sum to get a usable
> > > > >>>>>>>>> top-level
> > > > >>>>>>>>>>> view
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> This is a fair concern, but I don't share the
> > conclusion.
> > > > >>>>>>>>>> Offering
> > > > >>>>>>>>>>> a
> > > > >>>>>>>>>>>>>>> built-in `KafkaStreams` "client" roll-up out of the
> box
> > > > >> might
> > > > >>>>>>>>> be
> > > > >>>>>>>>>> a
> > > > >>>>>>>>>>>>>>> better solution. In the past we did not offer this
> due
> > to
> > > > >>>>>>>>>>> performance
> > > > >>>>>>>>>>>>>>> concerns, but we could allow an "opt-in" mechanism.
> If
> > > you
> > > > >>>>>>>>>>> disagree,
> > > > >>>>>>>>>>>>> can
> > > > >>>>>>>>>>>>>>> you provide some reasoning and add them to the
> > "Rejected
> > > > >>>>>>>>>>>> alternatives"
> > > > >>>>>>>>>>>>>>> section.
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> To rephrase: I understand the issue about missing
> > > top-level
> > > > >>>>>>>>> view,
> > > > >>>>>>>>>>> but
> > > > >>>>>>>>>>>>>>> instead of going more fine grained, we should
> consider
> > to
> > > > >> add
> > > > >>>>>>>>>> this
> > > > >>>>>>>>>>>>>>> top-level view and add/keep the fine grained metrics
> at
> > > > >> DEBUG
> > > > >>>>>>>>>> level
> > > > >>>>>>>>>>>>> only
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> I am +1 to add TopologyTestDriver#metrics() and to
> > remove
> > > > >> old
> > > > >>>>>>>>>>> metrics
> > > > >>>>>>>>>>>>>>> directly as you suggested.
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> -Matthias
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>> On 3/28/18 6:42 PM, Ted Yu wrote:
> > > > >>>>>>>>>>>>>>>> Looks good to me.
> > > > >>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>> On Wed, Mar 28, 2018 at 3:11 PM, John Roesler <
> > > > >>>>>>>>>> john@confluent.io
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> wrote:
> > > > >>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>> Hello all,
> > > > >>>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>> I am proposing KIP-274 to improve the metrics
> around
> > > > >>>>>>>>> skipped
> > > > >>>>>>>>>>>> records
> > > > >>>>>>>>>>>>>> in
> > > > >>>>>>>>>>>>>>>>> Streams.
> > > > >>>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>> Please find the details here:
> > > > >>>>>>>>>>>>>>>>> https://cwiki.apache.org/
> > confluence/display/KAFKA/KIP-
> > > > >>>>>>>>>>>>>>>>> 274%3A+Kafka+Streams+Skipped+Records+Metrics
> > > > >>>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>> Please let me know what you think!
> > > > >>>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>> Thanks,
> > > > >>>>>>>>>>>>>>>>> -John
> > > > >>>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>> --
> > > > >>>>>>>>>>>>>> -- Guozhang
> > > > >>>>>>>>>>>>>>
> > > > >>>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>> --
> > > > >>>>>>>>>>>> -- Guozhang
> > > > >>>>>>>>>>>>
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> --
> > > > >>>>>>>>>> -- Guozhang
> > > > >>>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>
> > > > >>>
> > > > >>
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> > > --
> > > -- Guozhang
> > >
> >
>



-- 
-- Guozhang

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