fix: apply account/region to more metrics (#544)

Addressing a few misses from #533. Also updates the test in `MonitoringFacade.test.ts` since XAXR alarms aren't possible. Fixes #434, but only in cases where we can infer that the account/region is actually different from the construct scope to prevent unnecessary XAXR alarm errors. Fixes #428, in cases where the account/region is actually different from the construct scope. --- _By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license_
cdklabs · Aug 1, 2024 · 704d864 · 704d864
1 parent 7084424
commit 704d864
Show file tree

Hide file tree

Showing 22 changed files with 367 additions and 187 deletions.
diff --git a/API.md b/API.md
diff --git a/lib/common/metric/MetricFactory.ts b/lib/common/metric/MetricFactory.ts
@@ -1,10 +1,11 @@
-import { Duration } from "aws-cdk-lib";
+import { Duration, Stack } from "aws-cdk-lib";
 import {
   DimensionsMap,
   IMetric,
   MathExpression,
   Metric,
 } from "aws-cdk-lib/aws-cloudwatch";
+import { Construct } from "constructs";
 
 import { AnomalyDetectionMathExpression } from "./AnomalyDetectionMathExpression";
 import { BaseMetricFactoryProps } from "./BaseMetricFactory";
@@ -42,9 +43,12 @@ export interface MetricFactoryProps {
 
 export class MetricFactory {
   protected readonly globalDefaults: MetricFactoryDefaults;
+  protected readonly scope: Construct | undefined;
 
-  constructor(props?: MetricFactoryProps) {
+  // TODO: make scope required and first. This is for backwards compatability for now.
+  constructor(props?: MetricFactoryProps, scope?: Construct) {
     this.globalDefaults = props?.globalDefaults ?? {};
+    this.scope = scope;
   }
 
   /**
@@ -81,8 +85,8 @@ export class MetricFactory {
         : undefined,
       namespace: this.getNamespaceWithFallback(namespace),
       period: period ?? this.globalDefaults.period ?? DefaultMetricPeriod,
-      region: region ?? this.globalDefaults.region,
-      account: account ?? this.globalDefaults.account,
+      region: this.resolveRegion(region ?? this.globalDefaults.region),
+      account: this.resolveAccount(account ?? this.globalDefaults.account),
     });
   }
 
@@ -112,8 +116,10 @@ export class MetricFactory {
       expression,
       usingMetrics,
       period: period ?? this.globalDefaults.period ?? DefaultMetricPeriod,
-      searchRegion: region ?? this.globalDefaults.region,
-      searchAccount: account ?? this.globalDefaults.account,
+      searchRegion: this.resolveRegion(region ?? this.globalDefaults.region),
+      searchAccount: this.resolveAccount(
+        account ?? this.globalDefaults.account,
+      ),
     });
   }
 
@@ -165,8 +171,10 @@ export class MetricFactory {
       // cannot be an empty string and undefined is no good either
       label: label ?? " ",
       period: finalPeriod,
-      searchRegion: region ?? this.globalDefaults.region,
-      searchAccount: account ?? this.globalDefaults.account,
+      searchRegion: this.resolveRegion(region ?? this.globalDefaults.region),
+      searchAccount: this.resolveAccount(
+        account ?? this.globalDefaults.account,
+      ),
     });
   }
 
@@ -205,8 +213,10 @@ export class MetricFactory {
       usingMetrics,
       expression: `ANOMALY_DETECTION_BAND(${finalExpressionId},${stdev})`,
       period: period ?? this.globalDefaults.period ?? DefaultMetricPeriod,
-      searchRegion: region ?? this.globalDefaults.region,
-      searchAccount: account ?? this.globalDefaults.account,
+      searchRegion: this.resolveRegion(region ?? this.globalDefaults.region),
+      searchAccount: this.resolveAccount(
+        account ?? this.globalDefaults.account,
+      ),
     });
   }
 
@@ -263,6 +273,8 @@ export class MetricFactory {
         label,
         metric.color,
         metric.period,
+        this.getRegion(metric),
+        this.getAccount(metric),
       );
     }
   }
@@ -296,6 +308,8 @@ export class MetricFactory {
         label,
         metric.color,
         metric.period,
+        this.getRegion(metric),
+        this.getAccount(metric),
       );
     }
   }
@@ -351,6 +365,8 @@ export class MetricFactory {
             avgLabel,
             avgMetric.color,
             avgMetric.period,
+            this.getRegion(avgMetric),
+            this.getAccount(avgMetric),
           );
         }
         return avgMetric;
@@ -370,6 +386,8 @@ export class MetricFactory {
           perSecondLabel,
           metric.color,
           metric.period,
+          this.getRegion(metric),
+          this.getAccount(metric),
         );
       case RateComputationMethod.PER_MINUTE:
         return this.createMetricMath(
@@ -378,6 +396,8 @@ export class MetricFactory {
           `${labelPrefix}/m${labelAppendix}`,
           metric.color,
           metric.period,
+          this.getRegion(metric),
+          this.getAccount(metric),
         );
       case RateComputationMethod.PER_HOUR:
         return this.createMetricMath(
@@ -386,6 +406,8 @@ export class MetricFactory {
           `${labelPrefix}/h${labelAppendix}`,
           metric.color,
           metric.period,
+          this.getRegion(metric),
+          this.getAccount(metric),
         );
       case RateComputationMethod.PER_DAY:
         return this.createMetricMath(
@@ -394,6 +416,8 @@ export class MetricFactory {
           `${labelPrefix}/d${labelAppendix}`,
           metric.color,
           metric.period,
+          this.getRegion(metric),
+          this.getAccount(metric),
         );
     }
   }
@@ -452,4 +476,58 @@ export class MetricFactory {
 
     return copy;
   }
+
+  /**
+   * Attempts to get the account from the metric if it differs from the scope.
+   */
+  private resolveAccount(
+    metricAccount: string | undefined,
+  ): string | undefined {
+    if (!this.scope) {
+      return metricAccount;
+    }
+
+    const { account } = Stack.of(this.scope);
+    if (metricAccount !== account) {
+      return metricAccount;
+    }
+
+    return;
+  }
+  private getAccount(metric: MetricWithAlarmSupport): string | undefined {
+    let metricAccount: string | undefined;
+    if (metric instanceof MathExpression) {
+      metricAccount = metric.searchAccount;
+    } else {
+      metricAccount = metric.account;
+    }
+
+    return this.resolveAccount(metricAccount);
+  }
+
+  /**
+   * Attempts to get the region from the metric if it differs from the scope.
+   */
+  private resolveRegion(metricRegion: string | undefined): string | undefined {
+    if (!this.scope) {
+      return metricRegion;
+    }
+
+    const { region } = Stack.of(this.scope);
+    if (metricRegion !== region) {
+      return metricRegion;
+    }
+
+    return;
+  }
+  private getRegion(metric: MetricWithAlarmSupport): string | undefined {
+    let metricRegion: string | undefined;
+    if (metric instanceof MathExpression) {
+      metricRegion = metric.searchRegion;
+    } else {
+      metricRegion = metric.region;
+    }
+
+    return this.resolveRegion(metricRegion);
+  }
 }
diff --git a/lib/facade/MonitoringFacade.ts b/lib/facade/MonitoringFacade.ts
@@ -202,7 +202,12 @@ export class MonitoringFacade extends MonitoringScope {
   }
 
   createMetricFactory(): MetricFactory {
-    return new MetricFactory({ globalDefaults: this.metricFactoryDefaults });
+    return new MetricFactory(
+      {
+        globalDefaults: this.metricFactoryDefaults,
+      },
+      this,
+    );
   }
 
   createWidgetFactory(): IWidgetFactory {

diff --git a/lib/monitoring/aws-dynamo/DynamoTableGlobalSecondaryIndexMetricFactory.ts b/lib/monitoring/aws-dynamo/DynamoTableGlobalSecondaryIndexMetricFactory.ts
@@ -68,6 +68,8 @@ export class DynamoTableGlobalSecondaryIndexMetricFactory extends BaseMetricFact
         consumed_rcu_sum: this.table.metricConsumedReadCapacityUnits({
           statistic: MetricStatistic.SUM,
           dimensionsMap: this.dimensionsMap,
+          region: this.region,
+          account: this.account,
         }),
       },
       "Consumed",
@@ -81,6 +83,8 @@ export class DynamoTableGlobalSecondaryIndexMetricFactory extends BaseMetricFact
         consumed_wcu_sum: this.table.metricConsumedWriteCapacityUnits({
           statistic: MetricStatistic.SUM,
           dimensionsMap: this.dimensionsMap,
+          region: this.region,
+          account: this.account,
         }),
       },
       "Consumed",

diff --git a/lib/monitoring/aws-dynamo/DynamoTableMetricFactory.ts b/lib/monitoring/aws-dynamo/DynamoTableMetricFactory.ts
@@ -72,6 +72,10 @@ export class DynamoTableMetricFactory extends BaseMetricFactory<DynamoTableMetri
         }),
       },
       ConsumedLabel,
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -86,6 +90,10 @@ export class DynamoTableMetricFactory extends BaseMetricFactory<DynamoTableMetri
         }),
       },
       ConsumedLabel,
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -97,6 +105,10 @@ export class DynamoTableMetricFactory extends BaseMetricFactory<DynamoTableMetri
         provisioned_read_cap: this.metricProvisionedReadCapacityUnits(),
       },
       "Utilization",
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -108,6 +120,10 @@ export class DynamoTableMetricFactory extends BaseMetricFactory<DynamoTableMetri
         provisioned_write_cap: this.metricProvisionedWriteCapacityUnits(),
       },
       "Utilization",
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -121,6 +137,10 @@ export class DynamoTableMetricFactory extends BaseMetricFactory<DynamoTableMetri
       },
       MetricStatistic.AVERAGE,
       DynamoDbNamespace,
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 

diff --git a/lib/monitoring/aws-kinesis/KinesisFirehoseMetricFactory.ts b/lib/monitoring/aws-kinesis/KinesisFirehoseMetricFactory.ts
@@ -151,6 +151,10 @@ export class KinesisFirehoseMetricFactory extends BaseMetricFactory<KinesisFireh
         bytes_max: this.metricBytesPerSecondLimit(),
       },
       "Incoming Bytes / Limit",
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -162,6 +166,10 @@ export class KinesisFirehoseMetricFactory extends BaseMetricFactory<KinesisFireh
         records_max: this.metricRecordsPerSecondLimit(),
       },
       "Incoming Records / Limit",
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 
@@ -173,6 +181,10 @@ export class KinesisFirehoseMetricFactory extends BaseMetricFactory<KinesisFireh
         requests_max: this.metricPutRequestsPerSecondLimit(),
       },
       "Incoming PutRequests / Limit",
+      undefined,
+      undefined,
+      this.region,
+      this.account,
     );
   }
 

diff --git a/lib/monitoring/aws-lambda/LambdaFunctionEnhancedMetricFactory.ts b/lib/monitoring/aws-lambda/LambdaFunctionEnhancedMetricFactory.ts
@@ -89,6 +89,8 @@ export class LambdaFunctionEnhancedMetricFactory extends BaseMetricFactory<Lambd
     return this.metricFactory.adaptMetric(
       this.lambdaFunction.metricDuration({
         statistic: MetricStatistic.SUM,
+        region: this.region,
+        account: this.account,
       }),
     );
   }

diff --git a/lib/monitoring/aws-lambda/LambdaFunctionMetricFactory.ts b/lib/monitoring/aws-lambda/LambdaFunctionMetricFactory.ts
@@ -77,6 +77,8 @@ export class LambdaFunctionMetricFactory extends BaseMetricFactory<LambdaFunctio
     return this.metricFactory.adaptMetric(
       this.lambdaFunction.metricInvocations({
         label: "Invocations",
+        region: this.region,
+        account: this.account,
       }),
     );
   }