Skip to content

Commit

Permalink
fix(autoscaling): AutoScaling on percentile metrics doesn't work
Browse files Browse the repository at this point in the history 
AutoScaling on percentile metrics did not work because the
`MetricAggregationType` was trying to be derived from the metric, and it can
only be MIN, MAX or AVG.

Figure out what the metric aggregation type does, default it to
AVERAGE if no other suitable value can be determined, and also make
it and the evaluation periods configurable while we're at it.

Fixes #13144.
  • Loading branch information
@rix0rrr
rix0rrr committed Mar 3, 2021
1 parent 84c9960 commit 5698677
Show file tree
Hide file tree
Showing 4 changed files with 190 additions and 12 deletions.
29 changes: 24 additions & 5 deletions packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,25 @@ export interface BasicStepScalingPolicyProps {
* @default No minimum scaling effect
*/
readonly minAdjustmentMagnitude?: number;

/**
* How many evaluation periods of the metric to wait before triggering a scaling action
*
* Raising this value can be used to smooth out the metric, at the expense
* of slower response times.
*
* @default 1
*/
readonly evaluationPeriods?: number;

/**
* Aggregation to apply to all data points over the evaluation periods
*
* Only has meaning if `evaluationPeriods != 1`.
*
* @default - The statistic from the metric if applicable (MIN, MAX, AVERAGE), otherwise AVERAGE.
*/
readonly metricAggregationType?: MetricAggregationType;
}

export interface StepScalingPolicyProps extends BasicStepScalingPolicyProps {
Expand Down Expand Up @@ -92,7 +111,7 @@ export class StepScalingPolicy extends CoreConstruct {
this.lowerAction = new StepScalingAction(this, 'LowerPolicy', {
adjustmentType,
cooldown: props.cooldown,
metricAggregationType: aggregationTypeFromMetric(props.metric),
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
minAdjustmentMagnitude: props.minAdjustmentMagnitude,
scalingTarget: props.scalingTarget,
});
Expand All @@ -110,7 +129,7 @@ export class StepScalingPolicy extends CoreConstruct {
metric: props.metric,
alarmDescription: 'Lower threshold scaling alarm',
comparisonOperator: cloudwatch.ComparisonOperator.LESS_THAN_OR_EQUAL_TO_THRESHOLD,
evaluationPeriods: 1,
evaluationPeriods: props.evaluationPeriods ?? 1,
threshold,
});
this.lowerAlarm.addAlarmAction(new StepScalingAlarmAction(this.lowerAction));
Expand All @@ -122,7 +141,7 @@ export class StepScalingPolicy extends CoreConstruct {
this.upperAction = new StepScalingAction(this, 'UpperPolicy', {
adjustmentType,
cooldown: props.cooldown,
metricAggregationType: aggregationTypeFromMetric(props.metric),
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
minAdjustmentMagnitude: props.minAdjustmentMagnitude,
scalingTarget: props.scalingTarget,
});
Expand All @@ -140,7 +159,7 @@ export class StepScalingPolicy extends CoreConstruct {
metric: props.metric,
alarmDescription: 'Upper threshold scaling alarm',
comparisonOperator: cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
evaluationPeriods: 1,
evaluationPeriods: props.evaluationPeriods ?? 1,
threshold,
});
this.upperAlarm.addAlarmAction(new StepScalingAlarmAction(this.upperAction));
Expand Down Expand Up @@ -197,7 +216,7 @@ function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregatio
case 'Maximum':
return MetricAggregationType.MAXIMUM;
default:
throw new Error(`Cannot only scale on 'Minimum', 'Maximum', 'Average' metrics, got ${statistic}`);
return MetricAggregationType.AVERAGE;
}
}

Expand Down
77 changes: 76 additions & 1 deletion packages/@aws-cdk/aws-applicationautoscaling/test/test.step-scaling-policy.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, SynthUtils } from '@aws-cdk/assert';
import { expect, haveResource, haveResourceLike, SynthUtils } from '@aws-cdk/assert';
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as cdk from '@aws-cdk/core';
import * as fc from 'fast-check';
Expand Down Expand Up @@ -152,6 +152,81 @@ export = {

test.done();
},

'step scaling from percentile metric'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const target = createScalableTarget(stack);

// WHEN
target.scaleOnMetric('Tracking', {
metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric', statistic: 'p99' }),
scalingSteps: [
{ upper: 0, change: -1 },
{ lower: 100, change: +1 },
{ lower: 500, change: +5 },
],
});

// THEN
expect(stack).to(haveResourceLike('AWS::ApplicationAutoScaling::ScalingPolicy', {
PolicyType: 'StepScaling',
StepScalingPolicyConfiguration: {
AdjustmentType: 'ChangeInCapacity',
MetricAggregationType: 'Average',
},
}));
expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
ComparisonOperator: 'GreaterThanOrEqualToThreshold',
EvaluationPeriods: 1,
AlarmActions: [
{ Ref: 'TargetTrackingUpperPolicy72CEFA77' },
],
ExtendedStatistic: 'p99',
MetricName: 'Metric',
Namespace: 'Test',
Threshold: 100,
}));

test.done();
},

'step scaling with evaluation period configured'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const target = createScalableTarget(stack);

// WHEN
target.scaleOnMetric('Tracking', {
metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric', statistic: 'p99' }),
scalingSteps: [
{ upper: 0, change: -1 },
{ lower: 100, change: +1 },
{ lower: 500, change: +5 },
],
evaluationPeriods: 10,
metricAggregationType: appscaling.MetricAggregationType.MAXIMUM,
});

// THEN
expect(stack).to(haveResourceLike('AWS::ApplicationAutoScaling::ScalingPolicy', {
PolicyType: 'StepScaling',
StepScalingPolicyConfiguration: {
AdjustmentType: 'ChangeInCapacity',
MetricAggregationType: 'Maximum',
},
}));
expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
ComparisonOperator: 'GreaterThanOrEqualToThreshold',
EvaluationPeriods: 10,
ExtendedStatistic: 'p99',
MetricName: 'Metric',
Namespace: 'Test',
Threshold: 100,
}));

test.done();
},
};

/**
Expand Down
29 changes: 24 additions & 5 deletions packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,25 @@ export interface BasicStepScalingPolicyProps {
* @default No minimum scaling effect
*/
readonly minAdjustmentMagnitude?: number;

/**
* How many evaluation periods of the metric to wait before triggering a scaling action
*
* Raising this value can be used to smooth out the metric, at the expense
* of slower response times.
*
* @default 1
*/
readonly evaluationPeriods?: number;

/**
* Aggregation to apply to all data points over the evaluation periods
*
* Only has meaning if `evaluationPeriods != 1`.
*
* @default - The statistic from the metric if applicable (MIN, MAX, AVERAGE), otherwise AVERAGE.
*/
readonly metricAggregationType?: MetricAggregationType;
}

export interface StepScalingPolicyProps extends BasicStepScalingPolicyProps {
Expand Down Expand Up @@ -93,7 +112,7 @@ export class StepScalingPolicy extends CoreConstruct {
this.lowerAction = new StepScalingAction(this, 'LowerPolicy', {
adjustmentType: props.adjustmentType,
cooldown: props.cooldown,
metricAggregationType: aggregationTypeFromMetric(props.metric),
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
minAdjustmentMagnitude: props.minAdjustmentMagnitude,
autoScalingGroup: props.autoScalingGroup,
});
Expand All @@ -111,7 +130,7 @@ export class StepScalingPolicy extends CoreConstruct {
metric: props.metric,
alarmDescription: 'Lower threshold scaling alarm',
comparisonOperator: cloudwatch.ComparisonOperator.LESS_THAN_OR_EQUAL_TO_THRESHOLD,
evaluationPeriods: 1,
evaluationPeriods: props.evaluationPeriods ?? 1,
threshold,
});
this.lowerAlarm.addAlarmAction(new StepScalingAlarmAction(this.lowerAction));
Expand All @@ -123,7 +142,7 @@ export class StepScalingPolicy extends CoreConstruct {
this.upperAction = new StepScalingAction(this, 'UpperPolicy', {
adjustmentType: props.adjustmentType,
cooldown: props.cooldown,
metricAggregationType: aggregationTypeFromMetric(props.metric),
metricAggregationType: props.metricAggregationType ?? aggregationTypeFromMetric(props.metric),
minAdjustmentMagnitude: props.minAdjustmentMagnitude,
autoScalingGroup: props.autoScalingGroup,
});
Expand All @@ -141,7 +160,7 @@ export class StepScalingPolicy extends CoreConstruct {
metric: props.metric,
alarmDescription: 'Upper threshold scaling alarm',
comparisonOperator: cloudwatch.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
evaluationPeriods: 1,
evaluationPeriods: props.evaluationPeriods ?? 1,
threshold,
});
this.upperAlarm.addAlarmAction(new StepScalingAlarmAction(this.upperAction));
Expand All @@ -161,7 +180,7 @@ function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregatio
case 'Maximum':
return MetricAggregationType.MAXIMUM;
default:
throw new Error(`Cannot only scale on 'Minimum', 'Maximum', 'Average' metrics, got ${statistic}`);
return MetricAggregationType.AVERAGE;
}
}

Expand Down
67 changes: 66 additions & 1 deletion packages/@aws-cdk/aws-autoscaling/test/scaling.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource } from '@aws-cdk/assert';
import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
Expand Down Expand Up @@ -277,6 +277,71 @@ nodeunitShim({
},
});

test('step scaling from percentile metric', () => {
// GIVEN
const stack = new cdk.Stack();
const fixture = new ASGFixture(stack, 'Fixture');

// WHEN
fixture.asg.scaleOnMetric('Tracking', {
metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric', statistic: 'p99' }),
scalingSteps: [
{ upper: 0, change: -1 },
{ lower: 100, change: +1 },
{ lower: 500, change: +5 },
],
});

// THEN
expect(stack).to(haveResourceLike('AWS::AutoScaling::ScalingPolicy', {
PolicyType: 'StepScaling',
MetricAggregationType: 'Average',
}));
expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
ComparisonOperator: 'GreaterThanOrEqualToThreshold',
EvaluationPeriods: 1,
AlarmActions: [
{ Ref: 'FixtureASGTrackingUpperPolicy27D4301F' },
],
ExtendedStatistic: 'p99',
MetricName: 'Metric',
Namespace: 'Test',
Threshold: 100,
}));
});

test('step scaling with evaluation period configured', () => {
// GIVEN
const stack = new cdk.Stack();
const fixture = new ASGFixture(stack, 'Fixture');

// WHEN
fixture.asg.scaleOnMetric('Tracking', {
metric: new cloudwatch.Metric({ namespace: 'Test', metricName: 'Metric', statistic: 'p99' }),
scalingSteps: [
{ upper: 0, change: -1 },
{ lower: 100, change: +1 },
{ lower: 500, change: +5 },
],
evaluationPeriods: 10,
metricAggregationType: autoscaling.MetricAggregationType.MAXIMUM,
});

// THEN
expect(stack).to(haveResourceLike('AWS::AutoScaling::ScalingPolicy', {
PolicyType: 'StepScaling',
MetricAggregationType: 'Maximum',
}));
expect(stack).to(haveResource('AWS::CloudWatch::Alarm', {
ComparisonOperator: 'GreaterThanOrEqualToThreshold',
EvaluationPeriods: 10,
ExtendedStatistic: 'p99',
MetricName: 'Metric',
Namespace: 'Test',
Threshold: 100,
}));
});

class ASGFixture extends Construct {
public readonly vpc: ec2.Vpc;
public readonly asg: autoscaling.AutoScalingGroup;
Expand Down

0 comments on commit 5698677

Please sign in to comment.