Add support for dynamic rule detection for pipeline config transformation #4601

srikanthjg · 2024-06-05T05:07:30Z

Description

Dynamically detects rules present in rules folder and applies a corresponding transformation based on plugin name.

Issues Resolved

Resolves #4600

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- 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.

oeyh · 2024-06-11T20:55:34Z

...rc/main/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactory.java

+
+        Path rulesFolderPath;
+
+        if ("jar".equals(uri.getScheme())) {


When do we expect the rules in a jar file?

in runtime, we would expect everything inside a jar.

oeyh · 2024-06-11T21:39:36Z

...est/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactoryTest.java

+        List<Path> mockRuleFiles = Arrays.asList(
+                Paths.get("src/test/resources/transformation/rules/documentdb-rule1.yaml"),
+                Paths.get("src/test/resources/transformation/rules/documentdb-rule.yaml")
+        );
+        doReturn(mockRuleFiles).when(transformersFactory).getRuleFiles();
+        assertTrue(mockRuleFiles.size() > 0, "There should be at least one rule file.");


You defined mockRuleFiles and assert it's not empty. Does it actually test the method?

dlvenable · 2024-06-13T18:34:24Z

...rc/main/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactory.java

+        // Get the URI of the rules folder
+        URI uri = null;
+        try {
+            uri = getClass().getClassLoader().getResource("rules").toURI();


Let's use a predefined directory structure to ensure that we are only getting the transform rules:

org/opensearch/dataprepper/transforms/

Thus, we'd have:

org/opensearch/dataprepper/transforms/rules/ org/opensearch/dataprepper/transforms/templates/

The way i understand this comment and this comment, we want to move the template and rules to be plugin specific and move this into data-prepper-plugin directory, ie, each plugin gets a rule and template but there are some limitations to this approach. If for example, there is an intent by an user for which plugin does not exist and we want to tranform that to other processors, where do we place the rules and templates in this case. Example for this is with respect to security lake.

dlvenable · 2024-06-13T18:40:07Z

...rc/main/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactory.java

+            }
+        } else {
+            // File is not inside a JAR
+            rulesFolderPath = Paths.get(uri);


When would this ever occur?

I was trying to address 2 cases:

"jar" scheme: This case occurs when the code is running from within a JAR file, ie during runtime.

Non-"jar" scheme: This case occurs when the code is running in a development environment where the rules directory is available in the file system and not within a jar.

dlvenable · 2024-06-13T18:42:47Z

...rc/main/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactory.java

-    public InputStream getPluginRuleFileStream(String pluginName) {
-        if(pluginName == null || pluginName.isEmpty()){
-            throw  new RuntimeException("Transformation plugin not found");
+    public List<Path> getRuleFiles() {


This API contract lists out Paths for rules. Then the caller comes back and get the input stream by calling readRuleFile. You should be able to consolidate these.

Just make one method: Collection<InputStream> loadRules().

I do see that you use the file name later, I think for debugging purposes. You could make a class that holds both a name and an InputStream.

class RuleInputStream { private String name; private InputStream inputStream }

dlvenable · 2024-06-13T18:44:36Z

...parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleFileEvaluation.java

+@AllArgsConstructor
+@Data
+public class RuleFileEvaluation {
+    Boolean result;


These should all be private. And I think they can also be private.

dlvenable · 2024-06-13T18:47:11Z

data-prepper-pipeline-parser/src/test/resources/transformation/rules/documentdb-rule.yaml

@@ -1,3 +1,4 @@
+plugin_name: "documentdb"


As this is for testing, let's add test in the name. Maybe test-documentdb.

@srikanthjg do you want to make this small change?

I think it will be inconsistent even within the same rule yaml. The apply_when condition for the rule yaml, we still define it as documentdb and it would looks as follows:

plugin_name: "documentdbtest" apply_when: - "$..source.documentdb"

We still want use documentdb and not documentdbtest in the apply_when condition as that is what is used in the pipeline yaml; and that cannot be mocked that easily.

The plugin name in the rule yaml is only used to get the corresponding template, but it seems inconsistent if we use documentdb vs documentdbtest even within the same file for the same plugin we want to test.

Why wouldn't this work?

plugin_name: "documentdbtest" apply_when: - "$..source.documentdbtest"

These files are only used for unit testing, right? So, there is no need to keep the names matching the others.

This is where you use it:

https://github.com/opensearch-project/data-prepper/pull/4601/files#diff-40ef318469b1b8fb28c6ee16a998f746921ecee5471a9f481d497617b60a2180R38

i will need to change like 20 files(test yaml files) in the test as all of them use documentdb. i cant just change the rule to documentdbtest and make it work. The userconfig, expected output and template depends on this. can i add this as another PR? it should it mostly replace all but i see some other issues with it.

dlvenable · 2024-06-13T18:47:23Z

data-prepper-pipeline-parser/src/test/resources/transformation/rules/documentdb1-rule.yaml

@@ -1,2 +1,3 @@
+plugin_name: "documentdb"


Similar comment as above, please add test into the name.

dlvenable · 2024-06-13T18:48:13Z

...line-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluator.java

+                            break;
+                        }
+                    } catch (PathNotFoundException e) {
+                        LOG.debug("Json Path not found for {}", ruleFile.getFileName().toString());


No need to call .toString() here.

dlvenable · 2024-06-13T18:49:40Z

...-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluatorTest.java

-        Map sourceOptions = new HashMap<String, Object>();
-        Map s3_bucket = new HashMap<>();
+        Map<String, Object> sourceOptions = new HashMap<>();
+        Map<String, Object> s3_bucket = new HashMap<>();


Please use correct naming conventions: s3Bucket.

dlvenable · 2024-06-13T18:49:55Z

...-parser/src/test/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluatorTest.java

-        sourceOptions.put("option2", null);
-        final PluginModel source = new PluginModel("http", sourceOptions);
+        Map<String, Object> sourceOptions = new HashMap<>();
+        Map<String, Object> s3_bucket = new HashMap<>();


Similar comment about variable naming conventions.

dlvenable · 2024-06-13T18:51:48Z

data-prepper-pipeline-parser/src/main/resources/rules/documentdb-rule.yaml

@@ -1,3 +1,4 @@
+plugin_name: "documentdb"


Let's move this into the mongodb plugin project.

#4601 (comment)

kkondaka · 2024-08-05T03:47:44Z

@srikanthjg Please address David's other comments. I will approve again and we can ask Hai to review too.

kkondaka · 2024-08-05T19:17:55Z

@srikanthjg I am not sure if we are going to have a "real" plugin for each of the plugins in the pre-transformation config file. So, while moving documentdb related rules and templates to document db directory is a good suggestion by David, it might not be possible in all cases. For now, I suggest we just commit what you implemented in this PR and if needed, we can take the approach suggested by David in a future PR

…tion Signed-off-by: Srikanth Govindarajan <srikanthjg123@gmail.com>

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable · 2024-08-16T19:06:38Z

data-prepper-plugins/dummy-plugin/build.gradle

You can remove this empty file. You don't need a build.gradle file at all.

build.gradle is needed to this plugin as a module. When i tried without build.gradle, classloader search was not picking up the rule file from the folder.

dlvenable · 2024-08-16T19:06:57Z

...plugin/src/main/resources/org/opensearch/dataprepper/transforms/rules/dummy-plugin-rule.yaml

Please add a README.md to this project to explain how to use it and test with it.

dlvenable · 2024-08-16T19:08:39Z

...line-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluator.java

-            } catch (FileNotFoundException e){
-                LOG.error("Template File Not Found for {}", PLUGIN_NAME);
+            } catch (FileNotFoundException e) {
+                LOG.error("Template File Not Found");


Why did you remove the additional information for tracking the problem?

will add it back.

dlvenable · 2024-08-16T19:09:13Z

...line-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluator.java

                }
            }
+
+        } catch (FileNotFoundException e) {
+            LOG.debug("Rule File Not Found");


Should this be an error?

Also, can you include the file name in the log?

dlvenable · 2024-08-16T19:12:34Z

...rc/main/java/org/opensearch/dataprepper/pipeline/parser/transformer/TransformersFactory.java

+                        rulesPath = Paths.get(rulesURL.toURI());
+                    } catch (FileSystemNotFoundException e) {
+                        // Handle the case where the file system is not accessible (e.g., in a JAR)
+                        FileSystem fileSystem = FileSystems.newFileSystem(rulesURL.toURI(), Collections.emptyMap());


Is this going to work? These will always be in jar files.

this works. It will always hit this code when dataprepper is running because it is always in jar during execution but will differ when unit testing.

dlvenable · 2024-08-16T20:31:50Z

...line-parser/src/main/java/org/opensearch/dataprepper/pipeline/parser/rule/RuleEvaluator.java

@@ -64,7 +65,7 @@ public RuleEvaluatorResult isTransformationNeeded(PipelinesDataFlowModel pipelin
                            .build();
                }
            } catch (FileNotFoundException e) {
-                LOG.error("Template File Not Found");
+                LOG.error("Template File Not Found for {}", PLUGIN_NAME);


Actually, you don't even need this catch block. You can remove it. And then go back to using a local pluginName variable.

dlvenable · 2024-08-16T20:32:04Z

data-prepper-plugins/dummy-plugin/README.md

+For Example:
+
+User Config:
+```aidl


Suggested change

```aidl

```yaml

dlvenable · 2024-08-16T20:32:13Z

data-prepper-plugins/dummy-plugin/README.md

+on the template.
+
+Rule:
+```aidl


Suggested change

```aidl

```yaml

dlvenable · 2024-08-16T20:32:22Z

data-prepper-plugins/dummy-plugin/README.md

+```
+
+Template:
+```aidl


Suggested change

```aidl

```yaml

dlvenable · 2024-08-16T20:32:29Z

data-prepper-plugins/dummy-plugin/README.md

+```
+
+Output:
+```aidl


Suggested change

```aidl

```yaml

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

srikanthjg requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, asifsmohammed, KarstenSchnitter, dlvenable and oeyh as code owners June 5, 2024 05:07

kkondaka previously approved these changes Jun 8, 2024

View reviewed changes

oeyh reviewed Jun 11, 2024

View reviewed changes

dlvenable requested changes Jun 13, 2024

View reviewed changes

srikanthjg dismissed kkondaka’s stale review via 4a7d5d4 August 7, 2024 19:59

srikanthjg force-pushed the dynamic branch from 7de88ce to 4a7d5d4 Compare August 7, 2024 19:59

srikanthjg and others added 3 commits August 14, 2024 12:20

Add support for dynamic rule detection for pipeline config transforma…

ac2b6e8

…tion Signed-off-by: Srikanth Govindarajan <srikanthjg123@gmail.com>

Address comments

9f02768

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

Move rules and templates to plugin level

d7a0999

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

srikanthjg force-pushed the dynamic branch from 4a7d5d4 to d7a0999 Compare August 16, 2024 17:29

Add dummy plugin for testing dynamic rule detection

d8827d3

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

dlvenable requested changes Aug 16, 2024

View reviewed changes

Address comments

2eaa778

Signed-off-by: Srikanth Govindarajan <srigovs@amazon.com>

srikanthjg force-pushed the dynamic branch from 0de03e3 to 2eaa778 Compare August 16, 2024 21:06

dlvenable approved these changes Aug 16, 2024

View reviewed changes

kkondaka approved these changes Aug 17, 2024

View reviewed changes

kkondaka merged commit 38f8079 into opensearch-project:main Aug 17, 2024
72 of 74 checks passed

+              ```
+              Output:
+              ```aidl

Add support for dynamic rule detection for pipeline config transformation #4601

Add support for dynamic rule detection for pipeline config transformation #4601

Conversation

srikanthjg commented Jun 5, 2024 • edited Loading

Description

Issues Resolved

Check List

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

srikanthjg Aug 5, 2024 • 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

kkondaka commented Aug 5, 2024

kkondaka commented Aug 5, 2024

Choose a reason for hiding this comment

srikanthjg Aug 16, 2024 • 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

srikanthjg Aug 16, 2024 • 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

srikanthjg commented Jun 5, 2024 •

edited

Loading

srikanthjg Aug 5, 2024 •

edited

Loading

srikanthjg Aug 16, 2024 •

edited

Loading

srikanthjg Aug 16, 2024 •

edited

Loading