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

Cannot create an Alarm based on a MathExpression which specifies a searchAccount or searchRegion #428

Open
Laxenade opened this issue Sep 14, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@Laxenade
Copy link
Contributor

Laxenade commented Sep 14, 2023

Version

5.10.0

Steps and/or minimal code example to reproduce

I have a few constructs that create DDB alarms, they used to be work with 5.4.2 but fails to build with 5.10.0, I am getting

Cannot create an Alarm based on a MathExpression which specifies a searchAccount or searchRegion

Here is my alarm config

addReadThrottledEventsCountAlarm: {
  Critical: {
    maxThrottledEventsThreshold: 0,
    datapointsToAlarm: 1,
    period: Duration.minutes(1),
    treatMissingDataOverride: TreatMissingData.NOT_BREACHING,
  }
}

Expected behavior

DDB alarms are created.

Actual behavior

Not able to get alarms created.

Other details

I think this is related to #415 where we added searchAccount and searchRegion to createMetricMath and ReadThrottledEventsCountAlarm and WriteThrottledEventsCountAlarm use math expression.

return this.metricFactory.createMetricMath(
"FILL(readThrottles,0)",
{ readThrottles },
ReadThrottleEventsLabel

The change impacts more than just DDB alarms I feel, it affects all alarm creations where math expression will be used

@Laxenade Laxenade added the bug Something isn't working label Sep 14, 2023
@Laxenade
Copy link
Contributor Author

Laxenade commented Sep 15, 2023

I think this also has something to do with different versions of aws-cdk-lib. My package is using 2.91, whereas cdk-monitoring-constructs is using 2.65. Cloudwatch might have introduced a compile time check between the two versions.

I think this is because of the global defaults. With #415, the account and region in the global defaults will be passed into MathExpression and I happened to have the global defaults defined.

@Laxenade
Copy link
Contributor Author

Laxenade commented Sep 15, 2023

Easiest fix that I could think of is just strip out account and region somewhere in

if the metric is a math expression. Though this might give the caller a false impression that MathExpression alarm can have custom searchAccount and searchRegion.


I have also tried to workaround the problem by providing a metricAdjuster to unset searchAccount and searchRegion, however, there is a short-circuiting logic in aws-cdk-lib that prevents me from unsetting them :(

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L652-L660

  public with(props: MathExpressionOptions): MathExpression {
    // Short-circuit creating a new object if there would be no effective change
    if ((props.label === undefined || props.label === this.label)
      && (props.color === undefined || props.color === this.color)
      && (props.period === undefined || props.period.toSeconds() === this.period.toSeconds())
      && (props.searchAccount === undefined || props.searchAccount === this.searchAccount)
      && (props.searchRegion === undefined || props.searchRegion === this.searchRegion)) {
      return this;
    }

    return new MathExpression({
      expression: this.expression,
      usingMetrics: this.usingMetrics,
      label: ifUndefined(props.label, this.label),
      color: ifUndefined(props.color, this.color),
      period: ifUndefined(props.period, this.period),
      searchAccount: ifUndefined(props.searchAccount, this.searchAccount),
      searchRegion: ifUndefined(props.searchRegion, this.searchRegion),
    });
  }

Something like the following will do for the time being

adjustMetric: (metric: MetricWithAlarmSupport, alarmScope: Construct, props: AddAlarmProps) => {
        if (metric.toMetricConfig().mathExpression) {
          return new MathExpression({...metric, searchAccount: undefined, searchRegion: undefined} as MathExpressionProps);
        } else {
          return metric;
        }
      }

@echeung-amzn
Copy link
Member

Yeah, #415 would be the cause of this and you should be right with the relation to global defaults being set.

We could potentially pass in the stack's info to MetricFactory and only use the global defaults for the searchRegion and searchAccount if they're actually different.

You'd still encounter the error if you try to alarm on those given you can't do search expressions cross-region/region anyway.


Where the error's thrown, for reference

@edisongustavo
Copy link
Contributor

edisongustavo commented Nov 7, 2023

Does this mean that I can't deploy a stack with alarms containing a MathExpression where the resources being monitored are in another account?

I started having this problem when monitoring my RDS cluster (monitorRdsCluster()). I specify the account at metricDefaults.account for this to work, but if I upgrade the cdk-monitoring-constructs library I get the error mentioned in this issue..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants