(aws-applicationautoscaling): Cannot use percentile metrics for step scaling

[otterley]

It is not currently possible to produce step scaling policies from percentile metric queries with CDK using code like this:

    const service = new Ec2Service(this, 'Service', {
      cluster,
      taskDefinition,
    });
    service.autoScaleTaskCount({
      maxCapacity: 1000
    }).scaleOnMetric('P95MemoryUtilization', {
      metric: new Metric({
        metricName: 'MemoryUtilized',
        namespace: 'ECS/ContainerInsights',
        dimensions: {
          ClusterName: cluster.clusterName,
          ServiceName: service.serviceName
        },
        period: cdk.Duration.minutes(1),
        statistic: 'p95', // offends CDK
        unit: Unit.MEGABYTES,
      }),
      adjustmentType: AdjustmentType.CHANGE_IN_CAPACITY,
      scalingSteps: [
        {
          change: 1,
          lower: props.TaskMemory * 0.9,
        },
        {
          change: -1,
          upper: props.TaskMemory * 0.8,
        }
      ],
    });

The following error results:

Error: Cannot only scale on 'Minimum', 'Maximum', 'Average' metrics, got p95
    at aggregationTypeFromMetric (.../node_modules/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts:200:13)

The error results from the following code (link):

      this.lowerAction = new StepScalingAction(this, 'LowerPolicy', {
        adjustmentType,
        cooldown: props.cooldown,
        metricAggregationType: aggregationTypeFromMetric(props.metric),
        minAdjustmentMagnitude: props.minAdjustmentMagnitude,
        scalingTarget: props.scalingTarget,
      });

function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType | undefined {
  const statistic = metric.toMetricConfig().metricStat?.statistic;
  if (statistic == null) { return undefined; } // Math expression, don't know aggregation, leave default

  switch (statistic) {
    case 'Average':
      return MetricAggregationType.AVERAGE;
    case 'Minimum':
      return MetricAggregationType.MINIMUM;
    case 'Maximum':
      return MetricAggregationType.MAXIMUM;
    default:
      throw new Error(`Cannot only scale on 'Minimum', 'Maximum', 'Average' metrics, got ${statistic}`);
  }
}

The aggregationTypeFromMetric function shouldn't be used. The aggregation type may be specified by the user, but can default to MetricAggregationType.AVERAGE. The aggregation type is independent of the statistic of the metric query; it is not safe to derive one from the other. For example, it is totally legal and OK for the metric statistic to be Minimum and the aggregation type to be Maximum. The first function pertains to what metric query is made to deliver values to Application Auto Scaling; the aggregation type tells Application Autoscaling how to then process those values once received.

---

This is 🐛 Bug Report

[otterley]

Temporary workaround:

class MetricFixer implements cdk.IAspect {
  stat: string;

  constructor(stat: string) {
    this.stat = stat;
  }

  public visit(node: cdk.IConstruct): void {
    if (node instanceof CfnAlarm) {
      node.extendedStatistic = this.stat;
      node.statistic = undefined;
    }
  }
}

cdk.Aspects.of(service).add(new MetricFixer('p95'));

[NetaNir]

From CloudFormation docs:

MetricAggregationType
The aggregation type for the CloudWatch metrics. Valid values are Minimum, Maximum, and Average. If the aggregation type is null, the value is treated as Average.
Required: No
Type: String
Allowed values: Average | Maximum | Minimum

The extendedStatistic is a property of the alarm, not the scalingPolicy. There might be another API you are thinking of, but for this API the validation is required.

[otterley]

The extendedStatistic is a property of the alarm, not the scalingPolicy. There might be another API you are thinking of, but for this API the validation is required.

Yes, you're right. I'm saying that the alarm and the scaling policy aggregations are independent. You cannot reliably derive the scaling policy's aggregation from the alarm's aggregation. They are not identical, but the current implementation treats them like they are.

[rix0rrr]

This is mea culpa. I wrote this code, and I might have made unwarranted snap decisions while writing it.

It was a different time back then 😅.

@otterley, do you know of a place where it is clearly explained how an AggregationType is used, what it means?

I'm going to take a guess and say that a minimum-of-minimums, maximum-of-maximums and average-of-averages still makes sense as defaults... and unless specified any other statistic will also get averaged I suppose? (I imagine that undefined gets interpreted as Average, at least)

All of this will be unless the user specifies differently, of course.

(What's bothering me is that an average-of-p95s doesn't really mean anything...)

[otterley]

@otterley, do you know of a place where it is clearly explained how an AggregationType is used, what it means?

The only public reference I've got are the docs:

CloudWatch aggregates metric data points based on the statistic for the metric associated with your CloudWatch alarm. When the alarm is breached, the appropriate scaling policy is triggered. Application Auto Scaling applies your specified aggregation type to the most recent metric data points from CloudWatch (as opposed to the raw metric data). It compares this aggregated metric value against the upper and lower bounds defined by the step adjustments to determine which step adjustment to perform.

This paragraph is dense and somewhat challenging to parse because the word "aggregation" is used multiple times but refers to different things. There's CloudWatch's aggregation of a metric, which is a non-time-dimension aggregate (e.g. there are a bunch of EC2 instances all reporting a metric value during the same time window and the p95/avg/min/max is taken of those). Then there's Application Auto Scaling's aggregation, which is time-dimensional (i.e. it takes the pre-aggregated values from CloudWatch and then aggregates those across a larger window of time). Conceptually it might make sense to think of it as x-axis vs. y-axis aggregation, where y is the time axis.

I'm going to take a guess and say that a minimum-of-minimums, maximum-of-maximums and average-of-averages still makes sense as defaults... and unless specified any other statistic will also get averaged I suppose? (I imagine that undefined gets interpreted as Average, at least)

All of this will be unless the user specifies differently, of course.

These sound like reasonable defaults to me. We should take pains to make this explicit in the docs.

I'll see if I've got the cycles to help with a PR but I'm suuuuuper impacted right now. :)

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.