cdklabs / cdk-monitoring-constructs

Easy-to-use CDK constructs for monitoring your AWS infrastructure
https://constructs.dev/packages/cdk-monitoring-constructs
Apache License 2.0
468 stars 61 forks source link

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

Closed Laxenade closed 2 months ago

Laxenade commented 1 year ago

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.

https://github.com/cdklabs/cdk-monitoring-constructs/blob/b07f3615de87e160081834d4816a1617565a1e76/lib/monitoring/aws-dynamo/DynamoTableMetricFactory.ts#L138-L141

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

Laxenade commented 1 year ago

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 commented 1 year ago

Easiest fix that I could think of is just strip out account and region somewhere in https://github.com/cdklabs/cdk-monitoring-constructs/blob/b07f3615de87e160081834d4816a1617565a1e76/lib/common/alarm/AlarmFactory.ts#L496 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 commented 1 year ago

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 commented 11 months ago

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..