Bumping this thread. If there are no other comments, I'll restart a vote in the next couple of weeks.
Thanks, Mickael On Thu, Apr 25, 2024 at 3:28 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > > Hi Greg, > > Thanks for taking a close look at the KIP. > > 1/2) I understand your concern about leaking resources. I've played a > bit more with the code and I think we should be able to handle the > closing of the metrics internally rather than delegating it to the > user code. I built a small PoC inspired by your MonitorablePlugin > class example and that looked fine. I think we can even keep that > class internal. I updated the KIP accordingly. > > 3) An earlier version of the proposal used connector and task contexts > to allow them to retrieve their PluginMetrics instance. In a previous > comment Chris suggested switching to implementing Monitorable for > consistency. I think both approaches have pros and cons. I agree with > you that implementing Monitorable with cause compatibility issues with > older Connect runtimes. For that reason, I'm leaning towards > reintroducing the context mechanism. However we would still have this > issue with Converters/Transformations/Predicates. I think it's > typically a bit less problematic with these plugins but it's worth > considering the different approaches. If we can't agree on an approach > we can exclude Connect from this proposal and revisit it at a later > point. > > 4) If this KIP is accepted, I plan to follow up with another KIP to > make MirrorMaker use this mechanism instead of the custom metrics > logic it currently uses. > > Thanks, > Mickael > > > > > On Wed, Apr 24, 2024 at 9:03 PM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > > Hi Matthias, > > > > I'm not sure making the Monitorable interface Closeable really solves the > > issue. > > Ultimately you need to understand the lifecycle of a plugin to > > determine when it make sense to close it and which part of the code is > > responsible for doing it. I'd rather have this described properly in > > the interface of the plugin itself than it being a side effect of > > implementing Monitorable. > > > > Looking at Streams, as far as I can tell the only pluggable interfaces > > that are Closeable today are the Serdes. It seems Streams can accept > > Serdes instances created by the user [0]. In that case, I think it's > > probably best to ignore Streams in this KIP. Nothing should prevent > > Streams for adopting it, in a way that makes sense for Streams, in a > > future KIP if needed. > > > > 0: > > https://github.com/apache/kafka/blob/trunk/streams/examples/src/main/java/org/apache/kafka/streams/examples/wordcount/WordCountDemo.java#L84 > > > > Thanks, > > Mickael > > > > > > > > > > > > On Fri, Feb 9, 2024 at 1:15 AM Greg Harris <greg.har...@aiven.io.invalid> > > wrote: > > > > > > Hi Mickael, > > > > > > Thanks for the KIP, this looks like a great change! > > > > > > 1. I see that one of my concerns was already discussed, and appears to > > > have been concluded with: > > > > > > > I considered Chris' idea of automatically removing metrics but decided > > > > to leave that responsibility to the plugins. > > > > > > After chasing resource leaks for the last few years, I've internalized > > > that preventing leaks through careful implementation is always > > > inadequate, and that leaks need to be prevented by design. > > > If a leak is possible in a design, then we should count on it > > > happening somewhere as a certainty, and should be prepared for the > > > behavior afterwards. > > > > > > Chris already brought up one of the negative behaviors: Connect > > > plugins which are cancelled may keep their metrics open past the point > > > that a replacement plugin is instantiated. > > > This will have the effect of showing incorrect metrics, which is > > > harmful and confusing for operators. > > > If you are constantly skeptical of the accuracy of your metrics, and > > > there is no "source of truth" to verify against, then what use are the > > > metrics? > > > > > > I think that managing the lifecycle of the PluginMetrics on the > > > framework side would be acceptable if we had an internal class like > > > the following, to keep a reference to the metrics adjacent to the > > > plugin: > > > class MonitorablePlugin<T> implements Supplier<T>, Closeable { > > > MonitorablePlugin(T plugin, PluginMetrics metrics); > > > } > > > I already believe that we need similar wrapper classes in Connect [1] > > > to manage classloader swapping & exception safety, and this simpler > > > interface could be applied to non-connect call-sites that don't need > > > to swap the classloader. > > > > > > 2. Your "MyInterceptor" class doesn't have a "metrics" field, and > > > doesn't perform a null-check on the field in close(). > > > Keeping the PluginMetrics as an non-final instance variable in every > > > plugin implementation is another burden on the plugin implementations, > > > as they will need to perform null checks in-case the metrics are never > > > initialized, such as in a test environment. > > > It also shows that without the Closeable interface, plugins may not > > > need the PluginMetrics object after the initial setup if they only > > > have a fixed set of sensors that need to be made instance fields. > > > > > > 3. I realized when writing the above that this implicitly sets a > > > minimum framework version for plugins, as the Monitorable interface > > > must exist in order to be able to load the plugin classes. This is a > > > test which confirms that behavior: [2] [3] > > > There's no way for plugins to at-runtime disable plugin metrics when > > > put in an environment without support for them, which was one of the > > > features which was incorporated in KIP-610: [4] which meaningfully > > > changes the control flow of the connector. > > > > > > 4. For plugins which themselves use AbstractConfig (nearly all of > > > them) and use the getConfiguredInstance methods to instantiate plugins > > > of their own (e.g. MirrorMaker2 [5]), could these plugins also use > > > PluginMetrics from the framework, with a small change to the > > > signatures or class hierarchy? > > > > > > Thanks so much! > > > Greg > > > > > > [1] https://issues.apache.org/jira/browse/KAFKA-14670 > > > [2] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/runtime/src/test/resources/test-plugins/bad-packaging/test/plugins/MissingSuperclassConverter.java#L28-L30 > > > [3] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java#L233-L239 > > > [4] > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors > > > [5] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorCheckpointConfig.java#L114 > > > > > > On Thu, Feb 8, 2024 at 11:49 AM Matthias J. Sax <mj...@apache.org> wrote: > > > > > > > > Still need to digest the KIP, but one thing coming to mind: > > > > > > > > Instead of requiring existing interfaces to implement `Closable`, would > > > > it make sense to make `Monitorable extends Closable` to sidestep this > > > > issue? > > > > > > > > > > > > -Matthias > > > > > > > > On 1/25/24 9:03 AM, Mickael Maison wrote: > > > > > Hi Luke, > > > > > > > > > > The reason vary for each plugin, I've added details to most plugins in > > > > > the table. > > > > > The plugins without an explanation are all from Streams. I admit I > > > > > don't know these interfaces enough to decide if it makes sense making > > > > > them closeable and instrumenting them. It would be nice to get some > > > > > input from Streams contributors to know. > > > > > > > > > > Thanks, > > > > > Mickael > > > > > > > > > > On Thu, Jan 25, 2024 at 5:50 PM Mickael Maison > > > > > <mickael.mai...@gmail.com> wrote: > > > > >> > > > > >> Hi Tom, > > > > >> > > > > >> Thanks for taking a look at the KIP! > > > > >> > > > > >> 1. Yes I considered several names (see the previous messages in the > > > > >> discussion). KIP-608, which this KIP superseeds, used "monitor()" for > > > > >> the method name. I find "withMetrics()" to be nicer due to the way > > > > >> the > > > > >> method should be used. That said, I'm not attached to the name so if > > > > >> more people prefer "monitor()", or can come up with a better name, > > > > >> I'm > > > > >> happy to make the change. I updated the javadoc to clarify the usage > > > > >> and mention when to close the PluginMetrics instance. > > > > >> > > > > >> 2. Yes I added a note to the PluginMetrics interface > > > > >> > > > > >> 3. I used this exception to follow the behavior of > > > > >> Metrics.addMetric() > > > > >> which throws IllegalArgumentException if a metric with the same name > > > > >> already exist. > > > > >> > > > > >> 4. I added details to the javadoc > > > > >> > > > > >> Thanks, > > > > >> Mickael > > > > >> > > > > >> > > > > >> On Thu, Jan 25, 2024 at 10:32 AM Luke Chen <show...@gmail.com> wrote: > > > > >>> > > > > >>> Hi Mickael, > > > > >>> > > > > >>> Thanks for the KIP. > > > > >>> The motivation and solution makes sense to me. > > > > >>> > > > > >>> Just one question: > > > > >>> If we could extend `closable` for Converter plugin, why don't we do > > > > >>> that > > > > >>> for the "Unsupported Plugins" without close method? > > > > >>> I don't say we must do that in this KIP, but maybe you could add > > > > >>> the reason > > > > >>> in the "rejected alternatives". > > > > >>> > > > > >>> Thanks. > > > > >>> Luke > > > > >>> > > > > >>> On Thu, Jan 25, 2024 at 3:46 PM Slathia p <slat...@votecgroup.com> > > > > >>> wrote: > > > > >>> > > > > >>>> Hi Team, > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> Greetings, > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> Apologies for the delay in reply as I was down with flu. > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> We actually reached out to you for IT/ SAP/ Oracle/ Infor / > > > > >>>> Microsoft > > > > >>>> “VOTEC IT SERVICE PARTNERSHIP” “IT SERVICE OUTSOURCING” “ > > > > >>>> “PARTNER SERVICE > > > > >>>> SUBCONTRACTING” > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> We have very attractive newly introduce reasonably price PARTNER IT > > > > >>>> SERVICE ODC SUBCONTRACTING MODEL in USA, Philippines, India and > > > > >>>> Singapore > > > > >>>> etc with White Label Model. > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> Our LOW COST IT SERVICE ODC MODEL eliminate the cost of expensive > > > > >>>> employee > > > > >>>> payroll, Help partner to get profit more than 50% on each > > > > >>>> project.. ..We > > > > >>>> really mean it. > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> We are already working with platinum partner like NTT DATA, NEC > > > > >>>> Singapore, > > > > >>>> Deloitte, Hitachi consulting. ACCENTURE, Abeam Singapore etc. > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> Are u keen to understand VOTEC IT SERVICE MODEL PARTNERSHIP > > > > >>>> offerings? > > > > >>>> > > > > >>>> > > > > >>>> > > > > >>>> Let us know your availability this week OR Next week?? We can > > > > >>>> arrange > > > > >>>> discussion with Partner Manager. > > > > >>>>> On 01/25/2024 9:56 AM +08 Tom Bentley <tbent...@redhat.com> wrote: > > > > >>>>> > > > > >>>>> > > > > >>>>> Hi Mickael, > > > > >>>>> > > > > >>>>> Thanks for the KIP! I can tell a lot of thought went into this. I > > > > >>>>> have a > > > > >>>>> few comments, but they're all pretty trivial and aimed at making > > > > >>>>> the > > > > >>>>> correct use of this API clearer to implementors. > > > > >>>>> > > > > >>>>> 1. Configurable and Reconfigurable both use a verb in the > > > > >>>>> imperative mood > > > > >>>>> for their method name. Monitorable doesn't, which initially > > > > >>>>> seemed a bit > > > > >>>>> inconsistent to me, but I think your intention is to allow > > > > >>>>> plugins to > > > > >>>>> merely retain a reference to the PluginMetrics, and allow > > > > >>>>> registering > > > > >>>>> metrics at any later point? If that's the case you could add > > > > >>>>> something > > > > >>>> like > > > > >>>>> "Plugins can register and unregister metrics using the given > > > > >>>> PluginMetrics > > > > >>>>> at any point in their lifecycle prior to their close method being > > > > >>>>> called" > > > > >>>>> to the javadoc to make clear how this can be used. > > > > >>>>> 2. I assume PluginMetrics will be thread-safe? We should document > > > > >>>>> that as > > > > >>>>> part of the contract. > > > > >>>>> 3. I don't think IAE is quite right for duplicate metrics. In > > > > >>>>> this case > > > > >>>> the > > > > >>>>> arguments themselves are fine, it's the current state of the > > > > >>>> PluginMetrics > > > > >>>>> which causes the problem. If the earlier point about plugins being > > > > >>>> allowed > > > > >>>>> to register and unregister metrics at any point is correct then > > > > >>>>> this > > > > >>>>> exception could be thrown after configuration time. That being > > > > >>>>> the case I > > > > >>>>> think a new exception type might be clearer. > > > > >>>>> 4. You define some semantics for PluginMetrics.close(): It might > > > > >>>>> be a > > > > >>>> good > > > > >>>>> idea to override the inherited method and add that as javadoc. > > > > >>>>> 5. You say "It will be the responsibility of the plugin that > > > > >>>>> creates > > > > >>>>> metrics to call close() of the PluginMetrics instance they were > > > > >>>>> given to > > > > >>>>> remove their metrics." But you don't provide any guidance to > > > > >>>>> users about > > > > >>>>> when they need to do this. I guess that they should be doing this > > > > >>>>> in > > > > >>>> their > > > > >>>>> plugin's close method (and that's why you're only adding > > > > >>>>> Monitorable to > > > > >>>>> plugins which implement Closeable and AutoCloseable), but if > > > > >>>>> that's the > > > > >>>>> case then let's say so in the Javadoc. > > > > >>>>> > > > > >>>>> Thanks again, > > > > >>>>> > > > > >>>>> Tom > > > > >>>>> > > > > >>>>> On Wed, 13 Dec 2023 at 06:09, Mickael Maison > > > > >>>>> <mickael.mai...@gmail.com> > > > > >>>>> wrote: > > > > >>>>> > > > > >>>>>> Hi, > > > > >>>>>> > > > > >>>>>> I've not received any feedback since I updated the KIP. > > > > >>>>>> I'll wait a few more days and if there's no further feedback I'll > > > > >>>> start a > > > > >>>>>> vote. > > > > >>>>>> > > > > >>>>>> Thanks, > > > > >>>>>> Mickael > > > > >>>>>> > > > > >>>>>> On Tue, Nov 7, 2023 at 6:29 PM Mickael Maison < > > > > >>>> mickael.mai...@gmail.com> > > > > >>>>>> wrote: > > > > >>>>>>> > > > > >>>>>>> Hi, > > > > >>>>>>> > > > > >>>>>>> A bit later than initially planned I finally restarted looking > > > > >>>>>>> at > > > > >>>> this > > > > >>>>>> KIP. > > > > >>>>>>> > > > > >>>>>>> I made a few significant changes to the proposed APIs. > > > > >>>>>>> I considered Chris' idea of automatically removing metrics but > > > > >>>> decided > > > > >>>>>>> to leave that responsibility to the plugins. All plugins that > > > > >>>>>>> will > > > > >>>>>>> support this feature have close/stop methods and will need to > > > > >>>>>>> close > > > > >>>>>>> their PluginMetrics instance. This simplifies the required > > > > >>>>>>> changes a > > > > >>>>>>> lot and I think it's not a big ask on users implementing > > > > >>>>>>> plugins. > > > > >>>>>>> > > > > >>>>>>> Thanks, > > > > >>>>>>> Mickael > > > > >>>>>>> > > > > >>>>>>> On Tue, May 30, 2023 at 11:32 AM Mickael Maison > > > > >>>>>>> <mickael.mai...@gmail.com> wrote: > > > > >>>>>>>> > > > > >>>>>>>> Hi Jorge, > > > > >>>>>>>> > > > > >>>>>>>> There are a few issues with the current proposal. Once 3.5 is > > > > >>>>>>>> out, > > > > >>>> I > > > > >>>>>>>> plan to start looking at this again. > > > > >>>>>>>> > > > > >>>>>>>> Thanks, > > > > >>>>>>>> Mickael > > > > >>>>>>>> > > > > >>>>>>>> On Mon, May 15, 2023 at 3:19 PM Jorge Esteban Quilcate Otoya > > > > >>>>>>>> <quilcate.jo...@gmail.com> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>> Hi Mickael, > > > > >>>>>>>>> > > > > >>>>>>>>> Just to check the status of this KIP as it looks very useful. > > > > >>>>>>>>> I > > > > >>>> can > > > > >>>>>> see how > > > > >>>>>>>>> new Tiered Storage interfaces and plugins may benefit from > > > > >>>>>>>>> this. > > > > >>>>>>>>> > > > > >>>>>>>>> Cheers, > > > > >>>>>>>>> Jorge. > > > > >>>>>>>>> > > > > >>>>>>>>> On Mon, 6 Feb 2023 at 23:00, Chris Egerton > > > > >>>> <chr...@aiven.io.invalid> > > > > >>>>>> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>>> Hi Mickael, > > > > >>>>>>>>>> > > > > >>>>>>>>>> I agree that adding a getter method for Monitorable isn't > > > > >>>> great. A > > > > >>>>>> few > > > > >>>>>>>>>> alternatives come to mind: > > > > >>>>>>>>>> > > > > >>>>>>>>>> 1. Introduce a new ConfiguredInstance<T> (name subject to > > > > >>>> change) > > > > >>>>>> interface > > > > >>>>>>>>>> that wraps an instance of type T, but also contains a getter > > > > >>>>>> method for any > > > > >>>>>>>>>> PluginMetrics instances that the plugin was instantiated with > > > > >>>>>> (which may > > > > >>>>>>>>>> return null either if no PluginMetrics instance could be > > > > >>>> created > > > > >>>>>> for the > > > > >>>>>>>>>> plugin, or if it did not implement the Monitorable > > > > >>>>>>>>>> interface). > > > > >>>>>> This can be > > > > >>>>>>>>>> the return type of the new > > > > >>>> AbstractConfig::getConfiguredInstance > > > > >>>>>> variants. > > > > >>>>>>>>>> It would give us room to move forward with other > > > > >>>>>> plugin-for-your-plugin > > > > >>>>>>>>>> style interfaces without cluttering things up with getter > > > > >>>> methods. > > > > >>>>>> We could > > > > >>>>>>>>>> even add a close method to this interface which would handle > > > > >>>>>> cleanup of all > > > > >>>>>>>>>> extra resources allocated for the plugin by the runtime, and > > > > >>>> even > > > > >>>>>> possibly > > > > >>>>>>>>>> the plugin itself. > > > > >>>>>>>>>> > > > > >>>>>>>>>> 2. Break out the instantiation logic into two separate steps. > > > > >>>> The > > > > >>>>>> first > > > > >>>>>>>>>> step, creating a PluginMetrics instance, can be either > > > > >>>>>>>>>> private > > > > >>>> or > > > > >>>>>> public > > > > >>>>>>>>>> API. The second step, providing that PluginMetrics instance > > > > >>>>>>>>>> to > > > > >>>> a > > > > >>>>>>>>>> newly-created object, can be achieved with a small tweak of > > > > >>>>>>>>>> the > > > > >>>>>> proposed > > > > >>>>>>>>>> new methods for the AbstractConfig class; instead of > > > > >>>>>>>>>> accepting > > > > >>>> a > > > > >>>>>> Metrics > > > > >>>>>>>>>> instance, they would now accept a PluginMetrics instance. For > > > > >>>> the > > > > >>>>>> first > > > > >>>>>>>>>> step, we might even introduce a new CloseablePluginMetrics > > > > >>>>>> interface which > > > > >>>>>>>>>> would be the return type of whatever method we use to create > > > > >>>> the > > > > >>>>>>>>>> PluginMetrics instance. We can track that > > > > >>>> CloseablePluginMetrics > > > > >>>>>> instance > > > > >>>>>>>>>> in tandem with the plugin it applies to, and close it when > > > > >>>> we're > > > > >>>>>> done with > > > > >>>>>>>>>> the plugin. > > > > >>>>>>>>>> > > > > >>>>>>>>>> I know that this adds some complexity to the API design and > > > > >>>> some > > > > >>>>>>>>>> bookkeeping responsibilities for our implementation, but I > > > > >>>> can't > > > > >>>>>> shake the > > > > >>>>>>>>>> feeling that if we don't feel comfortable taking on the > > > > >>>>>> responsibility to > > > > >>>>>>>>>> clean up these resources ourselves, it's not really fair to > > > > >>>>>>>>>> ask > > > > >>>>>> users to > > > > >>>>>>>>>> handle it for us instead. And with the case of Connect, > > > > >>>> sometimes > > > > >>>>>> Connector > > > > >>>>>>>>>> or Task instances that are scheduled for shutdown block for a > > > > >>>>>> while, at > > > > >>>>>>>>>> which point we abandon them and bring up new instances in > > > > >>>>>>>>>> their > > > > >>>>>> place; it'd > > > > >>>>>>>>>> be nice to have a way to forcibly clear out all the metrics > > > > >>>>>> allocated by > > > > >>>>>>>>>> that Connector or Task instance before bringing up a new one, > > > > >>>> in > > > > >>>>>> order to > > > > >>>>>>>>>> prevent issues due to naming conflicts. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Regardless, and whether or not it ends up being relevant to > > > > >>>> this > > > > >>>>>> KIP, I'd > > > > >>>>>>>>>> love to see a new Converter::close method. It's irked me for > > > > >>>> quite > > > > >>>>>> a while > > > > >>>>>>>>>> that we don't have one already. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Cheers, > > > > >>>>>>>>>> > > > > >>>>>>>>>> Chris > > > > >>>>>>>>>> > > > > >>>>>>>>>> On Mon, Feb 6, 2023 at 1:50 PM Mickael Maison < > > > > >>>>>> mickael.mai...@gmail.com> > > > > >>>>>>>>>> wrote: > > > > >>>>>>>>>> > > > > >>>>>>>>>>> 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 > > > > >>>>>>>>>>>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>>> > > > > >>>>>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>> > > > > >>>>>> > > > > >>>> > > > > >>>> > > > > > > > >
