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.

Sign up for GitHub

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

Jump to bottom

New Resource: azurerm_monitor_scheduled_query_rules_alert & azurerm_monitor_scheduled_query_rules_log #5053

Merged
merged 31 commits into from
Mar 4, 2020
Merged

New Resource: azurerm_monitor_scheduled_query_rules_alert & azurerm_monitor_scheduled_query_rules_log #5053

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.

@mcdafydd
Fix linting errors; minor cleanup
607a00d
@mcdafydd
Minor updates and bug fixes
ebf84e9 
- Fix some sprintf formatting
- Improve documentation for cross-resource query
- Don't create an empty metric_trigger {} if user doesn't specify block
@mcdafydd
Remove heredoc in docs example
ce59b1a
@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 🙏

@tombuildsstuff tombuildsstuff added this to the v1.40.0 milestone Dec 19, 2019
@tombuildsstuff tombuildsstuff self-assigned this Dec 19, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.40.0, v1.41.0 Dec 19, 2019
@tombuildsstuff tombuildsstuff removed their assignment Dec 19, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.41.0, v1.42.0 Jan 14, 2020
@mcdafydd mcdafydd mentioned this pull request Jan 17, 2020
Closed
@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
@mcdafydd
Address review comments
651c41f 
- 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.

@mcdafydd
Fix type assertions; simplify cross-resource query example
db64f0d
@mcdafydd
Review updates
37b2c39 
- 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!

mbfrahry
mbfrahry requested changes Mar 2, 2020
View reviewed changes
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
azurerm/internal/services/monitor/tests/data_source_monitor_scheduled_query_rules_alert_test.go Outdated Show resolved Hide resolved
website/docs/r/monitor_scheduled_query_rules_alert.html.markdown Outdated Show resolved Hide resolved
@mcdafydd
Address review comments
27ad7ae 
- Improve docs
- Use heredoc strings in docs and test queries
- Move shared functions into monitor package
- Add underscores to test function names
@mcdafydd
Fix markdown formatting
0625b05
@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
mbfrahry
mbfrahry approved these changes Mar 4, 2020
View reviewed changes
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
@mbfrahry
Update Changelog for #5053
4dbfde8
@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.
Reviewers

@davidtom davidtom davidtom left review comments

@mbfrahry mbfrahry mbfrahry approved these changes

@katbyte katbyte Awaiting requested review from katbyte

Assignees
No one assigned
Labels
documentation new-data-source new-resource service/monitor size/XXL
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.

Support alerts based on Log analytics queries
6 participants
@mcdafydd @tshafeev @mbfrahry @katbyte @davidtom @tombuildsstuff