Metric publisher implementation for CloudWatch #1375

spfink · 2019-08-06T20:22:49Z

Continuing from #1354

Description

Adds a metric publisher implementation for publishing collected metrics to CloudWatch.

Testing

Tests included.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

License

I confirm that this pull request can be released under the Apache 2 license

zoewangg

Still going through the PR

zoewangg · 2019-08-07T21:23:33Z

.../src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisher.java

+                consumerExecutorService.shutdown();
+            }
+        } catch (Throwable t) {
+            log.debug(() -> "An error occurred when closing the CloudWatch publisher", t);


Should we do warn? We might also want to do try-catch for individual resources here so that the failure to close the first resource would not affect the closing of the subsequent resources.

zoewangg · 2019-08-07T21:25:32Z

.../src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisher.java

+        private Duration publishFrequency = DEFAULT_PUBLISH_FREQUENCY;
+        private String namespace = DEFAULT_NAMESPACE;
+        private int metricQueueSize = QUEUE_SIZE;
+


Missing private ctor

zoewangg · 2019-08-07T21:28:49Z

.../src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisher.java

+
+    private final CloudWatchAsyncClient client;
+
+    private String namespace;


zoewangg · 2019-08-07T21:31:15Z

...er/src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/Consumer.java

+ * Retrieves {@link MetricDatum}s from the shared blocking queue and upload them to Amazon CloudWatch.
+ */
+@SdkInternalApi
+public class Consumer implements Callable<CompletableFuture<PutMetricDataResponse>> {


Can we rename it to something like MetricConsumer? Because it can be confused with the Java Consumer class.

zoewangg · 2019-08-07T21:33:09Z

metrics-publishers/cloudwatch-publisher/pom.xml

+        <version>2.7.12-SNAPSHOT</version>
+    </parent>
+
+    <artifactId>cloudwatch-publisher</artifactId>


Should we rename it to cloudwatch-metrics-publisher? My concern is that there might be a cloudwatch publisher service in the future, who knows.

zoewangg · 2019-08-07T21:57:09Z

...er/src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/Producer.java

+            log.debug(() -> "Due to capacity restrictions, some items from the metric registry are not added to the queue.",
+                      exception);
+        } catch (Exception e) {
+            log.debug(() -> "Some error adding metrics to queue", e);


warn? nit, An error occurred when adding metrics to the queue?

zoewangg · 2019-08-07T22:00:24Z

...er/src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/Producer.java

+
+            queue.addAll(results);
+            log.debug(() -> "Number of metrics added to queue: " + results.size());
+        } catch (IllegalStateException exception) {


Do we need to catch IllegalStateException just for extra logging?
I feel the logging at line 65 is sufficient

zoewangg · 2019-08-07T22:00:47Z

...er/src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/Producer.java

+
+        private BlockingQueue<MetricDatum> queue;
+        private MetricTransformer metricTransformer;
+


missing private ctor

zoewangg · 2019-08-07T22:01:29Z

...software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisherIntegrationTest.java

+import software.amazon.awssdk.testutils.service.AwsIntegrationTestBase;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CloudWatchPublisherIntegrationTest extends AwsIntegrationTestBase {


Hmm, this is not integration test though. Can we add some integration tests?

zoewangg · 2019-08-07T22:08:50Z

.../src/main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisher.java

+            log.debug(() -> "An error occurred when uploading metrics to CloudWatch.", throwable);
+        }
+
+        CompletableFuture<Void> finalFuture = CompletableFuture.allOf(futures.toArray(new CompletableFuture[futures.size()]));


Do we need to cancel other futures if one of future fails of non-recoverable errors like 400? because it's likely all the futures will fail of the same reason. We are doing it for transfermanager.

https://github.com/aws/aws-sdk-java-v2/blob/s3-transfermanager/utils/src/main/java/software/amazon/awssdk/utils/CompletableFutureUtils.java#L113

Why do we think that one 400 will make all 400?

It might not necessary be 400, maybe something like permission issue and all requests would fail if the credential is not correct. We don't have to do it right now, just something to think about.

zoewangg · 2019-08-09T18:28:16Z

...in/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchMetricsPublisher.java

+                producerExecutorService.shutdown();
+            }
+
+            if (client != null) {


Should we do try-catch for individual resources here so that the failure to close the first resource would not affect the closing of the subsequent resources?

zoewangg · 2019-08-09T18:30:41Z

.../main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/MetricConsumer.java

+    private final String namespace;
+
+    private MetricConsumer(Builder builder) {
+        this.client = Validate.notNull(builder.cloudWatchClient, "CloudWatch client cannot be null");


nit, we can use Validate.paramNotNull(builder.cloudWatchClient, "cloudWatchClient")

zoewangg · 2019-08-09T18:30:59Z

.../main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/MetricConsumer.java

+    private MetricConsumer(Builder builder) {
+        this.client = Validate.notNull(builder.cloudWatchClient, "CloudWatch client cannot be null");
+        this.queue = Validate.notNull(builder.queue, "Queue cannot be null");
+        this.namespace = Validate.notNull(builder.namespace, "Namespace cannot be null or empty.");


This seems only checking null and not checking empty

zoewangg · 2019-08-09T18:32:43Z

...software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchPublisherIntegrationTest.java

+import software.amazon.awssdk.testutils.service.AwsIntegrationTestBase;
+
+@RunWith(MockitoJUnitRunner.class)
+public class CloudWatchPublisherIntegrationTest extends AwsIntegrationTestBase {


Can we rename the test since it's not an integration test? Can we add an integration test?

Renamed. Will add integration test once I combine this and the other PR.

zoewangg · 2019-08-09T18:41:03Z

.../main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/MetricProducer.java

+
+    private MetricProducer(Builder builder) {
+        this.queue = Validate.notNull(builder.queue, "Queue cannot be null");
+        this.metricTransformer = builder.metricTransformer != null ? builder.metricTransformer


Can customer provide metricTransformer? It doesn't look like so from the CloudWatchMetricsPublisher apis.
Should we remove it from the builder?

Removed and added note to sync with Varun on this. Not sure why it was ever exposed.

zoewangg · 2019-08-09T18:45:56Z

...in/java/software/amazon/awssdk/metrics/publishers/cloudwatch/CloudWatchMetricsPublisher.java

+        int count = 0;
+
+        try {
+            while (queue.peek() != null && ++count <= MAX_SERVICE_CALLS_PER_PUBLISH) {


What do we think about moving the queue in the MetricConsumer class? This class doesn't need to have knowledge of the queue at all and two classes having access to the same queue might be problematic in the future.

As in move this logic to the MetricConsumer class? Not sure I follow otherwise.

Yeah, I meant moving the creation of the queue and this logic into MetricConsumer class.

The MetricProducer also uses the queue.

Oh I missed that part. Do we need to check queue.peek here? can we move this logic to consumer?

zoewangg · 2019-08-09T18:47:31Z

.../main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/MetricConsumer.java

+        return client.putMetricData(request);
+    }
+
+    private List<MetricDatum> metricDatums() {


Do we need this list? Can we poll from the queue directly?

We have to have a separate list to guarantee the size of the data being published to cloudwatch.

zoewangg · 2019-08-09T18:49:25Z

.../main/java/software/amazon/awssdk/metrics/publishers/cloudwatch/internal/MetricProducer.java

+     */
+    public void addMetrics(MetricRegistry metricRegistry) {
+        try {
+            List<MetricDatum> results = new ArrayList<>();


Same question, do we need this list? can we offer the item to the queue directly?

In this case, it helps maintain a bit of grouping. Rather than adding to the transforming and adding to the queue X times, where other threads could also be touching the queue, this will add the entire list all together at once.

dagnir · 2020-01-09T16:56:04Z

What's the status of this PR? @spfink

…nks/metrics-cw

- Fix compile issues related to renamed/changed class - Fix failing tests - Fix undeclared dependencies

sonarqubecloud · 2020-01-10T22:14:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
7 Code Smells

No Coverage information
No Duplication information

spfink mentioned this pull request Aug 6, 2019

Implement CloudWatch MetricPublisher #1354

Closed

zoewangg reviewed Aug 7, 2019

View reviewed changes

spfink force-pushed the finks/metrics-cw branch from ac755ff to 05bf05e Compare August 8, 2019 20:36

zoewangg reviewed Aug 9, 2019

View reviewed changes

spfink force-pushed the finks/metrics-cw branch from 05bf05e to e3a5ef1 Compare August 12, 2019 17:01

spfink closed this Aug 12, 2019

spfink force-pushed the finks/metrics-cw branch from e3a5ef1 to f8e7fb6 Compare August 12, 2019 18:58

Metric publisher implementation for CloudWatch

2b46ab7

spfink reopened this Aug 12, 2019

zoewangg approved these changes Aug 12, 2019

View reviewed changes

Merge branch 'master' into finks/metrics-cw

4055497

dagnir changed the base branch from master to sdk-metrics-development January 9, 2020 22:24

dagnir added 3 commits January 9, 2020 14:24

Merge remote-tracking branch 'origin/sdk-metrics-development' into fi…

9ddd4da

…nks/metrics-cw

Checkstyle and version fixes

9d673b7

Add build failure fixes

faa36a9

- Fix compile issues related to renamed/changed class - Fix failing tests - Fix undeclared dependencies

dagnir merged commit 8ff69a0 into sdk-metrics-development Jan 10, 2020

dagnir deleted the finks/metrics-cw branch January 10, 2020 22:18


		private final CloudWatchAsyncClient client;

		private String namespace;


		private BlockingQueue<MetricDatum> queue;
		private MetricTransformer metricTransformer;

Metric publisher implementation for CloudWatch #1375

Metric publisher implementation for CloudWatch #1375

Uh oh!

Conversation

spfink commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Types of changes

Checklist

License

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoewangg Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagnir commented Jan 9, 2020

spfink commented Aug 6, 2019 •

edited

Loading

zoewangg Aug 9, 2019 •

edited

Loading