Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cloudwatch): period in GraphWidget #13555

Closed
wants to merge 4 commits into from

Conversation

bentonkribbs
Copy link

This PR adds support for the period property in CloudWatch GraphWidgets.

The property in question can be seen here, under "period": https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Metric-Widget-Object

This fixes #6367


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

@gitpod-io
Copy link

gitpod-io bot commented Mar 11, 2021

@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Mar 11, 2021
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 18, 2021

I think doing this will break how metrics are rendered. Right now, when metrics render themselves they will render their own period if it is != 300, assuming that the graph period is going to be 5 minutes (and so no need to render duplicate numbers).

With this change, if you set the graph period to 1 minute and add a 5 minute metric, the metric will NOT RENDER its period and hence appear at a 1 minute rate (which is counter to what you were expecting).

Can you describe the problem you are solving? We may need to do something else/more other than just forward a property to the user.

Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see Rico's comment

@bentonkribbs
Copy link
Author

My primary use case was creating a GraphWidget with a MathExpression, and trying to set all MathExpressions and Metrics in this GraphWidget to use a period of 60 seconds. At the time, the MathExpression's period wasn't working as expected in our local version of CDK, but this: ( #13078) now solves my use case.

I am not sure if there is an other use case where the GraphWidget's period property would need set which can't be satisfied by setting the individual MathExpressions and Metrics periods. Based on the fact that IMetric is only implemented by these two classes, I think this PR is no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request for Ability to Modify Graph Widget Period
4 participants