Repository navigation
Conversation
Expose WarmUpConfiguration through Alarm, PromQLAlarm, and metric alarm factories. Validate the supported 1-2880 minute range while preserving the existing default behavior when the option is omitted.
Remove warm-up configuration from PromQL and anomaly detection alarm props. Reject math and search expression alarms at synthesis so the option is limited to a single Metric.
Allow warm-up for single-output metric math, collapsed Metrics Insights queries, and anomaly detection. Reject direct Metrics Insights GROUP BY contributor alarms while keeping PromQL excluded.
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR surfaces CloudWatch's The change is cleanly additive and well-structured: the nested value object makes the illegal "flag set, period unset" state unrepresentable, the new validation fires only on the brand-new opt-in field so no existing stack regresses, omitting the config renders no template change, and the documented defaults and range match the service exactly. Unit and integration coverage exercise the new branches and the backward-compatible omitted case. One OPTIONAL item is worth settling before merge: the L2 "warmup" casing diverges from the service/L1 "WarmUp" spelling, which cannot be corrected after shipping without a breaking change. 🔴 0 blocking · 🟡 0 recommended · ⚪ 1 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| export interface AlarmWarmupConfiguration { | ||
| /** | ||
| * The amount of time that the alarm remains in `INSUFFICIENT_DATA` after it is created or updated. | ||
| * | ||
| * This must be between 1 minute and 2 days. | ||
| */ | ||
| readonly warmupPeriod: Duration; | ||
|
|
||
| /** | ||
| * Whether the alarm must wait for the full warm-up period before it starts evaluating. | ||
| * | ||
| * When false, the alarm starts evaluating as soon as it has enough metric data to fill its evaluation window. | ||
| * | ||
| * @default false | ||
| */ | ||
| readonly onlyStartEvaluatingAfterWarmupPeriodEnds?: boolean; | ||
| } |
There was a problem hiding this comment.
⚪ Optional — The new L2 props spell the AWS term as one word — AlarmWarmupConfiguration, warmupPeriod, onlyStartEvaluatingAfterWarmupPeriodEnds (and warmupConfiguration on CreateAlarmOptionsBase) — while the official CloudWatch term is two words, "WarmUp". The generated L1 camelCases the service property to warmUpConfiguration / warmUpPeriodDurationInMinutes / onlyStartEvaluatingAfterWarmUpPeriodEnds, and the render helper maps to exactly those capital-U keys. A user who starts with the L2 prop and later reaches for the L1 escape hatch sees the same concept spelled two different ways within one feature, and because prop names are permanent API surface once shipped, the casing can only be aligned before merge without a later breaking change (AGENTS_CONSTRUCT_DESIGN.md#property-naming, WarmUpConfiguration).
Suggested change: Rename interface AlarmWarmupConfiguration → AlarmWarmUpConfiguration, prop warmupConfiguration → warmUpConfiguration, warmupPeriod → warmUpPeriod, and onlyStartEvaluatingAfterWarmupPeriodEnds → onlyStartEvaluatingAfterWarmUpPeriodEnds to match the service term and the L1, updating the README, tests, and the integ test accordingly.
There was a problem hiding this comment.
+1, please rename these before merge: AlarmWarmUpConfiguration, warmUpConfiguration, warmUpPeriod and onlyStartEvaluatingAfterWarmUpPeriodEnds. The service, CloudFormation and the L1 all spell it WarmUp, and so does the open Log Alarm L2 in #38724 in this same module. If both land as they are, the same concept would be spelled two ways in aws-cloudwatch. Public names can't change once released. The existing *Warmup names in autoscaling and ECS follow CloudFormation properties that are spelled Warmup, so they don't apply here.
ozelalisen
left a comment
There was a problem hiding this comment.
Thanks @topra-aws, this is a nice addition. I checked the rendered WarmUpConfiguration against the CloudFormation and CloudWatch API docs: the mapping and the 1–2880 minute range are right, and alarms without the option synthesize exactly as before.
A few things before this can merge:
- Names. Please spell it
WarmUp, as the service and the L1 do (details on the existing thread). - The integ test. Please rebase onto
main, drop the F3002 acknowledgement, and commit a snapshot from a real deploy (details inline). - The
GROUP BYcheck. Could you point to the source for this restriction (details inline)? - The README. Please clarify what happens on updates, and say the period is in whole minutes (details inline).
| cdk.Validations.of(stack).acknowledge({ | ||
| id: 'CloudFormation-Validate::F3002', | ||
| reason: 'WarmUpConfiguration is a newly launched property not yet in the bundled schema', | ||
| }); |
There was a problem hiding this comment.
Two things for this test:
- Drop the acknowledgement. I don't think it's needed. The
@aws/cloudformation-validateversions pinned at your base (1.8.0-beta) and onmain(1.12.1) both acceptWarmUpConfiguration; only 1.7.0 flagged it. That's the version your snapshot'svalidation-report.jsonrecords, so it likely came from an older install. Could you remove it here and inalarm.test.ts, so these stacks still catch a real F3002 later? - Deploy the test. The description says the test was run with
--dry-run, and the contributing guide asks not to make snapshots that way. A deploy is also the only proof we'd have that CloudFormation accepts the property. After rebasing, could you runyarn integ test/aws-cloudwatch/test/integ.alarm-warmup.js --update-on-failedand commit the result? If you can't deploy, say so and a maintainer can run it.
| if (props.warmupConfiguration !== undefined && isMultiTimeSeriesMetricsInsightsQuery(props.metric)) { | ||
| throw new ValidationError( | ||
| lit`AlarmWarmupUnsupportedForMultiTimeSeriesMetricsInsights`, | ||
| 'warmupConfiguration cannot be used with a multi-time-series Metrics Insights query; ' + | ||
| 'remove GROUP BY or wrap the query in metric math that returns a single time series', | ||
| this, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Could you link the source for this restriction? The warm-up guide says "Warm-up periods work with metric alarms and log alarms", and neither it nor PutMetricAlarm mentions an exception for GROUP BY queries. If CloudWatch rejects or ignores warm-up there, keep the check, cite the source, and note in the docs that the detection is best effort. For example, a query wrapped in parentheses isn't caught. If CloudWatch accepts it, this check (and stripQuotedStrings) can go, so valid alarms aren't blocked.
|
|
||
| ### Alarm warm-up periods | ||
|
|
||
| A warm-up period delays alarm evaluation after the alarm is created or updated. During this period, |
There was a problem hiding this comment.
The warm-up guide says both "after you create or update an alarm" and "warm-up applies only once at alarm creation and does not restart". So it isn't clear whether adding warmupConfiguration to an alarm that's already deployed does anything. Do you know which applies? Your integ deploy plus an update deploy would show it. Either way, could the README and the warmupPeriod JSDoc say it the same way, and mention that the period must be a whole number of minutes?
| export interface AlarmWarmupConfiguration { | ||
| /** | ||
| * The amount of time that the alarm remains in `INSUFFICIENT_DATA` after it is created or updated. | ||
| * | ||
| * This must be between 1 minute and 2 days. | ||
| */ | ||
| readonly warmupPeriod: Duration; | ||
|
|
||
| /** | ||
| * Whether the alarm must wait for the full warm-up period before it starts evaluating. | ||
| * | ||
| * When false, the alarm starts evaluating as soon as it has enough metric data to fill its evaluation window. | ||
| * | ||
| * @default false | ||
| */ | ||
| readonly onlyStartEvaluatingAfterWarmupPeriodEnds?: boolean; | ||
| } |
There was a problem hiding this comment.
+1, please rename these before merge: AlarmWarmUpConfiguration, warmUpConfiguration, warmUpPeriod and onlyStartEvaluatingAfterWarmUpPeriodEnds. The service, CloudFormation and the L1 all spell it WarmUp, and so does the open Log Alarm L2 in #38724 in this same module. If both land as they are, the same concept would be spelled two ways in aws-cloudwatch. Public names can't change once released. The existing *Warmup names in autoscaling and ECS follow CloudFormation properties that are spelled Warmup, so they don't apply here.
Issue # (if applicable)
Closes #38743.
Reason for this change
CloudFormation supports delaying CloudWatch alarm evaluation after an alarm is created or updated, which helps prevent alarm noise while a resource begins publishing metric data. The generated
CfnAlarmexposes this capability, but the existing alarm L2 APIs require users to reach through to the L1 construct.Warm-up configuration is supported for alarms that evaluate one time series, including ordinary metric math, collapsed Metrics Insights queries, and anomaly detection. It is not supported for PromQL contributor alarms or direct multi-time-series Metrics Insights queries.
Description of changes
AlarmWarmupConfigurationwith a strongly typedDurationand the option to wait for the complete warm-up period.warmupConfigurationthroughAlarm,AnomalyDetectionAlarm,Metric.createAlarm, andMathExpression.createAlarm.GROUP BY, and grouped Metrics Insights queries collapsed by an outer metric math expression.SELECT ... GROUP BYcontributor alarm when warm-up is configured.PromQLAlarmProps.WarmUpConfigurationproperty and validate CloudFormation's 1–2880 whole-minute range while preserving unresolved tokens.Generated L1 files such as
cloudwatch.generated.tsare intentionally ignored and regenerated byspec2cdk; they must not be committed manually. The pinned@aws-cdk/aws-service-spec0.1.208 generates bothCfnAlarm.WarmUpConfigurationPropertyandCfnAlarmProps.warmUpConfiguration. The temporary F3002 acknowledgement is for the separate CloudFormation validation schema, which currently lags the L1 service spec.Describe any new or updated permissions being added
None.
Description of how you validated changes
@aws-cdk/aws-service-specresolved to the pinned 0.1.208.aws-cdk-libsuccessfully twice after the final changes; jsii reported 0 errors and 0 warnings.aws-cdk-liblint command successfully; only pre-existing repository warnings were reported.integ.alarm-warmupin dry-run mode; the snapshot was unchanged and passed.git diff --checksuccessfully.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license