Add math metric alarm

[mike-zorn]

Fixes #6551

Changes proposed in this pull request:

• added metric_query to support adding MetricDataQuerys to alarms via the Metrics property. An example follows.

resource "aws_cloudwatch_metric_alarm" "foobar" {
  alarm_name                = "terraform-test-foobar%d"
  comparison_operator       = "GreaterThanOrEqualToThreshold"
  evaluation_periods        = "2"
	threshold                 = "80"
  alarm_description         = "This metric monitors ec2 cpu utilization"
  insufficient_data_actions = []
	metric_query {
		id = "e1"
		expression = "m1"
		label = "cat"
		return_data = "true"
	}
	metric_query {
		id = "m1"
		metric {
			metric_name = "CPUUtilization"
			namespace   = "AWS/EC2"
			period      = "120"
			stat        = "Average"
			unit        = "Count"
			dimensions {
				InstanceId = "i-abc123"
			}
		}
	}
}

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCloudWatchMetricAlarm_expression'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCloudWatchMetricAlarm_expression -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSCloudWatchMetricAlarm_expression
=== PAUSE TestAccAWSCloudWatchMetricAlarm_expression
=== CONT  TestAccAWSCloudWatchMetricAlarm_expression
--- PASS: TestAccAWSCloudWatchMetricAlarm_expression (20.02s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       20.063s

This is my first attempt at a PR to this repo, so apologies in advance for all the conventions I've violated 😉

[ghost]

@bflad any chance to get this merged?

[humayunjamal]

when this can be merged ?

[orthimnas]

I would also like to know when this can be merged.

[amiteswar]

Seems like a lot of interest for this PR, When this can be merged? Thanks in advance.

[arun251]

This would be great to have.

[shaiguitar]

Thanks @ApeChimp! Am looking forward to using this.

[benlamonica]

I’ll be more blunt since reacting to other people’s requests for merge isnt getting me any closer to this feature.

@bflad - Can we have feedback on what is wrong with this PR and we can help fix it? Can we get an ETA to merge it or a similar feature? If you are not planning on merging it, will you please let us know?

[bflad]

Reviewing this on my to-do list this week unless another maintainer beats me to it.

[bflad]

Hey @ApeChimp 👋 This is awesome! Nice work so far, this is off to a really good start. A few feedback items below and hopefully we can get this in. Since this has entered the review phase there won't be any lengthy delays from the maintainers, apologies about the long lead time before.

One important piece missing in this pull request is updating the resource documentation in website/docs/r/cloudwatch_metric_alarm.html.markdown. Since the concept is much more complicated than the previous metric alarm usage, this page should be updated to include an example of metrics expressions (something similar to the written acceptance test configuration, but maybe a little more real world with a addition/subtraction/multiplication/division would be a great). Each of the new arguments should also be appropriately documented where the description wording can be borrowed from the CloudWatch API Reference documentation.

Please reach out with any questions or if you do not have time to implement any of the items. Thanks!

[bflad]

While the intention for this here is good, this should be removed from here as it does not work as you would expect. 👍

Unfortunately, ConflictsWith is not supported at this time for attributes nested under a TypeSet attribute (or TypeList with more than 1 element). The resource schema in this case is using a static, absolute reference while these references are based on the flat map representation of attribute locations in the schema and TypeSet attributes wind up including a value in the middle based on the configuration, e.g. metric_query.123.metric.

To double check the behavior, I modified the provided acceptance test configuration to include both a metric and expression in the same configuration block:

resource "aws_cloudwatch_metric_alarm" "foobar" {
  # ... other configuration ...
	metric_query {
		id = "m1"
		expression = "m2"
		metric {
			metric_name = "CPUUtilization"
			namespace   = "AWS/EC2"
			period      = "120"
			stat        = "Average"
			unit        = "Count"
			dimensions {
				InstanceId = "i-abc123"
			}
		}
	}
}

Which generated this structure for the API request:

2019/02/07 18:41:28 [DEBUG] Creating CloudWatch Metric Alarm: {
...
  Metrics: [{
...
    },{
      Expression: "m2",
      Id: "m1",
      MetricStat: {
        Metric: {
          Dimensions: [{
              Name: "InstanceId",
              Value: "i-abc123"
            }],
          MetricName: "CPUUtilization",
          Namespace: "AWS/EC2"
        },
        Period: 120,
        Stat: "Average",
        Unit: "Count"
      },
      ReturnData: false
    }],
...
}

And received a fairly confusing validation error message back from the CloudWatch API:

--- FAIL: TestAccAWSCloudWatchMetricAlarm_expression (4.61s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
        	* aws_cloudwatch_metric_alarm.foobar: 1 error occurred:
        	* aws_cloudwatch_metric_alarm.foobar: Creating metric alarm failed: ValidationError: Metrics list must contain at least one metric

We may want to consider adding a helpful error in the Terraform resource code below (along with adding a "bad" acceptance testing configuration and utilizing ExpectError) since the API error does not directly point to the root cause to help operators with their errant configuration.

We do have future plans after Terraform 0.12 work settles down to enhance the Terraform provider SDK to either allow for relative references or solve this problem in some other way. 😄

[bflad]

Same as above, we should remove this. 👍

[bflad]

Nit: A lot of nil checking logic here and below can be removed if this code is switched to use the AWS Go SDK functions for converting the values (which returns the Go type's zero value if it is nil and matches the Terraform value in the state if unset), e.g.

metricQuery := map[string]interface{}{
  "expression":  aws.StringValue(mq.Expression),
  "id":          aws.StringValue(mq.Id),
  "label":       aws.StringValue(mq.Label),
  "return_data": aws.BoolValue(mq.ReturnData),
}

[mike-zorn]

Thanks for the review, @bflad. I've addressed all the comments on the code and am currently working on the docs. Let me know if any more code changes are necessary.

[bflad]

The code and testing looks great! This should be ready to merge once the docs are in and reviewed.

--- PASS: TestAccAWSCloudWatchMetricAlarm_missingStatistic (4.10s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_extendedStatistic (9.44s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm (9.52s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (10.42s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_AlarmActions_SWFAction (11.40s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_AlarmActions_SNSTopic (12.48s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_treatMissingData (14.27s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles (14.30s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_expression (16.20s)
--- PASS: TestAccAWSCloudWatchMetricAlarm_AlarmActions_EC2Automate (200.75s)

[bflad]

Looks great, thanks @ApeChimp! 🚀

[bflad]

😍

[bflad]

This has been released in version 1.59.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!