Thanks Ismael, Damian, I have updated KIP to go with the conservative option.
Eno > On 9 Jan 2017, at 09:44, Ismael Juma <ism...@juma.me.uk> wrote: > > 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 >>>>>>> >>>>> >>>>> >>> >> >>
