Ismael, Based on the streams user demand for registering their sensors and metrics we decided to expose the metrics registry. I'm not a fan of replicating a ton of code and add yet another layer/interface that does the same thing that the existing Metrics class and I think the methods are pretty basic (e.g., we do need 'removeSensor').
I can look into a subsequent JIRA to fix style issues such as your point 4 (remove 'get') directly on the Metrics class. Thanks Eno > On 8 Jan 2017, at 14:42, Ismael Juma <ism...@juma.me.uk> wrote: > > Thanks for updating the KIP to include the Metrics API that we are now > exposing, very helpful! Looking at it, do we really want to expose it to > users? The API seems to have grown organically (as an internal API) and > doesn't look particularly user-friendly to me. Questions I have: > > 1. `metricName` and `sensor` have a huge number of overloads. We usually > try to avoid that in public APIs by using a class that encapsulates the > parameters. Also, `inactiveSensorExpirationTimeSeconds` doesn't make sense > for clients since the thread that purges inactive sensors is only enabled > in the broker. > 2. Do we want to expose `removeSensor`? > 3. Do we want to expose `addReporter`? > 4. We typically have methods without `get` as accessors, but here we have > `getSensor` for the accessor and `sensor` is really `getOrCreateSensor`. > Maybe it's justified based the typical usage (but it would be good to > confirm)? > 5. I didn't understand the comment about why an interface wouldn't work. If > it's a MetricsRegistry interface, it could live in the clients module > (outside Streams as you said), but that is not necessarily an issue as far > as I can see. > > Thanks, > Ismael > > P.S. Here's the list of methods grouped and without javadoc to make it > easier to see what I mean: > > //metricName overloads > public MetricName metricName(String name, String group, String description, > Map<String, String> tags); > public MetricName metricName(String name, String group, String description); > public MetricName metricName(String name, String group); > public MetricName metricName(String name, String group, String description, > String... keyValue); > public MetricName metricName(String name, String group, Map<String, String> > tags); > > //sensor overloads > public Sensor sensor(String name); > public Sensor sensor(String name, Sensor.RecordLevel recordLevel); > public Sensor sensor(String name, Sensor... parents); > public Sensor sensor(String name, Sensor.RecordLevel recordLevel, Sensor... > parents); > public synchronized Sensor sensor(String name, MetricConfig config, > Sensor... parents); > public synchronized Sensor sensor(String name, MetricConfig config, > Sensor.RecordLevel recordLevel, Sensor... parents); > public synchronized Sensor sensor(String name, MetricConfig config, > Sensor.RecordLevel recordLevel, Sensor... parents); > public synchronized Sensor sensor(String name, MetricConfig config, long > inactiveSensorExpirationTimeSeconds, Sensor.RecordLevel recordLevel, > Sensor... parents); > > public MetricConfig config(); > > public Sensor getSensor(String name); > > public void removeSensor(String name); > > public void addMetric(MetricName metricName, Measurable measurable); > public synchronized void addMetric(MetricName metricName, MetricConfig > config, Measurable measurable); > public synchronized KafkaMetric removeMetric(MetricName metricName); > > public synchronized void addReporter(MetricsReporter reporter); > > public Map<MetricName, KafkaMetric> metrics(); > > public KafkaMetric metric(MetricName metricName); > > On Sat, Jan 7, 2017 at 12:15 AM, Eno Thereska <eno.there...@gmail.com> > wrote: > >> Ok, I'll list all the methods in the Metrics class for completion. An >> interface won't work since it will have to reside outside of streams >> unfortunately. >> >> Thanks >> Eno >>> On 6 Jan 2017, at 23:54, Ismael Juma <ism...@juma.me.uk> wrote: >>> >>> 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 >>>> >> >>
