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

Implement baseline alerts #136

Open
5 tasks
hknutsen opened this issue Apr 8, 2024 · 9 comments
Open
5 tasks

Implement baseline alerts #136

hknutsen opened this issue Apr 8, 2024 · 9 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@hknutsen
Copy link
Member

hknutsen commented Apr 8, 2024

Implement baseline alerts according to: https://azure.github.io/azure-monitor-baseline-alerts/welcome/

This opens up the possibility of moving the app-service and aci modules out of archive, because by implementing alerts we'll have a reason to keep them 🙂

Example: equinor/terraform-azurerm-app-service#10. Note though that alert rules can be reused for multiple resources of the same type, so it might be a better idea to create an alerts submodule for each module. Discuss! 🙂

Tasks

@hknutsen hknutsen added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 8, 2024
@hknutsen
Copy link
Member Author

Alerts also have a cost, so reusing alerts could be a good idea with that in mind.

@hknutsen
Copy link
Member Author

hknutsen commented May 27, 2024

Interesting finding...

I was playing around with setting the scope of an alert rule to multiple resources, which gave me the following warning:

Metric and Log signals might not be available if the scope includes multiple resources.

Investigate this warning, as it might be important for the decision on whether or not alert rules should be created for each individual resource, or if alert rules should be reused across resources of the same type.

Update: overview of resource types that support multi-resource alert rules: https://learn.microsoft.com/en-us/azure/azure-monitor/alerts/alerts-metric-near-real-time#metrics-and-dimensions-supported

Seems like very few resource types support multi-resource alert rules. Maybe ignore for now?

@hknutsen hknutsen self-assigned this Aug 27, 2024
@hknutsen
Copy link
Member Author

Added task list with link to PR for SQL module.

@hknutsen
Copy link
Member Author

hknutsen commented Sep 2, 2024

Arguments against including alerts in baseline modules:

  • Alerts have very granular configuration. It could prove difficult to find a default configuration that works for most people, and it could require a lot of variables to expose that granularity.
  • A new resource is created for each alert, which could result in our modules becoming bloated as people request more alerts to be added.
  • While we always want modules to contain at least two resources, we also want to keep modules as small and simple as possible. We don't want people to shy away from using our modules because it's difficult to understand how they work.

For comparison, including a diagnostic setting in our modules made sense since it only requires the creation of a single simple resource that can be modified as needed by using a single module input (diagnostic_setting_enabled_log_categories).

@helenakallekleiv
Copy link
Contributor

An addition, if we created the alerts as a sub module, we would avoid a bloated main module. Although this would then mean that for each resource in the users configuration, they would need one alert sub-module block, for each resource. Doubling the amount of module blocks and cause the config to be bloated.

As this is not required per se, we could define the use of alerts as an example for now.

@hknutsen
Copy link
Member Author

hknutsen commented Sep 11, 2024

Another potential solution... Using the Azure Key Vault module as an example:

Current implementation

With the current implementation, the module equinor/key-vault/azurerm creates two resources:

  1. azurerm_key_vault.this
  2. azurerm_monitor_diagnostic_setting.this

Proposed implementation

A new submodule equinor/key-vault/azurerm//modules/vault creates 2 resources:

  1. azurerm_key_vault.this
  2. azurerm_monitor_diagnostic_setting.this

A new submodule equinor/key-vault/azurerm//modules/alerts creates 2 resources:

  1. azurerm_monitor_metric_alert.this
  2. azurerm_scheduled_query_alert_v2.this

The module equinor/key-vault/azurerm creates 4 resources:

  1. module.vault.azurerm_key_vault.this
  2. module.vault.azurerm_monitor_diagnostic_setting.this
  3. module.alerts.azurerm_monitor_metric_alert.this
  4. module.alerts.azurerm_scheduled_query_alert_v2.this

So by calling the main equinor/key-vault/azurerm module you get everything, however if you only need the Key Vault with a diagnostic setting your can call the equinor/key-vault/azurerm//modules/vault submodule directly.

@ErlendT
Copy link
Contributor

ErlendT commented Oct 9, 2024

Waiting for POC from @hknutsen before moving into todo

@hknutsen
Copy link
Member Author

equinor/terraform-azurerm-key-vault#98 has been updated with a POC of the proposed implementation.

@hknutsen
Copy link
Member Author

Putting on hold until equinor/terraform-azurerm-key-vault#98 (comment) has been discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants