Thanks Eno, a couple of follow-up comments: 1. About `sensor()` functions, could we rename them to `addSensor()` to be consistent with other APIs? Also do we want to still add the "String scopeName, String entityName, String operationName" parameters to enforce the naming hierarchy or just let users to freely define their own Sensor name?
2. About `removeSensor`: for users to remove their registered sensors, the sensor name may not be known beforehand if they called `addLatencySensor` etc where the sensor name is internally created, plus they may need to remember the `Sensor` objects anyways in order to call its `recordXX()` functions, so would it be better to change the parameter to just `Sensor` instead of `String`? Guozhang On Mon, Jan 9, 2017 at 10:13 AM, Eno Thereska <eno.there...@gmail.com> wrote: > 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 > >>>>>>>> > >>>>>> > >>>>>> > >>>> > >>> > >>> > > > > -- -- Guozhang
