WIP: feat(cloudwatch): enable expressions and multiple metrics #4942

Amri91 · 2019-11-09T21:18:18Z

fixes #1077

Introduction

Hi,

Issue #1077 is not an easy one, but I thought I'd give it a go. This is still a WIP/RFC, I want to get feedback as soon as possible, especially if the decisions made are not suitable for the maintainers or if I made wrong assumptions about the APIs.

Thanks :)

Approach

Follow the official CloudFormation API.
Allow for a strongly-typed DSL approach to be used later (Not in this PR).
Start with a weakly-typed approach and push a little bit of responsibility to the client when constructing metrics and alarms.
Break a few APIs (unfortunately)

Basic usage

Graphs

const timeSeries = [
  new Metric({ namespace: 'CDK', metricName: 'Test' }),
  new Expression({expression: 'SUM(METRICS())', id: 'e1'})    
];

const widget = new GraphWidget({
  timeSeries: timeSeries.map(ts => ts.toJson())
});

Alarms

new Alarm(stack, 'Alarm', {
  alarmTimeSeries: [testMetric, testExpression].map((ts, index) => ts.toAlarmTimeSeries({returnData: index === 1})),
  threshold: 1000,
  evaluationPeriods: 2
});

Todo

Lint Exceptions

"no-unused-type:@aws-cdk/aws-cloudwatch.MetricOption"
Used by other modules, not sure yet if I should change them.
"props-struct-name:@aws-cdk/aws-cloudwatch.IAlarm"
I think this another false positive.
"no-unused-type:@aws-cdk/aws-cloudwatch.MetricRenderingProperties"
False-positive for the following:

// As close as I could get to https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Metric-Widget-Structure.html#CloudWatch-Metric-Widget-Metrics-Array-Format
export type MetricJson = [string, string, ...Array<string|MetricRenderingProperties>];

Breaking changes (WIP)

aws-cloudwatch You can no longer create an alarm via a Metric method. You can only create an alarm using the Alarm constructor.
aws-cloudwatch GraphWidget no longer has a left and right array of metrics, just 1 array of time series
aws-cloudwatch IMetric no longer has the method toGraphConfig, you can use use the method toJson instead.
aws-cloudwatch An optional id property is added to Metric, the metricName property is used if id is not undefined.
aws-cloudwatch The constructor of Alarm takes a union of ComplexAlarmProps and SimpleAlarmProps
aws-cloudwatch The metric property is no longer required in Alarm
aws-cloudwatch The metrics property has been renamed to timeseries in SingleValueWidgetProps

Some of the breaking changes above can be avoided, which ones are you OK with?

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

…and alarms

mergify · 2019-11-09T21:18:55Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-09T21:23:27Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-09T21:44:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-09T21:56:21Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-09T23:17:48Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-10T01:04:05Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-10T09:53:28Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-10T10:37:01Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-10T15:09:38Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

rix0rrr · 2019-11-11T13:01:26Z

Thanks for all the hard work and for writing such a thoughtful PR description!

However, I'm afraid I have to rain on your parade a little... we cannot accept the level of breaking changes you've introduced here. People will be using many of the code paths that are now broken, and we really cannot afford to break existing code, however much we would have liked to have had a different API previously.

Whatever we do, it will have to be in a way that doesn't break existing call paths. I haven't looked at the changes in much detail yet, but this will probably mean introducing new types and/or methods.

Amri91 · 2019-11-11T13:18:40Z

@rix0rrr Thanks for your reply. I know this has a few breaking changes, that is why I listed them and asked which ones (if any) are acceptable. I expected this and my plan is to change this PR to reduce the broken changes, but I did not want to assume which APIs are most important without some feedback.

I would understand if you would rather reject this PR, I had fun creating it :) But kindly review it first as well as this related PR #1396, I think this feature will require at least some breaking changes.

mergify · 2019-12-16T12:34:04Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:01:17Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:24Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:39:11Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:51Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:18:08Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

rix0rrr · 2020-01-02T12:04:58Z

Superseded by #5582

feat(cloudwatch): enable expressions and multiple metrics for graphs …

430814d

…and alarms

Amri91 requested a review from eladb as a code owner November 9, 2019 21:18

refactor: 💡 clean up quotes and commented out code

f6c4d1a

fix: 🐛 include metric options

bed0dc1

Amri added 2 commits November 9, 2019 23:19

fix: 🐛 add an exception for metric options

d4c0dca

fix: 🐛 add id to metric tests across multiple packages

31fabd3

Amri91 requested review from garnaat, nija-at and rix0rrr as code owners November 9, 2019 23:06

Amri added 2 commits November 10, 2019 01:34

fix: 🐛 add missing id to ecs and elastic load balancing

443040f

fix: 🐛 add missing id to rds

d06bf66

Amri91 requested review from skinny85 and SoManyHs as code owners November 10, 2019 00:47

refactor metric toJson andn style imports

c2d45a9

test: 💍 include feature in integration test

0fa306c

Amri added 2 commits November 10, 2019 15:41

chore: 🤖 update metricJson type

8ecd44e

refactor: 💡 enable tslint

67f75dd

eladb assigned rix0rrr Nov 11, 2019

rix0rrr closed this Jan 2, 2020

bappa601 mentioned this pull request Apr 7, 2020

Metric SEARCH Expression support for GraphWidget CDK Construct #7237

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat(cloudwatch): enable expressions and multiple metrics #4942

WIP: feat(cloudwatch): enable expressions and multiple metrics #4942

Amri91 commented Nov 9, 2019 •

edited

Loading

mergify bot commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

aws-cdk-automation commented Nov 10, 2019

aws-cdk-automation commented Nov 10, 2019

aws-cdk-automation commented Nov 10, 2019

aws-cdk-automation commented Nov 10, 2019

rix0rrr commented Nov 11, 2019

Amri91 commented Nov 11, 2019 •

edited

Loading

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

rix0rrr commented Jan 2, 2020

WIP: feat(cloudwatch): enable expressions and multiple metrics #4942

WIP: feat(cloudwatch): enable expressions and multiple metrics #4942

Conversation

Amri91 commented Nov 9, 2019 • edited Loading

fixes #1077

Introduction

Approach

Basic usage

Graphs

Alarms

Todo

Lint Exceptions

Breaking changes (WIP)

mergify bot commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 9, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 9, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 9, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 10, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 10, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 10, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 10, 2019

AWS CodeBuild CI Report

rix0rrr commented Nov 11, 2019

Amri91 commented Nov 11, 2019 • edited Loading

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

rix0rrr commented Jan 2, 2020

Amri91 commented Nov 9, 2019 •

edited

Loading

Amri91 commented Nov 11, 2019 •

edited

Loading