Adds LogStash::PluginMetadata for simple key/value plugin metadata#10691
Adds LogStash::PluginMetadata for simple key/value plugin metadata#10691robbavey wants to merge 4 commits intoelastic:masterfrom
Conversation
We need a way for a plugin to register simple metadata about external resources it connects to in order to implement a Monitoring feature in which an Elasticsearch Output Plugin can store the connected cluster's uuid (elastic#10602) Here, we add a generic `LogStash::PluginMetadata` along with a registry, and expose an accessor on `LogStash::Plugin#plugin_metadata` so that instances can access their own metadata object.
Previously the `do_close` method would never be called on a failing plugin, because the retry call stops the code under `ensure` from ever being called.
danhermann
left a comment
There was a problem hiding this comment.
I'm +1 on the metadata cleanup at pipeline shutdown/reload/etc. I do think some thought should be given to how this would work with Java plugins given that they're scheduled to go GA in the release this feature is targeting.
|
@robbavey @danhermann @jsvd Is there anything we can do to help move this along? It is blocking some work that @elastic/stack-monitoring would like to do for 7.1. Thanks! |
|
@danhermann Is adding support for Java plugins something that we could add in a follow up PR? |
I think adding support for Java plugins a bit later is fine if the time-sensitive need for this feature is currently in Ruby plugins. The one thing I think we need to determine beforehand, though, is a uniform interface for plugin metadata that works for both Ruby and Java plugins. The only sticking point I see currently is the requirement that metadata keys be Ruby symbols which doesn't work for Java plugins. |
|
@danhermann on the Java side, we could use interned strings, since ruby symbols are backed by interned strings. I would prefer to move the whole implementation to Java with a JRuby ext wrapper that matches this implementation once we figure that out. |
|
I'm +1 on moving this to Java with a JRuby ext wrapper, but should we first work to merge this to unblock progress, and add that later on? |
@yaauie, perhaps I'm not understanding something. I can see how interned strings would be of some benefit in the Java implementation of the plugin metadata store, but whether a Java string is interned or not can't be expressed in an interface, so I'm not sure how that helps us get to a uniform interface for both Ruby and Java plugins. I'm fine with merging this now to unblock other work so long as the eventual metadata interface for Java plugins contains no Ruby types since that would mean that all Java plugins would then require a JRuby dependency. |
|
+1 to the java interface containing no Ruby types. |
danhermann
left a comment
There was a problem hiding this comment.
LGTM with the above conclusions that the interface exposed to Java plugins won't use Ruby datatypes.
|
Thanks @danhermann! |
Previously the `do_close` method would never be called on a failing plugin, because the retry call stops the code under `ensure` from ever being called. Fixes #10691
We need a way for a plugin to register simple metadata about external resources it connects to in order to implement a Monitoring feature in which an Elasticsearch Output Plugin can store the connected cluster's uuid (#10602) Here, we add a generic `LogStash::PluginMetadata` along with a registry, and expose an accessor on `LogStash::Plugin#plugin_metadata` so that instances can access their own metadata object. Fixes #10691
Previously the `do_close` method would never be called on a failing plugin, because the retry call stops the code under `ensure` from ever being called. Fixes #10691
This change allows the plugin to inform other parts of Logstash about which ES cluster it is connecting to. It is built on top of the Plugin Metadata registry: elastic/logstash#10691 Note: The DLQ tests were written to just assume a clean buffer in the logs but this is not a safe assumption. Now we temporarily replace the logging subsystem with a double during these tests so that we can be assured that we are checking a clean instance.
Continuation of #10636, which will also attempt to clean up after a plugin is shutdown