Added support for nested syntax in converter #1088

asifsmohammed · 2022-02-24T06:05:17Z

Signed-off-by: Asif Sohail Mohammed [email protected]

Description

This PR only covers nested syntax for Date, Grok (key in match, named captures) and DefaultLogstashPluginAttributesMapper

Logstash nested syntax: [outer][inner]
Data Prepper JSON pointer: /outer/inner

List of plugin configurations where nested syntax can be used in Logstash

Grok
- match
- pattern_definitions
- overwrite
Date
- match
- target
Mutate
- add
- rename
- copy
- delete
KV
- surce
- target

Integration tests will give a high level idea of where nested syntax can be used in different processors. Files for Inegration tests are log-ingest-to-opensearch-nested-syntax.conf and log-ingest-to-opensearch-nested-syntax.expected.yaml

Issues Resolved

#627

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

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.

codecov-commenter · 2022-02-24T06:14:07Z

Codecov Report

Merging #1088 (6b0690a) into main (9dce183) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1088   +/-   ##
=========================================
  Coverage     91.32%   91.32%           
  Complexity      692      692           
=========================================
  Files            85       85           
  Lines          2017     2017           
  Branches        170      170           
=========================================
  Hits           1842     1842           
  Misses          133      133           
  Partials         42       42

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dce183...6b0690a. Read the comment docs.

dinujoh · 2022-02-24T18:25:15Z

...ion/src/main/java/org/opensearch/dataprepper/logstash/mapping/NestedSyntaxConverterUtil.java

+
+    private static String convertAttributeValueNestedSyntax(Object attributeValue) {
+        StringBuilder convertedAttribute = new StringBuilder();
+        String nestedSyntaxRegex = "\\[([^\\]]+)\\]";


Consider making them static constant.

This regex has some flaws in it. For example, things like [foo[bar] will pass. After the first [foo] is found, you are not verifying any more (so [foo]anythingcangohere passes. I think something closer to this pattern should work to allow one or more non-empty [] that does not contain brackets inside of it. That way we can verify the entire string matches first, and then we can go through and match on each bracket.

(\\[([^\\]\\[]+)\\])+

The easiest way to do use this pattern would be something like this. I am sure there is a way to go to each [foo] section and append a / like you are doing, but this way may be easier

Matcher matcher = NESTED_SYNTAX_PATTERN.matcher(logstashString); if (matcher.matches() { String result = logstatshString.replaceAll("\\]\\[", "/").replaceAll("\\[|\\]", "/"); return result; }

Thanks for the feedback Taylor. I'll work on regex. It's very hard to find the regex for complete nested syntax string. That's the reason I'm looking for groups of []. One way to do it is just check starting and ending characters like^\[.+\]$.

I also wrote the regex based on the assumption that we get valid Logstash configuration.

dinujoh · 2022-02-24T18:25:46Z

...ion/src/main/java/org/opensearch/dataprepper/logstash/mapping/NestedSyntaxConverterUtil.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+class NestedSyntaxConverterUtil {


Consider making the method parameters and other variables final.

dinujoh · 2022-02-24T18:30:57Z

...ain/java/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributesMapper.java

@@ -66,20 +66,22 @@ protected void mapCustomAttributes(List<LogstashAttribute> logstashAttributes, L
            if (logstashGrokMatchValueType.equals(LogstashValueType.HASH)) {


Not related to this PR, we should consider refactoring the if-else block. We can create an interface and implementation for specific type of match attribute (HASH vs ARRAY vs Others...) to follow single responsibility principle and keep code maintainable.

graytaylor0 · 2022-02-25T18:39:49Z

...a/org/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapperTest.java

@@ -138,4 +139,21 @@ void mapAttributes_with_negation_expression_negates_boolean_value(boolean inputV
        assertThat(actualPluginModel.get(0).getPluginSettings().size(), equalTo(1));
        assertThat(actualPluginModel.get(0).getPluginSettings().get(dataPrepperAttribute.substring(1)), equalTo(!inputValue));
    }
+
+    @ParameterizedTest
+    @CsvSource({"[outer_key][inner_key],/outer_key/inner_key", "[array][0][key], /array/0/key"})


Let's add one case for single brackets (ex: ["message"])

graytaylor0 · 2022-02-25T18:52:02Z

...ion/src/main/java/org/opensearch/dataprepper/logstash/mapping/NestedSyntaxConverterUtil.java

+    private NestedSyntaxConverterUtil() {}
+
+    public static Object checkAndConvertLogstashNestedSyntax(Object logstashAttributeValue) {
+        if (logstashAttributeValue instanceof String && ((String) logstashAttributeValue).startsWith("[") && ((String) logstashAttributeValue).endsWith("]")) {


This check shouldn't be necessary if the regex pattern is correct. I would instead compile the regex pattern as a static variable in the class, and do a check for if the logstashAttributeValue matches the regex pattern.

graytaylor0 · 2022-02-25T19:03:05Z

...ion/src/main/java/org/opensearch/dataprepper/logstash/mapping/NestedSyntaxConverterUtil.java

+
+    private static String convertAttributeValueNestedSyntax(Object attributeValue) {
+        StringBuilder convertedAttribute = new StringBuilder();
+        String nestedSyntaxRegex = "\\[([^\\]]+)\\]";


This regex has some flaws in it. For example, things like [foo[bar] will pass. After the first [foo] is found, you are not verifying any more (so [foo]anythingcangohere passes. I think something closer to this pattern should work to allow one or more non-empty [] that does not contain brackets inside of it. That way we can verify the entire string matches first, and then we can go through and match on each bracket.

(\\[([^\\]\\[]+)\\])+

dinujoh · 2022-02-28T17:46:47Z

...ain/java/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributesMapper.java

    }

    @Override
    protected HashSet<String> getCustomMappedAttributeNames() {
-        return new HashSet<>(Arrays.asList(LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME, LOGSTASH_GROK_PATTERN_DEFINITIONS_ATTRIBUTE_NAME));
+        return new HashSet<>(Arrays.asList(LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME,


Consider moving this to static constant.

dinujoh · 2022-02-28T17:48:25Z

...ain/java/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributesMapper.java

        logstashAttributes.forEach(logstashAttribute -> {
            if (logstashAttribute.getAttributeName().equals(LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME)) {
                matchAttributes.add(logstashAttribute);
            } else if (logstashAttribute.getAttributeName().equals(LOGSTASH_GROK_PATTERN_DEFINITIONS_ATTRIBUTE_NAME)) {
                patternDefinitions.putAll((Map<String, String>) logstashAttribute.getAttributeValue().getValue());
+            } else if (logstashAttribute.getAttributeName().equals(LOGSTASH_GROK_OVERWRITE_ATTRIBUTE_NAME)) {
+                keysToOverwrite.addAll((List<String>) logstashAttribute.getAttributeValue().getValue());
            }


This class is doing multiple things. Consider moving the responsibility of each if condition to separate class to make this class more maintainable.

Do you mean extract this into a method? Creating a new class for this would be confusing for plugin authors for adding converter logic. I can extract it to a separate method.

treddeni-amazon · 2022-03-02T22:57:47Z

...h-configuration/src/main/java/org/opensearch/dataprepper/logstash/mapping/GrokMatchUtil.java

+        Matcher grokMatchPatternMatcher = GROK_MATCH_PATTERN.matcher(matchPattern);
+        StringBuilder convertedGrokMatchPattern = new StringBuilder();
+        while(grokMatchPatternMatcher.find()) {
+            convertedGrokMatchPattern.append(grokMatchPatternMatcher.group(1));


These are magic numbers. What do they mean? We should define constants for them that describe what they mean. What is in group 1? Group 2? etc.

I'll add group names for regex.

dlvenable · 2022-03-02T23:22:54Z

...ava/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributeMapperHelper.java

+
+import java.util.Map;
+
+public interface GrokLogstashPluginAttributeMapperHelper {


These should be package protected. I'd also be happy to see them as static inner classes because their scope of use is so small.

dlvenable · 2022-03-02T23:24:39Z

...on/src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashAttributesMappings.java

+     * @return A Map
+     * @since 1.2
+     */
+    List<String> getNestedSyntaxAttributeNames();


I think this approach has helped reduce complexity for the simpler scenarios such as kv.

dlvenable · 2022-03-02T23:25:32Z

...a/org/opensearch/dataprepper/logstash/mapping/DefaultLogstashPluginAttributesMapperTest.java

@@ -138,4 +139,22 @@ void mapAttributes_with_negation_expression_negates_boolean_value(boolean inputV
        assertThat(actualPluginModel.get(0).getPluginSettings().size(), equalTo(1));
        assertThat(actualPluginModel.get(0).getPluginSettings().get(dataPrepperAttribute.substring(1)), equalTo(!inputValue));
    }
+
+    @ParameterizedTest
+    @CsvSource({"[outer_key][inner_key],/outer_key/inner_key", "[array][0][key], /array/0/key", "[message], /message"})


treddeni-amazon · 2022-03-03T00:15:16Z

...h-configuration/src/main/java/org/opensearch/dataprepper/logstash/mapping/GrokMatchUtil.java

+        Matcher grokMatchPatternMatcher = GROK_MATCH_PATTERN.matcher(matchPattern);
+        StringBuilder convertedGrokMatchPattern = new StringBuilder();
+        while(grokMatchPatternMatcher.find()) {
+            convertedGrokMatchPattern.append(grokMatchPatternMatcher.group("openingBrackets"));


It would be better to define these (openiingBrackets, grokSyntax, etc) as constants. That way there's only one place that they are defined. That way if someone changes the regex pattern it doesn't break the code below.

Thanks for the feedback Phill. I made the changes to the logic to remove regex groups as it is getting complex.

dinujoh · 2022-03-03T22:35:31Z

...ain/java/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributesMapper.java

-        final List<LogstashAttribute> matchAttributes = new ArrayList<>();
-        final Map<String, String> patternDefinitions = new HashMap<>();
+        GrokLogstashPluginAttributeMapperHelper matchAttributesHelper = new GrokMatchAttributeHelper();
+        GrokLogstashPluginAttributeMapperHelper patternDefinitionsAttributeHelper = new GrokPatternDefinitionsAttributeHelper();


Move this to constructor.

dinujoh · 2022-03-03T22:40:44Z

...tion/src/main/java/org/opensearch/dataprepper/logstash/mapping/GrokMatchAttributeHelper.java

+
+import static org.opensearch.dataprepper.logstash.mapping.GrokLogstashPluginAttributesMapper.LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME;
+
+class GrokMatchAttributeHelper implements GrokLogstashPluginAttributeMapperHelper {


Add documentation

dinujoh · 2022-03-03T22:42:25Z

...h-configuration/src/main/java/org/opensearch/dataprepper/logstash/mapping/GrokMatchUtil.java

+        return convertedGrokMatchPattern.toString();
+    }
+
+    private static String getConvertedMatchPattern(String matchPatternGroup) {


nit: final to method parameter and other variables in this class.

dlvenable · 2022-03-04T16:42:41Z

...h-configuration/src/main/java/org/opensearch/dataprepper/logstash/mapping/GrokMatchUtil.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class GrokMatchUtil {


This class also needs to be package protected.

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dinujoh · 2022-03-04T19:18:12Z

...ain/java/org/opensearch/dataprepper/logstash/mapping/GrokLogstashPluginAttributesMapper.java

+        final Map<String, GrokLogstashPluginAttributeMapperHelper> helperInstances = new HashMap<>();
+        helperInstances.put(LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME, matchAttributesHelper);
+        helperInstances.put(LOGSTASH_GROK_PATTERN_DEFINITIONS_ATTRIBUTE_NAME, patternDefinitionsAttributeHelper);
+        helperInstances.put(LOGSTASH_GROK_OVERWRITE_ATTRIBUTE_NAME, overwriteAttributeHelper);


This can be moved to constructor as well.

Define helperInstances as class instance variable and initialize in the constructor.
private final Map<String, GrokLogstashPluginAttributeMapperHelper> helperInstances

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed requested a review from a team as a code owner February 24, 2022 06:05

asifsmohammed marked this pull request as draft February 24, 2022 06:10

dinujoh reviewed Feb 24, 2022

View reviewed changes

graytaylor0 reviewed Feb 25, 2022

View reviewed changes

asifsmohammed force-pushed the nested-syntax branch 2 times, most recently from 0ed5f58 to 4ec61bc Compare February 27, 2022 18:37

dinujoh requested changes Feb 28, 2022

View reviewed changes

asifsmohammed force-pushed the nested-syntax branch 2 times, most recently from 4e17bcb to b42bfb4 Compare March 2, 2022 22:26

asifsmohammed marked this pull request as ready for review March 2, 2022 22:27

asifsmohammed force-pushed the nested-syntax branch from b42bfb4 to 0be236f Compare March 2, 2022 22:51

treddeni-amazon reviewed Mar 2, 2022

View reviewed changes

dlvenable requested changes Mar 2, 2022

View reviewed changes

asifsmohammed force-pushed the nested-syntax branch from 2cb6183 to d64f16c Compare March 2, 2022 23:54

treddeni-amazon reviewed Mar 3, 2022

View reviewed changes

dinujoh reviewed Mar 3, 2022

View reviewed changes

dinujoh self-requested a review March 3, 2022 22:49

asifsmohammed force-pushed the nested-syntax branch 2 times, most recently from 5729383 to 6b0690a Compare March 4, 2022 04:55

dlvenable requested changes Mar 4, 2022

View reviewed changes

asifsmohammed added 9 commits March 4, 2022 12:34

Added support for nested syntax in converter

fafb864

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Added integration tests

9f38551

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Added converter for overwrite option in Grok

813ae10

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Added converter for kv processor

7262987

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Refactoring and merge conflicts

d596180

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Updated regex for nested syntax

0fc7008

Signed-off-by: Asif Sohail Mohammed <[email protected]>

fix merge conflicts

932bdca

Signed-off-by: Asif Sohail Mohammed <[email protected]>

fix merge conflicts in MutateMapper

ded0e49

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Added group names for regex and support for data type in match pattern

fa364ce

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed added 4 commits March 4, 2022 12:45

fix merge conflicts in MutateMapper

6c7bb83

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Updated javadoc for GrokLogstashPluginAttributeMapperHelper

cf5ceb2

Signed-off-by: Asif Sohail Mohammed <[email protected]>

Added javadoc and refactoring

714fd7c

Signed-off-by: Asif Sohail Mohammed <[email protected]>

make GrokMatchUtil package protected

61a08c0

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dinujoh previously approved these changes Mar 4, 2022

View reviewed changes

dinujoh self-requested a review March 4, 2022 19:15

dinujoh reviewed Mar 4, 2022

View reviewed changes

dlvenable previously approved these changes Mar 4, 2022

View reviewed changes

fix merge conflicts in MutateMapper

52bdbcc

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed dismissed stale reviews from dlvenable and dinujoh via 52bdbcc March 4, 2022 19:41

asifsmohammed force-pushed the nested-syntax branch from eda8dcc to 52bdbcc Compare March 4, 2022 19:41

dlvenable previously approved these changes Mar 4, 2022

View reviewed changes

move initialization to constructor of Grok Mapper

8f226af

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed dismissed dlvenable’s stale review via 8f226af March 4, 2022 22:52

dinujoh approved these changes Mar 4, 2022

View reviewed changes

asifsmohammed merged commit 890e026 into opensearch-project:main Mar 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for nested syntax in converter #1088

Added support for nested syntax in converter #1088

asifsmohammed commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Feb 24, 2022 •

edited

Loading

dinujoh Feb 24, 2022

graytaylor0 Feb 25, 2022 •

edited

Loading

graytaylor0 Feb 25, 2022

asifsmohammed Feb 26, 2022

dinujoh Feb 24, 2022

dinujoh Feb 24, 2022

graytaylor0 Feb 25, 2022

graytaylor0 Feb 25, 2022

graytaylor0 Feb 25, 2022 •

edited

Loading

dinujoh Feb 28, 2022

dinujoh Feb 28, 2022

asifsmohammed Feb 28, 2022

treddeni-amazon Mar 2, 2022

asifsmohammed Mar 2, 2022

dlvenable Mar 2, 2022

dlvenable Mar 2, 2022

dlvenable Mar 2, 2022

treddeni-amazon Mar 3, 2022

asifsmohammed Mar 3, 2022

dinujoh Mar 3, 2022

dinujoh Mar 3, 2022

dinujoh Mar 3, 2022

dlvenable Mar 4, 2022

dinujoh Mar 4, 2022

		@@ -66,20 +66,22 @@ protected void mapCustomAttributes(List<LogstashAttribute> logstashAttributes, L
		if (logstashGrokMatchValueType.equals(LogstashValueType.HASH)) {


		import java.util.Map;

		public interface GrokLogstashPluginAttributeMapperHelper {


		import static org.opensearch.dataprepper.logstash.mapping.GrokLogstashPluginAttributesMapper.LOGSTASH_GROK_MATCH_ATTRIBUTE_NAME;

		class GrokMatchAttributeHelper implements GrokLogstashPluginAttributeMapperHelper {

Added support for nested syntax in converter #1088

Added support for nested syntax in converter #1088

Conversation

asifsmohammed commented Feb 24, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Feb 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

graytaylor0 Feb 25, 2022 • 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

graytaylor0 Feb 25, 2022 • 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

asifsmohammed commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Feb 24, 2022 •

edited

Loading

graytaylor0 Feb 25, 2022 •

edited

Loading

graytaylor0 Feb 25, 2022 •

edited

Loading