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.

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

AutoScalingGroup.scaleOnMetric with MathExpression as metric created invalid template #5776

Closed
leocurrie opened this issue Jan 13, 2020 · 2 comments · Fixed by #5789
Closed
Assignees
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. in-progress This issue is being actively worked on. p1

Comments

@leocurrie
Copy link

leocurrie commented Jan 13, 2020

When attempting to create an Autoscaling Group that uses a Math Expression as the scaling metric, CDK will generate a template that is rejected by CloudFormation.

Reproduction Steps

Starting from the Application Load Balancer Typescript example (https://github.com/aws-samples/aws-cdk-examples/blob/master/typescript/application-load-balancer/index.ts), add the following:

import { MathExpression } from '@aws-cdk/aws-cloudwatch';
asg.scaleOnMetric('MyScalingMetric', {
      metric: new MathExpression({
        expression: 'activeConnections / processedBytes',
        usingMetrics: {
          activeConnections: lb.metricActiveConnectionCount({
            statistic: 'avg',
            period: Duration.minutes(5)
          }),
          processedBytes: lb.metricProcessedBytes({
            statistic: 'avg',
            period: Duration.minutes(5)
          })
        }
      }),
      adjustmentType: AdjustmentType.CHANGE_IN_CAPACITY,
      scalingSteps: [
        { change: -1, lower: 0, upper: 49 },
        { change: 0, lower: 50, upper: 99 },
        { change: 1, lower: 100 }
      ]
    });

(I'm not suggesting that this is a useful metric! It's just an example)
run cdk stack synth to generate the stack.
Note in the generated output, the generated ASGMyScalingMetricUpperAlarm ASGMyScalingMetricLowerAlarm both include the Period property, which according to the docs, should not be supplied for Math expression metrics: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-alarm.html#cfn-cloudwatch-alarms-period

When deploying the stack, the upper and lower alarms will fail to be created with an error.

Error Log

Period should not be set if list of Metrics is set. (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError; Request ID: 869ad658-bae4-4006-b93f-5bd085eeaa17)

--

Environment

  • **CLI Version : 1.20.0
  • **Framework Version: 1.20.0
  • **OS : MacOSX
  • **Language : Typescript

Other


This is 🐛 Bug Report

@leocurrie leocurrie added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 13, 2020
@SomayaB SomayaB added the @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling label Jan 13, 2020
@rix0rrr rix0rrr added the p1 label Jan 14, 2020
rix0rrr pushed a commit that referenced this issue Jan 14, 2020
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.
rix0rrr pushed a commit that referenced this issue Jan 14, 2020
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.
@SomayaB
Copy link
Contributor

SomayaB commented Jan 15, 2020

HI @leocurrie, thanks for reporting this issue. We are actively working on this.

@SomayaB SomayaB added in-progress This issue is being actively worked on. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 15, 2020
@mergify mergify bot closed this as completed in #5789 Jan 16, 2020
mergify bot pushed a commit that referenced this issue Jan 16, 2020
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.
@mordr
Copy link

mordr commented May 22, 2023

Just to clarify, is this now usable in CFN for target tracking, or only for step scaling? From the doc, seems to suggest no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. in-progress This issue is being actively worked on. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants