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

[aws-cloudwatch] Missing ThresholdMetricId #10540

Open
alextriaca opened this issue Sep 25, 2020 · 16 comments · May be fixed by #31232
Open

[aws-cloudwatch] Missing ThresholdMetricId #10540

alextriaca opened this issue Sep 25, 2020 · 16 comments · May be fixed by #31232
Assignees
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/medium Medium work item – several days of effort in-progress This issue is being actively worked on. p1

Comments

@alextriaca
Copy link

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

  • CLI Version : 1.62.0
  • Framework Version: 1.64.0
  • Node.js Version: v12.14.1
  • OS : MacOS Catalina
  • Language (Version): Python 3.7

This is 🐛 Bug Report

@alextriaca alextriaca added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2020
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Sep 25, 2020
@rrhodes
Copy link
Contributor

rrhodes commented Sep 25, 2020

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

@alextriaca
Copy link
Author

Absolute legend! Thanks @rrhodes 😄

@rrhodes
Copy link
Contributor

rrhodes commented Sep 25, 2020

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

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Sep 25, 2020
@rix0rrr rix0rrr assigned rix0rrr and unassigned rix0rrr Oct 2, 2020
@rix0rrr rix0rrr added effort/medium Medium work item – several days of effort p1 labels Oct 2, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Nov 6, 2020
@NetaNir
Copy link
Contributor

NetaNir commented Nov 11, 2020

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
Copy link
Contributor

rrhodes commented Nov 11, 2020

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
Copy link
Contributor

any update so far?

@rix0rrr rix0rrr removed their assignment Jun 18, 2021
@jarruda
Copy link

jarruda commented Aug 16, 2021

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
Copy link

olaven commented Dec 1, 2021

👋 A status update on this would be highly appreciated!

brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 18, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 22, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 22, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 22, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Apr 29, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
@tuanardouin
Copy link

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
    });

brandondahler pushed a commit to brandondahler/aws-cdk that referenced this issue Aug 1, 2022
Allows customers to create Alarms with anomaly detection enabled.  Supports both direct metrics and math expressions.

Closes aws#10540
@alfaproject
Copy link

Is there any hope for this?

@arnulfojr
Copy link
Contributor

arnulfojr commented Jan 25, 2023

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
Copy link

Is this being worked upon?

@MichelangeloSorice
Copy link

Any update on this?

@shftlvch
Copy link

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

@mjgp2
Copy link

mjgp2 commented May 14, 2024

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
Copy link
Contributor

moltar commented Jul 3, 2024

This also gets triggered by threshold: 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment