Hi James, I updated the KIP to reflect the exact change to the metric as you suggested.
> kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce > > And the KIP is proposing to change it to: > > kafka.network:type=RequestMetrics,name=RequestsPerSec,request= Produce,version=1.0.0 > > Is is possible for the broker to have BOTH metrics? That way, we don’t have to change the name. > > Would that make querying/aggregating too annoying (since a query for name=RequestsPerSec and request=Produce would return BOTH metrics)? > This kind of change will be problematic to us as the total RequestsPerSec will be double counted in our metric system as we do automatic aggregation. It has been a headache for us as we always have to do some special handling when querying such metrics. Thanks, Allen On Wed, Mar 21, 2018 at 11:55 PM, James Cheng <wushuja...@gmail.com> wrote: > Regardless of what we decide, Allen, can you update the KIP and provide an > concrete example of what an mbean will look like with the proposed change? > Similar to what I said here: > > > An example of the metric we are thinking of changing is this: > > > > kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce > > > > And the KIP is proposing to change it to: > > > > kafka.network:type=RequestMetrics,name=RequestsPerSec,request= > Produce,version=1.0.0 > > > I think that would make the KIP easier to understand. > > Thanks, > -James > > Sent from my iPhone > > > On Mar 21, 2018, at 11:49 PM, James Cheng <wushuja...@gmail.com> wrote: > > > > An example of the metric we are thinking of changing is this: > > > > kafka.network:type=RequestMetrics,name=RequestsPerSec,request=Produce > > > > And the KIP is proposing to change it to: > > > > kafka.network:type=RequestMetrics,name=RequestsPerSec,request= > Produce,version=1.0.0 > > > > Is is possible for the broker to have BOTH metrics? That way, we don’t > have to change the name. > > > > Would that make querying/aggregating too annoying (since a query for > name=RequestsPerSec and request=Produce would return BOTH metrics)? > > > > Also, it might be hard to query for “name=RequestsPerSec and > request=Produce and version field NOT present” > > > > -James > > > > Sent from my iPhone > > > >> On Mar 21, 2018, at 10:17 PM, Jeff Widman <j...@jeffwidman.com> wrote: > >> > >> I agree with Allen. > >> > >> Go with the intuitive name, even if it means not deprecating. The > impact of > >> breakage here is small since it only breaks monitoring and the folks who > >> watch their dashboards closely are the ones likely to read the release > >> notes carefully and see this change. > >> > >>> On Wed, Mar 21, 2018, 3:24 PM Allen Wang <allenxw...@gmail.com> wrote: > >>> > >>> I understand the impact to jmx based tools. But adding a new metric is > >>> unnecessary for more advanced monitoring systems that can aggregate > with or > >>> without tags. Duplicating the metric (including the "request" tag) also > >>> adds performance cost to the broker, especially for the metric that > needs > >>> to be updated for every request. > >>> > >>> For KIP-225, the choice makes sense because the deprecated metric's > name is > >>> undesirable anyway and the new metric name is much better than the > prefixed > >>> metric name. Not the case for RequestsPerSec. It is hard for me to > come up > >>> with another intuitive name. > >>> > >>> Thanks, > >>> Allen > >>> > >>> > >>> > >>>> On Wed, Mar 21, 2018 at 2:01 AM, Ted Yu <yuzhih...@gmail.com> wrote: > >>>> > >>>> Creating new metric and deprecating existing one seems better from > >>>> compatibility point of view. > >>>> -------- Original message --------From: James Cheng < > >>> wushuja...@gmail.com> > >>>> Date: 3/21/18 1:39 AM (GMT-08:00) To: dev@kafka.apache.org Subject: > >>> Re: > >>>> [DISCUSS] KIP-272: Add API version tag to broker's RequestsPerSec > metric > >>>> Manikumar brings up a good point. This is a breaking change to the > >>>> existing metric. Do we want to break compatibility, or do we want to > add > >>> a > >>>> new metric and (optionally) deprecate the existing metric? > >>>> > >>>> For reference, in KIP-153 [1], we changed an existing metric without > >>> doing > >>>> proper deprecation. > >>>> > >>>> However, in KIP-225, we noticed that that we maybe shouldn't have done > >>>> that. For KIP-225 [2], we instead decided to create a new metric, and > >>>> deprecate (but not remove) the old one. > >>>> > >>>> -James > >>>> > >>>> [1] https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >>>> 153%3A+Include+only+client+traffic+in+BytesOutPerSec+metric > >>>> > >>>> [2] https://cwiki.apache.org/confluence/pages/viewpage. > >>>> action?pageId=74686649 > >>>> > >>>> > >>>>> On Mar 21, 2018, at 12:14 AM, Manikumar <manikumar.re...@gmail.com> > >>>> wrote: > >>>>> > >>>>> Can we retain total RequestsPerSec metric and add new version tag > >>> metric? > >>>>> When monitoring with simple jconsole/jmx based tools, It is useful to > >>>> have > >>>>> total metric > >>>>> to monitor request rate. > >>>>> > >>>>> > >>>>> Thanks, > >>>>> > >>>>> On Wed, Mar 21, 2018 at 11:01 AM, Gwen Shapira <g...@confluent.io> > >>>> wrote: > >>>>> > >>>>>> I love this. Not much to add - it is an elegant solution, clean > >>>>>> implementation and it addresses a real need, especially during > >>> upgrades. > >>>>>> > >>>>>>> On Tue, Mar 20, 2018 at 2:49 PM, Ted Yu <yuzhih...@gmail.com> > wrote: > >>>>>>> > >>>>>>> Thanks for the response. > >>>>>>> > >>>>>>> Assuming number of client versions is limited in a cluster, memory > >>>>>>> consumption is not a concern. > >>>>>>> > >>>>>>> Cheers > >>>>>>> > >>>>>>> On Tue, Mar 20, 2018 at 10:47 AM, Allen Wang <allenxw...@gmail.com > > > >>>>>> wrote: > >>>>>>> > >>>>>>>> Hi Ted, > >>>>>>>> > >>>>>>>> The additional hash map is very small, possibly a few KB. Each > >>> request > >>>>>>> type > >>>>>>>> ("produce", "fetch", etc.) will have such a map which have a few > >>>>>> entries > >>>>>>>> depending on the client API versions the broker will encounter. So > >>> if > >>>>>>>> broker encounters two client versions for "produce", there will be > >>> two > >>>>>>>> entries in the map for "produce" requests mapping from version to > >>>>>> meter. > >>>>>>> Of > >>>>>>>> course, hash map always have additional memory overhead. > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Allen > >>>>>>>> > >>>>>>>> > >>>>>>>> On Mon, Mar 19, 2018 at 3:49 PM, Ted Yu <yuzhih...@gmail.com> > >>> wrote: > >>>>>>>> > >>>>>>>>> bq. *additional hash lookup is needed when updating the metric to > >>>>>>> locate > >>>>>>>>> the metric * > >>>>>>>>> > >>>>>>>>> *Do you have estimate how much memory is needed for maintaining > the > >>>>>>> hash > >>>>>>>>> map ?* > >>>>>>>>> > >>>>>>>>> *Thanks* > >>>>>>>>> > >>>>>>>>> On Mon, Mar 19, 2018 at 3:19 PM, Allen Wang < > allenxw...@gmail.com> > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hi all, > >>>>>>>>>> > >>>>>>>>>> I have created KIP-272: Add API version tag to broker's > >>>>>>> RequestsPerSec > >>>>>>>>>> metric. > >>>>>>>>>> > >>>>>>>>>> Here is the link to the KIP: > >>>>>>>>>> > >>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > >>>>>>>>>> 272%3A+Add+API+version+tag+to+broker%27s+RequestsPerSec+metric > >>>>>>>>>> > >>>>>>>>>> Looking forward to the discussion. > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Allen > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>> -- > >>>>>> *Gwen Shapira* > >>>>>> Product Manager | Confluent > >>>>>> 650.450.2760 | @gwenshap > >>>>>> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog > >>>>>> <http://www.confluent.io/blog> > >>>>>> > >>>> > >>>> > >>> >
