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): Missing ids when creating math expressions #13942

Closed
zijuexiansheng opened this issue Apr 1, 2021 · 12 comments · Fixed by #19825
Closed

(aws-cloudwatch): Missing ids when creating math expressions #13942

zijuexiansheng opened this issue Apr 1, 2021 · 12 comments · Fixed by #19825
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

@zijuexiansheng
Copy link

There’s no way to assign an ID to a metric in CDK at this point. If you’re creating multiple math expressions within a single dashboard widget, the IDs for the math expressions are missing, and you’ll see that they have the same IDs in the CloudWatch Dashboard, and the metrics in the dashboard widget will be messed up.

Reproduction Steps

new cloudWatch.GraphWidget({
    left: [
        new MathExpression(...),
        new MathExpression(...),
        new MathExpression(...)
    ]
});

The generated CloudFormation file doesn't contain ID for MathExpression (see example below). After deployed, in the CloudWatch, we'll see that all the math expressions are assigned with the same ID, and the dashboard widget shows incorrect metrics

{ "label": "percentage", "expression": "m1 / m2", "region": "us-east-1" }

What did you expect to happen?

  • There should be a way to auto or manually set the ID for MathExpression
  • The above example should finally contain an id, something like
{ "label": "percentage", "expression": "m1 / m2", "region": "us-east-1", id: "e1" }

What actually happened?

The generated Math Expression widget doesn't have an id

{ "label": "percentage", "expression": "m1 / m2", "region": "us-east-1" }

Environment

  • CDK CLI Version : any
  • Framework Version: 1.95
  • Node.js Version: any
  • OS : any
  • Language (Version): TypeScript (but I think this issue is in all the languages)

Other


This is 🐛 Bug Report

@zijuexiansheng zijuexiansheng added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 1, 2021
@github-actions github-actions bot added the @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch label Apr 1, 2021
@YawenSun9
Copy link

+1, we are facing the same issue, and it's causing problem in showing 1-minute data for graphs containing MathExpression even we set period of MathExpression and all submetrics to 1 minute.

@NetaNir NetaNir removed their assignment Jun 17, 2021
@madeline-k
Copy link
Contributor

Hey @zijuexiansheng, thanks for opening this issue! I am working on repro-ing the issue before triaging it. Can you provide a minimal, example reproduction of the issue?

@madeline-k
Copy link
Contributor

Here is a code sample that repro's the issue:

import * as cdk from '@aws-cdk/core';
import * as cw from '@aws-cdk/aws-cloudwatch';

export class AppStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const dashboard = new cw.Dashboard(this, 'MyDashboard');

    const expr1 = new cw.MathExpression({
        expression: `metric1+metric2`,
        usingMetrics: {
            ['metric1']: new cw.Metric({
                metricName: 'metric',
                namespace: 'namespace',
                period: cdk.Duration.minutes(1),
                statistic: cw.Statistic.SUM,
                label: 'label1',
            }),
            ['metric2']: new cw.Metric({
                metricName: 'metric',
                namespace: 'namespace',
                period: cdk.Duration.minutes(1),
                statistic: cw.Statistic.SAMPLE_COUNT,
                label: 'label2',
            }),
        },
        label: 'Math expr label 1',
        period: cdk.Duration.minutes(1),
    });

    const expr2 = new cw.MathExpression({
        expression: `metric3+metric4`,
        usingMetrics: {
            ['metric3']: new cw.Metric({
                metricName: 'metric',
                namespace: 'namespace',
                period: cdk.Duration.minutes(1),
                statistic: cw.Statistic.SUM,
                label: 'testlabel3',
            }),
            ['metric4']: new cw.Metric({
                metricName: 'metric',
                namespace: 'namespace',
                period: cdk.Duration.minutes(1),
                statistic: cw.Statistic.SAMPLE_COUNT,
                label: 'testlabel4',
            }),
        },
        label: 'math expr label 2',
        period: cdk.Duration.minutes(1),
    });

    const testWidget1 = new cw.GraphWidget({
      title: 'testWidget',
      width: 12,
      left: [
          expr1,
          expr2,
      ],
      leftYAxis: {
          showUnits: true,
      },
    });

    dashboard.addWidgets(testWidget1);
  }
}

In the resulting cloudwatch dashboard, while each metric has an id, each Math Expression does not have an id in the dashboard body JSON. This results in both math expressions getting the default id 'm1' in the resulting widget.

I think the fix for this will need two things:

  1. Allow the user to specify the id for their Math Expressions.
  2. When the user doesn't supply ids for their Math Expressions, make sure that unique ids are assigned to each one.

@madeline-k madeline-k 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 Jun 21, 2021
@madeline-k
Copy link
Contributor

Assigning p2, which means we will not fix this right away but it is in the backlog, and we will get to it when we can.

Since this issue does not cause widgets with Math Expressions to render incorrectly, it just makes it impossible to reference a Math Expression within another Math Expression via its id.

@madeline-k
Copy link
Contributor

While we are working on a fix, if you want to be able to set the id of a Math Expression in a Cloudwatch dashboard, after creating the dashboard using Dashboard, then here are a couple of crazy workaround options.

Option 1
Use an escape hatch to get the underlying CfnDashboard.dashboardBody property and use string.replace() to modify it, then re-assign the new value to CfnDashboard.dashboardBody. This option requires you to only know the label of the Math Expression that you want to modify.

const cfnDashboard = dashboard.node.defaultChild as CfnDashboard;
const resolvedDashboardBody = this.resolve(cfnDashboard.dashboardBody);

let dashboardSubstring = resolvedDashboardBody['Fn::Join'][1][2].toString();
// Replace 'myMathExpressionLabel' with the label of the Math Expression whose ID you want to change
// Replace 'myIdOfChoice' with the ID that you want to use
dashboardSubstring = dashboardSubstring.replace('"label":"myMathExpressionLabel"','"label":"myMathExpressionLabel","id":"myIdOfChoice"');
resolvedDashboardBody['Fn::Join'][1][2] = dashboardSubstring;

cfnDashboard.dashboardBody = resolvedDashboardBody;

Option 2
Use an escape hatch to get the underlying CfnDashboard.dashboardBody property, turn it into a JSON object that can be manipulated, manipulate the JSON as needed, then re-assign the new value to CfnDashboard.dashboardBody. This option requires you to know where in the dashboard the Math Expression is - the index of the widget within the dashboard, and the index of the metric within that widget.

const cfnDashboard = dashboard.node.defaultChild as CfnDashboard;
const resolvedDashboardBody = this.resolve(cfnDashboard.dashboardBody);

const dashboardBodyAsJson = JSON.parse(resolvedDashboardBody['Fn::Join'][1][0] +  'Ref: AWS::Region' + resolvedDashboardBody['Fn::Join'][1][2]);
// Change the next two variables to the indeces for the widget and metric in the generated cloudwatch dashboard source that you want to change
const indexOfWidgetToChange = 0;
const indexOfMetricWithinWidgetToChange = 0;
// Assign the ID you want to use
dashboardBodyAsJson['widgets'][indexOfWidgetToChange]['properties']['metrics'][indexOfMetricWithinWidgetToChange][0]['id'] = 'myIdOfChoice';

cfnDashboard.dashboardBody = cdk.Fn.sub(JSON.stringify(dashboardBodyAsJson));

@cukierkowychleb
Copy link

+1 we are facing the same issue. Cannot render ANOMALY DETECTION properly because of it (using Java to generate graphs)

@adam-kiss-sg
Copy link

If any1 else is still facing this issue: I managed to hack around it in python, something similar could work in other languages as well:

helper class:

class MyMetric(aws_cloudwatch.Metric):
    def __init__(self, *args, **kwargs):
        super(MyMetric, self).__init__(*args, **kwargs)
    
    def addProperties(self, props):
        self.props = props
        return self
    
    def to_metric_config(self) -> aws_cloudwatch.MetricConfig:
        result = super(MyMetric, self).to_metric_config()
        if not hasattr(self, "props"):
            return result
        
        if "rendering_properties" in result._values:
            result._values["rendering_properties"].update(self.props)
        else:
            result._values["rendering_properties"] = self.props
        return result

Usage:
Replace:
aws_cloudwatch.Metric(namespace="AWS/EC2", metric_name="NetworkIn")
with:
MyMetric(namespace="AWS/EC2", metric_name="NetworkIn").addProperties({"id": "yourid", "visible": false})

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 8, 2022

Can someone explain their use case to me again? I'm not sure I understand the problem.

  • I can't distinguish the math expressions in a graph — don't they have a different label though?
  • I can't have a math expression in the graph and then reference it in a completely different math expression — that's correct, but that's on purpose. You can control the id if you need to reference it in a MathExpression by passing the metrics you are consuming in usingMetrics. If the metric is not being used in a mathexpression (signified by it not being in usingMetrics) then why would you need to control the ID?

Were you planning to write this:

const failing = new MathExpression({ expression: '...', label: 'Failing calls' });
const total = new MathExpression({ expression: '...', label: 'Total calls' });

const failPercentage = new MathExpression({
  expression: '(m1 / m2) * 100', // Rely on the fact that failing will render as m1 and total will render as m2
});

new cloudWatch.GraphWidget({
    left: [failing, total, failPercentage],
});

Because this would work:

const failing = new MathExpression({ expression: '...', label: 'Failing calls' });
const total = new MathExpression({ expression: '...', label: 'Total calls' });

const failPercentage = new MathExpression({
  expression: '(failing / total) * 100',
  usingMetrics: {
    failing: failing,
    total: total,
  },
});

new cloudWatch.GraphWidget({
    left: [failing, total, failPercentage],  // <-- % should probably be on the right but this is a demo
});

?

rix0rrr added a commit that referenced this issue Apr 8, 2022
It is intended that all metric identifiers referenced in a
MathExpression are included in the `usingMetrics` map. This
allows passing around complex metrics as a single object,
because the math expression object carries around its dependencies
with it.

This is slightly different than what people might be used
to from raw CloudWatch, where there is no hierarchy and all
metrics are supposed to be listed in the graph widget or alarm
with a unique ID, and then referenced by ID.

We can't make this contract obvious anymore by adding a hard
validation, but we can add warnings to hint people at the right
way to reference metrics in math expressions.

Fixes #13942, closes #17126.
@zijuexiansheng
Copy link
Author

@rix0rrr The generated dashboard widget for MathExpression doesn't have id. It has label if we set it. When there're multiple MathExpression without id, the CloudWatch will render the metrics in a weird way. Sometimes it renders correctly. Sometimes all lines are coincide together.

@mergify mergify bot closed this as completed in #19825 Apr 12, 2022
mergify bot pushed a commit that referenced this issue Apr 12, 2022
It is intended that all metric identifiers referenced in a
MathExpression are included in the `usingMetrics` map. This
allows passing around complex metrics as a single object,
because the math expression object carries around its dependencies
with it.

This is slightly different than what people might be used
to from raw CloudWatch, where there is no hierarchy and all
metrics are supposed to be listed in the graph widget or alarm
with a unique ID, and then referenced by ID.

We can't make this contract obvious anymore by adding a hard
validation, but we can add warnings to hint people at the right
way to reference metrics in math expressions.

Fixes #13942, closes #17126.


----

*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.

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this issue Apr 27, 2022
It is intended that all metric identifiers referenced in a
MathExpression are included in the `usingMetrics` map. This
allows passing around complex metrics as a single object,
because the math expression object carries around its dependencies
with it.

This is slightly different than what people might be used
to from raw CloudWatch, where there is no hierarchy and all
metrics are supposed to be listed in the graph widget or alarm
with a unique ID, and then referenced by ID.

We can't make this contract obvious anymore by adding a hard
validation, but we can add warnings to hint people at the right
way to reference metrics in math expressions.

Fixes aws#13942, closes aws#17126.


----

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

@madeline-k your workaround seem coherent with the aws docs 🙌https://docs.aws.amazon.com/cdk/v2/guide/cfn_layer.html but I am running into the problem where node is not a property defined for Dashboard. I am using cdkv2 and I do remember it working on cdk v1. Also the docs reference is for cdk v2 so I don't get why it won't work. SImilarly what is the class in which you are performing this operation in reference to what is this in the example here. Is the this referencing to a class extending Construct?

@gspetrou
Copy link

gspetrou commented May 7, 2024

I see this ticket is closed but Im not sure the underlying issue is resolved? Im looping over some list of resources, creating a math metric, and trying to overlay the metrics within one graph. Looks like thats not possible unless I change the label or expression for each, despite them using the same metric names?

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
9 participants