From 9ce8b36d2aa2e92aff5e467c821639cab68216d3 Mon Sep 17 00:00:00 2001 From: Roman Khavronenko 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 592d35226..d7e23b234 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 c646c1729..e5f10cc31 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 a3ce92342..6c9141e90 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 e71b549ea..fe1223f38 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 {