fix(MetricFactory): avoid specifying region/account if not different …

…from environment
cdklabs · Jul 31, 2024 · ac4c320 · ac4c320
1 parent 7084424
commit ac4c320
Show file tree

Hide file tree

Showing 10 changed files with 203 additions and 140 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,
+      ),
     });
   }
 
@@ -452,4 +462,38 @@ 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;
+  }
+
+  /**
+   * 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;
+  }
 }
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/test/common/metric/MetricFactory.test.ts b/test/common/metric/MetricFactory.test.ts
@@ -1,5 +1,5 @@
-import { Duration } from "aws-cdk-lib";
-import { Metric } from "aws-cdk-lib/aws-cloudwatch";
+import { Duration, Stack } from "aws-cdk-lib";
+import { Color, Metric } from "aws-cdk-lib/aws-cloudwatch";
 
 import {
   MetricFactory,
@@ -167,16 +167,26 @@ test("snapshot test: createMetricMath", () => {
 });
 
 test("snapshot test: toRate with detail", () => {
-  const metricFactory = new MetricFactory({
-    globalDefaults: {
-      namespace: "DummyNamespace",
+  const stack = new Stack();
+  const metricFactory = new MetricFactory(
+    {
+      globalDefaults: {
+        namespace: "DummyNamespace",
+      },
     },
-  });
+    stack,
+  );
 
   const metric = metricFactory.createMetric(
     "Metric",
     MetricStatistic.SUM,
     "Label",
+    undefined,
+    Color.ORANGE,
+    "Namespace",
+    undefined,
+    "eu-west-1",
+    "01234567890",
   );
 
   const metricAverage = metricFactory.toRate(
@@ -186,6 +196,13 @@ test("snapshot test: toRate with detail", () => {
   );
   expect(metricAverage).toMatchSnapshot();
 
+  const metricAverageWithAccount = metricFactory.toRate(
+    metric.with({ account: "1111111111" }),
+    RateComputationMethod.AVERAGE,
+    true,
+  );
+  expect(metricAverageWithAccount).toMatchSnapshot();
+
   const metricPerSecond = metricFactory.toRate(
     metric,
     RateComputationMethod.PER_SECOND,