aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.7k stars 3.93k forks source link

[aws-cloudwatch] Missing ThresholdMetricId #10540

Open alextriaca opened 4 years ago

alextriaca commented 4 years ago

When creating Cloudwatch Alarm that makes use of anomaly detection the following error is raised at deploy time:

ComparisonOperators for ranges require ThresholdMetricId to be set (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError; Request ID: xxx; Proxy: null)

Reproduction Steps

aws_cloudwatch.Alarm(
     self,
    "my_alarm",
    evaluation_periods=1,
    metric=sqs_queue.metric_number_of_messages_sent(),
    threshold=2,
    statistic="Average",
    comparison_operator=aws_cloudwatch.ComparisonOperator.LESS_THAN_LOWER_OR_GREATER_THAN_UPPER_THRESHOLD,
)

The Alarm construct works great for any of the other ComparisonOperators that do not rely on anomaly detection i.e. GreaterThanThreshold.

Environment


This is :bug: Bug Report

rrhodes commented 4 years ago

Hello, I'm working on a fix for this now, a PR should be open shortly.

alextriaca commented 4 years ago

Absolute legend! Thanks @rrhodes 😄

rrhodes commented 4 years ago

This will be my first contribution to CDK, so bear with me, I suspect regular contributors will have valuable feedback for me. :)

NetaNir commented 4 years ago

In the meantime, If you really need this feature you can configure it using escape hatch:

const anomalyDetectionExp = new cloudwatch.MathExpression({
  expression: 'ANOMALY_DETECTION_BAND(m1,2)',
  usingMetrics: {
    m1: metricA,
  },
  label: 'Anomaly detection',
  period: cdk.Duration.minutes(1),
});

const alarm = anomalyDetectionExp.createAlarm(this, 'MyAlarm', {
  evaluationPeriods: 1, 
  threshold: 5, // dummy value will be removed below 
  comparisonOperator: ComparisonOperator.LESS_THAN_LOWER_OR_GREATER_THAN_UPPER_THRESHOLD,
});

const cfnAlarm = alarm.node.defaultChild as CfnAlarm;
cfnAlarm.addPropertyDeletionOverride('Threshold');
(alarm.node.defaultChild as CfnAlarm).thresholdMetricId = 'expr_1'; // The id will always be 'expr_1' if there is one expression
rrhodes commented 4 years ago

An L2 solution for this problem remains outstanding. Initial work to embed support for threshold metric IDs into the existing Alarm and Metric functionality left the code in a less than desirable state for maintenance going forward, notably because any metric using thresholdMetricId cannot possess a threshold property. The PR for that approach is now closed but remains available for future reference.

@NetaNir suggested the following, which I agree would be the best approach at this time:

Since there are several such subtleties in the anomaly detection alarm (e.g the allowed operators, annotation, returnData), it probably makes more sense to implement a createAnomalyDetectionAlarm.

arnulfojr commented 3 years ago

any update so far?

jarruda commented 3 years ago

Is this still both p1 and in progress? Sounds like the preferred approach is a factory function that produces an Alarm with thresholdMetricId populated and other invariants respected? Is there any other guidance for a contributor?

olaven commented 2 years ago

👋 A status update on this would be highly appreciated!

tuanardouin commented 2 years ago

Hi, I see that a patch is in progress, any update on it ?

Got the exact same problem with this code :

const alarm = new cloudwatch.Alarm(this, 'Alarm', {
      alarmName: 'Alarm',
      actionsEnabled: true,
      metric: new Metric({
        namespace: 'AWS/ApiGateway',
        metricName: 'Count',
        dimensions: {
          'ApiName': 'The API',
          'Resource': '/an/endpoint',
          'Stage': 'prod',
          'Method': 'GET'
        },
        statistic: 'Sum'
      }),
      comparisonOperator: cloudwatch.ComparisonOperator.LESS_THAN_LOWER_THRESHOLD,
      threshold: 1,
      evaluationPeriods: 1,
      datapointsToAlarm: 1,
      treatMissingData: cloudwatch.TreatMissingData.BREACHING
    });
alfaproject commented 2 years ago

Is there any hope for this?

arnulfojr commented 1 year ago

I'm willing to contribute this but would like to have context of what's left to align on so we can have few back and forwards on the PR. Recent changes to the rendering logic made on the crossAccount and crossRegion do the workaround mentioned above impossible now.

mdtareque commented 11 months ago

Is this being worked upon?

MichelangeloSorice commented 9 months ago

Any update on this?

shftlvch commented 8 months ago

Run into this issue trying to use .monitorBilling() from cdk-monitoring-constructs module. Any updates?

mjgp2 commented 6 months ago

To give a cocurrently working example using escape hatch:

export function addLogAlarm(logGroup: LogGroup, metricNamespace: string, alarmName: string, alarmSnsTopic: ITopic, filterPattern = '%ERROR|WARNING%') {
  const metricFilter = logGroup.addMetricFilter(`${alarmName}-metric-filter`, {
    filterPattern: FilterPattern.literal(filterPattern),
    metricName: `${alarmName}-metric`,
    metricNamespace,
    defaultValue: 0,
    unit: Unit.COUNT,
  });

  const anomalyExpression = new MathExpression({
    expression: 'ANOMALY_DETECTION_BAND(m1,2)',
    usingMetrics: {
      m1: metricFilter.metric({statistic: "sum"}),
    },
    period: Duration.minutes(1),
  });
  const logsAlarm = new Alarm(logGroup.stack, alarmName, {
    alarmName,
    metric: anomalyExpression,
    evaluationPeriods: 10,
    datapointsToAlarm: 1,
    threshold: 5, // dummy value will be removed below
    comparisonOperator: ComparisonOperator.GREATER_THAN_UPPER_THRESHOLD,
  });
  const cfnAlarm = logsAlarm.node.defaultChild as CfnAlarm;
  cfnAlarm.addPropertyDeletionOverride('Threshold');
  (logsAlarm.node.defaultChild as CfnAlarm).thresholdMetricId = 'expr_1'; // The id will always be 'expr_1' if there is one expression
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  const metrics = cfnAlarm.metrics as Record<string, any>[];
  metrics[0].returnData = true;
  metrics[1].returnData = true;
  addAlarmActions(alarmSnsTopic, logsAlarm);
}
moltar commented 4 months ago

This also gets triggered by threshold: 0