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

[aws-cloudwatch] Support region and account in MathExpression #9039

Closed
1 of 2 tasks
arnulfojr opened this issue Jul 13, 2020 · 5 comments · Fixed by #16539
Closed
1 of 2 tasks

[aws-cloudwatch] Support region and account in MathExpression #9039

arnulfojr opened this issue Jul 13, 2020 · 5 comments · Fixed by #16539
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1

Comments

@arnulfojr
Copy link
Contributor

When trying to specify a cross region/cross account math expression the MathExpression requires also reference to the region and account.

Use Case

Creating cross account and cross region math expressions.

Proposed Solution

The Metric class already supports specifying region and accountId but the MathExpression metric doesnt, we could simply just add it

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@arnulfojr arnulfojr added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 13, 2020
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Jul 13, 2020
@arnulfojr
Copy link
Contributor Author

arnulfojr commented Jul 13, 2020

Is there a reason why when generating the metrics graph JSON the check regionIfDifferentFromStack and accountIfDifferentFromStack is made?

https:/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-cloudwatch/lib/private/env-tokens.ts#L3,L15

this seems to be breaking xaxr stacks e.g.,

  • metric A with data from [account x, region f]
  • metric B with data from [account y, region g]
  • Stack in [account z, region g]

regionIfDifferentFromStack will not render a region (g) for the metric B and the graph will not work.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jul 13, 2020
@SomayaB SomayaB assigned ericzbeard and unassigned rix0rrr Jul 13, 2020
@ericzbeard ericzbeard added needs-discussion This issue/PR requires more discussion with community. and removed needs-discussion This issue/PR requires more discussion with community. needs-triage This issue or PR still needs to be triaged. labels Jul 14, 2020
@ericzbeard ericzbeard assigned rix0rrr and unassigned ericzbeard Jul 15, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 4, 2020

I would have expected account and region to be independent. Are you saying they must both be present or both be absent?

Otherwise I don't really see how:

{ metric: B, account: Y }

Will fail to locate the metric when deployed into (account Z, region G).

@rix0rrr rix0rrr added effort/medium Medium work item – several days of effort p1 labels Aug 4, 2020
@priyajeet
Copy link
Member

Hi, is this still being worked on?

@CDillinger
Copy link

CDillinger commented May 27, 2021

I created a wrapper class as a workaround for my use case (using CDK version 1.97.0). Hoping this can help anyone else who is blocked.

import * as cloudwatch from 'monocdk/aws-cloudwatch';

export interface XaxrMathExpressionProps extends cloudwatch.MathExpressionProps {
    /**
     * (experimental) Account which this metric comes from.
     *
     * @default - Deployment account.
     * @experimental
     */
    readonly account?: string;
    /**
     * (experimental) Region which this metric comes from.
     *
     * @default - Deployment region.
     * @experimental
     */
    readonly region?: string;
}

export class XaxrMathExpression implements cloudwatch.IMetric {
    private mathExpression: cloudwatch.MathExpression;

    constructor(private readonly props: XaxrMathExpressionProps) {
        this.mathExpression = new cloudwatch.MathExpression(props);
    }

    toAlarmConfig(): cdk.aws_cloudwatch.MetricAlarmConfig {
        throw new Error('Method not implemented.');
    }
    toGraphConfig(): cdk.aws_cloudwatch.MetricGraphConfig {
        throw new Error('Method not implemented.');
    }

    public with(options: cloudwatch.MathExpressionOptions): cloudwatch.IMetric {
        return new XaxrMathExpression({
            ...this.props,
            ...options
        });
    }

    public toMetricConfig(): cloudwatch.MetricConfig {
        const defaultMetricConfig = this.mathExpression.toMetricConfig();
        return {
            ...defaultMetricConfig,
            renderingProperties: {
                ...defaultMetricConfig.renderingProperties,
                accountId: this.props.account,
                region: this.props.region
            }
        }
    }
}

@rix0rrr rix0rrr removed their assignment Jun 18, 2021
LeahHirst pushed a commit to LeahHirst/aws-cdk that referenced this issue Sep 18, 2021
…ironment math expressions (aws#9039)

Fixes: aws#9039

Currently CDK gives no way to specify account or region on MathExpressions which means it is not possible to run SEARCH or SERVICE_QUOTA expressions against environments other than the deployment environment.

The CloudWatch console currently has this capability, however the documentation still states that accountId or region should only be used on metric widgets: https://docs.aws.amazon.com/en_us/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Metric-Widget-Object . I've left feedback to ask whether this documentation is out of date.

This change adds in these properties which can be optionally specified for Dashboards. For the Alarm use case, this change will throw an error if an account or region is provided and it does not match the deployment account/region.
@mergify mergify bot closed this as completed in #16539 Sep 27, 2021
mergify bot pushed a commit that referenced this issue Sep 27, 2021
Fixes #9039 (also some prior discussion on #9034)

Currently CDK gives no way to specify account or region on MathExpressions which means it is not possible to run search expressions against environments other than the deployment environment.

The CloudWatch console currently has this capability, however [the documentation](https://docs.aws.amazon.com/en_us/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Metric-Widget-Object) still states that `accountId` or `region` should only be used on metric widgets. I've left feedback to ask whether this documentation is out of date or whether it is an undocumented feature.

This change adds in the `searchAccount` and `searchRegion` property which can be optionally specified for the Dashboards use case. When a MathExpression with a searchAccount or searchRegion set is attempted to be used within an Alarm, the change throws an error as Alarms do not support search expressions.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…6539)

Fixes aws#9039 (also some prior discussion on aws#9034)

Currently CDK gives no way to specify account or region on MathExpressions which means it is not possible to run search expressions against environments other than the deployment environment.

The CloudWatch console currently has this capability, however [the documentation](https://docs.aws.amazon.com/en_us/AmazonCloudWatch/latest/APIReference/CloudWatch-Dashboard-Body-Structure.html#CloudWatch-Dashboard-Properties-Metric-Widget-Object) still states that `accountId` or `region` should only be used on metric widgets. I've left feedback to ask whether this documentation is out of date or whether it is an undocumented feature.

This change adds in the `searchAccount` and `searchRegion` property which can be optionally specified for the Dashboards use case. When a MathExpression with a searchAccount or searchRegion set is attempted to be used within an Alarm, the change throws an error as Alarms do not support search expressions.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p1
Projects
None yet
6 participants