GitHub-Issue#2778: Added CloudWatchLogs Sink Config Files #2922

MaGonzalMayedo · 2023-06-21T18:56:56Z

Description

This change contains the Configuration files for the CloudWatchLogs Sink. It will act as a basis for reading the
user entered configuration yaml file.

Issues Resolved

This PR will work towards resolving CWL-Sink for Data-Prepper [Issue https://github.com//issues/2778]
Link: #2778 (comment)

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.

markkuhn · 2023-06-21T22:35:50Z

data-prepper-plugins/cwl-sink/.gitignore

@@ -0,0 +1,42 @@
+.gradle


Should this be at the root of the repo?

kkondaka

Regarding the package path data-prepper-plugins/cwl-sink. We have someone working on CWL as source as well. So, it probably makes sense to use the path data-prepper-plugins/cloudwatch-logs/ and then you can have data-prepper-plugins/cwl-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/ for this and the source will have data-prepper-plugins/cwl-sink/src/main/java/org/opensearch/dataprepper/plugins/source

kkondaka · 2023-06-22T01:08:57Z

data-prepper-plugins/cwl-sink/.gitignore

@@ -0,0 +1,42 @@
+.gradle
+build/
+!gradle/wrapper/gradle-wrapper.jar


This file should removed. I think it was not intended.

Those files should be removed now.

kkondaka · 2023-06-22T01:09:19Z

data-prepper-plugins/cwl-sink/src/main/java/org/opensearch/dataprepper/Main.java

@@ -0,0 +1,8 @@
+package org.opensearch.dataprepper;
+


This file should be removed too.

kkondaka · 2023-06-22T01:17:55Z

...ins/cwl-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/config/CwlSinkConfig.java

+    public static final String DEFAULT_BUFFER_TYPE = "in_memory";
+
+    //TODO: Change this to custom aws config class as its just a data container.
+    @JsonProperty("aws_config")


We use "aws:" for this. Please follow the same convention.

The name has been changed from 'aws_config' to 'aws'.

kkondaka · 2023-06-26T20:27:38Z

data-prepper-plugins/cloudwatch-logs/build.gradle

+}
+
+group = 'org.opensearch.dataprepper'
+version = '2.3.0-SNAPSHOT'


You don't need line 6 and 7.

I have removed these lines.

kkondaka · 2023-06-26T20:28:48Z

.../cloudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/AwsConfig.java

+ * where the configuration allows the sink to fetch Aws credentials
+ * and resources.
+ */
+public class AwsConfig {


Let's use this and not AwsAuthenticationOptions

I made the change to AwsConfig.

kkondaka · 2023-06-26T20:29:07Z

...udwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/CwlSinkConfig.java

+    @JsonProperty("aws")
+    @NotNull
+    @Valid
+    private AwsAuthenticationOptions awsConfig;


Let's do private AwsConfig awsConfig;?
Duplicate AwsAuthenticationOptions code if needed.
We are working on moving this to a common location. For now, it's ok if you duplicate it.

I have changed it to AwsConfig.

kkondaka · 2023-06-26T20:35:27Z

data-prepper-plugins/cloudwatch-logs/build.gradle

+dependencies {
+    api project(':data-prepper-api')
+    implementation project(':data-prepper-plugins:aws-plugin-api')
+    implementation project(path: ':data-prepper-plugins:s3-sink')


I think you are adding this to import AwsAuthenticationOptions. But we currently do not do like this. Ideally the AwsAuthenticationOptions should be moved to common directory. But for now, it is OK if you duplicate the code and put it in cloud watch logs directory

Yeah, I realized that by adding that dependency we are not isolating the separate plugins. I have removed the dependency and proceeded to use my own version of AwsConfig.

markkuhn · 2023-06-27T18:42:00Z

...watch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/ThresholdConfig.java

+
+    private int backOffTime = DEFAULT_BACKOFF_TIME;
+
+    public int getBatchSize() {


I think this might be too many config options for the customer. We also want to avoid a retry storm so maybe we should decide on a retry_count ourselves.
Up to DP team how they want to proceed

We can keep this open for configuration and provide default values. Customer might want to tweak based on the use case. We can use default values and document in Readme. I'm not aware of all the cloudwatch configuration, but we should allow CX to configure value to increase throughput and remove the ones that are not required.

dinujoh · 2023-06-27T18:58:19Z

.../cloudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/AwsConfig.java

+ * where the configuration allows the sink to fetch Aws credentials
+ * and resources.
+ */
+public class AwsConfig {


We should reuse AwsCredentialsOptions. Will this class be used to construct AwsCredentialsOptions ?

Currently, AwsConfig is re-using code from AwsCredentialsOptions as AwsCredentialsOptions is not a general utils class. This would make the CloudWatchLogs plugin rely on a dependency from another plugin if we used AwsCredentialsOptions. I changed it to AwsConfig after talking to Krishna about this, we can discuss this further as well.

dinujoh · 2023-06-27T19:05:06Z

...udwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/CwlSinkConfig.java

+public class CwlSinkConfig {
+    public static final String DEFAULT_BUFFER_TYPE = "in_memory";
+
+    //Class was utilized from the


incomplete documentation.

dinujoh · 2023-06-27T19:23:29Z

...watch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/ThresholdConfig.java

+
+    private int backOffTime = DEFAULT_BACKOFF_TIME;
+
+    public int getBatchSize() {


We can keep this open for configuration and provide default values. Customer might want to tweak based on the use case. We can use default values and document in Readme. I'm not aware of all the cloudwatch configuration, but we should allow CX to configure value to increase throughput and remove the ones that are not required.

dinujoh · 2023-06-27T19:27:23Z

.../cloudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/AwsConfig.java

+    @JsonProperty("path_to_credentials")
+    private String pathToCredentials;


Why do we need this ? Look like we dont use this parameter in other Sinks ?

I normally presented this parameter as an option to allow customers to specify credential files for SDK configuration, but after discussing this further with Mark and it being brought up here, it would be best to remove it. I will have this for the next revision. It is too much control for the customer and insecure as we require the pathing for the file.

dinujoh · 2023-06-27T19:29:01Z

...udwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/CwlSinkConfig.java

+    @JsonProperty("buffer_type")
+    private String bufferType = DEFAULT_BUFFER_TYPE;


what are other options ?

Currently there is only a default option for the buffer, and that is InMemoryBuffer, but this was made an option as in the future we could implement different buffering methods for customers who want to handle that application aspect differently. Similar to how the S3 handles InMemoryBuffering and InFileBuffering.

This sounds like Speculative Generality. Is there a plan to add additional buffers?

Currently within the scope of the Design of this plugin, there are no goals for this. But I have added the addition of other Buffering systems such as Kafka and File Buffering as Next Step additions that could benefit the plugin for future usage. The generalization of this Buffer will prove useful as our services grow and customers require variable sources for which to buffer information. It would make it easier for downstream additions to be applied and integrated.

dinujoh · 2023-06-27T19:31:20Z

...s/src/test/java/org/opensearch/dataprepper/plugins/sink/configuration/CwlSinkConfigTest.java

+    public static final String LOG_GROUP = "testGroup";
+    public static final String LOG_STREAM = "testStream";
+    public static final String BUFFER_TYPE = "in_memory";
+    public static final int BATCH_SIZE = 10;
+    public static final int MAX_RETRIES = 10;


How are these values used ?

Sorry, it seems that while working on the testing, I had left these values here although they were never used. I will get rid of these for the next revision.

asifsmohammed · 2023-06-28T15:19:52Z

...udwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/CwlSinkConfig.java

+    @Valid
+    private AwsConfig awsConfig;
+
+    @JsonProperty("threshold_config")


Rename this to threshold, having config here is redundant

The name has been changed for the next revision.

asifsmohammed · 2023-06-28T15:23:24Z

.../cloudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/AwsConfig.java

+    @Size(min = 2, max = 1224, message = "awsStsExternalId length should be between 2 and 1224 characters")
+    private String awsStsExternalId;
+
+    public int getDEFAULT_CONNECTION_ATTEMPTS() {


What is this used for? If it's not configurable by user you can use the static variable in CwlSink class directly.

I have changed the default_connections to a static variable, this should follow better standard conventions.

asifsmohammed · 2023-06-28T15:24:07Z

...watch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/config/ThresholdConfig.java

+    @Size(min = 1, max = 1048576, message = "max_batch_request_size amount should be between 1 and 1048576 bytes")
+    private int maxRequestSize = DEFAULT_SIZE_OF_REQUEST;
+
+    @JsonProperty("retry_count")


How is this retry count different from AWS SDK retry?

This is different from AWS SDK retry as the SDK retry is a built in error retransmission method that can still fail and throw exceptions that are not directly the fault of the client. This can be seen in the documentation for the exceptions thrown by a call to putLogEvents() that can return RetryableException or AwsServiceException.

This parameter is also based on the S3-Sink retry parameter "max_retries" as it attempts to retry recoverable requests.

asifsmohammed · 2023-06-28T15:27:07Z

...s/src/test/java/org/opensearch/dataprepper/plugins/sink/configuration/CwlSinkConfigTest.java

+public class CwlSinkConfigTest {
+    @Test
+    void check_null_auth_config_test() {
+        assertThat(new CwlSinkConfig().getAwsConfig(), equalTo(null));


You can use this ReflectivelySetField class to configure the options and verify if they're being set correctly.

Thank you for letting me in on this. I will re-write the test for the CwlSink to match these. I also used ObjectMapper.

asifsmohammed · 2023-06-28T15:28:27Z

data-prepper-plugins/cloudwatch-logs/build.gradle

+    api project(':data-prepper-api')
+    implementation project(':data-prepper-plugins:aws-plugin-api')
+    testImplementation platform('org.junit:junit-bom:5.9.1')
+    implementation platform('software.amazon.awssdk:bom:2.20.56')


You don't have to define bom dependency here, I think it's configured in root build.gradle. If you have to use latest version update it here.

I have removed the sdk bom dependecy for the next revision.

dinujoh · 2023-06-28T10:34:22Z

...src/test/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdConfigTest.java

+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+public class ThresholdConfigTest {


You can add test to check all default values.

void check_default_values() { ... }

I have added the default test to ThresholdConfig.

asifsmohammed

LGTM. All the files should include copyright header, you can fix that in next revision.

…pensearch-project#2910) Create Elasticsearch client, implement search and pit apis for ElasticsearchAccessor Signed-off-by: Taylor Gray <tylgry@amazon.com> Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added Config Files for CloudWatchLogs Sink. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

… syntax) Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added default params for back_off and log_send_interval alongside test cases for ThresholdConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

…ith AwsConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

…mer and params to AwsConfig Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

…tive mapping to tests CwlSink Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

…xRequestSize Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Signed-off-by: Marcos Gonzalez Mayedo <95880281+MaGonzalMayedo@users.noreply.github.com>

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

…-project#2922) * Elasticsearch client implementation with pit and no context search (opensearch-project#2910) Create Elasticsearch client, implement search and pit apis for ElasticsearchAccessor Signed-off-by: Taylor Gray <tylgry@amazon.com> Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * GitHub-Issue#2778: Refactoring config files for CloudWatchLogs Sink (#4) Added Config Files for CloudWatchLogs Sink. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added fixes from comments to code (including pathing and nomenclature syntax) Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Refactoring config (#5) Added default params for back_off and log_send_interval alongside test cases for ThresholdConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Fixed deleted AwsConfig file Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Removed the s3 dependency from build.gradle, replaced the AwsAuth.. with AwsConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added modifiable back_off_timer, added threshold test for back_off_timer and params to AwsConfig Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added fixes to gradle file, added tests to AwsConfig, and used Reflective mapping to tests CwlSink Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added default value test to ThresholdConfig and renamed getter for maxRequestSize Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Removed unnecessary imports Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added cloudwatch-logs to settings.gradle Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> * Added a quick fix to the back_off_time range Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> --------- Signed-off-by: Taylor Gray <tylgry@amazon.com> Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com> Signed-off-by: Marcos Gonzalez Mayedo <95880281+MaGonzalMayedo@users.noreply.github.com> Co-authored-by: Taylor Gray <tylgry@amazon.com> Co-authored-by: Marcos <alemayed@amazon.com> Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

MaGonzalMayedo requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, cmanning09, asifsmohammed, dlvenable and oeyh as code owners June 21, 2023 18:56

MaGonzalMayedo closed this Jun 21, 2023

MaGonzalMayedo force-pushed the main branch 2 times, most recently from 4104e08 to d4ad1b0 Compare June 21, 2023 21:42

MaGonzalMayedo reopened this Jun 21, 2023

markkuhn reviewed Jun 21, 2023

View reviewed changes

data-prepper-plugins/cwl-sink/.gitignore Outdated

@@ -0,0 +1,42 @@

.gradle

Copy link

markkuhn Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be at the root of the repo?

kkondaka requested changes Jun 22, 2023

View reviewed changes

MaGonzalMayedo requested review from kkondaka and markkuhn June 26, 2023 18:08

kkondaka reviewed Jun 26, 2023

View reviewed changes

MaGonzalMayedo requested a review from kkondaka June 26, 2023 22:16

markkuhn reviewed Jun 27, 2023

View reviewed changes

dinujoh requested changes Jun 27, 2023

View reviewed changes

MaGonzalMayedo requested review from dinujoh and markkuhn June 27, 2023 21:17

asifsmohammed reviewed Jun 28, 2023

View reviewed changes

MaGonzalMayedo force-pushed the main branch from 8c13085 to de9a58d Compare June 28, 2023 21:09

MaGonzalMayedo requested a review from asifsmohammed June 28, 2023 21:22

dinujoh reviewed Jun 28, 2023

View reviewed changes

dinujoh self-requested a review June 28, 2023 21:29

asifsmohammed previously approved these changes Jun 28, 2023

View reviewed changes

dinujoh previously approved these changes Jun 29, 2023

View reviewed changes

MaGonzalMayedo force-pushed the main branch from 326700e to de447ff Compare July 3, 2023 19:33

graytaylor0 and others added 11 commits July 3, 2023 12:56

GitHub-Issue#2778: Refactoring config files for CloudWatchLogs Sink (#4)

4387b29

Added Config Files for CloudWatchLogs Sink. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added fixes from comments to code (including pathing and nomenclature…

f1c25bb

… syntax) Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Refactoring config (#5)

9d640e5

Added default params for back_off and log_send_interval alongside test cases for ThresholdConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Fixed deleted AwsConfig file

b100ee3

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Removed the s3 dependency from build.gradle, replaced the AwsAuth.. w…

35859b0

…ith AwsConfig. Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added modifiable back_off_timer, added threshold test for back_off_ti…

7040186

…mer and params to AwsConfig Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added fixes to gradle file, added tests to AwsConfig, and used Reflec…

6b13e21

…tive mapping to tests CwlSink Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added default value test to ThresholdConfig and renamed getter for ma…

3bb125a

…xRequestSize Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Removed unnecessary imports

e5ee1e5

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

Added cloudwatch-logs to settings.gradle

e90b05a

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

MaGonzalMayedo force-pushed the main branch from de447ff to e90b05a Compare July 3, 2023 20:02

Merge branch 'main' into main

43a0a40

Signed-off-by: Marcos Gonzalez Mayedo <95880281+MaGonzalMayedo@users.noreply.github.com>

MaGonzalMayedo dismissed stale reviews from dinujoh and asifsmohammed via 43a0a40 July 3, 2023 20:12

MaGonzalMayedo requested review from dinujoh and asifsmohammed July 3, 2023 21:24

Added a quick fix to the back_off_time range

40cb280

Signed-off-by: Marcos Gonzalez Mayedo <alemayed@amazon.com>

dinujoh approved these changes Jul 5, 2023

View reviewed changes

kkondaka approved these changes Jul 5, 2023

View reviewed changes

kkondaka merged commit 4aa35a3 into opensearch-project:main Jul 5, 2023
45 of 46 checks passed


		private int backOffTime = DEFAULT_BACKOFF_TIME;

		public int getBatchSize() {

		@JsonProperty("path_to_credentials")
		private String pathToCredentials;

		@JsonProperty("buffer_type")
		private String bufferType = DEFAULT_BUFFER_TYPE;

GitHub-Issue#2778: Added CloudWatchLogs Sink Config Files #2922

GitHub-Issue#2778: Added CloudWatchLogs Sink Config Files #2922

Conversation

MaGonzalMayedo commented Jun 21, 2023 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

kkondaka 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

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

MaGonzalMayedo Jun 27, 2023 • 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

MaGonzalMayedo Jun 28, 2023 • 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

asifsmohammed left a comment

Choose a reason for hiding this comment

MaGonzalMayedo commented Jun 21, 2023 •

edited

Loading

MaGonzalMayedo Jun 27, 2023 •

edited

Loading

MaGonzalMayedo Jun 28, 2023 •

edited

Loading