From 404cbd152252a9090bd6ca69bd43be470be0c94c Mon Sep 17 00:00:00 2001
From: Roman Khavronenko <hagen1778@gmail.com>
Date: Sat, 19 Dec 2020 12:10:59 +0000
Subject: [PATCH] vmalert-974: fix order for labels templating (#975)
The change fixes bug caused by https://github.com/VictoriaMetrics/VictoriaMetrics/commit/3adf8c5a6f4a8eea5f2025dd53ba975d42f8b8d1.
https://github.com/VictoriaMetrics/VictoriaMetrics/issues/974
---
app/vmalert/alerting.go | 38 ++++---
app/vmalert/alerting_test.go | 101 ++++++++++++++++++
app/vmalert/config/testdata/rules2-good.rules | 5 +-
app/vmalert/notifier/alert.go | 18 ++--
4 files changed, 141 insertions(+), 21 deletions(-)
diff --git a/app/vmalert/alerting.go b/app/vmalert/alerting.go
index 592d352260..d7e23b234d 100644
--- a/app/vmalert/alerting.go
+++ b/app/vmalert/alerting.go
@@ -141,11 +141,16 @@ func (ar *AlertingRule) Exec(ctx context.Context, q datasource.Querier, series b
updated := make(map[uint64]struct{})
// update list of active alerts
for _, m := range qMetrics {
- for k, v := range ar.Labels {
- // apply extra labels
+ // extra labels could contain templates, so we expand them first
+ labels, err := expandLabels(m, qFn, ar)
+ if err != nil {
+ return nil, fmt.Errorf("failed to expand labels: %s", err)
+ }
+ for k, v := range labels {
+ // apply extra labels to datasource
+ // so the hash key will be consistent on restore
m.SetLabel(k, v)
}
-
h := hash(m)
if _, ok := updated[h]; ok {
// duplicate may be caused by extra labels
@@ -158,8 +163,8 @@ func (ar *AlertingRule) Exec(ctx context.Context, q datasource.Querier, series b
// update Value field with latest value
a.Value = m.Value
// and re-exec template since Value can be used
- // in templates
- err = ar.template(a, qFn)
+ // in annotations
+ a.Annotations, err = a.ExecTemplate(qFn, ar.Annotations)
if err != nil {
return nil, err
}
@@ -200,6 +205,19 @@ func (ar *AlertingRule) Exec(ctx context.Context, q datasource.Querier, series b
return nil, nil
}
+func expandLabels(m datasource.Metric, q notifier.QueryFn, ar *AlertingRule) (map[string]string, error) {
+ metricLabels := make(map[string]string)
+ for _, l := range m.Labels {
+ metricLabels[l.Name] = l.Value
+ }
+ tpl := notifier.AlertTplData{
+ Labels: metricLabels,
+ Value: m.Value,
+ Expr: ar.Expr,
+ }
+ return notifier.ExecTemplate(q, ar.Labels, tpl)
+}
+
func (ar *AlertingRule) toTimeSeries(timestamp time.Time) []prompbmarshal.TimeSeries {
var tss []prompbmarshal.TimeSeries
for _, a := range ar.alerts {
@@ -265,17 +283,9 @@ func (ar *AlertingRule) newAlert(m datasource.Metric, start time.Time, qFn notif
}
a.Labels[l.Name] = l.Value
}
- return a, ar.template(a, qFn)
-}
-
-func (ar *AlertingRule) template(a *notifier.Alert, qFn notifier.QueryFn) error {
var err error
- a.Labels, err = a.ExecTemplate(qFn, a.Labels)
- if err != nil {
- return err
- }
a.Annotations, err = a.ExecTemplate(qFn, ar.Annotations)
- return err
+ return a, err
}
// AlertAPI generates APIAlert object from alert by its id(hash)
diff --git a/app/vmalert/alerting_test.go b/app/vmalert/alerting_test.go
index c646c17298..e5f10cc31b 100644
--- a/app/vmalert/alerting_test.go
+++ b/app/vmalert/alerting_test.go
@@ -3,6 +3,7 @@ package main
import (
"context"
"errors"
+ "reflect"
"strings"
"testing"
"time"
@@ -464,6 +465,106 @@ func TestAlertingRule_Exec_Negative(t *testing.T) {
}
}
+func TestAlertingRule_Template(t *testing.T) {
+ testCases := []struct {
+ rule *AlertingRule
+ metrics []datasource.Metric
+ expAlerts map[uint64]*notifier.Alert
+ }{
+ {
+ newTestRuleWithLabels("common", "region", "east"),
+ []datasource.Metric{
+ metricWithValueAndLabels(t, 1, "instance", "foo"),
+ metricWithValueAndLabels(t, 1, "instance", "bar"),
+ },
+ map[uint64]*notifier.Alert{
+ hash(metricWithLabels(t, "region", "east", "instance", "foo")): {
+ Annotations: map[string]string{},
+ Labels: map[string]string{
+ alertGroupNameLabel: "",
+ "region": "east",
+ "instance": "foo",
+ },
+ },
+ hash(metricWithLabels(t, "region", "east", "instance", "bar")): {
+ Annotations: map[string]string{},
+ Labels: map[string]string{
+ alertGroupNameLabel: "",
+ "region": "east",
+ "instance": "bar",
+ },
+ },
+ },
+ },
+ {
+ &AlertingRule{
+ Name: "override label",
+ Labels: map[string]string{
+ "instance": "{{ $labels.instance }}",
+ "region": "east",
+ },
+ Annotations: map[string]string{
+ "summary": `Too high connection number for "{{ $labels.instance }}" for region {{ $labels.region }}`,
+ "description": `It is {{ $value }} connections for "{{ $labels.instance }}"`,
+ },
+ alerts: make(map[uint64]*notifier.Alert),
+ },
+ []datasource.Metric{
+ metricWithValueAndLabels(t, 2, "instance", "foo"),
+ metricWithValueAndLabels(t, 10, "instance", "bar"),
+ },
+ map[uint64]*notifier.Alert{
+ hash(metricWithLabels(t, "region", "east", "instance", "foo")): {
+ Labels: map[string]string{
+ alertGroupNameLabel: "",
+ "instance": "foo",
+ "region": "east",
+ },
+ Annotations: map[string]string{
+ "summary": `Too high connection number for "foo" for region east`,
+ "description": `It is 2 connections for "foo"`,
+ },
+ },
+ hash(metricWithLabels(t, "region", "east", "instance", "bar")): {
+ Labels: map[string]string{
+ alertGroupNameLabel: "",
+ "instance": "bar",
+ "region": "east",
+ },
+ Annotations: map[string]string{
+ "summary": `Too high connection number for "bar" for region east`,
+ "description": `It is 10 connections for "bar"`,
+ },
+ },
+ },
+ },
+ }
+ fakeGroup := Group{Name: "TestRule_Exec"}
+ for _, tc := range testCases {
+ t.Run(tc.rule.Name, func(t *testing.T) {
+ fq := &fakeQuerier{}
+ tc.rule.GroupID = fakeGroup.ID()
+ fq.add(tc.metrics...)
+ if _, err := tc.rule.Exec(context.TODO(), fq, false); err != nil {
+ t.Fatalf("unexpected err: %s", err)
+ }
+ for hash, expAlert := range tc.expAlerts {
+ gotAlert := tc.rule.alerts[hash]
+ if gotAlert == nil {
+ t.Fatalf("alert %d is missing; labels: %v; annotations: %v",
+ hash, expAlert.Labels, expAlert.Annotations)
+ }
+ if !reflect.DeepEqual(expAlert.Annotations, gotAlert.Annotations) {
+ t.Fatalf("expected to have annotations %#v; got %#v", expAlert.Annotations, gotAlert.Annotations)
+ }
+ if !reflect.DeepEqual(expAlert.Labels, gotAlert.Labels) {
+ t.Fatalf("expected to have labels %#v; got %#v", expAlert.Labels, gotAlert.Labels)
+ }
+ }
+ })
+ }
+}
+
func newTestRuleWithLabels(name string, labels ...string) *AlertingRule {
r := newTestAlertingRule(name, 0)
r.Labels = make(map[string]string)
diff --git a/app/vmalert/config/testdata/rules2-good.rules b/app/vmalert/config/testdata/rules2-good.rules
index a3ce92342b..6c9141e905 100644
--- a/app/vmalert/config/testdata/rules2-good.rules
+++ b/app/vmalert/config/testdata/rules2-good.rules
@@ -15,8 +15,11 @@ groups:
- alert: ExampleAlertAlwaysFiring
expr: sum by(job)
(up == 1)
+ labels:
+ job: '{{ $labels.job }}'
annotations:
- summary: Instances up {{ range query "up" }}
+ description: Job {{ $labels.job }} is up!
+ summary: All instances up {{ range query "up" }}
{{ . | label "instance" }}
{{ end }}
- record: handler:requests:rate5m
diff --git a/app/vmalert/notifier/alert.go b/app/vmalert/notifier/alert.go
index e71b549ead..fe1223f38c 100644
--- a/app/vmalert/notifier/alert.go
+++ b/app/vmalert/notifier/alert.go
@@ -52,7 +52,8 @@ func (as AlertState) String() string {
return "inactive"
}
-type alertTplData struct {
+// AlertTplData is used to execute templating
+type AlertTplData struct {
Labels map[string]string
Value float64
Expr string
@@ -60,25 +61,30 @@ type alertTplData struct {
const tplHeader = `{{ $value := .Value }}{{ $labels := .Labels }}{{ $expr := .Expr }}`
-// ExecTemplate executes the Alert template for give
+// ExecTemplate executes the Alert template for given
// map of annotations.
// Every alert could have a different datasource, so function
// requires a queryFunction as an argument.
func (a *Alert) ExecTemplate(q QueryFn, annotations map[string]string) (map[string]string, error) {
- tplData := alertTplData{Value: a.Value, Labels: a.Labels, Expr: a.Expr}
+ tplData := AlertTplData{Value: a.Value, Labels: a.Labels, Expr: a.Expr}
return templateAnnotations(annotations, tplData, funcsWithQuery(q))
}
+// ExecTemplate executes the given template for given annotations map.
+func ExecTemplate(q QueryFn, annotations map[string]string, tpl AlertTplData) (map[string]string, error) {
+ return templateAnnotations(annotations, tpl, funcsWithQuery(q))
+}
+
// ValidateTemplates validate annotations for possible template error, uses empty data for template population
func ValidateTemplates(annotations map[string]string) error {
- _, err := templateAnnotations(annotations, alertTplData{
+ _, err := templateAnnotations(annotations, AlertTplData{
Labels: map[string]string{},
Value: 0,
}, tmplFunc)
return err
}
-func templateAnnotations(annotations map[string]string, data alertTplData, funcs template.FuncMap) (map[string]string, error) {
+func templateAnnotations(annotations map[string]string, data AlertTplData, funcs template.FuncMap) (map[string]string, error) {
var builder strings.Builder
var buf bytes.Buffer
eg := new(utils.ErrGroup)
@@ -99,7 +105,7 @@ func templateAnnotations(annotations map[string]string, data alertTplData, funcs
return r, eg.Err()
}
-func templateAnnotation(dst io.Writer, text string, data alertTplData, funcs template.FuncMap) error {
+func templateAnnotation(dst io.Writer, text string, data AlertTplData, funcs template.FuncMap) error {
t := template.New("").Funcs(funcs).Option("missingkey=zero")
tpl, err := t.Parse(text)
if err != nil {