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