-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cloudwatch): render visible metrics first #36163
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
base: main
Are you sure you want to change the base?
Conversation
aws-cdk-automation
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter fails with the following errors:
❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
|
In the CONTRIBUTING GUIDE for features, it states:
In my opinion, this feature is small enough that it probably doesn't warrant a README update or additional integration test. I could be convinced otherwise, though, and am happy to add those things if requested. |
|
I saw this in your last CI error log Looks like this integ is failing
you probably need to cd framework-integ
yarn integ-runner test/aws-cloudwatch/test/integ.math-alarm-and-dashboard.jsand make sure it passes. If it failed you need to re-deploy and re-generate snapshots. see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#testing |
|
If you need to modify any integ test, a useful trick is to have a separate terminal This will incrementally compile when you update any .ts under that dir and re-generate .js see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#yarn-watch-optional Hope it helps! |
|
Thank you! I thought I had ran the integ tests locally, but I just built the framework. I was able to update the snapshots, but needed to use the |
|
Hi @jackrein --dry-run should be avoided as it doesn't guarantee a successful deployment and the generated snapshots can't be validated. As mentioned in the contributed guide
To ensure the updated integ tests are deployable, you need a valid AWS account with CLI configured and So let me clarify the process, you don't need the access to any AWS-owned integ test account, for integ testing, you just need to make sure you have your own AWS account configured with AWS CLI and Let me know if it makes sense to you. Thank you. |
|
I was able to get the integ tests working in my personal AWS account – there are no changes to commit, so leaving the PR as is I was originally trying to use Adding an Access Key and Secret Access Key to |
Issue #36079
Closes #36079.
Reason for this change
Multiple visible math expressions added to a graph are hard to distinguish, because the automatic CloudWatch color assignments assign contrasting colors to the invisible metrics. This change makes these types of graphs much easier to view.
Description of changes
Sorted
metricsinrendering.tsascending by level upon reading the property. Any top level metrics (i.e. math expressions) now appear before nested metrics. The visible metrics are now assigned contrasting colors.Describe any new or updated permissions being added
No new permissions added
Description of how you validated changes
One unit test was added, dedicated for this behavior. I also synthesized this example project, and saw the metrics in the desired order.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license