Grok Prepper Matching Functionality #324

graytaylor0 · 2021-09-29T18:00:39Z

Signed-off-by: graytaylor0 [email protected]

Description

Grok matching functionality (single/mutli field, single/multi pattern)
Support for named_captures_only
Support for keep_empty_captures
Unit tests for Grok Prepper and Integration tests using java-grok library

Issues Resolved

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.

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

dlvenable · 2021-09-29T20:34:45Z

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

+        try (MockedStatic<GrokCompiler> grokCompilerMockedStatic = mockStatic(GrokCompiler.class)) {
+
+            capture = new HashMap<>();
+            capture.put("field_capture_1", "value_capture_1");


We should leave the capture as an empty map in this setup. Tests which want specific values should set them directly. So, I'd recommend removing these three lines from here.

Additionally, you can make @Nested tests to help combine tests that should be more similar.

Additionally, this could give a test in which there are no matches. That test would have an empty capture map.

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

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

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

chenqi0805

Just minor comments.

chenqi0805 · 2021-09-30T15:28:31Z

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

+        String testData = "{\"message\":\"127.0.0.1 user-identifier frank [10/Oct/2000:13:55:36 -0700] \\\"GET /apache_pb.gif HTTP/1.0\\\" 200 2326\"}";
+        Record<String> record = new Record<>(testData);
+
+        String resultData = "{\"message\":\"127.0.0.1 user-identifier frank [10/Oct/2000:13:55:36 -0700] \\\"GET /apache_pb.gif HTTP/1.0\\\" 200 2326\","


nit-pick: we could construct it as map to avoid redundant deserialization.

chenqi0805 · 2021-09-30T15:35:46Z

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

+        pluginSetting.getSettings().put(GrokPrepperConfig.MATCH, matchConfig);
+        grokPrepper = new GrokPrepper(pluginSetting);
+
+        String testData = "{\"message\":\"127.0.0.1 user-identifier frank [10/Oct/2000:13:55:36 -0700] \\\"GET /apache_pb.gif HTTP/1.0\\\" 200 2326\"}";


nit-pick: It seems to me the testData does not include %{DATA:rawrequest}. From the context I infer it is to cover empty_match = False? If so, we should better comment on this field.

Yeah rawrequest being null makes it easier to test keep_empty_captures and to test merging maps with null values to make sure no NPE's can occur. Can you elaborate on what you mean by "better comment on this field"?

If I am correct, rawrequest key is not expected to appear in the keys of result data in this test cases while it appears in the grokking pattern. So we could call that out in the comment (maybe above result data)

I will put a comment somewhere for this

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

cmanning09 · 2021-09-30T14:19:20Z

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

@@ -43,10 +69,35 @@ public GrokPrepper(final PluginSetting pluginSetting) {
     *
     * @param records Input records that will be modified/processed
     * @return Record  modified output records
-    */
+     */
    @Override
    public Collection<Record<String>> doExecute(Collection<Record<String>> records) {


nit: parameters should be final

cmanning09 · 2021-09-30T14:21:00Z

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

+                for (Map.Entry<String, List<Grok>> entry : fieldToGrok.entrySet()) {
+                    for (Grok grok : entry.getValue()) {
+                        if (recordMap.containsKey(entry.getKey())) {
+                            Match match = grok.match(recordMap.get(entry.getKey()).toString());


cmanning09 · 2021-09-30T14:32:04Z

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

+        for (Map.Entry<String, List<String>> entry : grokPrepperConfig.getMatch().entrySet()) {
+            fieldToGrok.put(entry.getKey(), entry.getValue().stream().map(item -> grokCompiler.compile(item, grokPrepperConfig.isNamedCapturesOnly())).collect(Collectors.toList()));
+        }


This is very hard to read and I am not sure what is happening here. Let's pull this out into a separate function. A separate function will allow us to provide some context on what operation we are performing. Breaking up this line into multiple lines with each line focused on a concrete operation with help readability and not hide functionality.

cmanning09 · 2021-09-30T21:13:14Z

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

+                            Match match = grok.match(recordMap.get(entry.getKey()).toString());
+                            match.setKeepEmptyCaptures(grokPrepperConfig.isKeepEmptyCaptures());
+
+                            mergeUpdateWithOriginalMap(recordMap, match.capture());


This doesn't read very well. Might I recommend: "mergeCapturesWithRecordMap"

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

cmanning09 · 2021-10-01T15:37:55Z

data-prepper-plugins/grok-prepper/README.md

+
+## Notes on Patterns
+
+The Grok Prepper internally uses the [java-grok Library](https://github.com/thekrakken/java-grok), so any patterns compatible there are compatible with Grok Prepper.


A few comments here, but let's defer that to a separate PR where we can focus on the documentation for our users later. I don't want to block this functionality right now.

"so any patterns compatible there are compatible with Grok Prepper." - this is passive and "there" refers to the object of the sentence instead of the subject which is a little confusing. Might I recommend:
"The Grok Prepper uses the the java-grok Library internally and supports all java-grok Library compatible patterns."

We should link to the default patterns as well.

cmanning09 · 2021-10-01T15:47:34Z

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

@@ -71,20 +68,20 @@ public GrokPrepper(final PluginSetting pluginSetting) {
     * @return Record  modified output records
     */
    @Override
-    public Collection<Record<String>> doExecute(Collection<Record<String>> records) {
+    public Collection<Record<String>> doExecute(final Collection<Record<String>> records) {
        final List<Record<String>> recordsOut = new LinkedList<>();

        for (Record<String> record : records) {


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

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

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

graytaylor0 added 4 commits September 27, 2021 15:48

Beginning testing of Grok Prepper class

fe99318

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

Unit tests

e1ac248

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

Integration tests created that use java-grok library

f40207a

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

Cleaned up a little

a450fe1

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

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

Grok Prepper Basic Matching #304

Closed

graytaylor0 requested review from dlvenable, chenqi0805 and cmanning09 September 29, 2021 18:07

dlvenable requested changes Sep 29, 2021

View reviewed changes

chenqi0805 reviewed Sep 30, 2021

View reviewed changes

Addressing PR comments

40fcd85

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

cmanning09 suggested changes Sep 30, 2021

View reviewed changes

graytaylor0 added 2 commits September 30, 2021 16:34

Address more PR comments and begin README

b7e01ff

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

Remove anyString() in mock

d3f60d3

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

cmanning09 reviewed Oct 1, 2021

View reviewed changes

add final, comment on null rawrequest

793a417

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

dlvenable reviewed Oct 1, 2021

View reviewed changes

...grok-prepper/src/test/java/com/amazon/dataprepper/plugins/prepper/grok/GrokPrepperTests.java Outdated Show resolved Hide resolved

graytaylor0 added 2 commits October 1, 2021 15:03

Use singletonList for matchConfig

3838043

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

Fixed typo

1ebdfa3

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

dlvenable approved these changes Oct 1, 2021

View reviewed changes

cmanning09 approved these changes Oct 1, 2021

View reviewed changes

graytaylor0 merged commit f82db2b into opensearch-project:main Oct 1, 2021

graytaylor0 deleted the GrokPrepper branch November 1, 2021 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grok Prepper Matching Functionality #324

Grok Prepper Matching Functionality #324

graytaylor0 commented Sep 29, 2021

dlvenable Sep 29, 2021

dlvenable Sep 29, 2021

chenqi0805 left a comment

chenqi0805 Sep 30, 2021

chenqi0805 Sep 30, 2021

graytaylor0 Sep 30, 2021

chenqi0805 Oct 1, 2021

graytaylor0 Oct 1, 2021

cmanning09 Sep 30, 2021

cmanning09 Sep 30, 2021

cmanning09 Sep 30, 2021

cmanning09 Sep 30, 2021

cmanning09 Oct 1, 2021

cmanning09 Oct 1, 2021


		## Notes on Patterns

		The Grok Prepper internally uses the [java-grok Library](https://github.com/thekrakken/java-grok), so any patterns compatible there are compatible with Grok Prepper.

Grok Prepper Matching Functionality #324

Grok Prepper Matching Functionality #324

Conversation

graytaylor0 commented Sep 29, 2021

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 left a 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