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