Simplify latency alerting #1510
Conversation
googlebot
added
the
cla: yes
|
|
||
| variable "duration" { | ||
| type = number | ||
| description = "Duration of alert evaluation" |
There was a problem hiding this comment.
you added the unit for threshold in ms, what is the unit for duration? same ms?
There was a problem hiding this comment.
Added unit, thank you.
terraform/alerting/slos.tf
Outdated
| latency_threshold = 60000 # in ms | ||
| enable_alert = false | ||
| availability_goal = 0.995 | ||
| enable_latency_slo = false |
There was a problem hiding this comment.
can you add a comment that we are going to disable latency SLO in favor of the latency alert for services with low number of requests? Just to clarify since this can be confusing in the future.
terraform/alerting/slos.tf
Outdated
| @@ -71,11 +94,12 @@ module "latency-slos" { | |||
|
|
|||
| project = var.project | |||
|
|
|||
| enabled = var.https-forwarding-rule != "" | |||
| # enabled = var.https-forwarding-rule != "" | |||
There was a problem hiding this comment.
Good catch, thank you.
terraform/alerting/slos.tf
Outdated
| { enable_alert = true, | ||
| { enable_alert = true, | ||
| enable_latency_alert = true, | ||
| enable_latency_slo = false, |
There was a problem hiding this comment.
It seems you don't need to set this here, since it is false by default?
There was a problem hiding this comment.
Yes, thank you.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, yuriatgoogle The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Fixes #
Proposed Changes
Release Note