global/prometheus-alertmanager-operated: Added bedrock alerting #7628
Conversation
christophrichtersap
requested review from
kevin-fischer,
max-len,
richardtief,
viennaa,
himanip94,
Kuckkuck,
IvoGoman and
timojohlo
as code owners
christophrichtersap
changed the title
There was a problem hiding this comment.
Thanks for providing this PR and enhancing the vmware-rules with the bedrock use-case. Overall I'd recommend adding some notes around the helpers to help other colleagues understanding it.
Please bump the minor version of prometheus-vmware-rules and the patch version prometheus-alertmanager-operated.
global/prometheus-alertmanager-operated/templates/am-config-route-slack.yaml
Outdated
Show resolved
Hide resolved
| {{- $bedrockAlerts := .Values.bedrockAlerts }} | ||
| {{- $filteredBedrockAlerts := dict }} |
There was a problem hiding this comment.
Can you add an example in the values.yaml showing this behaviour, can be empty or commented out.
There was a problem hiding this comment.
Fixed on commit e7d48fd
| {{ printf "%s" $bytes | indent 2 }} | ||
|
|
||
| {{- $string := $bytes | toString }} | ||
| {{- $string := (regexReplaceAll "\\n\\s+\\n" $string "\n\n") }} |
There was a problem hiding this comment.
Why do we need this replace of newlines?
There was a problem hiding this comment.
This is required, cause in the past not alerts have been formatted properly in yaml. We can enforce the correct format between alerts to remove the replace.
What is your opinion about that?
There was a problem hiding this comment.
I'd go for proper formatting in the first place, but if we can not assure this drift it is ok to strip the newlines. However please add this as a comment here too.
Co-authored-by: Tommy Sauer <[email protected]>
Co-authored-by: Tommy Sauer <[email protected]>
Resolved: Added commented example of data in `prometheus-rules/prometheus-vmware-rules/values.yaml` Resolved: Added comments on `prometheus-rules/prometheus-vmware-rules/templates/_helper.tpl`
…ap/helm-charts into bedrock_alerts
Co-authored-by: Richard Tief <[email protected]>
Co-authored-by: Richard Tief <[email protected]>
@richardtief the context is that we need to expose some of the VMWare alerts to bedrock slack channel too not only to VMWare. The other option @viennaa mentioned was creating(duplicating) alerts for bedrock separate. Chris and I decided for the actual option to avoid double maintenance of nearly identical alerts in future. |
|
|
||
| # Maintained in the regional secrets | ||
| # bedrockAlerts: | ||
| # DatastoreDisconnectedWithVmsOnIt: hostsystem |
There was a problem hiding this comment.
What do we need hostsystem for? As of my understanding this could be a list, as hostsystem is only be checked for existence. Meaning it could be some arbitrary value in the current implementation. Can we make this a list?
There was a problem hiding this comment.
This ist the variable $mappingKey in prometheus-rules/prometheus-vmware-rules/templates/_helper.tpl
It varies depending on the query used...
There was a problem hiding this comment.
could this be any other thing than hostsystem? As for my understanding you are always deciding on this, no?
Please also check on this one ^. |
Fixed on ac8686c |
Co-authored-by: Tommy Sauer <[email protected]>
_helper.tplam-config-route-slack.yamlvalues.yamlhas keybedrockAlertswith list(alertname1 alertname2 alertname3 ...) onprometheus-alerts.yaml