Hi Guozhang, I understand the requirement and I don't have an issue with that. My point is that the `Metrics` registry API is becoming public via this KIP so we should ensure that it's suitable. It may make sense to introduce an interface (say MetricsRegistry) that exposes a reasonable subset (do we want to expose `removeSensor` for example?). This is relatively straightforward and little additional code.
Ismael On Fri, Jan 6, 2017 at 11:29 PM, Guozhang Wang <wangg...@gmail.com> wrote: > Unlike Producer and Consumer, Streams users may likely to add their own > sensors depending on their apps and that is the main reason we added > facilities to let them register customized "throughput" "latency" and any > generic sensors. > > I think Eno has thought about just adding an API in StreamsMetrics to > register any sensors, which will be directly translated into a > `metrics.sensor` call. In the end he decided to just expose the registry > itself since the functions itself seem just like duplicating the same > `sensor` functions in `Metrics`. > > > > Guozhang > > On Fri, Jan 6, 2017 at 2:16 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Thanks for the explanation Eno. The KIP did mention that the metrics > > registry would be exposed, yes. What is missing is that the registry is > not > > currently exposed by anything else. Traditionally, we list all public > APIs > > created by a KIP, which is effectively true for the registry in this > case. > > Did we consider using an interface instead of the concrete class? It > seems > > that a lot of these things were discussed in the PR, so it would be good > to > > have a summary in the KIP too. > > > > Ismael > > > > On Fri, Jan 6, 2017 at 9:10 PM, Eno Thereska <eno.there...@gmail.com> > > wrote: > > > > > So the KIP proposes exposing the metrics registry (second paragraph > under > > > motivation). The community has indicated that they would like to 1. > > access > > > all the metrics and 2. register their own. We provide some helper > > > interfaces for them to register throughput and latency metrics, but > > > ultimately we felt it's best for them to have access to the full > registry > > > as well. This is because application code is now intertwined with the > > > streams library and we don't want to limit the kinds of metrics they > > might > > > want to register, nor do we necessarily want to provide yet another > > wrapper > > > around Metrics. > > > > > > Thanks, > > > Eno > > > > > > > On 6 Jan 2017, at 20:34, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > > > > Thanks for the KIP. Sounds useful. One thing that wasn't made clear > is > > > that > > > > we are exposing `Metrics` as a public class for the first time. > Neither > > > the > > > > consumer or producer expose it at the moment. Do we want to expose > the > > > > whole class or would it be better to expose a more limited interface? > > > > > > > > Ismael > > > > > > > > On Sat, Dec 31, 2016 at 4:26 AM, Aarti Gupta <aartigup...@gmail.com> > > > wrote: > > > > > > > >> Hi all, > > > >> > > > >> I would like to start the discussion on KIP-104: Granular Sensors > for > > > >> Streams > > > >> <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > >> 104%3A+Granular+Sensors+for+Streams?src=contextnavchildmode> > > > >> > > > >> *https://cwiki.apache.org/confluence/pages/viewpage. > > > action?pageId=67636480 > > > >> <https://cwiki.apache.org/confluence/pages/viewpage. > > > action?pageId=67636480 > > > >>> * > > > >> > > > >> Looking forward to your feedback. > > > >> > > > >> Thanks, > > > >> Aarti and Eno > > > >> > > > > > > > > > > > > -- > -- Guozhang >
