Create decrompress processor to decompress gzipped keys #4118

graytaylor0 · 2024-02-12T21:41:16Z

Description

Adds a decompress processor to decompress specific fields in Events.

Base configuration is

- decompress:
     keys: [ "key_one", "key_two" ]
     type: gzip

This processor is extendible to more types, but it currently only supports gzip.

This processor assumes that all fields to be decompressed are base64 encoded.

Issues Resolved

Resolves #4016

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.

dlvenable

Nice work! I have a few requests.

dlvenable · 2024-02-13T02:43:34Z

...c/main/java/org/opensearch/dataprepper/plugins/processor/decompress/DecompressProcessor.java

+public class DecompressProcessor extends AbstractProcessor<Record<Event>, Record<Event>> {
+
+    private static final Logger LOG = LoggerFactory.getLogger(DecompressProcessor.class);
+    static final String DECOMPRESSION_PROCESSING_ERRORS = "decompressionProcessingErrors";


Why did you add decompression in this name? It will already be scoped to the processor, so this will yield:

my-pipeline.decompress.decompressionProcessingErrors

Why not just make it processingErrors?

dlvenable · 2024-02-13T02:44:02Z

...c/main/java/org/opensearch/dataprepper/plugins/processor/decompress/DecompressProcessor.java

+    @DataPrepperPluginConstructor
+    public DecompressProcessor(final PluginMetrics pluginMetrics,
+                               final DecompressProcessorConfig decompressProcessorConfig,
+                               final ExpressionEvaluator expressionEvaluator) {


Please validate the expression in the constructor.

Isn't it better to do the validation in the Config.java? I thought that's the convention we follow. I am ok with this too. Just checking.

dlvenable · 2024-02-13T02:46:51Z

...rc/main/java/org/opensearch/dataprepper/plugins/processor/decompress/IDecompressionType.java

+
+import org.opensearch.dataprepper.model.codec.DecompressionEngine;
+
+public interface IDecompressionType {


The IInterfaceName convention is a .NET convention and not used often in Java. Additionally, we can improve our interfaces by describing what they are capable of rather than name them based on a type. In this case, this could be HasDecompressionEngine, or DecompressionEngineFactory. Both of those names align with existing conventions in this project and in Java.

dlvenable · 2024-02-13T02:47:15Z

...ain/java/org/opensearch/dataprepper/plugins/processor/decompress/encoding/IEncodingType.java

+
+package org.opensearch.dataprepper.plugins.processor.decompress.encoding;
+
+public interface IEncodingType {


Similar comment about the I in the name.

dlvenable · 2024-02-13T02:47:53Z

.../java/org/opensearch/dataprepper/plugins/processor/decompress/ITDecompressProcessorTest.java

+import static org.opensearch.dataprepper.plugins.processor.decompress.DecompressProcessorTest.buildRecordWithEvent;
+
+@ExtendWith(MockitoExtension.class)
+public class ITDecompressProcessorTest {


Let's use the name DecompressProcessorIT. This works with Gradle and is how other integration tests in the project are named.

dlvenable · 2024-02-13T02:53:57Z

...c/main/java/org/opensearch/dataprepper/plugins/processor/decompress/DecompressProcessor.java

+
+    }
+
+    private String getDecompressedString(final BufferedReader bufferedReader) throws IOException {


You can probably use commons-io to replace this code. The class org.apache.commons.io.output.IOUtils has a method String toString(InputStream input, Charset charset). It should work for this.

dlvenable · 2024-02-13T02:55:03Z

data-prepper-plugins/decompress-processor/build.gradle

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+plugins {


You don't need this block. The base Gradle project adds it to all sub-projects. You can remove it.

dlvenable · 2024-02-13T03:01:29Z

...src/main/java/org/opensearch/dataprepper/plugins/processor/decompress/DecompressionType.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public enum DecompressionType implements IDecompressionType {


Please add some unit tests for the string to enum parsing. See ForwardingAuthenticationTest which does something similar for another enum. Also see that some tests use @EnumSource to help with future additions.

data-prepper/data-prepper-core/src/test/java/org/opensearch/dataprepper/peerforwarder/ForwardingAuthenticationTest.java

Line 24 in 7e2331f

class ForwardingAuthenticationTest {

dlvenable · 2024-02-13T03:01:47Z

...main/java/org/opensearch/dataprepper/plugins/processor/decompress/encoding/EncodingType.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public enum EncodingType implements IEncodingType {


Similar comment about adding tests for the name parsing.

dlvenable · 2024-02-13T03:02:51Z

...c/main/java/org/opensearch/dataprepper/plugins/processor/decompress/DecompressProcessor.java

+import java.io.InputStreamReader;
+import java.util.Collection;
+
+@DataPrepperPlugin(name = "decompress", pluginType = Processor.class, pluginConfigurationType = DecompressProcessorConfig.class)


Please open a documentation issue so that we track the need to document this.

Create this issue (opensearch-project/documentation-website#6407)

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

dlvenable

This looks great. Thanks!

graytaylor0 requested review from chenqi0805, engechas, dinujoh, kkondaka, asifsmohammed, dlvenable and oeyh as code owners February 12, 2024 21:41

dlvenable requested changes Feb 13, 2024

View reviewed changes

graytaylor0 added 2 commits February 14, 2024 11:07

Create decrompress processor to decompress gzipped keys

5f518fc

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

Address PR comments

3e06739

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

graytaylor0 force-pushed the DecompressProcessor branch from 80058c4 to 3e06739 Compare February 14, 2024 17:08

graytaylor0 requested a review from dlvenable February 14, 2024 17:30

dlvenable approved these changes Feb 14, 2024

View reviewed changes

kkondaka approved these changes Feb 14, 2024

View reviewed changes

graytaylor0 merged commit 3e3f302 into opensearch-project:main Feb 14, 2024
72 of 74 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create decrompress processor to decompress gzipped keys #4118

Create decrompress processor to decompress gzipped keys #4118

graytaylor0 commented Feb 12, 2024 •

edited

Loading

dlvenable left a comment

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

kkondaka Feb 14, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

dlvenable Feb 13, 2024

graytaylor0 Feb 14, 2024

dlvenable left a comment


		import org.opensearch.dataprepper.model.codec.DecompressionEngine;

		public interface IDecompressionType {


		package org.opensearch.dataprepper.plugins.processor.decompress.encoding;

		public interface IEncodingType {


		}

		private String getDecompressedString(final BufferedReader bufferedReader) throws IOException {

Create decrompress processor to decompress gzipped keys #4118

Create decrompress processor to decompress gzipped keys #4118

Conversation

graytaylor0 commented Feb 12, 2024 • edited Loading

Description

Issues Resolved

Check List

dlvenable 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

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

dlvenable left a comment

Choose a reason for hiding this comment

graytaylor0 commented Feb 12, 2024 •

edited

Loading