Repository: cdklabs/cdk-monitoring-constructs (language: python)

Issue:
minMetricSamplesToAlarm uses incorrect evaluation period
### Version

6.2.2

### Steps and/or minimal code example to reproduce

Reproducible with unit tests included in the package: https://github.com/cdklabs/cdk-monitoring-constructs/blob/84609a4d81ad5367ee14a442411c82fa4d368407/test/common/alarm/AlarmFactory.test.ts#L275

### Expected behavior

`NoSamples` child alarm should use the same `datapointsToAlarm` and `evaluationPeriods` as the alarm that we are attaching `minMetricSamplesToAlarm` to.

Example:

```
  factory.addAlarm(metric, {
    ...props,
    alarmNameSuffix: "none",
    comparisonOperator: ComparisonOperator.LESS_THAN_THRESHOLD,
    minMetricSamplesToAlarm: 500,
    datapointsToAlarm: 5,
    evaluationPeriods: 5,
  });
```

should create a `NoSamples` alarm with `datapointsToAlarm: 5` and ` evaluationPeriods: 5`.

### Actual behavior

`NoSamples` child alarm uses `datapointsToAlarm` and `evaluationPeriods` different than the main alarm - both are set `1` instead.

### Other details

The fact that the `datapointsToAlarm` and `evaluationPeriods`  for `NoSamples` child alarm are using different values (i.e. 1), causes incorrect Composite Alarm behaviour. 

Imagine monitor where you require 5 breaching datapoints to go into alarm with the `minMetricSamplesToAlarm` configured to an arbitrary value. Consider the following:

```
NoSamplesAlarm:         [X, X, X, X, 0, 0, 0, 0, 0]
MainAlarm:              [X, X, X, X, X, 0, 0, 0, 0]
---
CompositeWithSamples:   [0, 0, 0, 0, X, 0, 0, 0, 0]

where 0 is a non-breaching data point, X is a breaching data point
```

Even though there was only a single datapoint that satisfied both conditions, the monitor went into alarm despite the fact that the `MainAlarm` had `datapointsToAlarm` configured to 5 datapoints. `NoSamplesAlarm` generated by cdk-monitoring-constructs is based on `datapointsToAlarm: 1`, so if that single datapoint coincides with `MainAlarm` reaching its own threshold, the overall `CompositeWithSamples` will go into ALARM anyway. This is doesn't seem like a desirable behaviour - for the `CompositeWithSamples` to go into ALARM, all breaching datapoints should have sufficient number of samples.

If this is not a bug, is there any rationale behind the `NoSamples` using a different `datapointsToAlarm` and `evaluationPeriods`  than the `MainAlarm`?

minMetricSamplesToAlarm uses incorrect evaluation period
### Version

6.2.2

### Steps and/or minimal code example to reproduce

Reproducible with unit tests included in the package: https://github.com/cdklabs/cdk-monitoring-constructs/blob/84609a4d81ad5367ee14a442411c82fa4d368407/test/common/alarm/AlarmFactory.test.ts#L275

### Expected behavior

`NoSamples` child alarm should use the same `datapointsToAlarm` and `evaluationPeriods` as the alarm that we are attaching `minMetricSamplesToAlarm` to.

Example:

```
  factory.addAlarm(metric, {
    ...props,
    alarmNameSuffix: "none",
    comparisonOperator: ComparisonOperator.LESS_THAN_THRESHOLD,
    minMetricSamplesToAlarm: 500,
    datapointsToAlarm: 5,
    evaluationPeriods: 5,
  });
```

should create a `NoSamples` alarm with `datapointsToAlarm: 5` and ` evaluationPeriods: 5`.

### Actual behavior

`NoSamples` child alarm uses `datapointsToAlarm` and `evaluationPeriods` different than the main alarm - both are set `1` instead.

### Other details

The fact that the `datapointsToAlarm` and `evaluationPeriods`  for `NoSamples` child alarm are using different values (i.e. 1), causes incorrect Composite Alarm behaviour. 

Imagine monitor where you require 5 breaching datapoints to go into alarm with the `minMetricSamplesToAlarm` configured to an arbitrary value. Consider the following:

```
NoSamplesAlarm:         [X, X, X, X, 0, 0, 0, 0, 0]
MainAlarm:              [X, X, X, X, X, 0, 0, 0, 0]
---
CompositeWithSamples:   [0, 0, 0, 0, X, 0, 0, 0, 0]

where 0 is a non-breaching data point, X is a breaching data point
```

Even though there was only a single datapoint that satisfied both conditions, the monitor went into alarm despite the fact that the `MainAlarm` had `datapointsToAlarm` configured to 5 datapoints. `NoSamplesAlarm` generated by cdk-monitoring-constructs is based on `datapointsToAlarm: 1`, so if that single datapoint coincides with `MainAlarm` reaching its own threshold, the overall `CompositeWithSamples` will go into ALARM anyway. This is doesn't seem like a desirable behaviour - for the `CompositeWithSamples` to go into ALARM, all breaching datapoints should have sufficient number of samples.

If this is not a bug, is there any rationale behind the `NoSamples` using a different `datapointsToAlarm` and `evaluationPeriods`  than the `MainAlarm`?



Discussion / hints:

Fix patch:
diff --git a/API.md b/API.md
index abdf7e82..a7dc7f51 100644
--- a/API.md
+++ b/API.md
@@ -2367,6 +2367,7 @@ const addAlarmProps: AddAlarmProps = { ... }
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.fillAlarmRange">fillAlarmRange</a></code> | <code>boolean</code> | Indicates whether the alarming range of values should be highlighted in the widget. |
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.metricAdjuster">metricAdjuster</a></code> | <code><a href="#cdk-monitoring-constructs.IMetricAdjuster">IMetricAdjuster</a></code> | If specified, adjusts the metric before creating an alarm from it. |
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.minMetricSamplesToAlarm">minMetricSamplesToAlarm</a></code> | <code>number</code> | Specifies how many samples (N) of the metric is needed to trigger the alarm. |
+| <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.minSampleCountToEvaluateDatapoint">minSampleCountToEvaluateDatapoint</a></code> | <code>number</code> | Specifies how many samples (N) of the metric is needed in a datapoint to be evaluated for alarming. |
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.overrideAnnotationColor">overrideAnnotationColor</a></code> | <code>string</code> | If specified, it modifies the final alarm annotation color. |
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.overrideAnnotationLabel">overrideAnnotationLabel</a></code> | <code>string</code> | If specified, it modifies the final alarm annotation label. |
 | <code><a href="#cdk-monitoring-constructs.AddAlarmProps.property.overrideAnnotationVisibility">overrideAnnotationVisibility</a></code> | <code>boolean</code> | If specified, it modifies the final alarm annotation visibility. |
@@ -2640,7 +2641,11 @@ If specified, adjusts the metric before creating an alarm from it.
 
 ---
 
-##### `minMetricSamplesToAlarm`<sup>Optional</sup> <a name="minMetricSamplesToAlarm" id="cdk-monitoring-constructs.AddAlarmProps.property.minMetricSamplesToAlarm"></a>
+##### ~~`minMetricSamplesToAlarm`~~<sup>Optional</sup> <a name="minMetricSamplesToAlarm" id="cdk-monitoring-constructs.AddAlarmProps.property.minMetricSamplesToAlarm"></a>
+
+- *Deprecated:* Use minSampleCountToEvaluateDatapoint instead. minMetricSamplesAlarm uses different evaluation
+period for its child alarms, so it doesn't guarantee that each datapoint in the evaluation period has
+sufficient number of samples
 
 ```typescript
 public readonly minMetricSamplesToAlarm: number;
@@ -2660,6 +2665,25 @@ The newly created composite alarm will be returned as a result, and it will take
 
 ---
 
+##### `minSampleCountToEvaluateDatapoint`<sup>Optional</sup> <a name="minSampleCountToEvaluateDatapoint" id="cdk-monitoring-constructs.AddAlarmProps.property.minSampleCountToEvaluateDatapoint"></a>
+
+```typescript
+public readonly minSampleCountToEvaluateDatapoint: number;
+```
+
+- *Type:* number
+- *Default:* default behaviour - no condition on sample count will be used
+
+Specifies how many samples (N) of the metric is needed in a datapoint to be evaluated for alarming.
+
+If this property is specified, your metric will be subject to MathExpression that will add an IF condition
+to your metric to make sure that each datapoint is evaluated only if it has sufficient number of samples.
+If the number of samples is not sufficient, the datapoint will be treated as missing data and will be evaluated
+according to the treatMissingData parameter.
+If specified, deprecated minMetricSamplesToAlarm has no effect.
+
+---
+
 ##### `overrideAnnotationColor`<sup>Optional</sup> <a name="overrideAnnotationColor" id="cdk-monitoring-constructs.AddAlarmProps.property.overrideAnnotationColor"></a>
 
 ```typescript
@@ -3073,6 +3097,7 @@ const alarmAnnotationStrategyProps: AlarmAnnotationStrategyProps = { ... }
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.metric">metric</a></code> | <code>aws-cdk-lib.aws_cloudwatch.Metric \| aws-cdk-lib.aws_cloudwatch.MathExpression</code> | *No description.* |
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.threshold">threshold</a></code> | <code>number</code> | *No description.* |
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.minMetricSamplesToAlarm">minMetricSamplesToAlarm</a></code> | <code>number</code> | *No description.* |
+| <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.minSampleCountToEvaluateDatapoint">minSampleCountToEvaluateDatapoint</a></code> | <code>number</code> | *No description.* |
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.overrideAnnotationColor">overrideAnnotationColor</a></code> | <code>string</code> | *No description.* |
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.overrideAnnotationLabel">overrideAnnotationLabel</a></code> | <code>string</code> | *No description.* |
 | <code><a href="#cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.overrideAnnotationVisibility">overrideAnnotationVisibility</a></code> | <code>boolean</code> | *No description.* |
@@ -3209,6 +3234,16 @@ public readonly minMetricSamplesToAlarm: number;
 
 ---
 
+##### `minSampleCountToEvaluateDatapoint`<sup>Optional</sup> <a name="minSampleCountToEvaluateDatapoint" id="cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.minSampleCountToEvaluateDatapoint"></a>
+
+```typescript
+public readonly minSampleCountToEvaluateDatapoint: number;
+```
+
+- *Type:* number
+
+---
+
 ##### `overrideAnnotationColor`<sup>Optional</sup> <a name="overrideAnnotationColor" id="cdk-monitoring-constructs.AlarmAnnotationStrategyProps.property.overrideAnnotationColor"></a>
 
 ```typescript
diff --git a/lib/common/alarm/AlarmFactory.ts b/lib/common/alarm/AlarmFactory.ts
index f2f167a0..5c8990ff 100644
--- a/lib/common/alarm/AlarmFactory.ts
+++ b/lib/common/alarm/AlarmFactory.ts
@@ -7,6 +7,7 @@ import {
   CompositeAlarm,
   HorizontalAnnotation,
   IAlarmRule,
+  MathExpression,
   TreatMissingData,
 } from "aws-cdk-lib/aws-cloudwatch";
 import { Construct } from "constructs";
@@ -186,6 +187,18 @@ export interface AddAlarmProps {
    */
   readonly evaluateLowSampleCountPercentile?: boolean;
 
+  /**
+   * Specifies how many samples (N) of the metric is needed in a datapoint to be evaluated for alarming.
+   * If this property is specified, your metric will be subject to MathExpression that will add an IF condition
+   * to your metric to make sure that each datapoint is evaluated only if it has sufficient number of samples.
+   * If the number of samples is not sufficient, the datapoint will be treated as missing data and will be evaluated
+   * according to the treatMissingData parameter.
+   * If specified, deprecated minMetricSamplesToAlarm has no effect.
+   *
+   * @default - default behaviour - no condition on sample count will be used
+   */
+  readonly minSampleCountToEvaluateDatapoint?: number;
+
   /**
    * Specifies how many samples (N) of the metric is needed to trigger the alarm.
    * If this property is specified, an artificial composite alarm is created of the following:
@@ -195,6 +208,9 @@ export interface AddAlarmProps {
    * </ul>
    * The newly created composite alarm will be returned as a result, and it will take the original alarm actions.
    * @default - default behaviour - no condition on sample count will be added to the alarm
+   * @deprecated Use minSampleCountToEvaluateDatapoint instead. minMetricSamplesAlarm uses different evaluation
+   *   period for its child alarms, so it doesn't guarantee that each datapoint in the evaluation period has
+   *   sufficient number of samples
    */
   readonly minMetricSamplesToAlarm?: number;
 
@@ -511,6 +527,9 @@ export class AlarmFactory {
       props
     );
 
+    // metric that will be ultimately used to create the alarm
+    let alarmMetric: MetricWithAlarmSupport = adjustedMetric;
+
     // prepare primary alarm properties
 
     const actionsEnabled = this.determineActionsEnabled(
@@ -549,32 +568,58 @@ export class AlarmFactory {
       );
     }
 
+    // apply metric math for minimum metric samples
+
+    if (props.minSampleCountToEvaluateDatapoint) {
+      if (adjustedMetric instanceof MathExpression) {
+        throw new Error(
+          "minSampleCountToEvaluateDatapoint is not supported for MathExpressions. " +
+            "If you already use MathExpression, you can extend your expression to evaluate " +
+            "the sample count using IF statement, e.g. IF(sampleCount > X, mathExpression)."
+        );
+      }
+
+      const metricSampleCount = adjustedMetric.with({
+        statistic: MetricStatistic.N,
+        label: "Sample count",
+      });
+
+      alarmMetric = new MathExpression({
+        label: `${adjustedMetric}`,
+        expression: `IF(sampleCount > ${props.minSampleCountToEvaluateDatapoint}, metric)`,
+        usingMetrics: {
+          metric: adjustedMetric,
+          sampleCount: metricSampleCount,
+        },
+      });
+    }
+
     // create primary alarm
 
-    const primaryAlarm = adjustedMetric.createAlarm(
-      this.alarmScope,
+    const primaryAlarm = alarmMetric.createAlarm(this.alarmScope, alarmName, {
       alarmName,
-      {
-        alarmName,
-        alarmDescription,
-        threshold: props.threshold,
-        comparisonOperator: props.comparisonOperator,
-        treatMissingData: props.treatMissingData,
-        // default value (undefined) means "evaluate"
-        evaluateLowSampleCountPercentile: evaluateLowSampleCountPercentile
-          ? undefined
-          : "ignore",
-        datapointsToAlarm,
-        evaluationPeriods,
-        actionsEnabled,
-      }
-    );
+      alarmDescription,
+      threshold: props.threshold,
+      comparisonOperator: props.comparisonOperator,
+      treatMissingData: props.treatMissingData,
+      // default value (undefined) means "evaluate"
+      evaluateLowSampleCountPercentile: evaluateLowSampleCountPercentile
+        ? undefined
+        : "ignore",
+      datapointsToAlarm,
+      evaluationPeriods,
+      actionsEnabled,
+    });
 
     let alarm: AlarmBase = primaryAlarm;
 
     // create composite alarm for min metric samples (if defined)
+    // deprecated in favour of minSampleCountToEvaluateDatapoint
 
-    if (props.minMetricSamplesToAlarm) {
+    if (
+      !props.minSampleCountToEvaluateDatapoint &&
+      props.minMetricSamplesToAlarm
+    ) {
       const metricSampleCount = adjustedMetric.with({
         statistic: MetricStatistic.N,
       });
@@ -627,6 +672,8 @@ export class AlarmFactory {
       datapointsToAlarm,
       dedupeString,
       minMetricSamplesToAlarm: props.minMetricSamplesToAlarm,
+      minSampleCountToEvaluateDatapoint:
+        props.minSampleCountToEvaluateDatapoint,
       fillAlarmRange: props.fillAlarmRange ?? false,
       overrideAnnotationColor: props.overrideAnnotationColor,
       overrideAnnotationLabel: props.overrideAnnotationLabel,
diff --git a/lib/common/alarm/IAlarmAnnotationStrategy.ts b/lib/common/alarm/IAlarmAnnotationStrategy.ts
index 12b2b01e..80561151 100644
--- a/lib/common/alarm/IAlarmAnnotationStrategy.ts
+++ b/lib/common/alarm/IAlarmAnnotationStrategy.ts
@@ -13,6 +13,7 @@ export interface AlarmAnnotationStrategyProps extends AlarmMetadata {
   readonly metric: MetricWithAlarmSupport;
   readonly comparisonOperator: ComparisonOperator;
   readonly minMetricSamplesToAlarm?: number;
+  readonly minSampleCountToEvaluateDatapoint?: number;
   readonly threshold: number;
   readonly datapointsToAlarm: number;
   readonly evaluationPeriods: number;


Test patch:
diff --git a/test/common/alarm/AlarmFactory.test.ts b/test/common/alarm/AlarmFactory.test.ts
index dc9653c8..8357acf7 100644
--- a/test/common/alarm/AlarmFactory.test.ts
+++ b/test/common/alarm/AlarmFactory.test.ts
@@ -1,9 +1,10 @@
 import { Duration, Stack } from "aws-cdk-lib";
-import { Capture, Template } from "aws-cdk-lib/assertions";
+import { Capture, Match, Template } from "aws-cdk-lib/assertions";
 import {
   Alarm,
   CfnAlarm,
   ComparisonOperator,
+  MathExpression,
   Metric,
   Shading,
   TreatMissingData,
@@ -330,6 +331,84 @@ test("addAlarm: check created alarms when minMetricSamplesToAlarm is used", () =
   });
 });
 
+test("addAlarm: check created alarms when minSampleCountToEvaluateDatapoint is used", () => {
+  const stack = new Stack();
+  const factory = new AlarmFactory(stack, {
+    globalMetricDefaults,
+    globalAlarmDefaults,
+    localAlarmNamePrefix: "prefix",
+  });
+  factory.addAlarm(metric, {
+    ...props,
+    alarmNameSuffix: "none",
+    comparisonOperator: ComparisonOperator.LESS_THAN_THRESHOLD,
+    minSampleCountToEvaluateDatapoint: 42,
+    minMetricSamplesToAlarm: 55, // not used if minSampleCountToEvaluateDatapoint defined
+  });
+
+  const template = Template.fromStack(stack);
+  template.hasResourceProperties("AWS::CloudWatch::Alarm", {
+    AlarmName: "DummyServiceAlarms-prefix-none",
+    AlarmDescription: "Description",
+    ComparisonOperator: "LessThanThreshold",
+    DatapointsToAlarm: 10,
+    EvaluationPeriods: 10,
+    TreatMissingData: "notBreaching",
+    Metrics: [
+      Match.objectLike({
+        Expression: "IF(sampleCount > 42, metric)",
+        Label: "DummyMetric1",
+      }),
+      {
+        Id: "metric",
+        MetricStat: {
+          Metric: Match.objectLike({
+            MetricName: "DummyMetric1",
+          }),
+          Period: 300,
+          Stat: "Average",
+        },
+        ReturnData: false,
+      },
+      {
+        Id: "sampleCount",
+        MetricStat: {
+          Metric: Match.objectLike({
+            MetricName: "DummyMetric1",
+          }),
+          Period: 300,
+          Stat: "SampleCount",
+        },
+        ReturnData: false,
+      },
+    ],
+  });
+});
+
+test("addAlarm: minSampleCountToEvaluateDatapoint used with Math Expression throws error", () => {
+  const stack = new Stack();
+  const factory = new AlarmFactory(stack, {
+    globalMetricDefaults,
+    globalAlarmDefaults,
+    localAlarmNamePrefix: "prefix",
+  });
+  const mathExpression = new MathExpression({
+    expression: "MAX(metric)",
+    usingMetrics: {
+      metric,
+    },
+  });
+
+  expect(() =>
+    factory.addAlarm(mathExpression, {
+      ...props,
+      minSampleCountToEvaluateDatapoint: 42,
+    })
+  ).toThrow(
+    "minSampleCountToEvaluateDatapoint is not supported for MathExpressions"
+  );
+});
+
 test("addCompositeAlarm: snapshot for operator", () => {
   const stack = new Stack();
   const factory = new AlarmFactory(stack, {