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

Attempting to create an Alarm on an INSIGHT_RULE_METRIC results in "Period must not be null" #7155

Closed
scubbo opened this issue Apr 3, 2020 · 8 comments · Fixed by #7644
Closed
Assignees
Labels
@aws-cdk/aws-cloudwatch Related to Amazon CloudWatch @aws-cdk/aws-logs Related to Amazon CloudWatch Logs bug This issue is a bug. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1

Comments

@scubbo
Copy link

scubbo commented Apr 3, 2020

Action: create a stack containing a Log Insights Rule, and an Alarm that uses the Insight Rule as a source
Expected: successful creation
Actual: Stack creation fails, with an ValidationError when trying to create the Alarm, with message "Period must not be null"

Reproduction Steps

import json
from aws_cdk.core import App, Stack, Duration, Construct
from aws_cdk.aws_logs import LogGroup
from aws_cdk.aws_cloudwatch import CfnInsightRule, Alarm, MathExpression


def create_log_group_insight_rule(
        scope: Construct,
        rule_name: str,
        aggregate_metric: str,
        log_group_names: list,
        keys: list = [],
        filters: list = []
):
    rule_body = json.dumps({
        "Schema": {
            "Name": "CloudWatchLogRule",
            "Version": 1
        },
        "LogGroupNames": log_group_names,
        "LogFormat": "JSON",
        "Contribution": {
            "Keys": keys,
            "Filters": filters
        },
        "AggregateOn": aggregate_metric
    })

    return CfnInsightRule(
        scope,
        rule_name,
        rule_body=rule_body,
        rule_name=rule_name,
        rule_state='ENABLED'
    )


app = App()
stack = Stack(app, 'MinimalReproOfPeriodBug')
log_group = LogGroup(stack, 'log_group')
insight_rule = create_log_group_insight_rule(
    stack,
    'rule_name',
    'Count',
    [log_group.log_group_name],
    ['$.customerId']
)
insight_rule.node.add_dependency(log_group)
Alarm(
    stack, "Alarm",
    alarm_name='alarm_name',
    datapoints_to_alarm=2,
    evaluation_periods=1,
    metric=MathExpression(
            expression='INSIGHT_RULE_METRIC("' + insight_rule.attr_rule_name + \
                       '", UniqueContributors")',
            using_metrics={},
            period=Duration.seconds(60)
        ),
    period=Duration.seconds(60),
    threshold=1)
app.synth()

cdk deploy the code above to reproduce.

Note that, according to the docs here, Alarm.period should not be used with MathExpressions - I tried it both with and without, same error both times.

Error Log

Period must not be null (Service: AmazonCloudWatch; Status Code: 400; Error Code: ValidationError; Request ID: <requestId>)

Environment

  • CLI Version : 1.31.0 (build 8f3ac79)
  • Framework Version: Unknown
  • OS : MacOS 10.14.6
  • Language : Python3

This is 🐛 Bug Report

@scubbo scubbo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 3, 2020
@scubbo
Copy link
Author

scubbo commented Apr 3, 2020

Similarly reproducible using CfnAlarm:

import json
from aws_cdk.core import App, Stack, Duration, Construct
from aws_cdk.aws_logs import LogGroup
from aws_cdk.aws_cloudwatch import CfnInsightRule, CfnAlarm


def create_log_group_insight_rule(
        scope: Construct,
        rule_name: str,
        aggregate_metric: str,
        log_group_names: list,
        keys: list = [],
        filters: list = []
):
    rule_body = json.dumps({
        "Schema": {
            "Name": "CloudWatchLogRule",
            "Version": 1
        },
        "LogGroupNames": log_group_names,
        "LogFormat": "JSON",
        "Contribution": {
            "Keys": keys,
            "Filters": filters
        },
        "AggregateOn": aggregate_metric
    })

    return CfnInsightRule(
        scope,
        rule_name,
        rule_body=rule_body,
        rule_name=rule_name,
        rule_state='ENABLED'
    )


app = App()
stack = Stack(app, 'MinimalReproOfPeriodBug')
log_group = LogGroup(stack, 'log_group')
insight_rule = create_log_group_insight_rule(
    stack,
    'rule_name',
    'Count',
    [log_group.log_group_name],
    ['$.customerId']
)
insight_rule.node.add_dependency(log_group)
CfnAlarm(
    stack, 'Alarm',
    alarm_name='alarm_name',
    comparison_operator='GreaterThanThreshold',
    evaluation_periods=1,
    metrics=[
        CfnAlarm.MetricDataQueryProperty(
            id='customerRate',
            expression='INSIGHT_RULE_METRIC("' + insight_rule.attr_rule_name + \
                       '", UniqueContributors")'
        )
    ],
    period=60,
    threshold=1
)
app.synth()

@SomayaB SomayaB added @aws-cdk/aws-logs Related to Amazon CloudWatch Logs @aws-cdk/aws-cloudwatch Related to Amazon CloudWatch labels Apr 3, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 6, 2020

Can you confirm it's even possible to use Insights in an Alarm? I'm can't find documentation on it and the console doesn't seem to let me create one.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 6, 2020

The docs seem to indicate that it should be possible, but they only give examples for dashboard graphs.

BTW, you shouldn't specify the top-level period argument to Alarm. Not that it matters, but it's exclusive with the Metrics array.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 6, 2020

Ah hah, here's the rub:

          {
            "Expression": {
              "Fn::Join": [
                "",
                [
                  "INSIGHT_RULE_METRIC(\"",
                  {
                    "Fn::GetAtt": [
                      "Rule",
                      "RuleName"
                    ]
                  },
                  "\", \"UniqueContributors\")"
                ]
              ]
            },
            "Period": 60,
            "Id": "expr_1"
          }

There is an undocumented Period parameter in the MetricDataQuery which is currently not in the CloudFormation schema.

We need to:

  • File a bug report with CloudFormation
  • Add the property to our own copy of the schema temporarily.
  • Emit the Period parameter whenever necessary from a MathExpression. It is at least necessary when using a MathExpression that itself doesn't have any submetrics, but maybe it doesn't hurt to always emit it? We need to investigate.

This seems to be a bigger ticket that can't be addressed quickly.

@rix0rrr rix0rrr added needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1 labels Apr 6, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 6, 2020

Created issue for CFN here: aws-cloudformation/cloudformation-coverage-roadmap#436

@JasontheMonster
Copy link

Any update on the timeline to support this?

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Apr 14, 2020
rix0rrr added a commit that referenced this issue Apr 28, 2020
`MathExpression`s without submetrics will end up without a `period`,
which is not allowed.

Add a `period` field to the schema (it's not in the upstream schema
yet), and render it out when submetrics are missing.

Fixes #7155.
@mergify mergify bot closed this as completed in #7644 Apr 30, 2020
mergify bot pushed a commit that referenced this issue Apr 30, 2020
`MathExpression`s without submetrics (like for example, `INSIGHT_RULE_METRIC`) will end up without a `period`, which is not allowed.

Add a `period` field to the schema (it's not in the upstream schema
yet), and render it out when submetrics are missing.

Fixes #7155.
@scubbo
Copy link
Author

scubbo commented May 4, 2020

I'm still getting this error. How can I track which release of CDK will contain this fix?

@mihirthatte
Copy link

mihirthatte commented May 20, 2020

@scubbo You may add Period property to MetricDataQueryProperty using addPropertyOverride

// First create an empty CfnAlarm
const myAlarm = new CfnAlarm(this, 'myAlarm', {
  alarmName: 'myAlarm',
  comparisonOperator: "GreaterThanThreshold",
  evaluationPeriods: 5,
  datapointsToAlarm: 5,
  threshold: 100
});

// Then add periodOverride with expression and period fields to myalarm
myAlarm.addPropertyOverride('Metrics', [{
  Expression: "INSIGHT_RULE_METRIC(<rule-name>', 'SampleCount')",
  Id: "e1",
  Period: 300
}]);

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 @aws-cdk/aws-logs Related to Amazon CloudWatch Logs bug This issue is a bug. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants