Expanded plugin constructor capabilities #481

dlvenable · 2021-10-29T19:30:24Z

Description

Introduces the @DataPrepperPluginConstructor annotation
When creating a plugin instance, use the @DataPrepperPluginConstructor annotated-constructor. If not, then use the constructor which takes the PluginSetting or configured model class. Finally, use the default constructor as the last option.
The plugin framework can also inject a PluginMetrics object in the constructor.
Updated some of the simple plugins to use the default constructor where appropriate.
Updated HTTPSource to use a configuration model. It is provided into the constructor, and the PluginMetrics is also now injected.

Issues Resolved

This has some of the proposed functionality from #469.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dlvenable · 2021-10-29T19:34:04Z

data-prepper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/PluginFactory.java

+     * This exists so that this class can still exhibit the correct behavior when creating new plugin
+     * classes. This whole class is going to be deleted in the next major version, as will this method.
+     */
+    public static void dangerousMethod_setPluginFunction(final BiFunction<PluginSetting, Class<?>, Object> newPluginFunction) {


I want to call this method out. This class is in the common package, but will be removed. Because the implementation for creating plugins is now in data-prepper-core, this class cannot use those classes. So, I created this as a way to allow this deprecated class to generate plugins the same way at that the new framework does.

As this class is going away, I felt fine with this quick hack. An alternative would be to remove all the old plugin code now rather than in v2.0.

I am fine with this as well because of the reasons you provided. Thanks!

cmanning09 · 2021-10-30T17:47:56Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+    private PluginArgumentsContext(final Builder builder) {
+        typedArgumentsSuppliers = new HashMap<>();
+
+        Objects.requireNonNull(builder.pluginSetting);


Let's include an error message here.

cmanning09 · 2021-10-30T17:50:23Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+        private Object pluginConfiguration;
+        private PluginSetting pluginSetting;
+
+        Builder withPluginConfiguration(final Object pluginConfiguration) {


What is a pluginConfiguration?

It is the custom configuration supplied which is the type directed by this field: https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-api/src/main/java/com/amazon/dataprepper/model/annotations/DataPrepperPlugin.java#L80

cmanning09 · 2021-10-30T18:07:09Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginCreator.java

+                String.format("Data Prepper plugin requires a constructor which is either: " +
+                        "1. Annotated with @DataPrepperPlugin; " +
+                        "2. Contains a single argument of type %s; or " +
+                        "3. Has a default constructor. " +
+                        "Plugin %s with name %s is missing such constructor.",


We are putting the solution to an error before providing the context for the error. I would recommend putting "Plugin %s with name %s is missing such constructor." first and then the solution.

I would also recommend the solution have a stronger call to action rather than stating the requirements. "Please ensure the plugin's constructor is either:"

cmanning09 · 2021-10-30T18:14:20Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginCreator.java


 class PluginCreator {
    private static final Logger LOG = LoggerFactory.getLogger(PluginCreator.class);

    <T> T newPluginInstance(final Class<T> pluginClass,
-                            final Object pluginConfiguration,
+                            final PluginArgumentsContext pluginConstructionContext,


Why do you want to refer to this as a pluginConstructionContext?

I would recommend just keeping it as pluginArgumentsContext.

I went through a few class names and it appears that IntelliJ didn't pick up renaming the parameter here. I'll fix this up to match.

cmanning09 · 2021-10-30T18:15:13Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+ */
+class PluginArgumentsContext {
+    private final Map<Class<?>, Supplier<Object>> typedArgumentsSuppliers;
+    private final Class<?> singleParameterObject;


Let's be consistent on naming here and refer to this as an argument. singleArgument

cmanning09 · 2021-10-30T18:37:24Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+
+        if(builder.pluginConfiguration != null) {
+            typedArgumentsSuppliers.put(builder.pluginConfiguration.getClass(), () -> builder.pluginConfiguration);
+            singleParameterObject = builder.pluginConfiguration.getClass();


Will this singleParameterObject ever be used when a builder.pluginConfiguration is provided?

Yes, the singleParameterObject is going to be the custom POJO type or the PluginSetting.

cmanning09 · 2021-10-30T18:37:59Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+            return this;
+        }
+
+        Builder withPluginSetting(final PluginSetting pluginSetting) {


There is some nuance to how the parameters impact how the plugins are loaded. I really wish we had plugin documentation. I am worried we are going to build out the plugin documentation at the very end and miss some details. Is there any way we can start building out documentation of this framework now? It doesn't have to be perfect or finalized. Just a README in this package would be nice.

Yes, I can add this next.

I have added the documentation and linked it from the Development guide. As plugin support is extended, I would like to reconsidering the linking, but I believe this is sufficient for now.

cmanning09 · 2021-10-30T18:39:44Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginArgumentsContext.java

+            singleParameterObject = builder.pluginSetting.getClass();
+        }
+
+        typedArgumentsSuppliers.put(PluginMetrics.class, () -> PluginMetrics.fromPluginSetting(builder.pluginSetting));


Should this only be added if the pluginMetrics value is non null?

Also, are we supporting an annotated constructor like pulbic SomePlugin(final PluginSettings pluginSettings, final PluginMetrics pluginMetrics) {...}?

To answer your second question, yes, we are support that type of constructor. But, only if it is annotated with @DataPrepperConstructor. The HTTPSource shows something very similar to this.

I'm not quite sure what your first question is asking. At this point of the code we are only adding a Supplier. So, no PluginMetric is constructor unless the annotated constructor requests it.

cmanning09 · 2021-10-30T19:01:39Z

data-prepper-core/src/main/java/com/amazon/dataprepper/plugin/PluginCreator.java

+        final String error =
+                String.format("Data Prepper plugin requires a constructor which is either: " +
+                        "1. Annotated with @DataPrepperPlugin; " +
+                        "2. Contains a single argument of type %s; or " +


What happens if there are two separate single argument constructors? What is the failure mode?

What is your vision for v2.0? Do you plan on supporting all of these scenarios in the future as well? Would it be possible to move to an explicit model which requires an annotated constructor?

What happens if there are two separate single argument constructors?

Because the @DataPrepperPlugin allows configuring the pluginConfigurationType, the single argument constructor will only match that type. So it should only ever be what is defined by pluginConfigurationType.

What is your vision for v2.0? Do you plan on supporting all of these scenarios in the future as well? Would it be possible to move to an explicit model which requires an annotated constructor?

For 2.0, I'd like to remove the single argument constructor. Thus, the constructor must be either annotated with @DataPrepperConstructor or be the default constructor.
We need to keep at least support for a single argument PluginSetting in 1.x since that is the current behavior. I could change this so that the only implicit single argument constructor is PluginSetting and disallow the POJO-based single-argument constructor. Would that help?

@cmanning09 ,

I'm going to change this behavior so that the only single parameter constructor supported is PluginSetting. Thus, plugins which use a POJO for the configuration must provide the @DataPrepperConstructor annotation. I believe this should be simpler and is one fewer behavior to deprecate.

I pushed a commit which, among other changes, now only permits a single-parameter constructor for PluginSetting. To use the custom POJO configuration, plugin authors must add the @DataPrepperPluginConstructor annotation.

cmanning09 · 2021-10-30T19:09:48Z

data-prepper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/PluginFactory.java

+     * This exists so that this class can still exhibit the correct behavior when creating new plugin
+     * classes. This whole class is going to be deleted in the next major version, as will this method.
+     */
+    public static void dangerousMethod_setPluginFunction(final BiFunction<PluginSetting, Class<?>, Object> newPluginFunction) {


I am fine with this as well because of the reasons you provided. Thanks!

...gins/http-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSource.java

chenqi0805 · 2021-11-01T16:43:48Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

-                             final String sslKeyFile,
-                             final String sslKeyPassword) {
+    private int port = DEFAULT_PORT;
+    @JsonProperty("request_timeout")


Nit: can we use REQUEST_TIMEOUT constant here?

chenqi0805 · 2021-11-01T16:44:05Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

    static final String REQUEST_TIMEOUT = "request_timeout";
    static final String THREAD_COUNT = "thread_count";
-    static final String MAX_CONNECTION_COUNT = "max_connection_count";


Why can we remove this?

This string is no longer being used because the tests use a POJO. I pushed a change which added the string literal into the @JsonProperty. For those properties which are being used elsewhere, I kept those as constants.

chenqi0805 · 2021-11-01T16:44:31Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

+    @JsonProperty("request_timeout")
+    private int requestTimeoutInMillis = DEFAULT_REQUEST_TIMEOUT_MS;
+    private int threadCount = DEFAULT_THREAD_COUNT;
+    private int maxConnectionCount = DEFAULT_MAX_CONNECTION_COUNT;


Why no jsonProperty annotation on top of this variable?

The default conversion will support snake case, so this should serialize. But, I can add add it to be somewhat more thorough and consistent.

chenqi0805 · 2021-11-01T16:44:44Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

+    private int requestTimeoutInMillis = DEFAULT_REQUEST_TIMEOUT_MS;
+    private int threadCount = DEFAULT_THREAD_COUNT;
+    private int maxConnectionCount = DEFAULT_MAX_CONNECTION_COUNT;
+    private int maxPendingRequests = DEFAULT_MAX_PENDING_REQUESTS;


Same as above

…Constructor annotation. This is the preferred constructor. If no other constructor is available for a plugin, use the no-op constructor. Updated the HTTPSource plugin to use this capability to receive both a configuration model and PluginMetrics via the constructor. Signed-off-by: David Venable <[email protected]>

…g in a single parameter, un-annotated constructor. Cleaned up the HTTPSourceConfig. Other small changes. Signed-off-by: David Venable <[email protected]>

cmanning09 · 2021-11-02T14:05:31Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

    private String sslCertificateFile;
+
+    @JsonProperty(SSL_KEY_FILE)


Is there a difference between upper case and lower case JsonProperties? I am wondering why ssl_key_password and SSL_KEY_FILE are different.

SSL_KEY_FILE is a static final field whereas "ssl_key_password" is a string literal. The former is used in some error strings so I kept it as constant. We could make them all constants if we wanted.

oh, I missed the quotes. For a second I thought the properties were defined in a different format. I'd prefer consistency but will defer to your preference here. I don't want to hold this up.

Signed-off-by: David Venable <[email protected]>

dlvenable · 2021-11-02T21:05:01Z

I also had left some commented out code in place. I just pushed a commit which cleaned that up.

cmanning09 · 2021-11-02T21:15:39Z

...ttp-source/src/main/java/com/amazon/dataprepper/plugins/source/loghttp/HTTPSourceConfig.java

    private String sslCertificateFile;
+
+    @JsonProperty(SSL_KEY_FILE)


oh, I missed the quotes. For a second I thought the properties were defined in a different format. I'd prefer consistency but will defer to your preference here. I don't want to hold this up.

Support more constructors for plugins by adding the DataPrepperPluginConstructor annotation. This is the preferred constructor. If no other constructor is available for a plugin, use the no-op constructor. Updated the HTTPSource plugin to use this capability to receive both a configuration model and PluginMetrics via the constructor. For a single parameter, un-annotated constructor in plugins, the only supported parameter is once again PluginSetting. Signed-off-by: David Venable <[email protected]>

dlvenable commented Oct 29, 2021

View reviewed changes

dlvenable requested a review from cmanning09 October 29, 2021 19:41

cmanning09 suggested changes Oct 30, 2021

View reviewed changes

chenqi0805 reviewed Nov 1, 2021

View reviewed changes

graytaylor0 mentioned this pull request Nov 1, 2021

Grok prepper metrics #510

Merged

5 tasks

dlvenable added 2 commits November 1, 2021 16:53

Addressed PR feedback: Added documentation, support only PluginSettin…

d35b3b7

…g in a single parameter, un-annotated constructor. Cleaned up the HTTPSourceConfig. Other small changes. Signed-off-by: David Venable <[email protected]>

dlvenable force-pushed the plugin-constructor-injection branch from b53c5dc to d35b3b7 Compare November 1, 2021 22:42

cmanning09 reviewed Nov 2, 2021

View reviewed changes

Cleaned up unused, commented out code.

f86ddb8

Signed-off-by: David Venable <[email protected]>

cmanning09 approved these changes Nov 2, 2021

View reviewed changes

chenqi0805 approved these changes Nov 2, 2021

View reviewed changes

dlvenable merged commit 3c7ceec into opensearch-project:main Nov 2, 2021

dlvenable deleted the plugin-constructor-injection branch November 29, 2021 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expanded plugin constructor capabilities #481

Expanded plugin constructor capabilities #481

dlvenable commented Oct 29, 2021 •

edited

Loading

dlvenable Oct 29, 2021

cmanning09 Oct 30, 2021

cmanning09 Oct 30, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

dlvenable Nov 1, 2021 •

edited

Loading

dlvenable Nov 1, 2021

dlvenable Nov 1, 2021

cmanning09 Oct 30, 2021

chenqi0805 Nov 1, 2021

chenqi0805 Nov 1, 2021

dlvenable Nov 1, 2021

chenqi0805 Nov 1, 2021

dlvenable Nov 1, 2021

chenqi0805 Nov 1, 2021

cmanning09 Nov 2, 2021

dlvenable Nov 2, 2021

cmanning09 Nov 2, 2021

dlvenable commented Nov 2, 2021

cmanning09 Nov 2, 2021

		private String sslCertificateFile;

		@JsonProperty(SSL_KEY_FILE)

Expanded plugin constructor capabilities #481

Expanded plugin constructor capabilities #481

Conversation

dlvenable commented Oct 29, 2021 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable commented Nov 2, 2021

Choose a reason for hiding this comment

dlvenable commented Oct 29, 2021 •

edited

Loading

dlvenable Nov 1, 2021 •

edited

Loading