Thanks Eno. It is indeed a good point about additional classes that users would have to be able to construct in order to add metrics to a sensor. We have generally assumed those classes to be internal and have queued one of them (SimpleRate) for removal (KAFKA-4178). They are simple for the most part, as you said.
Exposing the metrics (maybe call it simply `metrics` since that's the name we use in the consumer and producer instead of `readOnlyRegistry`?) is fine by me as the conservative option. If we think that access to the full registry is really important and others are fine with the current `Metrics` API, I won't stand in the way though. Ismael On Sun, Jan 8, 2017 at 8:45 PM, Eno Thereska <eno.there...@gmail.com> wrote: > I can see the point that all of a sudden exposing the internal Metrics > class might be too big a bite to take in this KIP, since the Metrics class > might have to be cleaned up further. I was perhaps naively assuming it's > good since it has been reviewed when it was first introduced into Kafka. > Furthermore, that class brings in a whole bunch of other stuff, such as > Min/Avg. They are all simple things, but others might argue differently. > > An intermediate alternative would be to not expose the Metrics class for > now. Users can still get a read-only Map of all the streams metrics, as > they do in the Producer/Consumer classes. Furthermore, we already have > helper functions to help users add throughput and latency metrics and that > could be a good enough step for now. So: > > - Metrics registry() > + public Map<MetricName, ? extends Metric> readOnlyRegistry() > > Ideally either vote on the previous approach or add a note to this discuss > thread please since we're getting close to the feature freeze deadline. > > Thanks > Eno > > > On 8 Jan 2017, at 15:06, Eno Thereska <eno.there...@gmail.com> wrote: > > > > 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 > >>>>> > >>> > >>> > > > >
