File source improvements and change to event model #601

graytaylor0 · 2021-11-16T03:21:07Z

Description

Update the file source to use the Event model
Update the file source to use @DataPrepperPluginConstructor
Add new functionality to the file source. This includes a new format and type configuration options.
The type configuration will be added to the eventType of the eventMetada. Once conditionals are implemented, this could be useful for users to route their data depending on the type
The format configuration is an enum that allows for either json or plain lines to be read from a file. The plain format will add a message key to the Event. This message, or whatever else is decided, should be a standard for Data Prepper going forward when dealing with plaintext.

Issues Resolved

Closes #600

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-16T03:29:16Z

Codecov Report

Merging #601 (187a3f3) into main (54f8ce7) will decrease coverage by 0.10%.
The diff coverage is 91.93%.

@@             Coverage Diff              @@
##               main     #601      +/-   ##
============================================
- Coverage     91.70%   91.59%   -0.11%     
- Complexity      571      584      +13     
============================================
  Files            72       74       +2     
  Lines          1747     1784      +37     
  Branches        145      148       +3     
============================================
+ Hits           1602     1634      +32     
- Misses          113      115       +2     
- Partials         32       35       +3

Impacted Files	Coverage Δ
...on/dataprepper/plugins/source/file/FileSource.java	`88.09% <88.09%> (ø)`
...on/dataprepper/plugins/source/file/FileFormat.java	`100.00% <100.00%> (ø)`
...aprepper/plugins/source/file/FileSourceConfig.java	`100.00% <100.00%> (ø)`

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 54f8ce7...187a3f3. Read the comment docs.

chenqi0805 · 2021-11-16T15:07:50Z

data-prepper-plugins/common/README.md

-A source plugin to read input data from the specified file path.
-
- path (String): absolute input data file path
+A source plugin to read input data from the specified file path. The file source creates a new Record for eadh line of data in the file.


chenqi0805 · 2021-11-16T15:14:26Z

data-prepper-plugins/common/README.md

+    * `json`: Reads data that is in the form of a JSON string from a file. If the json string is unable to be parsed, the file source will treat it as a plaintext line.
+
+
+* type (String): The Event type that will be stored in the metadata of the Event. Default is `event`. 


Nit: `type`

chenqi0805 · 2021-11-16T15:15:46Z

data-prepper-plugins/common/README.md

+    { "message": "Example log line in file" }
+    ```
+
+    * `json`: Reads data that is in the form of a JSON string from a file. If the json string is unable to be parsed, the file source will treat it as a plaintext line.


From the code change, maybe we should specify that this format expects json lines as follows

{ "key1": "val1" } { "key2": "val2" } { "key3": "val3" }

instead of a single json blob

chenqi0805 · 2021-11-16T15:44:37Z

...pper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/source/file/FileSource.java

+
+    @DataPrepperPluginConstructor
+    public FileSource(final FileSourceConfig fileSourceConfig, final PluginMetrics pluginMetrics, final PluginFactory pluginFactory) {
+        Objects.requireNonNull(fileSourceConfig.getFilePathToRead(), "File path is required");


Optional: we can tighten our validation on file path by Files.exists(...) check.

chenqi0805

LGTM!

graytaylor0 · 2021-11-16T17:23:58Z

Looks like the DataPrepperTests are using a file source and that's failing the test. It's a little weird that this was being used but I will make the change to it.

chenqi0805 · 2021-11-16T19:20:50Z

data-prepper-core/src/test/resources/valid_multiple_pipeline_configuration.yml

@@ -1,8 +1,7 @@
 # this configuration file is solely for testing formatting
 test-pipeline-1:
  source:
-    file:
-      path: "/tmp/file-source.tmp"
+    stdin:


What is the failure traceback with file source here? Asking b/c we might run into the failure again when stdin is being migrated to use Event model

This is actually a problem with how the plugin is being loaded and the fact that I changed to using @DataPrepperPluginConstructor and PluginSetting is not being used anymore. StdinSink still uses PluginSetting so it works. We should have a separate issue for this bug as these tests should be refactored.

Resolved. Validation of path existing was breaking the test

cmanning09 · 2021-11-16T22:41:09Z

data-prepper-plugins/common/build.gradle

 }

 jacocoTestCoverageVerification {
    dependsOn jacocoTestReport
    violationRules {
        rule { //in addition to core projects rule
            limit {
-                minimum = 0.90
+                minimum = 0.89


Why do we need to lower code coverage here?

cmanning09 · 2021-11-16T22:43:11Z

data-prepper-plugins/common/README.md

+
+
+
+* `type` (String): The Event type that will be stored in the metadata of the Event. Default is `event`. 


type is very generic. Let's call this even_type.

Going to call it record_type for now as I am using this type to support both event and string

cmanning09 · 2021-11-16T22:52:44Z

data-prepper-plugins/common/README.md

+
+* `path` (String): absolute input data file path. It is required
+
+* `write_timeout` (int): The amount of time to attempt writing a Record to the Buffer before timing out. Unit is milliseconds and default is `5,000`  


Can a user disable the timeout? Is there a maximum or minimum value?

I'm just going to make this unconfigurable for now. It is too coupled to Data Prepper internals, and it takes away from the current use case of the FileSource, which is to test and get things up and running quickly

cmanning09 · 2021-11-16T22:54:31Z

...lugins/common/src/main/java/com/amazon/dataprepper/plugins/source/file/FileSourceConfig.java

+    public String getType() {
+        return type;
+    }
+}


Let's introduce validation to this config. We should be able to do this while we are waiting on JSR-303. Check out the HttpSourceConfig for an example.

cmanning09 · 2021-11-16T22:57:37Z

...plugins/common/src/test/java/com/amazon/dataprepper/plugins/source/file/FileSourceTests.java

+
+    final List<Record<Event>> expectedEventsPlain = new ArrayList<>();
+    final List<Record<Event>> expectedEventsJson = new ArrayList<>();
+    final List<Record<Event>> expectedEventsInvalidJson = new ArrayList<>();


The current way you are instantiating and adding to this list is problemmatic. I would recommend removing the final modifier from these class variables so you can reassign the values for each test as part ofthe @BeforeEach method. This will ensure your unit tests are passing some left over state through the class variables.

cmanning09 · 2021-11-16T22:59:47Z

...pper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/source/file/FileSource.java

+                structuredLine.put(MESSAGE_KEY, line);
+                break;
+            default:
+                LOG.error("The file source type is [{}]. It must be \"json\" or \"plain\"", fileSourceConfig.getType().toString());


This is going to create an event with no data. Is that what we want?

Introducing config validation will prevent this.

cmanning09 · 2021-11-16T23:00:15Z

...pper-plugins/common/src/main/java/com/amazon/dataprepper/plugins/source/file/FileSource.java

+    }
+
+    private Record<Event> getEventRecordFromLine(final String line) {
+        Map<String, Object> structuredLine = new HashMap<>();


It can't be final it's getting assigned in the JSON case. I could pass it as an argument but I like how it is now better.

Signed-off-by: Taylor Gray <[email protected]>

…or json and plaintext Signed-off-by: Taylor Gray <[email protected]>

Signed-off-by: Taylor Gray <[email protected]>

…repperTest Signed-off-by: Taylor Gray <[email protected]>

…ts call Signed-off-by: Taylor Gray <[email protected]>

Signed-off-by: Taylor Gray <[email protected]>

dlvenable · 2021-11-17T15:13:55Z

data-prepper-plugins/common/README.md

+      ```
+
+
+* `record_type` (String): The Event type that will be stored in the metadata of the Event. Default is `event`. 


Thanks for adding this, even though it is temporary.

I'd generally prefer to keep the default as string since that is the default behavior. But, since this is more for test and non-Trace Analytics pipelines, it could be a reasonable breaking change. If the default is event, the release notes should state this, and how to get past it. #618 .

Hmm I can make the default string, but will have to temporarily change some grok documentation, which is less painful in the end than making it a breaking change.

dlvenable · 2021-11-17T15:17:13Z

data-prepper-plugins/common/build.gradle

@@ -14,6 +14,7 @@ plugins {
 }
 dependencies {
    api project(':data-prepper-api')
+    implementation project(':data-prepper-plugins:blocking-buffer')


I think you only need this for the tests. It can be made to be testImplementation and should sit with the other testImplementation dependencies.

dlvenable · 2021-11-17T15:20:32Z

...plugins/common/src/test/java/com/amazon/dataprepper/plugins/source/file/FileSourceTests.java

+        final HashMap<String, Object> integerHashMap = new HashMap<>();
+        integerHashMap.put("buffer_size", 2);
+        integerHashMap.put("batch_size", 2);
+        final PluginSetting pluginSetting = new PluginSetting("blocking_buffer", integerHashMap) {{


This is an anti-pattern which I'm hoping to get away from in Data Prepper. This is creating an anonymous class with a static initializer. It is better to to break this into two statements - one for new and the other to setPipelineName.

You can probably even create a mock which is better.

Signed-off-by: Taylor Gray <[email protected]>

graytaylor0 requested review from dlvenable and cmanning09 November 16, 2021 03:21

graytaylor0 requested a review from a team as a code owner November 16, 2021 03:21

graytaylor0 changed the title ~~File source improvements~~ File source improvements and change to event model Nov 16, 2021

chenqi0805 reviewed Nov 16, 2021

View reviewed changes

chenqi0805 previously approved these changes Nov 16, 2021

View reviewed changes

graytaylor0 dismissed chenqi0805’s stale review via f82750f November 16, 2021 18:20

chenqi0805 reviewed Nov 16, 2021

View reviewed changes

chenqi0805 previously approved these changes Nov 16, 2021

View reviewed changes

cmanning09 suggested changes Nov 16, 2021

View reviewed changes

graytaylor0 dismissed chenqi0805’s stale review via f1d8751 November 16, 2021 23:37

graytaylor0 added 7 commits November 16, 2021 17:39

Added md for Log Ingestion Demo Guide

c76483a

Signed-off-by: Taylor Gray <[email protected]>

More fixes

0e8bf7f

Signed-off-by: Taylor Gray <[email protected]>

Refactored file source, added type and format configuration options f…

bf8d638

…or json and plaintext Signed-off-by: Taylor Gray <[email protected]>

Address PR comments

7fb9329

Signed-off-by: Taylor Gray <[email protected]>

Changed format JsonProperty to String, removed file source from DataP…

3b580ec

…repperTest Signed-off-by: Taylor Gray <[email protected]>

Allow FileSource to support both Event and String, removed Paths.exis…

b76ed51

…ts call Signed-off-by: Taylor Gray <[email protected]>

Address PR comments

187a3f3

Signed-off-by: Taylor Gray <[email protected]>

graytaylor0 force-pushed the FileSourceImprovements branch from f1d8751 to 187a3f3 Compare November 17, 2021 00:32

dlvenable requested changes Nov 17, 2021

View reviewed changes

Address PR comments, make string the default record_type

9d5050f

Signed-off-by: Taylor Gray <[email protected]>

graytaylor0 requested review from cmanning09 and dlvenable November 17, 2021 18:04

cmanning09 approved these changes Nov 18, 2021

View reviewed changes

chenqi0805 approved these changes Nov 18, 2021

View reviewed changes

dlvenable approved these changes Nov 18, 2021

View reviewed changes

graytaylor0 merged commit 515fa4d into opensearch-project:main Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File source improvements and change to event model #601

File source improvements and change to event model #601

graytaylor0 commented Nov 16, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading

chenqi0805 Nov 16, 2021

chenqi0805 Nov 16, 2021 •

edited

Loading

chenqi0805 Nov 16, 2021

chenqi0805 Nov 16, 2021

chenqi0805 left a comment

graytaylor0 commented Nov 16, 2021

chenqi0805 Nov 16, 2021 •

edited

Loading

graytaylor0 Nov 16, 2021

graytaylor0 Nov 17, 2021

cmanning09 Nov 16, 2021

cmanning09 Nov 16, 2021

graytaylor0 Nov 16, 2021

cmanning09 Nov 16, 2021

graytaylor0 Nov 16, 2021 •

edited

Loading

cmanning09 Nov 16, 2021

cmanning09 Nov 16, 2021

cmanning09 Nov 16, 2021

cmanning09 Nov 16, 2021

graytaylor0 Nov 16, 2021

dlvenable Nov 17, 2021

graytaylor0 Nov 17, 2021 •

edited

Loading

dlvenable Nov 17, 2021

dlvenable Nov 17, 2021

		* `json`: Reads data that is in the form of a JSON string from a file. If the json string is unable to be parsed, the file source will treat it as a plaintext line.


		* type (String): The Event type that will be stored in the metadata of the Event. Default is `event`.




		* `type` (String): The Event type that will be stored in the metadata of the Event. Default is `event`.


		* `path` (String): absolute input data file path. It is required

		* `write_timeout` (int): The amount of time to attempt writing a Record to the Buffer before timing out. Unit is milliseconds and default is `5,000`

		```


		* `record_type` (String): The Event type that will be stored in the metadata of the Event. Default is `event`.

File source improvements and change to event model #601

File source improvements and change to event model #601

Conversation

graytaylor0 commented Nov 16, 2021 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Nov 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

chenqi0805 Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

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

graytaylor0 commented Nov 16, 2021

chenqi0805 Nov 16, 2021 • 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 Nov 16, 2021 • 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 Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graytaylor0 commented Nov 16, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading

chenqi0805 Nov 16, 2021 •

edited

Loading

chenqi0805 Nov 16, 2021 •

edited

Loading

graytaylor0 Nov 16, 2021 •

edited

Loading

graytaylor0 Nov 17, 2021 •

edited

Loading