ParseJsonProcessor initial implementation #1688

finnroblin · 2022-08-23T18:56:16Z

Description

Starting point to implement the parse_json processor. This PR adds a configuration class, unit tests, and implementation of core features of the parse_json processor. With these changes the processor can deserialize JSON strings and will deserialize all nested fields. A JSON pointer feature to parse only a part of the JSON still needs to be added. The behavior of deserialized JSON Arrays still needs to be tested and some modifications to the logic might be necessary.

Issues Resolved

Step towards resolving #831

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.

Signed-off-by: Finn Roblin <[email protected]>

finnroblin · 2022-08-23T21:36:16Z

...c/main/java/com/amazon/dataprepper/plugins/processor/parsejson/ParseJsonProcessorConfig.java

+     *
+     * @return The name of the destination field.
+     */
+    public String getDestination() {


I think it would be better for the default destination (Event root) to be / instead of null. Then we avoid some null values and my opinion is that / is more declarative. What do other people think?

If we used / as the root what would the expected behavior behavior be if someone had multiple /s in the destination? One might expect it to create a nested field similar to directory syntax .

ex: Would destination: /foo/bar become foo.bar: <parsed_json> where bar is a nested field?

That might be useful, but I don't think that's what we're going for here so maybe / would be misleading. I do think your point about a more declarative default is valid though

Right now the behavior if someone had multiple / in the destination (/foo/bar) is that the parsed json will be written to the most nested field in the destination. So if the json is {"key":"value"} and the destination is (/foo/bar), event.get("/foo/bar", Map.class) == {"key":"value"} and event.get("/foo/bar/key", String.class) == "value". Right now the parse_json processor doesn't flatten nested fields (so /foo/bar is not flattened to foo.bar), it just puts any nested json into the event without changing it.

I do see how / is misleading. It's also misleading for another reason: calling event.put("/", value) will not put value into the event root because the JacksonEvent implementation trims leading slashes. So it would not write value to the event at all.

I think we can stick with a null default and be more declarative by mentioning that destination is null -> write to root in documentation.

travisbenedict · 2022-08-24T15:53:44Z

...sor/src/main/java/com/amazon/dataprepper/plugins/processor/parsejson/ParseJsonProcessor.java

+            final Event event = record.getData();
+            final String message = event.get(source, String.class);
+            try {
+                TypeReference<HashMap<String, Object>> hashMapTypeReference = new TypeReference<HashMap<String, Object>>() {};


Thanks for catching! I'll add final in the next revision or the next parse_json PR to avoid triggering re-review.

travisbenedict · 2022-08-24T16:12:23Z

...c/main/java/com/amazon/dataprepper/plugins/processor/parsejson/ParseJsonProcessorConfig.java

+     *
+     * @return The name of the destination field.
+     */
+    public String getDestination() {


If we used / as the root what would the expected behavior behavior be if someone had multiple /s in the destination? One might expect it to create a nested field similar to directory syntax .

ex: Would destination: /foo/bar become foo.bar: <parsed_json> where bar is a nested field?

That might be useful, but I don't think that's what we're going for here so maybe / would be misleading. I do think your point about a more declarative default is valid though

sbayer55

Overall the new ParseJsonProcessor looks good, thanks for adding it!

sbayer55 · 2022-08-25T15:56:11Z

data-prepper-plugins/parse-json-processor/build.gradle

+    implementation 'com.fasterxml.jackson.core:jackson-databind'
+}
+
+test {


I recommend setting a minimum test coverage to maintain our high coding standards. Here is an example for another plugin.

Thanks for the recommendation!

I set the threshold to 96% but it's slightly higher in reality because there aren't any unit tests for the prepareForShutdown, isReadyForShutdown, and shutdown methods. These methods are needed to extend AbstractProcessor but they're empty because parse_json doesn't have any special behavior for shutdown. The implemented line and branch coverage are 100%.

sbayer55 · 2022-08-25T16:07:05Z

...src/test/java/com/amazon/dataprepper/plugins/processor/parsejson/ParseJsonProcessorTest.java

+    @BeforeEach
+    void setup() {
+        ParseJsonProcessorConfig defaultConfig = new ParseJsonProcessorConfig();
+        lenient().when(processorConfig.getSource()).thenReturn(defaultConfig.getSource());


nit: lenient() can hide unexpected function calls in tests.

Signed-off-by: Finn Roblin <[email protected]>

codecov-commenter · 2022-08-25T17:06:09Z

Codecov Report

Merging #1688 (949011c) into main (a087000) will decrease coverage by 0.16%.
The diff coverage is 90.32%.

@@             Coverage Diff              @@
##               main    #1688      +/-   ##
============================================
- Coverage     93.58%   93.41%   -0.17%     
- Complexity     1304     1307       +3     
============================================
  Files           170      172       +2     
  Lines          3804     3829      +25     
  Branches        303      303              
============================================
+ Hits           3560     3577      +17     
- Misses          173      181       +8     
  Partials         71       71

Impacted Files	Coverage Δ
...mazon/dataprepper/plugins/prepper/NoOpPrepper.java	`0.00% <ø> (ø)`
...zon/dataprepper/plugins/prepper/StringPrepper.java	`88.88% <ø> (ø)`
...lugins/processor/parsejson/ParseJsonProcessor.java	`88.88% <88.88%> (ø)`
.../processor/parsejson/ParseJsonProcessorConfig.java	`100.00% <100.00%> (ø)`
.../opensearch/dataprepper/parser/PipelineParser.java	`83.33% <0.00%> (-5.24%)`	⬇️
...rwarder/discovery/AwsCloudMapPeerListProvider.java	`92.68% <0.00%> (-2.44%)`	⬇️
...ataprepper/parser/model/PipelineConfiguration.java	`97.95% <0.00%> (-2.05%)`	⬇️
...prepper/parser/PipelineConfigurationValidator.java	`71.18% <0.00%> (+1.69%)`	⬆️
.../org/opensearch/dataprepper/pipeline/Pipeline.java	`92.85% <0.00%> (+4.28%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

asifsmohammed · 2022-08-25T17:15:34Z

...c/main/java/com/amazon/dataprepper/plugins/processor/parsejson/ParseJsonProcessorConfig.java

+    @NotEmpty
+    @NotNull


nit: @notempty also checks for not null. I think @NotNull is required in this case.

asifsmohammed · 2022-08-25T17:19:33Z

data-prepper-plugins/parse-json-processor/build.gradle

+group 'org.opensearch.dataprepper'
+version '2.0.0-SNAPSHOT'


Please remove these 2 lines. it's not required

asifsmohammed · 2022-08-25T17:24:28Z

data-prepper-plugins/parse-json-processor/build.gradle

+    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
+    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'


You can remove these dependencies. I think these are used from parent project.

Signed-off-by: Finn Roblin <[email protected]>

…espace-only sources Signed-off-by: Finn Roblin <[email protected]>

sbayer55

Looks good, thanks Finn!

* Initial implementation of parse_json processor Signed-off-by: Finn Roblin <[email protected]>

finnroblin added 2 commits August 23, 2022 10:14

Initial implementation of parse_json processor

cd8713a

Signed-off-by: Finn Roblin <[email protected]>

Fixed some nits & mistakes

e4ecae2

Signed-off-by: Finn Roblin <[email protected]>

finnroblin requested a review from a team as a code owner August 23, 2022 18:56

finnroblin commented Aug 23, 2022

View reviewed changes

travisbenedict previously approved these changes Aug 24, 2022

View reviewed changes

sbayer55 suggested changes Aug 25, 2022

View reviewed changes

Addressed Travis & Steven's feedback

949011c

Signed-off-by: Finn Roblin <[email protected]>

finnroblin dismissed travisbenedict’s stale review via 949011c August 25, 2022 16:52

asifsmohammed reviewed Aug 25, 2022

View reviewed changes

finnroblin added 2 commits August 25, 2022 10:35

Addressed Asif's feedback

68e707e

Signed-off-by: Finn Roblin <[email protected]>

Changed source annotation from @notempty to @notblank to prevent whit…

263f4f1

…espace-only sources Signed-off-by: Finn Roblin <[email protected]>

travisbenedict approved these changes Aug 25, 2022

View reviewed changes

asifsmohammed approved these changes Aug 26, 2022

View reviewed changes

finnroblin requested a review from sbayer55 August 26, 2022 15:39

sbayer55 approved these changes Aug 26, 2022

View reviewed changes

asifsmohammed merged commit 6757385 into opensearch-project:main Aug 26, 2022

engechas pushed a commit to engechas/data-prepper that referenced this pull request Sep 12, 2022

ParseJsonProcessor initial implementation (opensearch-project#1688)

e8d3f83

* Initial implementation of parse_json processor Signed-off-by: Finn Roblin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ParseJsonProcessor initial implementation #1688

ParseJsonProcessor initial implementation #1688

finnroblin commented Aug 23, 2022

finnroblin Aug 23, 2022 •

edited

Loading

travisbenedict Aug 24, 2022

finnroblin Aug 24, 2022

travisbenedict Aug 24, 2022

finnroblin Aug 24, 2022

travisbenedict Aug 24, 2022

sbayer55 left a comment

sbayer55 Aug 25, 2022

finnroblin Aug 25, 2022

sbayer55 Aug 25, 2022

codecov-commenter commented Aug 25, 2022 •

edited

Loading

asifsmohammed Aug 25, 2022

asifsmohammed Aug 25, 2022

asifsmohammed Aug 25, 2022

sbayer55 left a comment

		testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
		testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'

		group 'org.opensearch.dataprepper'
		version '2.0.0-SNAPSHOT'

ParseJsonProcessor initial implementation #1688

ParseJsonProcessor initial implementation #1688

Conversation

finnroblin commented Aug 23, 2022

Description

Issues Resolved

Check List

finnroblin Aug 23, 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

sbayer55 left a 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

codecov-commenter commented Aug 25, 2022 • 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

sbayer55 left a comment

Choose a reason for hiding this comment

finnroblin Aug 23, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading