Hi Chris, I envisioned plugins to be responsible for closing the PluginMetrics instance. This is mostly important for Connect connector plugins as they can be closed while the runtime keeps running (and keeps its Metrics instance). As far as I can tell, other plugins should only be closed when their runtime closes, so we should not be leaking metrics even if those don't explicitly call close().
For Connect plugin, as you said, it would be nice to automatically close their associated PluginMetrics rather than relying on user logic. The issue is that with the current API there's no way to retrieve the PluginMetrics instance once it's passed to the plugin. I'm not super keen on having a getter method on the Monitorable interface and tracking PluginMetrics instances associated with each plugin would require a lot of changes. I just noticed Converter does not have a close() method so it's problematic for that type of plugin. The other Connect plugins all have close() or stop() methods. I wonder if the simplest is to make Converter extend Closeable. WDYT? Thanks, Mickael On Mon, Feb 6, 2023 at 6:39 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
