Ees 5687 add resource utilisation metric alerts #5464
Conversation
duncan-at-hiveit
force-pushed
the
EES-5687-add-resource-utilisation-metric-alerts
branch
2 times, most recently
from
2cb2aa9 to
3eca3d8
Compare
| module alerts '../dynamicMetricAlert.bicep' = [for name in resourceNames: { | ||
| name: '${name}ResponseTimeAlertModule' | ||
| module alerts '../baseResponseTimeAlert.bicep' = { | ||
| name: '${resourceNames[0]}ResponseTimeAlertModule' |
There was a problem hiding this comment.
Given we're only using the first resourceNames item to construct the name, feels like the abstraction isn't quite right. The implication is that we intend this to only work with a single resource, so having a list of resourceNames seems a little confusing.
Should we change this so that we only work with a single resource by changing the resourceName to a string parameter?
We're doing a similar thing in a bunch of places throughout this PR so it'd be good to change this across the board if we make this change.
| @description('Tags with which to tag the resource in Azure.') | ||
| param tagValues object | ||
|
|
||
| module alerts '../baseCpuPercentageAlert.bicep' = { |
There was a problem hiding this comment.
Are we potentially over-abstracting by having loads of extra modules that wrap baseCpuPercentageAlert.bicep?
It's not clear what extra value we're getting from the additional wrapping and feels like we're just copy-pasting the same set of parameters and configuration in a bunch of different modules. It also looks we're generating a bunch of additional resource group deployments due to the extra wrapping, which is a little less clear to debug.
I'd potentially suggest we simplify things and remove the modules that are re-using the 'base' modules like baseCpuPercentageAlert.bicep. Instead, we can just use the base modules directly and avoid the extra abstraction layer.
Similar thing would apply across this PR.
| param resourceNames string[] | ||
|
|
||
| @description('Names of the resources that these alerts are being applied to.') | ||
| param resourceType string |
There was a problem hiding this comment.
Could create and use a new CpuResourceType that is a union of the specific resources with CPUs (rather than every resource type). This would help with auto-completion and general type safety.
| param resourceType string | ||
|
|
||
| @description('Names of the resources that these alerts are being applied to.') | ||
| param metricName string |
There was a problem hiding this comment.
As above - could create a CpuMetricName type that could assist in auto-completion and general type safety.
| @description('Names of the resources that these alerts are being applied to.') | ||
| param resourceType string | ||
|
|
||
| @description('Names of the resources that these alerts are being applied to.') | ||
| param metricName string |
There was a problem hiding this comment.
Similar to baseCpuPercentageAlert.bicep, we could create some memory-specific types that can constrain these two parameters.
| @description('Names of the resources that these alerts are being applied to.') | ||
| param resourceType string | ||
|
|
||
| @description('Names of the resources that these alerts are being applied to.') | ||
| param metricName string |
There was a problem hiding this comment.
Similar to baseCpuPercentageAlert.bicep, we could create some response time-specific types that can constrain these two parameters.
| param sensitivity Sensitivity = 'Low' | ||
|
|
||
| param minFailingPeriodsToAlert int = 1 | ||
| param minFailingPeriodsToAlert int = 3 | ||
|
|
||
| param numberOfEvaluationPeriods int = 1 | ||
| param numberOfEvaluationPeriods int = 3 |
There was a problem hiding this comment.
Minor, but shouldn't these changes be in the EES-5686 PR given we introduce dynamicMetricAlert.bicep there?
Doesn't make a lot of sense to have a follow-up PR that immediately changes the previous one.
duncan-at-hiveit
force-pushed
the
EES-5687-add-resource-utilisation-metric-alerts
branch
from
3eca3d8 to
a3e1b44
Compare
duncan-at-hiveit
force-pushed
the
EES-5686-add-public-api-performance-metric-alerts
branch
2 times, most recently
from
aaaccaf to
6cefbfc
Compare
…ns (and the Data Processor Function App)
…t configuration inherited by different resource-specific alerts
duncan-at-hiveit
force-pushed
the
EES-5687-add-resource-utilisation-metric-alerts
branch
from
a3e1b44 to
dd56fa7
Compare
Explain the motivation for making this change. What existing problem does the pull request solve?
You might also want to think about the following:
Related changes
Are there any changes that were closely related to this change but not directly part of the main work e.g. refactoring, fixes, etc?
Screenshots
Are there any screenshots that help illustrate the change made?