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 > >> > >
