[DISCUSS] KIP-104: Granular Sensors for Streams

[Aarti Gupta <aa...@gmail.com>]

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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

Thanks Guozhang, I adjusted the KIP to explicitly mention that we are exposing the Metrics object now as Ismael suggested, since it was lost in the details.

Eno
> On 6 Jan 2017, at 22:16, Ismael Juma <is...@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 <en...@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 <is...@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 <aa...@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
>>>> 
>> 
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Damian Guy <da...@gmail.com>]

Eno - I'm +1 on this change.
Thanks,
Damian

On Sun, 8 Jan 2017 at 20:45 Eno Thereska <en...@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 <en...@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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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.thereska@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 <is...@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 <
> aartiguptaa@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
> >>>>>
> >>>
> >>>
> >
>
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Guozhang Wang <wa...@gmail.com>]

Thanks!!

On Mon, Jan 9, 2017 at 12:18 PM, Eno Thereska <en...@gmail.com>
wrote:

> Sure, updated the KIP for both.
>
> Thanks
> Eno
> > On 9 Jan 2017, at 19:23, Guozhang Wang <wa...@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 <en...@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 <en...@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 <is...@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 <en...@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 <en...@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 <is...@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.thereska@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 <is...@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 <
> wangguoz@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 <is...@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.thereska@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 <is...@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 <
> >>>>> aartiguptaa@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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

Sure, updated the KIP for both. 

Thanks
Eno
> On 9 Jan 2017, at 19:23, Guozhang Wang <wa...@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 <en...@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 <en...@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 <is...@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 <en...@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 <en...@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 <is...@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.thereska@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 <is...@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 <wangguoz@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 <is...@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.thereska@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 <is...@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 <
>>>>> aartiguptaa@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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Guozhang Wang <wa...@gmail.com>]

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 <en...@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 <en...@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 <is...@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 <en...@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 <en...@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 <is...@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.thereska@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 <is...@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 <wangguoz@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 <is...@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.thereska@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 <is...@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 <
> >>> aartiguptaa@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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

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 <en...@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 <is...@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 <en...@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 <en...@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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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.thereska@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 <is...@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 <
>>> aartiguptaa@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
>>>>>>>> 
>>>>>> 
>>>>>> 
>>>> 
>>> 
>>> 
> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

Thanks Ismael, Damian, I have updated KIP to go with the conservative option.

Eno
> On 9 Jan 2017, at 09:44, Ismael Juma <is...@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 <en...@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 <en...@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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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.thereska@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 <is...@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 <
>> aartiguptaa@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
>>>>>>> 
>>>>> 
>>>>> 
>>> 
>> 
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Ismael Juma <is...@juma.me.uk>]

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 <en...@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 <en...@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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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.thereska@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 <is...@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 <
> aartiguptaa@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
> >>>>>
> >>>
> >>>
> >
>
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

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 <en...@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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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 <en...@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 <is...@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 <aa...@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
>>>>> 
>>> 
>>> 
> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

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 <is...@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 <en...@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 <is...@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 <wa...@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 <is...@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 <en...@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 <is...@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 <aa...@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
>>>> 
>> 
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Ismael Juma <is...@juma.me.uk>]

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 <en...@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 <is...@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 <wa...@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 <is...@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 <en...@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 <is...@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 <aa...@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
> >>
>
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

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 <is...@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 <wa...@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 <is...@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 <en...@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 <is...@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 <aa...@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
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Ismael Juma <is...@juma.me.uk>]

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 <wa...@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 <is...@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 <en...@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 <is...@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 <aa...@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
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Guozhang Wang <wa...@gmail.com>]

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 <is...@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 <en...@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 <is...@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 <aa...@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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Ismael Juma <is...@juma.me.uk>]

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 <en...@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 <is...@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 <aa...@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
> >>
>
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

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 <is...@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 <aa...@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
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Ismael Juma <is...@juma.me.uk>]

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 <aa...@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
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Guozhang Wang <wa...@gmail.com>]

Thank Eno and Aarti.

On Thu, Jan 5, 2017 at 3:14 AM, Eno Thereska <en...@gmail.com> wrote:

> Thanks Guozhang, I've updated the KIP and the PR to address point 2. you
> raise.
>
> Eno
>
> > On 4 Jan 2017, at 17:41, Aarti Gupta <aa...@gmail.com> wrote:
> >
> > Thanks for the review, Guozhang,  addressed comment 1, 3 on the KIP and
> > left 2 for Eno to comment on. (He is back tomorrow)
> >
> > -aarti
> >
> > On Tue, Jan 3, 2017 at 11:12 AM, Guozhang Wang <wa...@gmail.com>
> wrote:
> >
> >> Thanks for the proposed KIP. Overall LGTM.
> >>
> >> A few comments:
> >>
> >> 1. "at the granularity of each processor node, in the addition to the
> >> global rate"
> >>
> >> I think you also add one sensor at the granularity of tasks, "Skipped
> >> records sensor in StreamTask" right?
> >>
> >> 2. From PR 1446 it seems you have also added a couple of APIs for
> allowing
> >> users to register arbitrary sensors via `StreamsMetrics`, could you also
> >> describe the changes and when / how users are expected to use them as
> well?
> >>
> >> 3. Could you also list the added overloaded APIs for throughput sensors
> as
> >> well as with the recordLevel parameters, and mention what will be the
> >> default values for those added parameters in the existing API functions?
> >>
> >>
> >>
> >> Guozhang
> >>
> >>
> >> On Fri, Dec 30, 2016 at 8:26 PM, Aarti Gupta <aa...@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

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Eno Thereska <en...@gmail.com>]

Thanks Guozhang, I've updated the KIP and the PR to address point 2. you raise. 

Eno

> On 4 Jan 2017, at 17:41, Aarti Gupta <aa...@gmail.com> wrote:
> 
> Thanks for the review, Guozhang,  addressed comment 1, 3 on the KIP and
> left 2 for Eno to comment on. (He is back tomorrow)
> 
> -aarti
> 
> On Tue, Jan 3, 2017 at 11:12 AM, Guozhang Wang <wa...@gmail.com> wrote:
> 
>> Thanks for the proposed KIP. Overall LGTM.
>> 
>> A few comments:
>> 
>> 1. "at the granularity of each processor node, in the addition to the
>> global rate"
>> 
>> I think you also add one sensor at the granularity of tasks, "Skipped
>> records sensor in StreamTask" right?
>> 
>> 2. From PR 1446 it seems you have also added a couple of APIs for allowing
>> users to register arbitrary sensors via `StreamsMetrics`, could you also
>> describe the changes and when / how users are expected to use them as well?
>> 
>> 3. Could you also list the added overloaded APIs for throughput sensors as
>> well as with the recordLevel parameters, and mention what will be the
>> default values for those added parameters in the existing API functions?
>> 
>> 
>> 
>> Guozhang
>> 
>> 
>> On Fri, Dec 30, 2016 at 8:26 PM, Aarti Gupta <aa...@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
>> 

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Aarti Gupta <aa...@gmail.com>]

Thanks for the review, Guozhang,  addressed comment 1, 3 on the KIP and
left 2 for Eno to comment on. (He is back tomorrow)

-aarti

On Tue, Jan 3, 2017 at 11:12 AM, Guozhang Wang <wa...@gmail.com> wrote:

> Thanks for the proposed KIP. Overall LGTM.
>
> A few comments:
>
> 1. "at the granularity of each processor node, in the addition to the
> global rate"
>
> I think you also add one sensor at the granularity of tasks, "Skipped
> records sensor in StreamTask" right?
>
> 2. From PR 1446 it seems you have also added a couple of APIs for allowing
> users to register arbitrary sensors via `StreamsMetrics`, could you also
> describe the changes and when / how users are expected to use them as well?
>
> 3. Could you also list the added overloaded APIs for throughput sensors as
> well as with the recordLevel parameters, and mention what will be the
> default values for those added parameters in the existing API functions?
>
>
>
> Guozhang
>
>
> On Fri, Dec 30, 2016 at 8:26 PM, Aarti Gupta <aa...@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
>

Re: [DISCUSS] KIP-104: Granular Sensors for Streams

[Guozhang Wang <wa...@gmail.com>]

Thanks for the proposed KIP. Overall LGTM.

A few comments:

1. "at the granularity of each processor node, in the addition to the
global rate"

I think you also add one sensor at the granularity of tasks, "Skipped
records sensor in StreamTask" right?

2. From PR 1446 it seems you have also added a couple of APIs for allowing
users to register arbitrary sensors via `StreamsMetrics`, could you also
describe the changes and when / how users are expected to use them as well?

3. Could you also list the added overloaded APIs for throughput sensors as
well as with the recordLevel parameters, and mention what will be the
default values for those added parameters in the existing API functions?



Guozhang


On Fri, Dec 30, 2016 at 8:26 PM, Aarti Gupta <aa...@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