feat(cloudwatch): add support for cross-account alarms #16007

kaizencc · 2021-08-11T22:06:15Z

Allows accountId to be specified in a CloudWatch Alarm. This opens up the ability to create cross-account alarms, which was just announced.

closes #15959.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…into xa-alarms

gitpod-io · 2021-08-11T22:06:18Z

rix0rrr

LGTM, leaving to Madeline to decide final vote

rix0rrr · 2021-08-13T12:01:09Z

packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts

@@ -257,7 +257,8 @@ export class Alarm extends AlarmBase {
    return dispatchMetric(metric, {
      withStat(stat, conf) {
        self.validateMetricStat(stat, metric);
-        if (conf.renderingProperties?.label == undefined) {
+        if (conf.renderingProperties?.label == undefined &&


💅 for readability, it might make sense to assign this expression to a variable to indicate what and why this is happening.

const canRenderAsLegacyMetric = /* ...expression... */; // Do this to disturb existing templates as little as possible if (canRenderAsLegacyMetric) { return /* ... */; }

faizalsomani · 2021-08-16T17:24:37Z

Hi, can we merge and close the ticket?

packages/@aws-cdk/aws-cloudwatch/README.md

madeline-k · 2021-08-23T15:16:42Z

packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts

@@ -257,7 +257,10 @@ export class Alarm extends AlarmBase {
    return dispatchMetric(metric, {
      withStat(stat, conf) {
        self.validateMetricStat(stat, metric);
-        if (conf.renderingProperties?.label == undefined) {
+        const canRenderAsLegacyMetric = conf.renderingProperties?.label == undefined &&


Can you explain what you mean by the term "legacy" here? This doesn't quite make sense to me, since the accountId is only set if this is true, which would imply that a cross-account alarm only works if the metric is in a "legacy" format of some kind. But that doesn't make sense, since cross-account alarms are a new feature.

The "legacy" term refers to the fact that cloudwatch has two ways to provision metrics on alarms, and only one way is being updated with new features like cross-account alarms. We want to avoid causing unnecessary diffs, so we shepherd metrics that do not require these "new" features to the "legacy" properties - the new way to provision metrics is via the CfnAlarm.MetricDataQueryProperty.

Here accountId is only set if canRenderAsLegacyMetric is false, not true. So it is correctly named here. (what seems to be the confusion is that github is hiding the unchanged lines and makes it seem like accountId is inside the if (canRenderAsLegacyMetric) scope when it is actually not.)

aha, thank you for the explanation! The github diff was confusing me here as well :)

madeline-k · 2021-08-23T15:17:02Z

packages/@aws-cdk/aws-cloudwatch/test/cross-environment.test.ts

@@ -112,6 +113,36 @@ describe('cross environment', () => {


    });
+
+    test('metric attached to stack3 will render in stack1 ', () => {


Can you add a test that uses the method of setting the account property on the metric instead of using attachTo? There should be a unit test for each way that it is possible to use the feature.

I added an additional test that uses the account property, but I do want to push back on this a bit (or ask for clarification). Under the hood, metric.AttachTo() simply updates the metric object with the account property, so I see it as a convenience method and not an additional way to use this feature. To me, it seems like the second test duplicates the first. Does this still warrant an additional unit test?

I suppose it is somewhat of a grey area. But I still think the extra test is helpful. Your reasoning for the additional unit test being unnecessary is based on an implementation detail. What if that changes? In this particular scenario, it doesn't seem likely. But in general, unit tests should test the API in all ways that a customer could use it, and make sure that the correct behaviour/result happens in all of them. So that if someone accidentally breaks something by changing an implementation detail, then the unit tests will catch it.

…into xa-alarms

madeline-k

Looks great! 👍

madeline-k · 2021-08-24T23:10:41Z

packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts

@@ -257,7 +257,10 @@ export class Alarm extends AlarmBase {
    return dispatchMetric(metric, {
      withStat(stat, conf) {
        self.validateMetricStat(stat, metric);
-        if (conf.renderingProperties?.label == undefined) {
+        const canRenderAsLegacyMetric = conf.renderingProperties?.label == undefined &&


aha, thank you for the explanation! The github diff was confusing me here as well :)

madeline-k · 2021-08-24T23:33:55Z

packages/@aws-cdk/aws-cloudwatch/test/cross-environment.test.ts

@@ -112,6 +113,36 @@ describe('cross environment', () => {


    });
+
+    test('metric attached to stack3 will render in stack1 ', () => {


I suppose it is somewhat of a grey area. But I still think the extra test is helpful. Your reasoning for the additional unit test being unnecessary is based on an implementation detail. What if that changes? In this particular scenario, it doesn't seem likely. But in general, unit tests should test the API in all ways that a customer could use it, and make sure that the correct behaviour/result happens in all of them. So that if someone accidentally breaks something by changing an implementation detail, then the unit tests will catch it.

mergify · 2021-08-24T23:35:17Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-08-25T00:16:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: e82b5b1
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-08-25T00:16:55Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

wangzlei · 2021-08-25T19:56:37Z

Looking forward to this function, may I know when will this change get delivered in monocdk?

Allows `accountId` to be specified in a CloudWatch Alarm. This opens up the ability to create cross-account alarms, which was just [announced](https://aws.amazon.com/about-aws/whats-new/2021/08/announcing-amazon-cloudwatch-cross-account-alarms/). closes aws#15959. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

kaizencc and others added 6 commits August 11, 2021 17:25

pipe account to metricdataquery if necessary

340f1df

Merge branch 'aws:master' into xa-alarms

2265b01

fix accidental imports

0821355

satisfy linter

d128fcc

Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …

317b7c1

…into xa-alarms

update readme for cross-account alarms

749fd0a

kaizencc requested review from rix0rrr and madeline-k August 11, 2021 22:06

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 11, 2021

rix0rrr reviewed Aug 13, 2021

View reviewed changes

kaizencc and others added 2 commits August 13, 2021 15:12

add renderAsLegacyMetric

ed1fd3a

Merge branch 'master' into xa-alarms

f178c89

peterwoodworth assigned madeline-k Aug 19, 2021

peterwoodworth added @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch p1 effort/small Small work item – less than a day of effort labels Aug 19, 2021

madeline-k suggested changes Aug 23, 2021

View reviewed changes

kaizencc added 3 commits August 23, 2021 13:40

add new test and more readme docs

f1451ff

Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …

d34db67

…into xa-alarms

remove 1 line

a7666f1

kaizencc requested a review from madeline-k August 23, 2021 17:45

madeline-k approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into xa-alarms

e82b5b1

mergify bot merged commit e547ba0 into aws:master Aug 25, 2021

eladb mentioned this pull request Oct 8, 2021

‼️ (aws-cloudwatch): cross account alarms cause failures in unsupported regions #16874

Closed

kaizencc deleted the xa-alarms branch November 17, 2021 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): add support for cross-account alarms #16007

feat(cloudwatch): add support for cross-account alarms #16007

kaizencc commented Aug 11, 2021

gitpod-io bot commented Aug 11, 2021

rix0rrr left a comment

rix0rrr Aug 13, 2021

faizalsomani commented Aug 16, 2021

madeline-k Aug 23, 2021

kaizencc Aug 23, 2021

madeline-k Aug 24, 2021

madeline-k Aug 23, 2021

kaizencc Aug 23, 2021

madeline-k Aug 24, 2021

madeline-k left a comment

madeline-k Aug 24, 2021

madeline-k Aug 24, 2021

mergify bot commented Aug 24, 2021

aws-cdk-automation commented Aug 25, 2021

mergify bot commented Aug 25, 2021

wangzlei commented Aug 25, 2021

		@@ -112,6 +113,36 @@ describe('cross environment', () => {


		});

		test('metric attached to stack3 will render in stack1 ', () => {

feat(cloudwatch): add support for cross-account alarms #16007

feat(cloudwatch): add support for cross-account alarms #16007

Conversation

kaizencc commented Aug 11, 2021

gitpod-io bot commented Aug 11, 2021

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faizalsomani commented Aug 16, 2021

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

madeline-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 24, 2021

aws-cdk-automation commented Aug 25, 2021

AWS CodeBuild CI Report

mergify bot commented Aug 25, 2021

wangzlei commented Aug 25, 2021