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

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

Open
TrevorBurnham opened this issue Jan 16, 2024 · 2 comments · May be fixed by #29935
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@TrevorBurnham
Copy link

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:

if (stat.account) { options.accountId = accountIfDifferentFromStack(stat.account); }
if (stat.region) { options.region = regionIfDifferentFromStack(stat.region); }

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 #18951?

@TrevorBurnham TrevorBurnham added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 16, 2024
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Jan 16, 2024
@pahud
Copy link
Contributor

pahud commented Jan 19, 2024

Looks like all conditions here have to be satisfied

if ((props.label === undefined || props.label === this.label)
&& (props.color === undefined || props.color === this.color)
&& (props.statistic === undefined || props.statistic === this.statistic)
&& (props.unit === undefined || props.unit === this.unit)
&& (props.account === undefined || props.account === this.account)
&& (props.region === undefined || props.region === this.region)
// For these we're not going to do deep equality, misses some opportunity for optimization
// but that's okay.
&& (props.dimensions === undefined)
&& (props.dimensionsMap === undefined)
&& (props.period === undefined || props.period.toSeconds() === this.period.toSeconds())) {

Otherwise a new Matric will be created and region would be undefined here:

region: ifUndefined(props.region, this.region),

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.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jan 19, 2024
TrevorBurnham pushed a commit to TrevorBurnham/aws-cdk that referenced this issue Apr 23, 2024
TrevorBurnham pushed a commit to TrevorBurnham/aws-cdk that referenced this issue Apr 23, 2024
@TrevorBurnham
Copy link
Author

@pahud I've submitted a pull request to fix this: #29935

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants