Hi Yash, I added a sentence to the sensor() method mentioning the sensor name must only be unique per plugin. Regarding having getters for sensors and metrics I considered this not strictly necessary as I expect the metrics logic in most plugins to be relatively simple. If you have a use case that would benefit from these methods, let me know I will reconsider.
Thanks, Mickael On Fri, Feb 3, 2023 at 9:16 AM Yash Mayya <yash.ma...@gmail.com> wrote: > > Hi Mickael, > > Thanks for the updates. > > > the PluginMetrics implementation will append a > > suffix to sensor names to unique identify > > the plugin (based on the class name and tags). > > Can we call this out explicitly in the KIP, since it is important to avoid > clashes in sensor naming? Also, should we allow plugins to retrieve sensors > from `PluginMetrics` if we can check / verify that they own the sensor > (based on the suffix)? > > Other than the above minor points, this looks good to me now! > > Thanks, > Yash > > On Fri, Feb 3, 2023 at 2:29 AM Chris Egerton <chr...@aiven.io.invalid> > wrote: > > > Hi Mickael, > > > > This is looking great. I have one small question left but I do not consider > > it a blocker. > > > > What is the intended use case for PluginMetrics::close? To me at least, it > > implies that plugin developers will be responsible for invoking that method > > themselves in order to clean up metrics that they've created, but wouldn't > > we want the runtime (i.e., KafkaProducer class, Connect framework, etc.) to > > handle that automatically when the resource that the plugin applies to is > > closed? > > > > Cheers, > > > > Chris > > > > On Thu, Jan 26, 2023 at 10:22 AM Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > Hi Yash, > > > > > > 1) To avoid conflicts with other sensors, the PluginMetrics > > > implementation will append a suffix to sensor names to unique identify > > > the plugin (based on the class name and tags). Also I changed the > > > semantics of the sensor() method to only create sensors (originally it > > > was get or create). If a sensor with the same name already exists, the > > > method will throw. > > > 2) Tags will be automatically added to metrics and sensors to unique > > > identify the plugin. For Connect plugins, the connector name, task id > > > and alias can be added if available. The class implementing > > > PluginMetrics will be similar to ConnectMetrics, as in it will provide > > > a simplified API wrapping Metrics. I'm planning to use PluginMetrics > > > for Connect plugin too and should not need to interact with > > > ConnectMetrics. > > > 3) Right, I fixed the last rejected alternative. > > > > > > Thanks, > > > Mickael > > > > > > On Thu, Jan 26, 2023 at 4:04 PM Mickael Maison <mickael.mai...@gmail.com > > > > > > wrote: > > > > > > > > Hi Federico, > > > > > > > > - The metricName() method does not register anything, it just builds a > > > > MetricName instance which is just a container holding a name, group, > > > > description and tags for a metric. Each time it is called, it returns > > > > a new instance. If called with the same arguments, the returned value > > > > will be equal. > > > > - Initially I just copied the API of Metrics. I made some small > > > > changes so the metric and sensor methods are a bit more similar > > > > - Good catch! I fixed the example. > > > > > > > > Thanks, > > > > Mickael > > > > > > > > > > > > On Thu, Jan 26, 2023 at 3:54 PM Mickael Maison < > > mickael.mai...@gmail.com> > > > wrote: > > > > > > > > > > Hi Chris, > > > > > > > > > > 1) I updated the KIP to only mention the interface. > > > > > 2) This was a mistake. I've added ReplicationPolicy to the list of > > > plugins. > > > > > > > > > > Thanks, > > > > > Mickael > > > > > > > > > > On Tue, Jan 24, 2023 at 11:16 AM Yash Mayya <yash.ma...@gmail.com> > > > wrote: > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > Thanks for the updated KIP, this is looking really good! I had a > > > couple > > > > > > more questions - > > > > > > > > > > > > 1) Sensor names need to be unique across all groups for a `Metrics` > > > > > > instance. How are we planning to avoid naming clashes (both between > > > > > > different plugins as well as with pre-defined sensors)? > > > > > > > > > > > > 2) Connect has a `ConnectMetrics` wrapper around `Metrics` via > > which > > > > > > rebalance / worker / connector / task metrics are recorded. Could > > you > > > > > > please elaborate in the KIP how the plugin metrics for connectors / > > > tasks > > > > > > will inter-operate with this? > > > > > > > > > > > > Another minor point is that the third rejected alternative appears > > > to be an > > > > > > incomplete sentence? > > > > > > > > > > > > Thanks, > > > > > > Yash > > > > > > > > > > > > On Fri, Jan 13, 2023 at 10:56 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > I've updated the KIP based on the feedback. > > > > > > > > > > > > > > Now instead of receiving a Metrics instance, plugins get access > > to > > > > > > > PluginMetrics that exposes a much smaller API. I've removed the > > > > > > > special handling for connectors and tasks and they must now > > > implement > > > > > > > the Monitorable interface as well to use this feature. Finally > > the > > > > > > > goal is to allow all plugins (apart from metrics reporters) to > > use > > > > > > > this feature. I've listed them all (there are over 30 pluggable > > > APIs) > > > > > > > but I've not added the list in the KIP. The reason is that new > > > plugins > > > > > > > could be added in the future and instead I'll focus on adding > > > support > > > > > > > in all the place that instantiate classes. > > > > > > > > > > > > > > Thanks, > > > > > > > Mickael > > > > > > > > > > > > > > On Tue, Jan 10, 2023 at 7:00 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Chris/Yash, > > > > > > > > > > > > > > > > Thanks for taking a look and providing feedback. > > > > > > > > > > > > > > > > 1) Yes you're right, when using incompatible version, metrics() > > > would > > > > > > > > trigger NoSuchMethodError. I thought using the context to pass > > > the > > > > > > > > Metrics object would be more idiomatic for Connect but maybe > > > > > > > > implementing Monitorable would be simpler. It would also allow > > > other > > > > > > > > Connect plugins (transformations, converters, etc) to register > > > > > > > > metrics. So I'll make that change. > > > > > > > > > > > > > > > > 2) As mentioned in the rejected alternatives, I considered > > > having a > > > > > > > > PluginMetrics class/interface with a limited API. But since > > > Metrics is > > > > > > > > part of the public API, I thought it would be simpler to reuse > > > it. > > > > > > > > That said you bring interesting points so I took another look > > > today. > > > > > > > > It's true that the Metrics API is pretty complex and most > > > methods are > > > > > > > > useless for plugin authors. I'd expect most use cases only need > > > one > > > > > > > > addMetric and one sensor methods. Rather than subclassing > > > Metrics, I > > > > > > > > think a delegate/forwarding pattern might work well here. A > > > > > > > > PluginMetric class would forward its method to the Metrics > > > instance > > > > > > > > and could perform some basic validations such as only letting > > > plugins > > > > > > > > delete metrics they created, or automatically injecting tags > > > with the > > > > > > > > class name for example. > > > > > > > > > > > > > > > > 3) Between the clients, brokers, streams and connect, Kafka has > > > quite > > > > > > > > a lot! In practice I think registering metrics should be > > > beneficial > > > > > > > > for all plugins, I think the only exception would be metrics > > > reporters > > > > > > > > (which are instantiated before the Metrics object). I'll try to > > > build > > > > > > > > a list of all plugin types and add that to the KIP. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Dec 27, 2022 at 4:54 PM Chris Egerton > > > <chr...@aiven.io.invalid> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Yash, > > > > > > > > > > > > > > > > > > Yes, a default no-op is exactly what I had in mind should the > > > > > > > Connector and > > > > > > > > > Task classes implement the Monitorable interface. > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > On Tue, Dec 20, 2022 at 2:46 AM Yash Mayya < > > > yash.ma...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > Thanks for creating this KIP, this will be a super useful > > > feature to > > > > > > > > > > enhance existing connectors in the Kafka Connect ecosystem. > > > > > > > > > > > > > > > > > > > > I have some similar concerns to the ones that Chris has > > > outlined > > > > > > > above, > > > > > > > > > > especially with regard to directly exposing Connect's > > > Metrics object > > > > > > > to > > > > > > > > > > plugins. I believe it would be a lot friendlier to > > > developers if we > > > > > > > instead > > > > > > > > > > exposed wrapper methods in the context classes - such as > > one > > > for > > > > > > > > > > registering a new metric, one for recording metric values > > > and so on. > > > > > > > This > > > > > > > > > > would also have the added benefit of minimizing the surface > > > area for > > > > > > > > > > potential misuse by custom plugins. > > > > > > > > > > > > > > > > > > > > > for connectors and tasks they should handle the > > > > > > > > > > > metrics() method returning null when deployed on > > > > > > > > > > > an older runtime. > > > > > > > > > > > > > > > > > > > > I believe this won't be the case, and instead they'll need > > > to handle > > > > > > > a > > > > > > > > > > `NoSuchMethodError` right? This is similar to previous KIPs > > > that > > > > > > > added > > > > > > > > > > methods to connector context classes and will arise due to > > an > > > > > > > > > > incompatibility between the `connect-api` dependency that a > > > plugin > > > > > > > will be > > > > > > > > > > compiled against versus what it will actually get at > > runtime. > > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > > WDYT about having the Connector and Task classes > > > > > > > > > > > implement the Monitorable interface, both for > > > > > > > > > > > consistency's sake, and to prevent classloading > > > > > > > > > > > headaches? > > > > > > > > > > > > > > > > > > > > Are you suggesting that the framework should configure > > > connectors / > > > > > > > tasks > > > > > > > > > > with a Metrics instance during their startup rather than > > the > > > > > > > connector / > > > > > > > > > > task asking the framework to provide one? In this case, I'm > > > guessing > > > > > > > you're > > > > > > > > > > envisioning a default no-op implementation for the metrics > > > > > > > configuration > > > > > > > > > > method rather than the framework having to handle the case > > > where the > > > > > > > > > > connector was compiled against an older version of Connect > > > right? > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Yash > > > > > > > > > > > > > > > > > > > > On Wed, Nov 30, 2022 at 1:38 AM Chris Egerton > > > > > > > <chr...@aiven.io.invalid> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Mickael, > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP! This seems especially useful to > > reduce > > > the > > > > > > > > > > > implementation cost and divergence in behavior for > > > connectors that > > > > > > > choose > > > > > > > > > > > to publish their own metrics. > > > > > > > > > > > > > > > > > > > > > > My initial thoughts: > > > > > > > > > > > > > > > > > > > > > > 1. Are you certain that the default implementation of the > > > "metrics" > > > > > > > > > > method > > > > > > > > > > > for the various connector/task context classes will be > > > used on > > > > > > > older > > > > > > > > > > > versions of the Connect runtime? My understanding was > > that > > > a > > > > > > > > > > > NoSuchMethodError (or some similar classloading > > exception) > > > would be > > > > > > > > > > thrown > > > > > > > > > > > in that case. If that turns out to be true, WDYT about > > > having the > > > > > > > > > > Connector > > > > > > > > > > > and Task classes implement the Monitorable interface, > > both > > > for > > > > > > > > > > > consistency's sake, and to prevent classloading > > headaches? > > > > > > > > > > > > > > > > > > > > > > 2. Although I agree that administrators should be careful > > > about > > > > > > > which > > > > > > > > > > > plugins they run on their clients, Connect clusters, > > etc., > > > I > > > > > > > wonder if > > > > > > > > > > > there might still be value in wrapping the Metrics class > > > behind a > > > > > > > new > > > > > > > > > > > interface, for a few reasons: > > > > > > > > > > > > > > > > > > > > > > a. Developers and administrators may still make > > > mistakes, and if > > > > > > > we can > > > > > > > > > > > reduce the blast radius by preventing plugins from, e.g., > > > closing > > > > > > > the > > > > > > > > > > > Metrics instance we give them, it may be worth it. This > > > could also > > > > > > > be > > > > > > > > > > > accomplished by forbidding plugins from invoking these > > > methods, and > > > > > > > > > > giving > > > > > > > > > > > them a subclass of Metrics that throws > > > > > > > UnsupportedOperationException from > > > > > > > > > > > these methods. > > > > > > > > > > > > > > > > > > > > > > b. If we don't know of any reasonable use cases for > > > closing the > > > > > > > > > > instance, > > > > > > > > > > > adding new reporters, removing metrics, etc., it can make > > > the API > > > > > > > cleaner > > > > > > > > > > > and easier for developers to grok if they don't even have > > > the > > > > > > > option to > > > > > > > > > > do > > > > > > > > > > > any of those things. > > > > > > > > > > > > > > > > > > > > > > c. Interoperability between plugins that implement > > > Monitorable > > > > > > > and > > > > > > > > > > their > > > > > > > > > > > runtime becomes complicated. For example, a connector may > > > be built > > > > > > > > > > against > > > > > > > > > > > a version of Kafka that introduces new methods for the > > > Metrics > > > > > > > class, > > > > > > > > > > which > > > > > > > > > > > introduces risks of incompatibility if its developer > > > chooses to > > > > > > > take > > > > > > > > > > > advantage of these methods without realizing that they > > > will not be > > > > > > > > > > > available on Connect runtimes built against an older > > > version of > > > > > > > Kafka. > > > > > > > > > > With > > > > > > > > > > > a wrapper interface, we at least have a chance to isolate > > > these > > > > > > > issues so > > > > > > > > > > > that the Metrics class can be expanded without adding > > > footguns for > > > > > > > > > > plugins > > > > > > > > > > > that implement Monitorable, and to call out potential > > > compatibility > > > > > > > > > > > problems in documentation more clearly if/when we do > > > expand the > > > > > > > wrapper > > > > > > > > > > > interface. > > > > > > > > > > > > > > > > > > > > > > 3. It'd be nice to see a list of exactly which plugins > > > will be > > > > > > > able to > > > > > > > > > > take > > > > > > > > > > > advantage of the new Monitorable interface. > > > > > > > > > > > > > > > > > > > > > > Looking forward to your thoughts! > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 7, 2022 at 11:42 AM Mickael Maison < > > > > > > > mickael.mai...@gmail.com > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > I have opened KIP-877 to make it easy for plugins and > > > connectors > > > > > > > to > > > > > > > > > > > > register their own metrics: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://eu01.z.antigena.com/l/9lWv8kbU9CKs2LajwgfKF~yMNQVM7rWRxYmYVNrHU_2nQbisTiXYZdowNfQ-NcgF1uai2lk-sv6hJASnbdr_gqVwyVae_~y-~oq5yQFgO_-IHD3UGDn3lsIyauAG2tG6giPJH-9yCYg3Hwe26sm7nep258qB6SNXRwpaVxbU3SaVTybfLQVvTn_uUlHKMhmVnpnc1dUnusK6x4j8JPPQQ1Ce~rrg-nsSLouHHMf0ewmpsFNy4BcbMaqHd4Y > > > > > > > > > > > > > > > > > > > > > > > > Let me know if you have any feedback or suggestions. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
