aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.72k stars 3.94k forks source link

cloudwatch: Explicitly set region and accountId fields are removed from metrics when they match the stack #28731

Open TrevorBurnham opened 10 months ago

TrevorBurnham commented 10 months ago

Describe the bug

In my dashboard created from the CDK, I noticed that the region property was missing from metrics when that metric matched the stack. For example, for a stack in us-east-1, the metrics would look like this:

[
  {
    "label": "My metric [us-west-2]",
    "region": "us-west-2",
    "period": 60,
    "stat": "Maximum"
  },
  {
    "label": "My metric [us-east-1]",
    "period": 60,
    "stat": "Maximum"
  },
  // ...
]

This broke some region-based filtering logic in my UI application.

Expected Behavior

I expected the region field to exist for all of my metrics, since I was setting it explicitly:

new Metric({
  label: `My metric [${region}]`,
  region: region,
  period: 60,
  stat: 'Maximum'
})

Current Behavior

During serialization, the region and accountId fields are removed if they match the corresponding region and account values on the stack.

Reproduction Steps

You can confirm the behavior by adding this unit test to cross-environment.test.ts:

test('metric with explicit account and region will render as-is in stack with same region and account', () => {
  // GIVEN
  const graph = new GraphWidget({
    left: [
      a.with({ account: '1234', region: 'us-north-5' }),
    ],
  });

  // THEN
  // Assertion fails. It would pass if the stack had different region and account values.
  graphMetricsAre(new Stack(undefined, undefined, { env: { region: 'us-north-5', account: '1234' } }), graph, [
    ['Test', 'ACount', { accountId: '1234', region: 'us-north-5' }],
  ]);
});

Possible Solution

The problem arises from these two lines in the metricGraphJson function: https://github.com/aws/aws-cdk/blob/109b2abe4c713624e731afa1b82c3c1a3ba064c9/packages/aws-cdk-lib/aws-cloudwatch/lib/private/rendering.ts#L50-L51

I'm not sure, but I think the intent here is to prevent the region and account from being included in the output if they were set implicitly by the attachTo function. As that function's description says:

If the metric is subsequently used in a Dashboard or Alarm in a different Stack defined in a different account or region, the appropriate 'region' and 'account' fields will be added to it.

I'm not sure why it's so important to prevent the region and account fields from being included when they match the stack, but if it is, you could have attachTo attach a placeholder that metricGraphJson can process appropriately (e.g. "${ifDifferentFromStack(us-east-1)}") rather than attaching a value that's indistinguishable from one that's been explicitly set.

Additional Information/Context

No response

CDK CLI Version

2.121.1

Framework Version

No response

Node.js Version

18.15.0

OS

macOS 13.6.3

Language

TypeScript

Language Version

No response

Other information

Possibly related to https://github.com/aws/aws-cdk/issues/18951?

pahud commented 10 months ago

Looks like all conditions here have to be satisfied

https://github.com/aws/aws-cdk/blob/20ad55e7aec7d387550db865257dc6f8ebcab067/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L339-L349

Otherwise a new Matric will be created and region would be undefined here: https://github.com/aws/aws-cdk/blob/20ad55e7aec7d387550db865257dc6f8ebcab067/packages/aws-cdk-lib/aws-cloudwatch/lib/metric.ts#L363

IMO, yes, if you specify the region explicitly, it should be passed and defined explicitly.

I am making it a p2 but we welcome and appreciate any pull requests.

TrevorBurnham commented 7 months ago

@pahud I've submitted a pull request to fix this: https://github.com/aws/aws-cdk/pull/29935

TrevorBurnham commented 5 days ago

@pahud I've made another attempt to fix this: https://github.com/aws/aws-cdk/pull/32325

AlbyM-dev commented 19 hours ago

I am in the same situation, when the 2 metrics are rendered, the second metric doesn't have a region specified and so the region for the above metric is used, breaking the second metric

image

this is the same graph generated manually without CDK:

image

I guess that happens because when a metric doesn't have a specific region and the one above has one, the metric inherits that region and not the stack region, so the logic around how cdk hides the region if not required (because it's in the environment) doesn't really follow how cloudwatch dashboards work.