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 > > > > > > > > > > > > > > > > >
