fix(autoscaling): can't use MathExpression in scaleOnMetric

It was not possible to use `MathExpression` objects in AutoScaling, because the `Alarm` interface has a bug: it takes "override" arguments like "period" and "statistic" that could and should have already been encoded into the `Metric` object passed to the alarm. Subsequently, AutoScaling used this ill-advised feature to force the period of metrics to 1 minute. This caused an invalid render for math expressions and was probably ill-advised to begin with. We should be respecting the customer's period on the metric they pass in. Fixes #5776. BREAKING CHANGE: AutoScaling by using `scaleOnMetric` will no longer force the alarm period to 1 minute, but use the period from the Metric object instead (5 minutes by default). Use `metric.with({ period: Duration.minute(1) })` to create a high-frequency scaling policy.
aws · Jan 16, 2020 · d4c1b0e · d4c1b0e
1 parent d5632ea
commit d4c1b0e
Show file tree

Hide file tree

Showing 5 changed files with 168 additions and 11 deletions.
diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts
@@ -103,7 +103,6 @@ export class StepScalingPolicy extends cdk.Construct {
       this.lowerAlarm = new cloudwatch.Alarm(this, 'LowerAlarm', {
         // Recommended by AutoScaling
         metric: props.metric,
-        period: cdk.Duration.minutes(1), // Recommended by AutoScaling
         alarmDescription: 'Lower threshold scaling alarm',
         comparisonOperator: cloudwatch.ComparisonOperator.LESS_THAN_OR_EQUAL_TO_THRESHOLD,
         evaluationPeriods: 1,
@@ -134,7 +133,6 @@ export class StepScalingPolicy extends cdk.Construct {
       this.upperAlarm = new cloudwatch.Alarm(this, 'UpperAlarm', {
         // Recommended by AutoScaling
         metric: props.metric,
-        period: cdk.Duration.minutes(1), // Recommended by AutoScaling
         alarmDescription: 'Upper threshold scaling alarm',
         comparisonOperator: cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
         evaluationPeriods: 1,

diff --git a/packages/@aws-cdk/aws-applicationautoscaling/test/test.scalable-target.ts b/packages/@aws-cdk/aws-applicationautoscaling/test/test.scalable-target.ts
@@ -1,4 +1,5 @@
 import { expect, haveResource } from '@aws-cdk/assert';
+import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
 import * as cdk from '@aws-cdk/core';
 import { Test } from 'nodeunit';
 import * as appscaling from '../lib';
@@ -82,5 +83,61 @@ export = {
     }));
 
     test.done();
-  }
+  },
+
+  'step scaling on MathExpression'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const target = createScalableTarget(stack);
+
+    // WHEN
+    target.scaleOnMetric('Metric', {
+      metric: new cloudwatch.MathExpression({
+        expression: 'a',
+        usingMetrics: {
+          a: new cloudwatch.Metric({
+            namespace: 'Test',
+            metricName: 'Metric',
+          })
+        },
+      }),
+      adjustmentType: appscaling.AdjustmentType.CHANGE_IN_CAPACITY,
+      scalingSteps: [
+        { change: -1, lower: 0, upper: 49 },
+        { change: 0, lower: 50, upper: 99 },
+        { change: 1, lower: 100 }
+      ]
+    });
+
+    // THEN
+    expect(stack).notTo(haveResource('AWS::CloudWatch::Alarm', {
+      Period: 60
+    }));
+
+    expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
+      ComparisonOperator: "LessThanOrEqualToThreshold",
+      EvaluationPeriods: 1,
+      Metrics: [
+        {
+          Expression: "a",
+          Id: "expr_1"
+        },
+        {
+          Id: "a",
+          MetricStat: {
+            Metric: {
+              MetricName: "Metric",
+              Namespace: "Test"
+            },
+            Period: 300,
+            Stat: "Average"
+          },
+          ReturnData: false
+        }
+      ],
+      Threshold: 49
+    }));
+
+    test.done();
+  },
 };
diff --git a/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts
@@ -104,7 +104,6 @@ export class StepScalingPolicy extends cdk.Construct {
       this.lowerAlarm = new cloudwatch.Alarm(this, 'LowerAlarm', {
         // Recommended by AutoScaling
         metric: props.metric,
-        period: cdk.Duration.minutes(1), // Recommended by AutoScaling
         alarmDescription: 'Lower threshold scaling alarm',
         comparisonOperator: cloudwatch.ComparisonOperator.LESS_THAN_OR_EQUAL_TO_THRESHOLD,
         evaluationPeriods: 1,
@@ -135,7 +134,6 @@ export class StepScalingPolicy extends cdk.Construct {
       this.upperAlarm = new cloudwatch.Alarm(this, 'UpperAlarm', {
         // Recommended by AutoScaling
         metric: props.metric,
-        period: cdk.Duration.minutes(1), // Recommended by AutoScaling
         alarmDescription: 'Upper threshold scaling alarm',
         comparisonOperator: cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
         evaluationPeriods: 1,

diff --git a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts
@@ -1,4 +1,5 @@
 import {expect, haveResource, haveResourceLike, InspectionFailure, ResourcePart} from '@aws-cdk/assert';
+import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as cdk from '@aws-cdk/core';
@@ -858,6 +859,104 @@ export = {
 
     test.done();
   },
+
+  'step scaling on metric'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const vpc = mockVpc(stack);
+    const asg = new autoscaling.AutoScalingGroup(stack, 'MyStack', {
+      instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
+      machineImage: new ec2.AmazonLinuxImage(),
+      vpc,
+    });
+
+    // WHEN
+    asg.scaleOnMetric('Metric', {
+      metric: new cloudwatch.Metric({
+        namespace: 'Test',
+        metricName: 'Metric',
+      }),
+      adjustmentType: autoscaling.AdjustmentType.CHANGE_IN_CAPACITY,
+      scalingSteps: [
+        { change: -1, lower: 0, upper: 49 },
+        { change: 0, lower: 50, upper: 99 },
+        { change: 1, lower: 100 }
+      ]
+    });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
+      ComparisonOperator: "LessThanOrEqualToThreshold",
+      EvaluationPeriods: 1,
+      MetricName: "Metric",
+      Namespace: "Test",
+      Period: 300,
+    }));
+
+    test.done();
+  },
+
+  'step scaling on MathExpression'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+    const vpc = mockVpc(stack);
+    const asg = new autoscaling.AutoScalingGroup(stack, 'MyStack', {
+      instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
+      machineImage: new ec2.AmazonLinuxImage(),
+      vpc,
+    });
+
+    // WHEN
+    asg.scaleOnMetric('Metric', {
+      metric: new cloudwatch.MathExpression({
+        expression: 'a',
+        usingMetrics: {
+          a: new cloudwatch.Metric({
+            namespace: 'Test',
+            metricName: 'Metric',
+          })
+        },
+      }),
+      adjustmentType: autoscaling.AdjustmentType.CHANGE_IN_CAPACITY,
+      scalingSteps: [
+        { change: -1, lower: 0, upper: 49 },
+        { change: 0, lower: 50, upper: 99 },
+        { change: 1, lower: 100 }
+      ]
+    });
+
+    // THEN
+    expect(stack).notTo(haveResource('AWS::CloudWatch::Alarm', {
+      Period: 60
+    }));
+
+    expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
+      "ComparisonOperator": "LessThanOrEqualToThreshold",
+      "EvaluationPeriods": 1,
+      "Metrics": [
+        {
+          "Expression": "a",
+          "Id": "expr_1"
+        },
+        {
+          "Id": "a",
+          "MetricStat": {
+            "Metric": {
+              "MetricName": "Metric",
+              "Namespace": "Test"
+            },
+            "Period": 300,
+            "Stat": "Average"
+          },
+          "ReturnData": false
+        }
+      ],
+      "Threshold": 49
+    }));
+
+    test.done();
+  },
+
 };
 
 function mockVpc(stack: cdk.Stack) {

diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
@@ -561,7 +561,10 @@ export interface CreateAlarmOptions {
   /**
    * The period over which the specified statistic is applied.
    *
-   * @default Duration.minutes(5)
+   * Cannot be used with `MathExpression` objects.
+   *
+   * @default - The period from the metric
+   * @deprecated Use `metric.with({ period: ... })` to encode the period into the Metric object
    */
   readonly period?: cdk.Duration;
 
@@ -577,7 +580,10 @@ export interface CreateAlarmOptions {
    * - "SampleCount | "n"
    * - "pNN.NN"
    *
-   * @default Average
+   * Cannot be used with `MathExpression` objects.
+   *
+   * @default - The statistic from the metric
+   * @deprecated Use `metric.with({ statistic: ... })` to encode the period into the Metric object
    */
   readonly statistic?: string;
 
@@ -668,11 +674,10 @@ function changeAllPeriods(metrics: Record<string, IMetric>, period: cdk.Duration
 /**
  * Return a new metric object which is the same type as the input object, but with the period changed
  *
- * Uses JavaScript prototyping hackery to achieve this. Relies on the fact that
- * both implementations of IMetric have a `period` member that contains that particular
- * value.
+ * Relies on the fact that implementations of `IMetric` are also supposed to have
+ * an implementation of `with` that accepts an argument called `period`. See `IModifiableMetric`.
  */
-function changePeriod<A extends IMetric>(metric: A, period: cdk.Duration): IMetric {
+function changePeriod(metric: IMetric, period: cdk.Duration): IMetric {
   if (isModifiableMetric(metric)) {
     return metric.with({ period });
   }