Hello, Thanks for the KIP, I'm happy we're converging on implementations.
I was wondering whether we need to deprecate the old metrics - are those classes public interfaces? I see we don't build a JavaDoc for them ( https://kafka.apache.org/22/javadoc/allclasses-noframe.html) and, as far as I know, that means we don't necessarily need to go with the deprecation route. I think we also have the Measurable and Gauge classes which seem to do the same thing. From the names, I intuitively thought that Measurable indicates something that might require some amount of processing to get the value of and Gauge to be something that is instantly read. Reading their usages, though, I see that is not the case. Measurable is inherited by MeasurableStat which itself is inherited by most of the classes you mentioned in the KIP. Is it worth it converging on those as well (perhaps deprecating/removing Measurable and opting for Gauge) or do we prefer to keep the scope of this KIP small? Thanks, Stanislav On Fri, Jul 12, 2019 at 7:42 PM John Roesler <j...@confluent.io> wrote: > Hey, thanks Matthias and Bruno, > > I agree, "Cumulative" is a mouthful. "TotalX" sounds fine to me. > > Also, yes, I would have liked to not have any modifier for > "non-sampled", but there is a name conflict with Sum. > > I'll update the KIP to reflect "TotalX" and then start the vote thread. > > Thanks again, > -John > > On Fri, Jul 12, 2019 at 11:27 AM Bruno Cadonna <br...@confluent.io> wrote: > > > > OK, makes sense. Then, I am in favour of TotalCount and TotalSum. > > > > Best, > > Bruno > > > > On Fri, Jul 12, 2019 at 12:57 AM Matthias J. Sax <matth...@confluent.io> > wrote: > > > > > > `Sum` is an existing name, for the "sampled sum" metric, that gets > > > deprecated. Hence, we cannot use it. > > > > > > If we cannot use `Sum` and use `TotalSum`, we should also not use > > > `Count` but `TotalCount` for consistency. > > > > > > > > > -Matthias > > > > > > > > > > > > On 7/11/19 12:58 PM, Bruno Cadonna wrote: > > > > Hi John, > > > > > > > > Thank you for the KIP. > > > > > > > > LGTM > > > > > > > > I also do not like CumulativeSum/Count so much. I propose to just > call > > > > it Sum and Count. > > > > > > > > I understand that you want to unequivocally distinguish the two > metric > > > > functions by their names, but I have the feeling the names become > > > > artificially complex. The exact semantics can also be documented in > > > > the javadocs, which btw could also be improved in those classes. > > > > > > > > Best, > > > > Bruno > > > > > > > > > > > > > > > > On Thu, Jul 11, 2019 at 8:25 PM Matthias J. Sax < > matth...@confluent.io> wrote: > > > >> > > > >> Thanks for the KIP. Overall LGTM. > > > >> > > > >> The only though I have is, if we may want to use `TotalSum` and > > > >> `TotalCount` instead of `CumulativeSum/Count` as names? > > > >> > > > >> > > > >> -Matthias > > > >> > > > >> > > > >> On 7/11/19 9:31 AM, John Roesler wrote: > > > >>> Hi Kafka devs, > > > >>> > > > >>> I'd like to propose KIP-488 as a minor cleanup of some of our > metric > > > >>> implementations. > > > >>> > > > >>> KIP-488: https://cwiki.apache.org/confluence/x/kkAyBw > > > >>> > > > >>> Over time, iterative updates to these metrics has resulted in a > pretty > > > >>> confusing little collection of classes, and I've personally been > > > >>> involved in three separate moderately time-consuming iterations of > me > > > >>> or someone else trying to work out which metrics are available, and > > > >>> which ones are desired for a given use case. One of these was > actually > > > >>> a long-running bug in Kafka Streams' metrics, so not only has this > > > >>> confusion been a time sink, but it has also led to bugs. > > > >>> > > > >>> I'm hoping this change won't be too controversial. > > > >>> > > > >>> Thanks, > > > >>> -John > > > >>> > > > >> > > > >
