Thanks!! On Mon, Jan 9, 2017 at 12:18 PM, Eno Thereska <eno.there...@gmail.com> wrote:
> Sure, updated the KIP for both. > > Thanks > Eno > > On 9 Jan 2017, at 19:23, Guozhang Wang <wangg...@gmail.com> wrote: > > > > 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 > > -- -- Guozhang
