fix(cloudwatch): MathExpression period of <5 minutes is not respected #13078
Conversation
|
|
github-actions
bot
added
the
@aws-cdk/aws-cloudwatch
| @@ -55,6 +55,7 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { | |||
|
|
|||
| withExpression(expr) { | |||
| options.expression = expr.expression; | |||
| if (expr.period && expr.period !== 300) { options.period = expr.period; } | |||
There was a problem hiding this comment.
I omitted this originally because what would happen is the expression period would be propagated down to the queried metrics.
Have we confirmed this is really necessary? Does it actually solve the problem it says it's addressing?
There was a problem hiding this comment.
I mean... I know I suggested this fix in the linked bug report.
But I'm very much interested in WHY this change is necessary.
It might also be because of the period field in the enclosing graph, for example.
Looking for someone to do a slightly deeper dive and experimenting a bit with the combinations and telling us WHAT EXACTLY the behavior of CloudWatch is (and by extension, what the construct library needs to produce).
There was a problem hiding this comment.
Ok, I can create a minimal stack with a graph widget and deploy to my AWS account to see the difference between the two cases.
There was a problem hiding this comment.
When you define a metric using MathExpression, CDK will generate a new invisible metric for each component of the expression. For example, let's say you want to create a widget with three metrics for a DynamoDB table: one for ConsumedReadCapacityUnits, one for ConsumedWriteCapacityUnits and one for the sum of the other two. The generated CloudFormation template will have five metrics altogether:
- A visible one for ConsumedReadCapacityUnits
- A visible one for ConsumedWriteCapacityUnits
- An invisible one for ConsumedReadCapacityUnits
- An invisible one for ConsumedWriteCapacityUnits
- A visible one for ConsumedReadCapacityUnits + ConsumedWriteCapacityUnits
Metric 5 is the sum of metrics 3 and 4.
As it is now, if you specify a period in a MathExpression, that period is not being respected in the aggregate metric, but it is already being propagated down to its components. Note that this only affects the components of the math expression. Their visible counterparts remain unchanged. In the example above, only metrics 3 and 4 have the period changed. Since they are invisible, there is no noticeable effect in the final widget, which shows data at a default interval for the aggregate metric. For DynamoDB tables, for example, this default is 10 minutes.
This change solves the problem by overriding the period in the aggregate metric and not changing anything else in the final CloudFormation template. I also deployed stacks with different combinations of periods and overrides and the result is as expected.
mergify
bot
dismissed
rix0rrr’s stale review
Pull request has been modified.
nija-at
changed the title
rix0rrr
changed the title
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
fixes #9156
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license