GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugin c… #3128

venkataraopasyavula · 2023-08-09T16:56:32Z

Description

Implementation of Cloudwatch metrics source plugin configuration

Issues Resolved

GitHub-issue #1994

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.

…onfiguration. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

…guration

…n configuration. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

…guration

…n configuration Junit test cases and source coordinator. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

kkondaka · 2023-08-26T23:03:47Z

...rce/src/test/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsSourceTest.java

+                    .thenReturn(bufferAccumulator);
+            cloudwatchMetricsSource.start(buffer);
+        }
+        assertThat(buffer.isEmpty(),equalTo(true));


If the test is asserting empty buffer, that means nothing is being pushed to the buffer, right? Please send N number of records to the source and assert that buffer has N records.

Thanks for your review comment Krishna.
Basically this test case is used to test with empty buffer. So we have renamed the method name to avoid the confusion. Testing the buffer with the records test cases is present in CloudwatchMetricsWorkerTest.java file

…guration

…n configuration Junit test cases and source coordinator. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

dlvenable · 2023-09-18T16:16:46Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsWorker.java

+            try {
+                Thread.sleep(waitTimeMillis);
+            } catch (InterruptedException e) {
+                e.printStackTrace();


Please do not use e.printStackTrace(). You can use the following instead.

LOG.error("Thread interrupted", e);

this has been fixed.

dlvenable · 2023-09-18T16:21:44Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsSource.java

+            throw new IllegalStateException("Buffer provided is null");
+        }
+
+        BufferAccumulator<Record<Event>> bufferAccumulator = BufferAccumulator.create(buffer, cloudwatchMetricsSourceConfig.getBatchSize(), BUFFER_TIMEOUT);


Why use a BufferAccumulator for this source? This was originally added for the S3 source to avoid lots of small calls to write() on the Buffer which is synchronized.

For this source, each call to GetMetricData can perform a single writeAll on the buffer.

this has been fixed.

dlvenable · 2023-09-18T16:22:00Z

...e/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsSourceConfig.java

+    private AwsAuthenticationOptions awsAuthenticationOptions;
+
+    @JsonProperty("batch_size")
+    private Integer batchSize = 1000;


This is for the BufferAccumulator. I think we can remove this configuration option.

this has been fixed.

dlvenable · 2023-09-18T16:24:46Z

...e/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsSourceConfig.java

+    @JsonProperty("namespaces")
+    @NotNull
+    @Valid
+    private List<NamespacesListConfig> namespacesListConfig;


We can simplify this YAML.

namespaces: - name: "AWS/S3" start_time: end_time: - name: "AWS/EC2" start_time: end_time:

Make this line: private List<NamespaceConfig> namespaceConfigs;`

Then delete the NamespacesListConfig class altogether.

this has been fixed.

dlvenable · 2023-09-18T16:27:26Z

...e/src/main/java/org/opensearch/dataprepper/plugins/source/configuration/NamespaceConfig.java

+    private String endTime;
+    @JsonProperty("metricDataQueries")
+    @NotNull
+    private List<MetricDataQueriesConfig> metricDataQueriesConfig;


Similar to my other comment about YAML simplification. We can have:

metricDataQueries: - name: id: "q1" period: - name: id: "q2" period: stat: "Average"

To resolve:

Change this line to: private List<MetricsConfig> metricsConfigs;

Change the getter and all corresponding usages.

Delete MetricDataQuerieisConfig entirely.

this has been fixed.

dlvenable · 2023-09-18T16:36:06Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsSource.java

+
+    private static final Logger LOG = LoggerFactory.getLogger(CloudwatchMetricsSource.class);
+
+    private final Collection<Dimension> dimensionCollection;


I don't think this needs to be a field. It should be a local variable where you use it. Otherwise, this is not thread-safe and may not work with all metrics.

this has been fixed.

dlvenable · 2023-09-18T16:37:23Z

...rce/src/test/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsWorkerTest.java

+        this.sourceCoordinator = mock(SourceCoordinator.class);
+        this.cloudWatchClient = mock(CloudWatchClient.class);
+        this.dimensionCollection = new ArrayList<>();
+        this.dimensionCollection.add(Dimension.builder().name("StorageType").value("StandardStorage").build());


I think you are trying to force dimensionCollection to be a field to help with testing. This may be an indication that you should split some of the logic into another class. That other class could be easier to test.

we have changed some implementation of CloudWatchMetricWorker constructor so no need of separate logic.

dlvenable · 2023-09-18T16:39:42Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsWorker.java

+
+                for (DimensionsListConfig dimensionsListConfig : metricDataQueriesConfig.getMetricsConfig().getDimensionsListConfigs()) {
+
+                    dimensionCollection.add(Dimension.builder()


You could build this object on start-up and then keep it in a map for future re-use. But, you'd need to not modify it.

private Map<String, Collection<Dimension>> metricNameToDimensions;

this has been fixed.

dlvenable · 2023-09-18T16:41:32Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsWorker.java

+            }
+        });
+        try {
+            bufferAccumulator.flush();


You are flushing the buffer accumulator here. As I noted above, you don't even need to BufferAccumulator.

Create a List<Record<Event>> metricEvents.

Then call buffer.writeAll(metricEvents).

this has been fixed.

dlvenable · 2023-09-18T16:43:47Z

...-source/src/main/java/org/opensearch/dataprepper/plugins/source/CloudwatchMetricsWorker.java

+                              final BufferAccumulator<Record<Event>> bufferAccumulator,
+                              final AcknowledgementSet acknowledgementSet) {
+        metricsData.forEach(message -> {
+            final Record<Event> eventRecord = new Record<Event>(JacksonEvent.fromMessage(message.toString()));


You are creating a string from the metrics. This is not very useful for pipeline authors. You need to convert the CloudWatch metrics into a org.opensearch.dataprepper.model.metric.Metric instance. This is an Event type already, so you can wrap it in a Record and save it.

This is a very important aspect of the source and it won't be very useful with it.

Also, we'd need unit tests for this. So, you should do all that translation in a new class that is dedicated to the translation of a CloudWatch metric to a Data Prepper Metric class.

@dlvenable - Cloudwatch metrics are in format https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_GetMetricData.html i.e. sample reponse
{ "MetricDataResults": [ { "Id": "m1", "StatusCode": "Complete", "Label": "CPUUtilization, peak of 31.5 was at 1-22 13:05", "Timestamps": [ 1518868032, 1518867732, 1518867432 ], "Values": [ 15000, 14000, 16000 ] }, }
which does not fit in to 'org.opensearch.dataprepper.model.metric.Metric' instance object structure. Should Event object be enhanced or a new Object to be created for CloudWatch in Metric ?

…guration

Signed-off-by: rajeshLovesToCode <rajesh.dharamdasani3021@gmail.com>

…guration

dlvenable

I'm approving this as code that we can pull in to help us get started with an implementation. It still requires corrections to the actual model.

kkondaka

Approving to merge the code for now.

Signed-off-by: David Venable <dlv@amazon.com>

dlvenable · 2024-01-16T15:58:11Z

I resolved the merge conflicts by accepting the settings.gradle exactly as it is in main. Thus, this project is not going into the Data Prepper build. Merging it in will bring in the code for future development.

GitHub-issue#1994 : Implementation Cloudwatch metrics source plugin c…

04b0cfb

…onfiguration. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

venkataraopasyavula requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, cmanning09, asifsmohammed, dlvenable and oeyh as code owners August 9, 2023 16:56

venkataraopasyavula and others added 5 commits August 10, 2023 20:06

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

99acb0a

…guration

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugi…

fde6f6b

…n configuration. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

1c0b305

…guration

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

21b26c5

…guration

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugi…

4ec407f

…n configuration Junit test cases and source coordinator. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

kkondaka reviewed Aug 26, 2023

View reviewed changes

venkataraopasyavula and others added 2 commits September 11, 2023 18:14

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

a2dc58d

…guration

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugi…

7df695e

…n configuration Junit test cases and source coordinator. Signed-off-by: venkataraopasyavula <venkataraopasyavula@gmail.com>

dlvenable requested changes Sep 18, 2023

View reviewed changes

rajeshLovesToCode and others added 5 commits September 27, 2023 07:40

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

2925e79

…guration

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

80e0ef6

…guration

-cloudwatch metric configuration fix

b87ebbb

Signed-off-by: rajeshLovesToCode <rajesh.dharamdasani3021@gmail.com>

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

096dd38

…guration

Merge branch 'opensearch-project:main' into cloud-watch-metrics-confi…

c742b6b

…guration

dlvenable previously approved these changes Jan 16, 2024

View reviewed changes

kkondaka previously approved these changes Jan 16, 2024

View reviewed changes

Merge branch 'main' into cloud-watch-metrics-configuration

24b3669

Signed-off-by: David Venable <dlv@amazon.com>

dlvenable dismissed stale reviews from kkondaka and themself via 24b3669 January 16, 2024 15:55

dlvenable approved these changes Jan 16, 2024

View reviewed changes

kkondaka approved these changes Jan 16, 2024

View reviewed changes

dlvenable merged commit 224518f into opensearch-project:main Jan 16, 2024
42 of 47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugin c… #3128

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugin c… #3128

venkataraopasyavula commented Aug 9, 2023

kkondaka Aug 26, 2023

venkataraopasyavula Sep 11, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 27, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

rajeshLovesToCode Sep 28, 2023

dlvenable Sep 18, 2023

ashoktelukuntla Sep 26, 2023 •

edited

Loading

dlvenable left a comment

kkondaka left a comment

dlvenable commented Jan 16, 2024


		private static final Logger LOG = LoggerFactory.getLogger(CloudwatchMetricsSource.class);

		private final Collection<Dimension> dimensionCollection;


		for (DimensionsListConfig dimensionsListConfig : metricDataQueriesConfig.getMetricsConfig().getDimensionsListConfigs()) {

		dimensionCollection.add(Dimension.builder()

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugin c… #3128

GitHub-issue#1994 : Implementation Of Cloudwatch metrics source plugin c… #3128

Conversation

venkataraopasyavula commented Aug 9, 2023

Description

Issues Resolved

Check List

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

ashoktelukuntla Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

kkondaka left a comment

Choose a reason for hiding this comment

dlvenable commented Jan 16, 2024

ashoktelukuntla Sep 26, 2023 •

edited

Loading