vmalert-974: fix order for labels templating (#975)

The change fixes bug caused by 3adf8c5a6f.

https://github.com/VictoriaMetrics/VictoriaMetrics/issues/974

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)

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)

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

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 {