One hopefully final update to the KIP to re-add two methods for generic sensor creation. Also the interface should have been marked unstable from the beginning, so did that now.
Thanks, and please vote if you haven't done so. The above changes shouldn't trigger a re-vote since they are more conservative than what was before. Eno > On 9 Jan 2017, at 10:31, Eno Thereska <eno.there...@gmail.com> wrote: > > 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 >>>>>>>> >>>>>> >>>>>> >>>> >>> >>> >
