Grok Prepper Configuration and Boilerplate #302

graytaylor0 · 2021-09-21T16:56:50Z

Signed-off-by: graytaylor0 [email protected]

Description

includes reading the configuration for Grok Prepper, which includes adding PluginSetting validation for Map<String, String>, List, and Map<String, List>.
includes boilerplate for Grok Prepper implementation of AbstractPrepper

Issues Resolved

Check List

[ X ] New functionality includes testing.
- [ X ] All tests pass
New functionality has been documented.
- New functionality has javadoc added
[ X ] 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.

Signed-off-by: graytaylor0 <[email protected]>

laneholloway · 2021-09-21T17:12:52Z

data-prepper-plugins/grok-prepper/build.gradle

+}
+
+dependencies {
+    implementation "com.fasterxml.jackson.core:jackson-databind:2.12.4"


Don't we have this already defined higher up in the hierarchy?

We actually don't. Could add this as a dependency to data-prepper-plugins but right now each individual plugin adds this if it is used. We do have mavenCentral() as part of the project's build.gradle but other plugins that have used dependencies from mavenCentral() had it again. Maybe they don't need it.

It is not a dependency, but you can inherit the version number. So, you should remove the version number from this line.

See: https://github.com/opensearch-project/data-prepper/blob/main/build.gradle#L74

laneholloway · 2021-09-21T17:17:53Z

...grok-prepper/src/test/java/com/amazon/dataprepper/plugins/prepper/grok/GrokPrepperTests.java

+import java.util.Collections;
+import java.util.List;
+
+public class GrokPrepperTests {


We are going to be adding more tests in here, right? ;)

Haha there will be more when actual functionality is added.

cmanning09 · 2021-09-21T21:34:21Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+            ((List<?>) object).stream().filter(o -> !(o instanceof String)).forEach(o -> {
+                throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
+            });
+            return (List<String>) object;


This can be simplified:

((List<?>) object).).forEach(o -> { if (!(o instanceof String)) { throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute)); } }

cmanning09 · 2021-09-21T21:34:23Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+     * @return the value of the specified attribute, or {@code defaultValue} if this settings contains no value for
+     * the attribute
+     */
+    @SuppressWarnings("unchecked")


This is dangerous. What are we suppressing here?

It suppresses this warning which only comes from Intellij as far as I can tell.
Unchecked cast: 'java.lang.Object' to 'java.util.Map<java.lang.String,java.util.List<java.lang.String>>' . It is because of the cast on the return values from Object.

cmanning09 · 2021-09-21T21:39:08Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+                        ((List<?>) value).stream().filter(o -> !(o instanceof String)).forEach(o -> {
+                            throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
+                        });


See other comment above about improving this check

Signed-off-by: graytaylor0 <[email protected]>

dlvenable · 2021-09-22T20:26:29Z

...grok-prepper/src/test/java/com/amazon/dataprepper/plugins/prepper/grok/GrokPrepperTests.java

+import com.amazon.dataprepper.model.record.Record;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;


We should start using JUnit 5 for tests.

dlvenable · 2021-09-22T20:29:03Z

...gins/grok-prepper/src/main/java/com/amazon/dataprepper/plugins/prepper/grok/GrokPrepper.java

+import java.util.Collection;
+
+
+@DataPrepperPlugin(name = "grok_prepper", type = PluginType.PREPPER)


We should name this grok so that users need only type grok in the configuration file.

e.g.

prepper: grok: match: "log"

dlvenable · 2021-09-22T20:30:31Z

data-prepper-plugins/grok-prepper/build.gradle

+dependencies {
+    implementation project(':data-prepper-api')
+    implementation project(':data-prepper-plugins:common')
+    testImplementation project(':data-prepper-api').sourceSets.test.output


Do we actually use this? Or was this line copied from another project?

dlvenable · 2021-09-22T20:36:49Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+     * @return the value of the specified attribute, or {@code defaultValue} if this settings contains no value for
+     * the attribute
+     */
+    public List<String> getStringListOrDefault(final String attribute, final List<String> defaultValue) {


This method and the other methods can probably be made somewhat more generic.

public <T> List<T> getTypedListOrDefault(final String attribute, final Class<T> type, final List<T> defaultValue)

You can replace your instanceof with isAssignableFrom.

Had thought about this and wasn't sure if passing the Class type was considered good practice. Doing this definitely scales PluginSettings better though.

dlvenable · 2021-09-22T20:38:59Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+        Object object = getAttributeOrDefault(attribute, defaultValue);
+        if (object == null) {
+            return null;
+        } else if (object instanceof Map) {


This code can be cleaner by throwing first.

I'd also create a variable just to avoid operating immediately after a cast, but I think that is of less importance.

if ( !(object instanceof Map)) throw new IllegalArgumentException(...) Map<?, ?> map = object; map.forEach( ...

This is definitely cleaner. Just to clarify you are not suggesting that this is done immediately after the getAttributeOrDefault call, but after the if statement for checking if object is null?

Since the behavior of a null value is to return null, then it should be after the null check.

Object object = getAttributeOrDefault(attribute, defaultValue); if (object == null) { return null; if ( !(object instanceof Map)) throw new IllegalArgumentException(...) Map<?, ?> map = object; map.forEach( ...

dlvenable · 2021-09-22T20:39:43Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+        Object object = getAttributeOrDefault(attribute, defaultValue);
+        if (object == null) {
+            return null;
+        } else if (object instanceof Map) {


We should try to avoid this nesting by throwing immediately. See my note above.

dinujoh · 2021-09-22T22:24:19Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

+                if (key instanceof String) {
+                    if (value instanceof List){
+                        ((List<?>) value).forEach(val -> {
+                            if (!(val instanceof String)) {
+                                throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
+                            }
+                        });
+                    } else {
+                        throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
+                    }
+                } else {
+                    throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
+                }
+            });


Instance check for String, List, Map is repeated in this class. Consider defining a function or java BiPredicate that takes in the object & type, and checks if they are of same type else throws exception. This code can be reused in different methods in this class.

Signed-off-by: graytaylor0 <[email protected]>

dinujoh · 2021-09-23T15:21:06Z

data-prepper-api/src/main/java/com/amazon/dataprepper/model/configuration/PluginSetting.java

@@ -180,4 +254,17 @@ public Long getLongOrDefault(final String attribute, final long defaultValue) {
        throw new IllegalArgumentException(String.format(UNEXPECTED_ATTRIBUTE_TYPE_MSG, object.getClass(), attribute));
    }

+    private <T> void checkObjectType(String attribute, Object object, Class<T> type) {


nit: final for method params line 257 & 263 to be consistent with other methods

dlvenable · 2021-09-27T14:49:39Z

settings.gradle

@@ -35,4 +35,5 @@ include 'data-prepper-plugins:otel-trace-group-prepper'
 include 'data-prepper-plugins:otel-trace-source'
 include 'data-prepper-plugins:peer-forwarder'
 include 'data-prepper-plugins:blocking-buffer'
+include 'data-prepper-plugins:grok-prepper'


Why did you add this dependency? In general, our plugins should not depend on each other.

The settings.gradle file defines all of the internal projects. All of the plugin projects are required to go in here.

Yes, you are correct on this.

dlvenable · 2021-09-27T15:51:56Z

settings.gradle

@@ -35,4 +35,5 @@ include 'data-prepper-plugins:otel-trace-group-prepper'
 include 'data-prepper-plugins:otel-trace-source'
 include 'data-prepper-plugins:peer-forwarder'
 include 'data-prepper-plugins:blocking-buffer'
+include 'data-prepper-plugins:grok-prepper'


Yes, you are correct on this.

Grok Prepper Configuration and Boilerplate

c972c4b

Signed-off-by: graytaylor0 <[email protected]>

graytaylor0 linked an issue Sep 21, 2021 that may be closed by this pull request

Grok Prepper Configuration and Boilerplate #301

Closed

laneholloway previously approved these changes Sep 21, 2021

View reviewed changes

cmanning09 reviewed Sep 21, 2021

View reviewed changes

Remove suppressed warnings and refactor String List check

d3fed9d

Signed-off-by: graytaylor0 <[email protected]>

graytaylor0 dismissed laneholloway’s stale review via d3fed9d September 22, 2021 15:35

dlvenable requested changes Sep 22, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into GrokPrepper

e491ac3

dinujoh reviewed Sep 22, 2021

View reviewed changes

Generics for PluginSetting and addressing other comments

40af804

Signed-off-by: graytaylor0 <[email protected]>

graytaylor0 force-pushed the GrokPrepper branch from c49ec4e to 40af804 Compare September 23, 2021 00:16

dinujoh reviewed Sep 23, 2021

View reviewed changes

dinujoh approved these changes Sep 23, 2021

View reviewed changes

dlvenable requested changes Sep 27, 2021

View reviewed changes

dlvenable approved these changes Sep 27, 2021

View reviewed changes

graytaylor0 merged commit 1cb44e1 into opensearch-project:main Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grok Prepper Configuration and Boilerplate #302

Grok Prepper Configuration and Boilerplate #302

graytaylor0 commented Sep 21, 2021 •

edited

Loading

laneholloway Sep 21, 2021

graytaylor0 Sep 21, 2021 •

edited

Loading

dlvenable Sep 22, 2021

laneholloway Sep 21, 2021

graytaylor0 Sep 21, 2021

cmanning09 Sep 21, 2021 •

edited

Loading

cmanning09 Sep 21, 2021

graytaylor0 Sep 21, 2021 •

edited

Loading

cmanning09 Sep 21, 2021

dlvenable Sep 22, 2021

dlvenable Sep 22, 2021

graytaylor0 Sep 22, 2021

dlvenable Sep 22, 2021

dlvenable Sep 22, 2021

graytaylor0 Sep 22, 2021

dlvenable Sep 22, 2021

graytaylor0 Sep 22, 2021

dlvenable Sep 27, 2021

dlvenable Sep 22, 2021

dinujoh Sep 22, 2021

dinujoh Sep 23, 2021

dlvenable Sep 27, 2021

graytaylor0 Sep 27, 2021

dlvenable Sep 27, 2021

dlvenable Sep 27, 2021

		import java.util.Collection;


		@DataPrepperPlugin(name = "grok_prepper", type = PluginType.PREPPER)

Grok Prepper Configuration and Boilerplate #302

Grok Prepper Configuration and Boilerplate #302

Conversation

graytaylor0 commented Sep 21, 2021 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

graytaylor0 Sep 21, 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

cmanning09 Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graytaylor0 Sep 21, 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

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

graytaylor0 commented Sep 21, 2021 •

edited

Loading

graytaylor0 Sep 21, 2021 •

edited

Loading

cmanning09 Sep 21, 2021 •

edited

Loading

graytaylor0 Sep 21, 2021 •

edited

Loading