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

New Resource: azurerm_monitor_scheduled_query_rules_alert & azurerm_monitor_scheduled_query_rules_log #5053

Merged
merged 31 commits into from
Mar 4, 2020

Conversation

mcdafydd
Copy link
Contributor

@mcdafydd mcdafydd commented Dec 3, 2019

Fixes #3951

This feature implements the ScheduledQueryRules API. The new resources should allow users to automate creation of monitoring and alerting rules alongside infrastructure.

- Fix some sprintf formatting
- Improve documentation for cross-resource query
- Don't create an empty metric_trigger {} if user doesn't specify block
@mcdafydd
Copy link
Contributor Author

mcdafydd commented Dec 5, 2019

I think I'm all good now and ready for review.

Thanks!

@tshafeev
Copy link

@tombuildsstuff @katbyte would be great to have it 1.39.0 release ;)

@tshafeev
Copy link

NY present maybe in this time 🙏

@tshafeev
Copy link

@katbyte as i understood from tagging it will be merged in current 1.42 release cycle?

@tombuildsstuff tombuildsstuff removed this from the v1.42.0 milestone Jan 24, 2020
@tombuildsstuff tombuildsstuff modified the milestones: v1.44.0, v2.0.0 Feb 12, 2020
- Fix test crash
- Move common functions to helpers
- Add more schema validations
- Add Update tests
@mcdafydd
Copy link
Contributor Author

I'm going to do a once over on everything again this weekend, but I think it's close enough for re-review. Thanks!

@mcdafydd
Copy link
Contributor Author

I do see one issue that I'll try and address as soon as I can. The LogToMetric tests and example don't reflect what's required to make that useful:

https://docs.microsoft.com/en-us/azure/azure-monitor/platform/alerts-metric-logs#resource-template-for-metric-alerts-for-logs

To use the LogToMetric API, there needs to be a second resource, an Azure Metric Alert, that alerts on the newly-created metric. I'll update the tests and example to reflect this.

- Add metric alerts to LogToMetric tests and docs
- Fix type assert from change to TypeList
- Fix a couple typos
@mcdafydd
Copy link
Contributor Author

Presuming this test passes, I'm ready for another review.

Thanks!

@mcdafydd
Copy link
Contributor Author

Oops - I found one additional published limit - 100 resources maximum for a cross-resource query:

https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-subscription-service-limits#log-queries-and-language

@tombuildsstuff tombuildsstuff modified the milestones: v2.0.0, v2.1.0 Feb 23, 2020
@mcdafydd
Copy link
Contributor Author

@tombuildsstuff @mbfrahry @katbyte

Just checking in since this is my first new resource PR. Is there anything else I should be doing to make sure it is mergeable to master? I'll be patient, just want to be able to streamline any requirements if possible.

Thanks for all your work on 2.0!

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mcdafydd. Tests are passing! I've left a few things to maneuver around to ease maintainability and a couple more around ease of reading for the docs but I think we're just about there

azurerm/helpers/azure/monitor.go Outdated Show resolved Hide resolved
azurerm/helpers/azure/monitor.go Outdated Show resolved Hide resolved
- Improve docs
- Use heredoc strings in docs and test queries
- Move shared functions into monitor package
- Add underscores to test function names
@mcdafydd
Copy link
Contributor Author

mcdafydd commented Mar 4, 2020

@mbfrahry Ready for another peek.

@ghost ghost removed the waiting-response label Mar 4, 2020
@mcdafydd mcdafydd requested a review from mbfrahry March 4, 2020 16:57
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mbfrahry mbfrahry changed the title Add support for ScheduledQueryRules New Resource: azurerm_monitor_scheduled_query_rules_alert & azurerm_monitor_scheduled_query_rules_log Mar 4, 2020
@mbfrahry mbfrahry merged commit 685016f into hashicorp:master Mar 4, 2020
@mbfrahry
Copy link
Member

mbfrahry commented Mar 4, 2020

Thanks for this @mcdafydd! We'll get it into the next release!

mbfrahry added a commit that referenced this pull request Mar 4, 2020
@ghost
Copy link

ghost commented Mar 11, 2020

This has been released in version 2.1.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.1.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 4, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support alerts based on Log analytics queries
6 participants