(Cloudwatch): Metrics with labels can't be used in alarms for API Gateway metrics

[juweeks]

Just getting this with v1.92 (worked fine <=1.91)
updating Cloudwatch alarm yields: Invalid metrics list (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError

Reproduction Steps

alarm = metric.create_alarm(
            self,
            id=id_alarm,
            alarm_description=kwargs['alarm_description'],
            threshold=kwargs['threshold'],
            period=core.Duration.minutes(kwargs['period_in_minutes']),
            comparison_operator=cw.ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD,
            evaluation_periods=kwargs['evaluation_periods'],
            statistic=kwargs['statistic']
        )

these are the parameters I'm passing to it:

{
        "metric_name": "5XXError",
        "label": "HTTP 5XX",
        "statistic": "sum",
        "color": "#d62728",
        "period_in_minutes": 1,
        "alarm": {
          "alarm_description": "5XX at 1 threshold",
          "threshold": 1,
          "period_in_minutes": 5,
          "evaluation_periods": 1,
          "statistic": "sum"
        }
}

What did you expect to happen?

a Cloudwatch alarm resource created/updated

What actually happened?

UPDATE_FAILED        | AWS::CloudWatch::Alarm     |  
Invalid metrics list (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError;

Environment

• CDK CLI Version : 1.92
• Framework Version: 1.92
• Node.js Version: 10
• OS : amazonlinux2-x86_64-standard:1.0
• Language (Version): Python (3.7)

Other

---

This is 🐛 Bug Report

[juweeks]

[robo-sk]

I believe there is an issue introduced by recent bug fix in this file:

packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts

commit: cbcc712

(if there is a label, then the ID is created from the metric name, but there is a limitation for ID : "The first character must be a lowercase letter" ... in this case, the metric name "5xxError" doesn't fit...
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudwatch-alarm-metricdataquery.html

Quick fix is to remove the label for now.

Take a look here what I believe is the root cause of the problem:

cbcc712#r47990368

[juweeks]

that temp fix worked perfectly. thank you so much! you're a lifesaver. keep on rockin' it !!!

[rix0rrr]

@robo-sk great catch! Since the ID doesn't matter too much here (at least, I don't think... hope I'm not wrong), we could just prefix the id with something like a lowercase m.

Feel like submitting a PR for this? :)

[BLasan]

@robo-sk great catch! Since the ID doesn't matter too much here (at least, I don't think... hope I'm not wrong), we could just prefix the id with something like a lowercase m.

Feel like submitting a PR for this? :)

Can I work on this?

[robo-sk]

@robo-sk great catch! Since the ID doesn't matter too much here (at least, I don't think... hope I'm not wrong), we could just prefix the id with something like a lowercase m.
Feel like submitting a PR for this? :)

Can I work on this?

Sure, go ahead...

[ksoeder]

Any idea on a timeline for a fix for this?

[BLasan]

Any idea on a timeline for a fix for this?

Will be starting tomorrow

[robo-sk]

I'm just re-evaluating the idea of "simple prefixing of metric name" ... Isn't there a general/root issue, that we are trying to compose the ID from metric name?

There is another requirement for ID: "A short name used to tie this object to the results in the response. This name must be unique within a single call to GetMetricData."

Metrics property on alarm is an array that enables you to create an alarm based on the result of a metric math expression.
And using a metric name for an ID can* create multiple metrics with the same ID and impossible to use it e.g. in math expressions or in ThresholdMetricId property.

*can -> is there a real-world example for same metric names but different dimension or stat in single alarm ?

@rix0rrr what do you think?
Or anyone else, any idea whether we are just fine with simple prefix... ?

I hope I'm wrong with this reevaluation and simple prefixing is a perfect fix ;-)

[robo-sk]

I'm just re-evaluating the idea of "simple prefixing of metric name" ... Isn't there a general/root issue, that we are trying to compose the ID from metric name?

There is another requirement for ID: "A short name used to tie this object to the results in the response. This name must be unique within a single call to GetMetricData."

Metrics property on alarm is an array that enables you to create an alarm based on the result of a metric math expression.
And using a metric name for an ID can* create multiple metrics with the same ID and impossible to use it e.g. in math expressions or in ThresholdMetricId property.

*can -> is there a real-world example for same metric names but different dimension or stat in single alarm ?

@rix0rrr what do you think?
Or anyone else, any idea whether we are just fine with simple prefix... ?

I hope I'm wrong with this reevaluation and simple prefixing is a perfect fix ;-)

Sorry for bothering... next look discovered that all should be fine, it's allowed only single metric per alarm in CDK implementation.

[BLasan]

I'm just re-evaluating the idea of "simple prefixing of metric name" ... Isn't there a general/root issue, that we are trying to compose the ID from metric name?

There is another requirement for ID: "A short name used to tie this object to the results in the response. This name must be unique within a single call to GetMetricData."

Metrics property on alarm is an array that enables you to create an alarm based on the result of a metric math expression.
And using a metric name for an ID can* create multiple metrics with the same ID and impossible to use it e.g. in math expressions or in ThresholdMetricId property.

*can -> is there a real-world example for same metric names but different dimension or stat in single alarm ?

@rix0rrr what do you think?
Or anyone else, any idea whether we are just fine with simple prefix... ?

I hope I'm wrong with this reevaluation and simple prefixing is a perfect fix ;-)

Can you put the link you referred?

[robo-sk]

It's just all about the CloudFormation specification and how the cdk creates underlying resources.

ID property: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudwatch-alarm-metricdataquery.html#cfn-cloudwatch-alarm-metricdataquery-id

Metrics property on alarm: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-alarm.html#cfn-cloudwatch-alarm-metrics

[BLasan]

It's just all about the CloudFormation specification and how the cdk creates underlying resources.

ID property: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudwatch-alarm-metricdataquery.html#cfn-cloudwatch-alarm-metricdataquery-id

Metrics property on alarm: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-alarm.html#cfn-cloudwatch-alarm-metrics

Will check on these. Thanks :)

[BLasan]

@robo-sk Just a suggestion! In order to make the Id value unique, can we append the timestamp it gets created? If the label name is a string, then we could make the first character to lowercase and if that label starts with a number, then we could append m_ as the prefix (like I've done in the PR). What're your thoughts on this ? @rix0rrr