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