Integrated logstash converter into Data Prepper core #591

asifsmohammed · 2021-11-13T08:09:29Z

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

Description

Integrated Logstash converter into Data Prepper core
Updated LogstashVisitor class name

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.

codecov-commenter · 2021-11-13T08:16:21Z

Codecov Report

Merging #591 (60a88ec) into main (368795f) will decrease coverage by 0.69%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main     #591      +/-   ##
============================================
- Coverage     92.31%   91.61%   -0.70%     
+ Complexity      571      568       -3     
============================================
  Files            72       72              
  Lines          1731     1742      +11     
  Branches        144      144              
============================================
- Hits           1598     1596       -2     
- Misses          102      113      +11     
- Partials         31       33       +2

Impacted Files	Coverage Δ
...ava/com/amazon/dataprepper/DataPrepperExecute.java	`0.00% <0.00%> (ø)`
...com/amazon/dataprepper/pipeline/ProcessWorker.java	`85.71% <0.00%> (-5.72%)`	⬇️

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 368795f...60a88ec. Read the comment docs.

graytaylor0 · 2021-11-13T17:29:09Z

...configuration/src/main/java/org/opensearch/dataprepper/logstash/LogstashConfigConverter.java

@@ -33,7 +33,8 @@ public String convertLogstashConfigurationToPipeline(String logstashConfiguratio
        LogstashParser parser = new LogstashParser(tokens);
        final ParseTree tree = parser.config();

-        org.opensearch.dataprepper.logstash.parser.LogstashVisitor visitor = new org.opensearch.dataprepper.logstash.parser.LogstashVisitor();
+        org.opensearch.dataprepper.logstash.parser.ModelConvertingLogstashVisitor visitor =


Is there a reason you didn't import org.opensearch.dataprepper.logstash.parser.LogstashVisitor

Prior to renaming LogstashVisitor, it had a conflicting class name with an ANTLR-generated class. So it required the fully qualified name. Now that it is renamed, it can be imported.

graytaylor0 · 2021-11-13T17:43:33Z

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

+            String configurationFileLocation = args[0];
+            if (args[0].endsWith(".conf")) {
+                LogstashConfigConverter logstashConfigConverter = new LogstashConfigConverter();
+                try {
+                    configurationFileLocation = logstashConfigConverter.convertLogstashConfigurationToPipeline(
+                            args[0], String.valueOf(Paths.get("data-prepper-core/build/libs/")));
+                } catch (IOException e) {
+                    e.printStackTrace();
+                }
+            }


I would consider pulling this out into a method that takes in the config file name to provide some context. Something like

private static String checkForLogstashConfAndConvert(String configurationFileLocation) and use it like this: configurationFileLocation = checkForLogstashConfigAndConvert(configurationFileLocation);

dlvenable · 2021-11-13T20:19:44Z

...configuration/src/main/java/org/opensearch/dataprepper/logstash/LogstashConfigConverter.java

@@ -33,7 +33,8 @@ public String convertLogstashConfigurationToPipeline(String logstashConfiguratio
        LogstashParser parser = new LogstashParser(tokens);
        final ParseTree tree = parser.config();

-        org.opensearch.dataprepper.logstash.parser.LogstashVisitor visitor = new org.opensearch.dataprepper.logstash.parser.LogstashVisitor();
+        org.opensearch.dataprepper.logstash.parser.ModelConvertingLogstashVisitor visitor =


Prior to renaming LogstashVisitor, it had a conflicting class name with an ANTLR-generated class. So it required the fully qualified name. Now that it is renamed, it can be imported.

dlvenable · 2021-11-13T20:20:40Z

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

+                    configurationFileLocation = logstashConfigConverter.convertLogstashConfigurationToPipeline(
+                            args[0], String.valueOf(Paths.get("data-prepper-core/build/libs/")));
+                } catch (IOException e) {
+                    e.printStackTrace();


We should be using SLF4J logging for this. Somewhat like: LOG.error("custom error message", ex).

dlvenable · 2021-11-13T20:22:44Z

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

+                LogstashConfigConverter logstashConfigConverter = new LogstashConfigConverter();
+                try {
+                    configurationFileLocation = logstashConfigConverter.convertLogstashConfigurationToPipeline(
+                            args[0], String.valueOf(Paths.get("data-prepper-core/build/libs/")));


This is the path at compile-time. But, when a user is running Data Prepper, this path will not exist. I think just using the current working directory is sufficient for now. It will need to be revisited along with implementing #305 .

This is in reference to the string "data-prepper-core/build/libs/".

Got it, just thinking where the .yaml file should be saved.
I think we can remove the 2nd parameter and save it in same directory as .conf file.

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

asifsmohammed requested a review from dlvenable November 13, 2021 08:09

asifsmohammed requested a review from a team as a code owner November 13, 2021 08:09

graytaylor0 reviewed Nov 13, 2021

View reviewed changes

dlvenable requested changes Nov 13, 2021

View reviewed changes

Integrated logstash converter into Data Prepper core

a7cd55d

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

asifsmohammed force-pushed the integratewithcore branch from 2b5c60f to e9bddab Compare November 13, 2021 21:23

dlvenable previously approved these changes Nov 13, 2021

View reviewed changes

Updated integration of logstash converter into Data Prepper core

60a88ec

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

asifsmohammed dismissed dlvenable’s stale review via 60a88ec November 13, 2021 22:14

asifsmohammed force-pushed the integratewithcore branch from e9bddab to 60a88ec Compare November 13, 2021 22:14

dlvenable approved these changes Nov 13, 2021

View reviewed changes

sshivanii approved these changes Nov 14, 2021

View reviewed changes

dlvenable merged commit f677480 into opensearch-project:main Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated logstash converter into Data Prepper core #591

Integrated logstash converter into Data Prepper core #591

asifsmohammed commented Nov 13, 2021 •

edited

Loading

codecov-commenter commented Nov 13, 2021 •

edited

Loading

graytaylor0 Nov 13, 2021

dlvenable Nov 13, 2021

graytaylor0 Nov 13, 2021

dlvenable Nov 13, 2021

dlvenable Nov 13, 2021

dlvenable Nov 13, 2021

dlvenable Nov 13, 2021

asifsmohammed Nov 13, 2021

Integrated logstash converter into Data Prepper core #591

Integrated logstash converter into Data Prepper core #591

Conversation

asifsmohammed commented Nov 13, 2021 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Nov 13, 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

asifsmohammed commented Nov 13, 2021 •

edited

Loading

codecov-commenter commented Nov 13, 2021 •

edited

Loading