New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GitHub-Issue#2778: Added CloudWatchLogsSink #3084

Merged

dlvenable merged 24 commits into opensearch-project:main from MaGonzalMayedo:cloud_watch_logs_sink

Aug 1, 2023

Contributor

MaGonzalMayedo commented Jul 31, 2023 •

edited

Loading

Description

This change implements the main Sink entry point for the CloudWatchLogs Sink. It extends the plugin by adding a single sink class alongside a test.

Issues Resolved

This PR will work towards resolving CWL-Sink for Data-Prepper [Issue https://github.com/https://github.com/https://github.com/https://github.com/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.

graytaylor0 and others added 19 commits

July 3, 2023 12:56


          Elasticsearch client implementation with pit and no context search (o…

f4dc9da

…pensearch-project#2910)

Create Elasticsearch client, implement search and pit apis for ElasticsearchAccessor

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


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

4387b29

Added Config Files for CloudWatchLogs Sink.

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


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

f1c25bb

… syntax)

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          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 <[email protected]>


          Fixed deleted AwsConfig file

b100ee3

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


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

35859b0

…ith AwsConfig.

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Added modifiable back_off_timer, added threshold test for back_off_ti…

…mer and params to AwsConfig

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


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

6b13e21

…tive mapping to tests CwlSink

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


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

3bb125a

…xRequestSize

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Removed unnecessary imports

e5ee1e5

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Added cloudwatch-logs to settings.gradle

e90b05a

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Merge branch 'main' into main

43a0a40

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Added a quick fix to the back_off_time range

40cb280

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Merge branch 'opensearch-project:main' into main

1aac686


          Merge branch 'opensearch-project:main' into main

903ea26


          Merge branch 'opensearch-project:main' into main

ffe5cbe


          Merge branch 'opensearch-project:main' into main

394bed7


          Merge branch 'opensearch-project:main' into main

4ddfb04


          Merge branch 'opensearch-project:main' into main

7b56a86

MaGonzalMayedo requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, cmanning09, asifsmohammed, dlvenable and oeyh as code owners

July 31, 2023 21:56


          Added fix to commit count

93f6fe5

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>

markkuhn reviewed

View reviewed changes

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+              import java.util.concurrent.Executor;
+              import java.util.concurrent.Executors;
+              @DataPrepperPlugin(name = "cloudwatchlogs-sink", pluginType = Sink.class, pluginConfigurationType = CloudWatchLogsSinkConfig.class)

markkuhn Jul 31, 2023

Looks like the convention for sink naming is without -sink. For example http and s3. Are you sure about this naming?

Member

dlvenable Jul 31, 2023

Yes, please drop the -sink. You can name it cloudwatchlogs or cloudwatch_logs if that matches the naming convention better. Data Prepper does use underscores in between words.

See for example:

data-prepper/data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/OTelTraceRawProcessor.java

Line 39 in 93d06db

    
           @DataPrepperPlugin(name = "otel_traces", deprecatedName = "otel_trace_raw", pluginType = Processor.class, pluginConfigurationType = OtelTraceRawProcessorConfig.class)

Contributor Author

MaGonzalMayedo Jul 31, 2023 •

edited

Loading

Thank you, I will have it change to "cloudwatch_logs".

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+                  @Override
+                  public void doInitialize() {
+                      isInitialized = true;

markkuhn Jul 31, 2023

What is the initialize method for? Should we be creating failable object here or in the class constructor?

Contributor Author

MaGonzalMayedo Jul 31, 2023

I believe it has to with data-prepper's way of initializing sinks. In the Pipeline class, data-prepper checks to see if every sink is active through an initializer thread. Which just waits until each sink is "initialized" or times out trying.

Contributor Author

MaGonzalMayedo Jul 31, 2023

@dlvenable Is there another reason fro this?

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+                  @Override
+                  public void doOutput(Collection<Record<Event>> records) {
+                      if (records.isEmpty()) {
+                          return;

markkuhn Jul 31, 2023

Can we have a test for this case?

Contributor Author

MaGonzalMayedo Jul 31, 2023

I will add the tests for this method for the next commit.

MaGonzalMayedo force-pushed the cloud_watch_logs_sink branch from a9dd0cb to 93f6fe5 Compare

July 31, 2023 22:13

dlvenable requested changes

View reviewed changes

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+              import java.util.concurrent.Executor;
+              import java.util.concurrent.Executors;
+              @DataPrepperPlugin(name = "cloudwatchlogs-sink", pluginType = Sink.class, pluginConfigurationType = CloudWatchLogsSinkConfig.class)

Member

dlvenable Jul 31, 2023

Yes, please drop the -sink. You can name it cloudwatchlogs or cloudwatch_logs if that matches the naming convention better. Data Prepper does use underscores in between words.

See for example:

data-prepper/data-prepper-plugins/otel-trace-raw-processor/src/main/java/org/opensearch/dataprepper/plugins/processor/oteltrace/OTelTraceRawProcessor.java

Line 39 in 93d06db

    
           @DataPrepperPlugin(name = "otel_traces", deprecatedName = "otel_trace_raw", pluginType = Processor.class, pluginConfigurationType = OtelTraceRawProcessorConfig.class)

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+                              thresholdConfig.getMaxRequestSize(),thresholdConfig.getLogSendInterval());
+                      CloudWatchLogsClient cloudWatchLogsClient = CloudWatchLogsClientFactory.createCwlClient(awsConfig, awsCredentialsSupplier);
+                      BufferFactory bufferFactory = new InMemoryBufferFactory();

Member

dlvenable Jul 31, 2023

Is there only one buffer available right now? Can you pull this from the config instead?

Contributor Author

MaGonzalMayedo Jul 31, 2023

There is only one sink right now. An "InMemoryBuffer". But I will make the change to extract it via string comparison so it can be expanded easily.

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+                          return;
+                      }
+                      cloudWatchLogsService.processLogEvents(records);

Member

dlvenable Jul 31, 2023

We should have unit test for this as well. Just to verify that the records are passed into the service.

...loudwatch-logs/src/main/java/org/opensearch/dataprepper/plugins/sink/CloudWatchLogsSink.java Outdated

+               * SPDX-License-Identifier: Apache-2.0
+               */
+              package org.opensearch.dataprepper.plugins.sink;

Member

dlvenable Jul 31, 2023

Please put this project into a unique package. This way no class in here clobbers any other classes.

package org.opensearch.dataprepper.plugins.sink.cloudwatch_logs;

Contributor Author

MaGonzalMayedo Jul 31, 2023

Got it, I will try to have them moved for the revision.


          Added conditional buffer config and tests for event processing from t…

645463e

…he sink. Also moved plugin within "cloudwatch_logs" package.

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>

MaGonzalMayedo force-pushed the cloud_watch_logs_sink branch from 897bc0d to 645463e Compare

August 1, 2023 00:14

MaGonzalMayedo requested review from dlvenable and markkuhn

August 1, 2023 00:14

markkuhn approved these changes

View reviewed changes

dlvenable previously approved these changes

View reviewed changes

Member

dlvenable left a comment

Nice, thank you @MaGonzalMayedo !

Member

dlvenable commented Aug 1, 2023

@MaGonzalMayedo , There are failing tests.

CloudWatchLogsSinkTest > WHEN_given_sample_empty_records_THEN_records_are_processed() FAILED
    software.amazon.awssdk.core.exception.SdkClientException at CloudWatchLogsSinkTest.java:65

CloudWatchLogsSinkTest > WHEN_sink_is_initialized_THEN_sink_is_ready_returns_true() FAILED
    software.amazon.awssdk.core.exception.SdkClientException at CloudWatchLogsSinkTest.java:65

CloudWatchLogsSinkTest > WHEN_given_sample_empty_records_THEN_records_are_not_processed() FAILED
    software.amazon.awssdk.core.exception.SdkClientException at CloudWatchLogsSinkTest.java:65


          Added mockedStatic methods to prevent sdk exceptions during testing

4afdba7

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>

MaGonzalMayedo dismissed dlvenable’s stale review via

4afdba7

August 1, 2023 16:03

MaGonzalMayedo requested a review from dlvenable

August 1, 2023 16:03

Marcos Gonzalez Mayedo added 2 commits

August 1, 2023 09:43


          Attempting to fix CloudWatchLogsSink sdkexception during testing

7235d0d

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>


          Attempting to fix CloudWatchLogsSink sdkexception during testing

ef57a2b

Signed-off-by: Marcos Gonzalez Mayedo <[email protected]>

dlvenable approved these changes

View reviewed changes

dlvenable merged commit 9cef0d5 into opensearch-project:main

24 checks passed

MaGonzalMayedo deleted the cloud_watch_logs_sink branch

August 1, 2023 22:08

MaGonzalMayedo restored the cloud_watch_logs_sink branch

August 1, 2023 22:34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dlvenable dlvenable approved these changes

markkuhn markkuhn approved these changes

chenqi0805 Awaiting requested review from chenqi0805 chenqi0805 is a code owner

engechas Awaiting requested review from engechas engechas is a code owner

graytaylor0 Awaiting requested review from graytaylor0 graytaylor0 is a code owner

dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

kkondaka Awaiting requested review from kkondaka kkondaka is a code owner

cmanning09 Awaiting requested review from cmanning09

asifsmohammed Awaiting requested review from asifsmohammed

oeyh Awaiting requested review from oeyh oeyh is a code owner

Labels

None yet