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.

Sign up for GitHub

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

Jump to bottom

fix(cloudwatch): MathExpression period of <5 minutes is not respected #13078

Merged
merged 5 commits into from
Feb 25, 2021
Merged

fix(cloudwatch): MathExpression period of <5 minutes is not respected #13078

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ea5c2e
fix(aws-cloudwatch): respect the period of MathExpressions #9156
Feb 16, 2021
e23dbc1
Merge branch 'master' into math-expression-period
otaviomacedo Feb 16, 2021
94504f8
Merge branch 'master' into math-expression-period
otaviomacedo Feb 21, 2021
9760237
Merge branch 'master' into math-expression-period
otaviomacedo Feb 24, 2021
0626a61
Merge branch 'master' into math-expression-period
mergify[bot] Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

@otaviomacedo otaviomacedo Feb 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. A visible one for ConsumedReadCapacityUnits
  2. A visible one for ConsumedWriteCapacityUnits
  3. An invisible one for ConsumedReadCapacityUnits
  4. An invisible one for ConsumedWriteCapacityUnits
  5. 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.

},
});

Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
{
"Ref": "AWS::Region"
},
"\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
"\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\",\"period\":60}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
{
"Fn::GetAtt": [
"queue",
Expand Down Expand Up @@ -120,7 +120,7 @@
{
"Ref": "AWS::Region"
},
"\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
"\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\",\"period\":60}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"",
{
"Fn::GetAtt": [
"queue",
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,28 @@ export = {
test.done();
},

'top level period in a MathExpression is respected in its metrics'(test: Test) {
const graph = new GraphWidget({
left: [
a,
new MathExpression({
expression: 'a + b',
usingMetrics: { a, b },
period: Duration.minutes(1),
}),
],
});

// THEN
graphMetricsAre(test, graph, [
['Test', 'ACount'],
[{ label: 'a + b', expression: 'a + b', period: 60 }],
['Test', 'ACount', { visible: false, id: 'a', period: 60 }],
['Test', 'BCount', { visible: false, id: 'b', period: 60 }],
]);
test.done();
},

'MathExpression controls period of metrics transitively used in it'(test: Test) {
// Same as the previous test, but recursively

Expand Down