feat(cloudwatch): allow overriding of metric graph rendering #4571

workeitel · 2019-10-17T21:24:21Z

This allows to create custom graphs easier by creating sub-class of
metric and overriding the toGraphConfig method. Before attributes have
been only selectively copied to the widget configuration.

class HiddenMetric extends Metric {
  public toGraphConfig(): any {
    const ret = super.toGraphConfig();
    ret.renderingProperties.visible = false;
  }
}

Before visible would not have been taken over since it was not part of
options.

Docs: https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Rendering-Object-Format

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

mergify · 2019-10-17T21:24:58Z

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-10-17T21:51:14Z

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

Please add a test

Pull request has been modified.

aws-cdk-automation · 2019-10-21T21:05:18Z

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-10-23T09:08:00Z

Aha yes. The change you are making breaks backwards compatibility:

err  - METHOD @aws-cdk/aws-cloudwatch.IMetric.toGraphConfig: returns @aws-cdk/aws-cloudwatch.MetricGraphConfig: formerly required property 'period' is missing in @aws-cdk/aws-cloudwatch.MetricGraphConfig [change-return-type:@aws-cdk/aws-cloudwatch.IMetric.toGraphConfig]

The reason this is breaking is that someone somewhere in the world could be writing this piece of code:

console.log(metric.toGraphConfig().period);

And that code would break with the current state of this change.

Please keep on generating the old properties, and mark them as @deprecated.

aws-cdk-automation · 2019-10-23T20:51:16Z

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-10-29T00:11:43Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

workeitel · 2019-10-29T07:29:12Z

Rebased on top of master. The build still fails because the old period field is now optional and ignored in favor of the new period field in the sub-structure:

API elements with incompatible changes:
err  - METHOD @aws-cdk/aws-cloudwatch.Metric.toGraphConfig: returns @aws-cdk/aws-cloudwatch.MetricGraphConfig: formerly required property 'period' is optional in @aws-cdk/aws-cloudwatch.MetricGraphConfig [change-return-type:@aws-cdk/aws-cloudwatch.Metric.toGraphConfig]
err  - METHOD @aws-cdk/aws-cloudwatch.IMetric.toGraphConfig: returns @aws-cdk/aws-cloudwatch.MetricGraphConfig: formerly required property 'period' is optional in @aws-cdk/aws-cloudwatch.MetricGraphConfig [change-return-type:@aws-cdk/aws-cloudwatch.IMetric.toGraphConfig]

rix0rrr · 2019-10-29T10:22:13Z

Rebased on top of master. The build still fails because the old period field is now optional and ignored in favor of the new period field in the sub-structure:

Yeah, you can't really do that. Again, someone could be writing (in Java for example):

if (metric.toGraphConfig().getPeriod().equals('30')) { ... }

And that would now throw a NullReferenceException (in case the property was actually not set).

Make it non-optional, mark it as @deprecated, and fill it with a value (that will be ignored by the rest of the framework, but may still be used by any of our 3rd party users).

Such are the burdens of API compatibility.

This allows to create custom graphs easier by creating sub-class of metric and overriding the `toGraphConfig` method. Before attributes have been only selectively copied to the widget configuration. ``` class HiddenMetric extends Metric { public toGraphConfig(): any { const ret = super.toGraphConfig(); ret.renderingProperties.visible = false; } } ``` Before `visible` would not have been taken over since it was not part of options. Docs: https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Rendering-Object-Format

aws-cdk-automation · 2019-10-29T13:57:04Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

…rds compatible

aws-cdk-automation · 2019-10-29T14:55:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-10-29T16:00:55Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-10-29T16:42:13Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-29T17:03:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

workeitel requested a review from eladb as a code owner October 17, 2019 21:24

SomayaB assigned rix0rrr Oct 18, 2019

rix0rrr changed the title ~~Create sub-structure for Metric rendering options~~ feat(cloudwatch): make it easier to override metric rendering Oct 21, 2019

rix0rrr changed the title ~~feat(cloudwatch): make it easier to override metric rendering~~ feat(cloudwatch): allow overriding of metric graph rendering Oct 21, 2019

rix0rrr previously requested changes Oct 21, 2019

View reviewed changes

workeitel force-pushed the cloudwatch-metric-rendering-option branch from 4721a17 to f8368bf Compare October 21, 2019 20:39

workeitel force-pushed the cloudwatch-metric-rendering-option branch from f8368bf to 02d1678 Compare October 23, 2019 20:25

workeitel force-pushed the cloudwatch-metric-rendering-option branch from 02d1678 to 8f26c2c Compare October 28, 2019 23:45

workeitel force-pushed the cloudwatch-metric-rendering-option branch from 8f26c2c to 672eb65 Compare October 29, 2019 13:30

feat(cloudwatch): allow overriding of metric graph rendering - backwa…

f44d266

…rds compatible

workeitel force-pushed the cloudwatch-metric-rendering-option branch from 672eb65 to f44d266 Compare October 29, 2019 14:06

rix0rrr approved these changes Oct 29, 2019

View reviewed changes

mergify bot added 2 commits October 29, 2019 16:01

Merge branch 'master' into cloudwatch-metric-rendering-option

ab09a32

Merge branch 'master' into cloudwatch-metric-rendering-option

49773a5

mergify bot merged commit 3643130 into aws:master Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudwatch): allow overriding of metric graph rendering #4571

feat(cloudwatch): allow overriding of metric graph rendering #4571

workeitel commented Oct 17, 2019

mergify bot commented Oct 17, 2019

aws-cdk-automation commented Oct 17, 2019

rix0rrr left a comment

aws-cdk-automation commented Oct 21, 2019

rix0rrr commented Oct 23, 2019

aws-cdk-automation commented Oct 23, 2019

aws-cdk-automation commented Oct 29, 2019

workeitel commented Oct 29, 2019

rix0rrr commented Oct 29, 2019 •

edited

Loading

aws-cdk-automation commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

mergify bot commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

feat(cloudwatch): allow overriding of metric graph rendering #4571

feat(cloudwatch): allow overriding of metric graph rendering #4571

Conversation

workeitel commented Oct 17, 2019

mergify bot commented Oct 17, 2019

aws-cdk-automation commented Oct 17, 2019

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

rix0rrr commented Oct 23, 2019

aws-cdk-automation commented Oct 23, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

workeitel commented Oct 29, 2019

rix0rrr commented Oct 29, 2019 • edited Loading

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

mergify bot commented Oct 29, 2019

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

rix0rrr commented Oct 29, 2019 •

edited

Loading