Sure, updated the KIP for both.
Thanks
Eno
> On 9 Jan 2017, at 19:23, Guozhang Wang <wangg...@gmail.com> wrote:
>
> Thanks Eno, a couple of follow-up comments:
>
> 1. About `sensor()` functions, could we rename them to `addSensor()` to be
> consistent with other APIs? Also do we want to still add the "String
> scopeName, String entityName, String operationName" parameters to enforce
> the naming hierarchy or just let users to freely define their own Sensor
> name?
>
> 2. About `removeSensor`: for users to remove their registered sensors, the
> sensor name may not be known beforehand if they called `addLatencySensor`
> etc where the sensor name is internally created, plus they may need to
> remember the `Sensor` objects anyways in order to call its `recordXX()`
> functions, so would it be better to change the parameter to just `Sensor`
> instead of `String`?
>
>
> Guozhang
>
>
>
>
>
> On Mon, Jan 9, 2017 at 10:13 AM, Eno Thereska <eno.there...@gmail.com>
> wrote:
>
>> One hopefully final update to the KIP to re-add two methods for generic
>> sensor creation. Also the interface should have been marked unstable from
>> the beginning, so did that now.
>>
>> Thanks, and please vote if you haven't done so. The above changes
>> shouldn't trigger a re-vote since they are more conservative than what was
>> before.
>> Eno
>>> On 9 Jan 2017, at 10:31, Eno Thereska <eno.there...@gmail.com> wrote:
>>>
>>> Thanks Ismael, Damian, I have updated KIP to go with the conservative
>> option.
>>>
>>> Eno
>>>> On 9 Jan 2017, at 09:44, Ismael Juma <ism...@juma.me.uk> wrote:
>>>>
>>>> Thanks Eno.
>>>>
>>>> It is indeed a good point about additional classes that users would
>> have to
>>>> be able to construct in order to add metrics to a sensor. We have
>> generally
>>>> assumed those classes to be internal and have queued one of them
>>>> (SimpleRate) for removal (KAFKA-4178). They are simple for the most
>> part,
>>>> as you said.
>>>>
>>>> Exposing the metrics (maybe call it simply `metrics` since that's the
>> name
>>>> we use in the consumer and producer instead of `readOnlyRegistry`?) is
>> fine
>>>> by me as the conservative option.
>>>>
>>>> If we think that access to the full registry is really important and
>> others
>>>> are fine with the current `Metrics` API, I won't stand in the way
>> though.
>>>>
>>>> Ismael
>>>>
>>>> On Sun, Jan 8, 2017 at 8:45 PM, Eno Thereska <eno.there...@gmail.com>
>> wrote:
>>>>
>>>>> I can see the point that all of a sudden exposing the internal Metrics
>>>>> class might be too big a bite to take in this KIP, since the Metrics
>> class
>>>>> might have to be cleaned up further. I was perhaps naively assuming
>> it's
>>>>> good since it has been reviewed when it was first introduced into
>> Kafka.
>>>>> Furthermore, that class brings in a whole bunch of other stuff, such as
>>>>> Min/Avg. They are all simple things, but others might argue
>> differently.
>>>>>
>>>>> An intermediate alternative would be to not expose the Metrics class
>> for
>>>>> now. Users can still get a read-only Map of all the streams metrics, as
>>>>> they do in the Producer/Consumer classes. Furthermore, we already have
>>>>> helper functions to help users add throughput and latency metrics and
>> that
>>>>> could be a good enough step for now. So:
>>>>>
>>>>> - Metrics registry()
>>>>> + public Map<MetricName, ? extends Metric> readOnlyRegistry()
>>>>>
>>>>> Ideally either vote on the previous approach or add a note to this
>> discuss
>>>>> thread please since we're getting close to the feature freeze deadline.
>>>>>
>>>>> Thanks
>>>>> Eno
>>>>>
>>>>>> On 8 Jan 2017, at 15:06, Eno Thereska <eno.there...@gmail.com> wrote:
>>>>>>
>>>>>> Ismael,
>>>>>>
>>>>>> Based on the streams user demand for registering their sensors and
>>>>> metrics we decided to expose the metrics registry. I'm not a fan of
>>>>> replicating a ton of code and add yet another layer/interface that
>> does the
>>>>> same thing that the existing Metrics class and I think the methods are
>>>>> pretty basic (e.g., we do need 'removeSensor').
>>>>>>
>>>>>> I can look into a subsequent JIRA to fix style issues such as your
>> point
>>>>> 4 (remove 'get') directly on the Metrics class.
>>>>>>
>>>>>> Thanks
>>>>>> Eno
>>>>>>
>>>>>>
>>>>>>> On 8 Jan 2017, at 14:42, Ismael Juma <ism...@juma.me.uk> wrote:
>>>>>>>
>>>>>>> Thanks for updating the KIP to include the Metrics API that we are
>> now
>>>>>>> exposing, very helpful! Looking at it, do we really want to expose
>> it to
>>>>>>> users? The API seems to have grown organically (as an internal API)
>> and
>>>>>>> doesn't look particularly user-friendly to me. Questions I have:
>>>>>>>
>>>>>>> 1. `metricName` and `sensor` have a huge number of overloads. We
>> usually
>>>>>>> try to avoid that in public APIs by using a class that encapsulates
>> the
>>>>>>> parameters. Also, `inactiveSensorExpirationTimeSeconds` doesn't make
>>>>> sense
>>>>>>> for clients since the thread that purges inactive sensors is only
>>>>> enabled
>>>>>>> in the broker.
>>>>>>> 2. Do we want to expose `removeSensor`?
>>>>>>> 3. Do we want to expose `addReporter`?
>>>>>>> 4. We typically have methods without `get` as accessors, but here we
>>>>> have
>>>>>>> `getSensor` for the accessor and `sensor` is really
>> `getOrCreateSensor`.
>>>>>>> Maybe it's justified based the typical usage (but it would be good to
>>>>>>> confirm)?
>>>>>>> 5. I didn't understand the comment about why an interface wouldn't
>>>>> work. If
>>>>>>> it's a MetricsRegistry interface, it could live in the clients module
>>>>>>> (outside Streams as you said), but that is not necessarily an issue
>> as
>>>>> far
>>>>>>> as I can see.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ismael
>>>>>>>
>>>>>>> P.S. Here's the list of methods grouped and without javadoc to make
>> it
>>>>>>> easier to see what I mean:
>>>>>>>
>>>>>>> //metricName overloads
>>>>>>> public MetricName metricName(String name, String group, String
>>>>> description,
>>>>>>> Map<String, String> tags);
>>>>>>> public MetricName metricName(String name, String group, String
>>>>> description);
>>>>>>> public MetricName metricName(String name, String group);
>>>>>>> public MetricName metricName(String name, String group, String
>>>>> description,
>>>>>>> String... keyValue);
>>>>>>> public MetricName metricName(String name, String group, Map<String,
>>>>> String>
>>>>>>> tags);
>>>>>>>
>>>>>>> //sensor overloads
>>>>>>> public Sensor sensor(String name);
>>>>>>> public Sensor sensor(String name, Sensor.RecordLevel recordLevel);
>>>>>>> public Sensor sensor(String name, Sensor... parents);
>>>>>>> public Sensor sensor(String name, Sensor.RecordLevel recordLevel,
>>>>> Sensor...
>>>>>>> parents);
>>>>>>> public synchronized Sensor sensor(String name, MetricConfig config,
>>>>>>> Sensor... parents);
>>>>>>> public synchronized Sensor sensor(String name, MetricConfig config,
>>>>>>> Sensor.RecordLevel recordLevel, Sensor... parents);
>>>>>>> public synchronized Sensor sensor(String name, MetricConfig config,
>>>>>>> Sensor.RecordLevel recordLevel, Sensor... parents);
>>>>>>> public synchronized Sensor sensor(String name, MetricConfig config,
>> long
>>>>>>> inactiveSensorExpirationTimeSeconds, Sensor.RecordLevel recordLevel,
>>>>>>> Sensor... parents);
>>>>>>>
>>>>>>> public MetricConfig config();
>>>>>>>
>>>>>>> public Sensor getSensor(String name);
>>>>>>>
>>>>>>> public void removeSensor(String name);
>>>>>>>
>>>>>>> public void addMetric(MetricName metricName, Measurable measurable);
>>>>>>> public synchronized void addMetric(MetricName metricName,
>> MetricConfig
>>>>>>> config, Measurable measurable);
>>>>>>> public synchronized KafkaMetric removeMetric(MetricName metricName);
>>>>>>>
>>>>>>> public synchronized void addReporter(MetricsReporter reporter);
>>>>>>>
>>>>>>> public Map<MetricName, KafkaMetric> metrics();
>>>>>>>
>>>>>>> public KafkaMetric metric(MetricName metricName);
>>>>>>>
>>>>>>> On Sat, Jan 7, 2017 at 12:15 AM, Eno Thereska <
>> eno.there...@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> Ok, I'll list all the methods in the Metrics class for completion.
>> An
>>>>>>>> interface won't work since it will have to reside outside of streams
>>>>>>>> unfortunately.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Eno
>>>>>>>>> On 6 Jan 2017, at 23:54, Ismael Juma <ism...@juma.me.uk> wrote:
>>>>>>>>>
>>>>>>>>> Hi Guozhang,
>>>>>>>>>
>>>>>>>>> I understand the requirement and I don't have an issue with that.
>> My
>>>>>>>> point
>>>>>>>>> is that the `Metrics` registry API is becoming public via this KIP
>> so
>>>>> we
>>>>>>>>> should ensure that it's suitable. It may make sense to introduce an
>>>>>>>>> interface (say MetricsRegistry) that exposes a reasonable subset
>> (do
>>>>> we
>>>>>>>>> want to expose `removeSensor` for example?). This is relatively
>>>>>>>>> straightforward and little additional code.
>>>>>>>>>
>>>>>>>>> Ismael
>>>>>>>>>
>>>>>>>>> On Fri, Jan 6, 2017 at 11:29 PM, Guozhang Wang <wangg...@gmail.com
>>>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Unlike Producer and Consumer, Streams users may likely to add
>> their
>>>>> own
>>>>>>>>>> sensors depending on their apps and that is the main reason we
>> added
>>>>>>>>>> facilities to let them register customized "throughput" "latency"
>> and
>>>>>>>> any
>>>>>>>>>> generic sensors.
>>>>>>>>>>
>>>>>>>>>> I think Eno has thought about just adding an API in
>> StreamsMetrics to
>>>>>>>>>> register any sensors, which will be directly translated into a
>>>>>>>>>> `metrics.sensor` call. In the end he decided to just expose the
>>>>> registry
>>>>>>>>>> itself since the functions itself seem just like duplicating the
>> same
>>>>>>>>>> `sensor` functions in `Metrics`.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Guozhang
>>>>>>>>>>
>>>>>>>>>> On Fri, Jan 6, 2017 at 2:16 PM, Ismael Juma <ism...@juma.me.uk>
>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Thanks for the explanation Eno. The KIP did mention that the
>> metrics
>>>>>>>>>>> registry would be exposed, yes. What is missing is that the
>>>>> registry is
>>>>>>>>>> not
>>>>>>>>>>> currently exposed by anything else. Traditionally, we list all
>>>>> public
>>>>>>>>>> APIs
>>>>>>>>>>> created by a KIP, which is effectively true for the registry in
>> this
>>>>>>>>>> case.
>>>>>>>>>>> Did we consider using an interface instead of the concrete
>> class? It
>>>>>>>>>> seems
>>>>>>>>>>> that a lot of these things were discussed in the PR, so it would
>> be
>>>>>>>> good
>>>>>>>>>> to
>>>>>>>>>>> have a summary in the KIP too.
>>>>>>>>>>>
>>>>>>>>>>> Ismael
>>>>>>>>>>>
>>>>>>>>>>> On Fri, Jan 6, 2017 at 9:10 PM, Eno Thereska <
>>>>> eno.there...@gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> So the KIP proposes exposing the metrics registry (second
>> paragraph
>>>>>>>>>> under
>>>>>>>>>>>> motivation). The community has indicated that they would like
>> to 1.
>>>>>>>>>>> access
>>>>>>>>>>>> all the metrics and 2. register their own. We provide some
>> helper
>>>>>>>>>>>> interfaces for them to register throughput and latency metrics,
>> but
>>>>>>>>>>>> ultimately we felt it's best for them to have access to the full
>>>>>>>>>> registry
>>>>>>>>>>>> as well. This is because application code is now intertwined
>> with
>>>>> the
>>>>>>>>>>>> streams library and we don't want to limit the kinds of metrics
>>>>> they
>>>>>>>>>>> might
>>>>>>>>>>>> want to register, nor do we necessarily want to provide yet
>> another
>>>>>>>>>>> wrapper
>>>>>>>>>>>> around Metrics.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Eno
>>>>>>>>>>>>
>>>>>>>>>>>>> On 6 Jan 2017, at 20:34, Ismael Juma <ism...@juma.me.uk>
>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for the KIP. Sounds useful. One thing that wasn't made
>>>>> clear
>>>>>>>>>> is
>>>>>>>>>>>> that
>>>>>>>>>>>>> we are exposing `Metrics` as a public class for the first time.
>>>>>>>>>> Neither
>>>>>>>>>>>> the
>>>>>>>>>>>>> consumer or producer expose it at the moment. Do we want to
>> expose
>>>>>>>>>> the
>>>>>>>>>>>>> whole class or would it be better to expose a more limited
>>>>> interface?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ismael
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sat, Dec 31, 2016 at 4:26 AM, Aarti Gupta <
>>>>> aartigup...@gmail.com>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I would like to start the discussion on KIP-104: Granular
>> Sensors
>>>>>>>>>> for
>>>>>>>>>>>>>> Streams
>>>>>>>>>>>>>> <https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>>>>>>>>>>>> 104%3A+Granular+Sensors+for+Streams?src=contextnavchildmode>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>>> <https://cwiki.apache.org/confluence/pages/viewpage.
>>>>>>>>>>>> action?pageId=67636480
>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looking forward to your feedback.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Aarti and Eno
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> -- Guozhang
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>
>>
>
>
> --
> -- Guozhang
