`azurerm_monitor_metric_alert` supports multiple scopes and different criteria #7159

magodo · 2020-06-01T13:13:31Z

Originally, the resource only allows users to specify one scope. Furthermore, it only allows the user to specify static criteria, while there are dynamic criteria and "web test location availability" criteria.
This PR tries to support both facets and not to introduce breaking change.

The complex part of kinds of criteria is that there are two levels of discriminator defined in swagger, as shown below:

Previous code was using the SingleResourceMultipleMetricCriteria, while this is superseded by MultipleResourceMultipleMetricCriteria, which in turns derives into StaticMetricCriteria (actually MetricCriteria, which was underused) and DynamicMetricCriteria. Additionally, there is another criteria WebtestLocationAvailabilityMetricCriteria. In order to keep schema use to use and (most importantly) to keep backward compatibility, I defined three criteria in schema:

criteria: same as before represents the StaticMetricCriteria. Each alert can specify one or more criteria.
dynamic_criteria: represents the DynamicMetricCriteria. Each alert can only have one such criterion.
webtest_location_availability_criteria: represents WebtestLocationAvailabilityMetricCriteria. Each alert can only have one such criteria.

And then set them Optional and add the ExactlyOneOf constraints among them.

Another change is that I added two more attributes called target_resource_type and target_resource_location, which are mandatory when either multiple scopes are set or the subscription/resource group scope is specified, in which case the service need to use this information to identify the resource type and resource location.

Test Result

💤 via 🦉 v1.14.3 make testacc TEST=./azurerm/internal/services/monitor/tests TESTARGS="-run TestAccAzureRMMonitorMetricAlert_"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/monitor/tests -v -run TestAccAzureRMMonitorMetricAlert_ -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMMonitorMetricAlert_basic
=== PAUSE TestAccAzureRMMonitorMetricAlert_basic
=== RUN   TestAccAzureRMMonitorMetricAlert_requiresImport
=== PAUSE TestAccAzureRMMonitorMetricAlert_requiresImport
=== RUN   TestAccAzureRMMonitorMetricAlert_complete
=== PAUSE TestAccAzureRMMonitorMetricAlert_complete
=== RUN   TestAccAzureRMMonitorMetricAlert_basicAndCompleteUpdate
=== PAUSE TestAccAzureRMMonitorMetricAlert_basicAndCompleteUpdate
=== RUN   TestAccAzureRMMonitorMetricAlert_multiScope
=== PAUSE TestAccAzureRMMonitorMetricAlert_multiScope
=== CONT  TestAccAzureRMMonitorMetricAlert_basic
=== CONT  TestAccAzureRMMonitorMetricAlert_basicAndCompleteUpdate
=== CONT  TestAccAzureRMMonitorMetricAlert_multiScope
=== CONT  TestAccAzureRMMonitorMetricAlert_complete
=== CONT  TestAccAzureRMMonitorMetricAlert_requiresImport
--- PASS: TestAccAzureRMMonitorMetricAlert_complete (160.67s)
--- PASS: TestAccAzureRMMonitorMetricAlert_basic (165.03s)
--- PASS: TestAccAzureRMMonitorMetricAlert_requiresImport (174.56s)
--- PASS: TestAccAzureRMMonitorMetricAlert_basicAndCompleteUpdate (344.57s)
--- PASS: TestAccAzureRMMonitorMetricAlert_multiScope (558.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/monitor/tests       558.329s

(fxes #3719, fixes #3886)

… criterias Originally, the resource only allows user to specify one scope. Furthermore, it only allow user to specify static criteria, while there are dynamic criterias and "webtest available location" criterias. This PR tries to support both facets and not to introduce breaking change.

AdamCoulterOz · 2020-06-19T04:51:32Z

:-( got pushed back a week ... was hanging on this one...

AdamCoulterOz · 2020-06-19T06:14:25Z

website/docs/r/monitor_metric_alert.html.markdown

+* `operator` - (Required) The criteria operator. Possible values are `LessThan`, `GreaterThan` and `GreaterOrLessThan`.
+* `alert_sensitivity` - (Required) The extent of deviation required to trigger an alert. Possible values are `Low`, `Medium` and `High`.
+* `dimension` - (Optional) One or more `dimension` blocks as defined below.
+* `evaluation_total_account` - (Optional) The number of aggregated lookback points. The lookback time window is calculated based on the aggregation granularity (`window_size`) and the selected number of aggregated points.


typo here, should be evaluation_total_count

Oh, Thank you!

AdamCoulterOz · 2020-06-24T06:20:06Z

@katbyte can i confirm if this will make the cut for 2.16?

tombuildsstuff

Left a few comments whilst passing through

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

azurerm/internal/services/monitor/tests/monitor_metric_alert_resource_test.go

tombuildsstuff · 2020-06-25T14:39:10Z

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

+
+	return []interface{}{
+		map[string]interface{}{
+			"webtest_id":            webtestID,


does this make more sense as web_test_id?

I think webtest is a better fit as it emphasize it is special name called webtest, rather than some test of web?

there's an existing resource for this with web_test - so it'd be worth making this consistent to be clearer

arguably this could make sense as application_insights_web_test_id rather than such a generic name?

I'll rename the criteria name (the name of the nested block) into application_insights_web_test_location_availability_criteria, and keep the including attributes still (e.g. component_id), except renaming the included webtest_id into web_test_id.

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

magodo · 2020-07-11T14:06:44Z

Hey @tombuildsstuff Thank you for your review!
I have updated the code accordingly, please take a look!

Test Result

💤 make testacc TEST=./azurerm/internal/services/monitor/tests TESTARGS="-run=TestAccAzureRMMonitorMetricAlert_multiScope"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/monitor/tests -v -run=TestAccAzureRMMonitorMetricAlert_multiScope -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMMonitorMetricAlert_multiScope
=== PAUSE TestAccAzureRMMonitorMetricAlert_multiScope
=== CONT  TestAccAzureRMMonitorMetricAlert_multiScope
--- PASS: TestAccAzureRMMonitorMetricAlert_multiScope (1060.25s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/monitor/tests       1060.277s

tombuildsstuff

@magodo I've taken another look through and this mostly LGTM - if we can ensure all the fields are set then this otherwise LGTM 👍

tombuildsstuff · 2020-07-14T09:26:44Z

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

+			"target_resource_type": {
+				Type:        schema.TypeString,
+				Optional:    true,
+				Computed:    true,


should this be Computed?

Yes, for the single scope of a certain resource, the target_resource_type will be deduced by service and returned in the response.

tombuildsstuff · 2020-07-14T09:26:49Z

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

+			"target_resource_location": {
+				Type:             schema.TypeString,
+				Optional:         true,
+				Computed:         true,


should this be Computed?

Same reason as above.

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

tombuildsstuff · 2020-07-14T09:46:31Z

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

+		return expandMonitorMetricAlertWebtestLocAvailCriteria(d.Get("webtest_location_availability_criteria").([]interface{})), nil
+	default:
+		// Guaranteed by schema `AtLeastOne` constraint
+		return nil, errors.New("unknwon criteria type")


Suggested change

return nil, errors.New("unknwon criteria type")

return nil, fmt.Errorf("unknown criteria type")

Not sure about this as there is no argument except the format string here.

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

tombuildsstuff · 2020-07-14T09:54:51Z

azurerm/internal/services/monitor/monitor_metric_alert_resource.go

+
+	return []interface{}{
+		map[string]interface{}{
+			"webtest_id":            webtestID,


there's an existing resource for this with web_test - so it'd be worth making this consistent to be clearer

arguably this could make sense as application_insights_web_test_id rather than such a generic name?

website/docs/r/monitor_metric_alert.html.markdown

magodo · 2020-07-15T04:27:35Z

@tombuildsstuff I have updated some code per your comments and replied the other comments with justifications, please take a look!

jackofallops · 2020-07-16T11:15:28Z

Tests passing:

ghost · 2020-07-16T15:44:45Z

This has been released in version 2.19.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.19.0"
}
# ... other configuration ...

AdamCoulterOz · 2020-08-06T01:58:28Z

thanks @jackofallops appreciate this! :-D

…tiMetricCriteria for legacy metric alerts (#7995) The change in #7159 deprecates the usage of SingleResourceMultiMetricCriteria outright (replaced by MultipleResourceMultipleMetricCriteria). Unfortunately, that breaks the users who have metric alert created before that PR merged, which was using SingleResourceMultiMetricCriteria as its type. Then once those metric alerts get updated, current code will trigger an update from SingleResourceMultiMetricCriteria to MultipleResourceMultipleMetricCriteria, which seems not supported by service (as reported in #7910). This PR keeps those legacy resource to use the SingleResourceMultiMetricCriteria. If user wants to use the MultipleResourceMultipleMetricCriteria, they will have to recreate the resource. (fixes #7910)

ghost · 2020-08-15T14:50:00Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

magodo added 3 commits May 31, 2020 12:59

Fix CI

b30f6ce

resolve review comment

fa94069

ghost added size/XL documentation labels Jun 1, 2020

jackofallops added enhancement service/monitor labels Jun 1, 2020

Workshop2 mentioned this pull request Jun 4, 2020

Support of dynamic alerts #3886

Closed

katbyte added this to the v2.14.0 milestone Jun 9, 2020

tombuildsstuff modified the milestones: v2.14.0, v2.15.0 Jun 11, 2020

yupwei68 mentioned this pull request Jun 12, 2020

azurerm_monitor_metric_alert error Detect invalid value: microsoft.insights/components #6424

Closed

mholttech mentioned this pull request Jun 17, 2020

Dynamic threshold support for monitor metric alert #3796

Closed

katbyte modified the milestones: v2.15.0, v2.16.0 Jun 18, 2020

AdamCoulterOz reviewed Jun 19, 2020

View reviewed changes

typo

f5a49aa

tombuildsstuff modified the milestones: v2.16.0, v2.17.0 Jun 25, 2020

This comment has been minimized.

Sign in to view

katbyte modified the milestones: v2.17.0, v2.18.0 Jul 3, 2020

magodo mentioned this pull request Jul 6, 2020

Importing an existing azurerm_monitor_metric_alert resource does not fetch all fields #7560

Closed

tombuildsstuff reviewed Jul 7, 2020

View reviewed changes

katbyte modified the milestones: v2.18.0, v2.19.0 Jul 10, 2020

tombuildsstuff reviewed Jul 10, 2020

View reviewed changes

azurerm/internal/services/monitor/monitor_metric_alert_resource.go Outdated Show resolved Hide resolved

modify per review

d7e49cb

tombuildsstuff suggested changes Jul 14, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Jul 14, 2020

magodo added 2 commits July 15, 2020 10:40

minor code/doc change

e407b98

Rename/complement test for application insight web test criteria

8369b4d

ghost added size/XXL and removed size/XL labels Jul 15, 2020

magodo requested a review from tombuildsstuff July 15, 2020 04:27

magodo removed the waiting-response label Jul 15, 2020

magodo and others added 2 commits July 15, 2020 13:04

typo

d974130

feedback updates

48941e6

jackofallops merged commit 1f316b3 into hashicorp:master Jul 16, 2020

jackofallops added a commit that referenced this pull request Jul 16, 2020

Update for #7159

a92df50

stack72 mentioned this pull request Jul 20, 2020

Upgrade to v2.19.0 of the AzureRM Terraform Provider pulumi/pulumi-azure#638

Merged

ghost locked and limited conversation to collaborators Aug 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_monitor_metric_alert` supports multiple scopes and different criteria #7159

`azurerm_monitor_metric_alert` supports multiple scopes and different criteria #7159

magodo commented Jun 1, 2020 •

edited

Loading

AdamCoulterOz commented Jun 19, 2020

AdamCoulterOz Jun 19, 2020

magodo Jun 19, 2020

AdamCoulterOz commented Jun 24, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tombuildsstuff left a comment

tombuildsstuff Jun 25, 2020

magodo Jul 11, 2020

tombuildsstuff Jul 14, 2020

magodo Jul 15, 2020 •

edited

Loading

magodo commented Jul 11, 2020

tombuildsstuff left a comment

tombuildsstuff Jul 14, 2020

magodo Jul 15, 2020

tombuildsstuff Jul 14, 2020

magodo Jul 15, 2020

tombuildsstuff Jul 14, 2020

magodo Jul 15, 2020

tombuildsstuff Jul 14, 2020

magodo commented Jul 15, 2020

jackofallops commented Jul 16, 2020

ghost commented Jul 16, 2020

AdamCoulterOz commented Aug 6, 2020

ghost commented Aug 15, 2020

	return nil, errors.New("unknwon criteria type")
	return nil, fmt.Errorf("unknown criteria type")

azurerm_monitor_metric_alert supports multiple scopes and different criteria #7159

azurerm_monitor_metric_alert supports multiple scopes and different criteria #7159

Conversation

magodo commented Jun 1, 2020 • edited Loading

Test Result

AdamCoulterOz commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamCoulterOz commented Jun 24, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

magodo commented Jul 11, 2020

Test Result

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo commented Jul 15, 2020

jackofallops commented Jul 16, 2020

ghost commented Jul 16, 2020

AdamCoulterOz commented Aug 6, 2020

ghost commented Aug 15, 2020

`azurerm_monitor_metric_alert` supports multiple scopes and different criteria #7159

`azurerm_monitor_metric_alert` supports multiple scopes and different criteria #7159

magodo commented Jun 1, 2020 •

edited

Loading

magodo Jul 15, 2020 •

edited

Loading