added default mapper for mapping logstash config models to data prepper plugin model #559

asifsmohammed · 2021-11-09T22:39:14Z

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

Description

Added default plugin mapper to convert Logstash config to Data Prepper models.
Added LogstashMappingException class for exceptions during mapping

Issues Resolved

#466

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.

codecov-commenter · 2021-11-09T22:49:02Z

Codecov Report

Merging #559 (90af350) into main (52e0468) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #559   +/-   ##
=========================================
  Coverage     92.28%   92.28%           
  Complexity      569      569           
=========================================
  Files            71       71           
  Lines          1725     1725           
  Branches        144      144           
=========================================
  Hits           1592     1592           
  Misses          102      102           
  Partials         31       31

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 52e0468...90af350. Read the comment docs.

dlvenable · 2021-11-09T23:20:07Z

...guration/src/main/java/org/opensearch/dataprepper/logstash/mapping/LogstashPluginMapper.java

@@ -12,5 +14,5 @@
     * @param logstashPlugin A Logstash plugin with its attributes
     * @return A Data Prepper plugin with its attributes
     */
-    PluginModel mapPlugin(LogstashPlugin logstashPlugin);
+    PluginModel mapPlugin(LogstashPlugin logstashPlugin) throws IOException;


This should not throw IOException. We should wrap any such exceptions in the LogstashMappingException.

dlvenable · 2021-11-09T23:21:55Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+            throw new LogstashMappingException("file not found! " + logstashPlugin.getPluginName() + ".mapping.yaml");
+        }
+
+        LogstashMappingModel logstashMappingModel = objectMapper.readValue(inputStream, LogstashMappingModel.class);


Please catch this IOException and wrap it in a LogstashMappingException.

Something like:

try { logstashMappingModel = objectMapper.readValue(inputStream, LogstashMappingModel.class); } catch(IOException ex) { throw new LogstashMappingException("Unable to parse mapping file " + logstashPlugin.getPluginName() + ".mapping.yaml", ex); }

You will need to add an overloaded constructor to LogstashMappingException.

dlvenable · 2021-11-09T23:22:27Z

...on/src/main/java/org/opensearch/dataprepper/logstash/exception/LogstashMappingException.java

+ */
+public class LogstashMappingException extends LogstashConfigurationException{
+
+    public LogstashMappingException(String errorMessage) {


Per my comment on wrapping IOException, you will need another constructor with (String message, Exception inner).

Thanks, I'll update LogstashMappingException

dlvenable · 2021-11-09T23:24:21Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+    @Override
+    public PluginModel mapPlugin(LogstashPlugin logstashPlugin) throws IOException {
+
+        InputStream inputStream = getClass().getClassLoader().getResourceAsStream(logstashPlugin.getPluginName() + ".mapping.yaml");


Please extract a variable for this: logstashPlugin.getPluginName() + ".mapping.yaml".

You use it below.

And another comment suggests another place to use it.

I would like to get a uniform naming convention on this.
Are we using extension .yml or .yaml or supporting both?
In our configuration README, we mention .yaml but in our examples we use .yml

I think we should use .yaml consistently.

It is also FAQ 1 out of 2.

https://yaml.org/faq.html

dlvenable · 2021-11-09T23:25:43Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+
+        InputStream inputStream = getClass().getClassLoader().getResourceAsStream(logstashPlugin.getPluginName() + ".mapping.yaml");
+        if (inputStream == null) {
+            throw new LogstashMappingException("file not found! " + logstashPlugin.getPluginName() + ".mapping.yaml");


Please rename this to: "Unable to find mapping resource " + ....

dlvenable · 2021-11-09T23:30:16Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+
+        LogstashMappingModel logstashMappingModel = objectMapper.readValue(inputStream, LogstashMappingModel.class);
+        if (logstashMappingModel.getPluginName() == null)
+            throw new LogstashMappingException("plugin name cannot be null in mapping.yaml");


I'd like to see the following in this message:

The actual name of the mapping resources (using extracted variable I suggested above).

The property name as defined in the LogstashMappingModel: pluginName.

Perhaps something like: "The mapping file " + mappingResourceName + " has a null value for 'pluginName'.".

dlvenable · 2021-11-09T23:32:28Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+                        (String) logstashMappingModel.getMappedAttributes().get(logstashAttribute.getAttributeName()),
+                        logstashAttribute.getAttributeValue().getValue()
+                );
+            }


I think we may wish to log a warning for unmapped Logstash attributes, in an else.

dlvenable · 2021-11-09T23:35:57Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+        logstashPlugin.getAttributes().forEach(logstashAttribute -> {
+            if (logstashMappingModel.getMappedAttributes().containsKey(logstashAttribute.getAttributeName())) {
+                pluginSettings.put(
+                        (String) logstashMappingModel.getMappedAttributes().get(logstashAttribute.getAttributeName()),


We should change the model for getMappedAttributes. It should be Map<String, String> since it always maps names.

Also, I think it might be clearer to rename it to attributeNames.

dlvenable · 2021-11-10T03:30:37Z

...iguration/src/main/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapper.java

+ *
+ * @since 1.2
+ */
+public class DefaultPluginMapper implements LogstashPluginMapper {


This class should have unit tests as well.

dlvenable · 2021-11-11T02:11:08Z

data-prepper-logstash-configuration/build.gradle

@@ -11,6 +11,9 @@ repositories {
 dependencies {
    antlr "org.antlr:antlr4:4.9.2"
    implementation project(':data-prepper-api')
+    implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml'
+    implementation 'com.fasterxml.jackson.core:jackson-databind'
+    implementation 'org.slf4j:slf4j-simple:1.7.32'


This should be testImplementation if we use it all. Otherwise, it goes into the main classpath, but there should be only one SLF4J in the classpath.

Without this there's a warning and it's not showing the Log.warn message in log
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.

dlvenable · 2021-11-11T02:11:56Z

...ation/src/test/java/org/opensearch/dataprepper/logstash/mapping/DefaultPluginMapperTest.java

+
+    @BeforeEach
+    void createObjectUnderTest() {
+        defaultPluginMapper = spy(new DefaultPluginMapper());


Does this need to be a Spy?

Yeah, It's not required

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

jianghancn · 2021-11-11T18:48:06Z

...ration/src/test/resources/org/opensearch/dataprepper/logstash/mapping/amazon_es.mapping.yaml

@@ -0,0 +1,8 @@
+pluginName: opensearch


Minor: Would it be clearer if the file name is something like: logstash_to_dataprepper_config.mapping.yaml
Minor: it would be nicer to cover more valid configuration mappings in the unit tests, such as username, password, and many more.

It's unit test. I am OK with you adding more tests later. I will approve the pull request now.

Thanks, I'll will add more tests later

dlvenable requested changes Nov 9, 2021

View reviewed changes

dlvenable reviewed Nov 10, 2021

View reviewed changes

asifsmohammed force-pushed the mapping branch from bfa8cd1 to 90af350 Compare November 10, 2021 23:08

dlvenable reviewed Nov 11, 2021

View reviewed changes

dlvenable requested changes Nov 11, 2021

View reviewed changes

asifsmohammed force-pushed the mapping branch from 90af350 to fa9dd0c Compare November 11, 2021 04:58

dlvenable previously approved these changes Nov 11, 2021

View reviewed changes

asifsmohammed and others added 3 commits November 11, 2021 11:52

added default mapper for mapping logstash config models to plugin model

cb4127d

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

updated default plugin mapper and added unit tests

a2dc586

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

updated resource paths

e880210

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

asifsmohammed dismissed dlvenable’s stale review via e880210 November 11, 2021 17:53

asifsmohammed force-pushed the mapping branch from fa9dd0c to e880210 Compare November 11, 2021 17:53

dlvenable approved these changes Nov 11, 2021

View reviewed changes

dlvenable mentioned this pull request Nov 11, 2021

Corrected the file path for Logstash mapping files. #467 #565

Merged

5 tasks

jianghancn reviewed Nov 11, 2021

View reviewed changes

jianghancn approved these changes Nov 11, 2021

View reviewed changes

asifsmohammed merged commit cd0f617 into opensearch-project:main Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added default mapper for mapping logstash config models to data prepper plugin model #559

added default mapper for mapping logstash config models to data prepper plugin model #559

asifsmohammed commented Nov 9, 2021 •

edited

Loading

codecov-commenter commented Nov 9, 2021 •

edited

Loading

dlvenable Nov 9, 2021

dlvenable Nov 9, 2021

dlvenable Nov 9, 2021

asifsmohammed Nov 10, 2021

dlvenable Nov 9, 2021

sshivanii Nov 10, 2021

dlvenable Nov 10, 2021

dlvenable Nov 9, 2021

dlvenable Nov 9, 2021

dlvenable Nov 9, 2021

dlvenable Nov 9, 2021

dlvenable Nov 10, 2021

dlvenable Nov 11, 2021

asifsmohammed Nov 11, 2021

dlvenable Nov 11, 2021

asifsmohammed Nov 11, 2021

jianghancn Nov 11, 2021

asifsmohammed Nov 11, 2021

added default mapper for mapping logstash config models to data prepper plugin model #559

added default mapper for mapping logstash config models to data prepper plugin model #559

Conversation

asifsmohammed commented Nov 9, 2021 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Nov 9, 2021 • edited Loading

Codecov Report

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

asifsmohammed commented Nov 9, 2021 •

edited

Loading

codecov-commenter commented Nov 9, 2021 •

edited

Loading