Repository: VictoriaMetrics/VictoriaMetrics
Pull request #975 (2020-12-19 10:17:28)

vmalert-974: fix order for labels templating
The change fixes bug caused by https://github.com/VictoriaMetrics/VictoriaMetrics/commit/3adf8c5a6f4a8eea5f2025dd53ba975d42f8b8d1.

https://github.com/VictoriaMetrics/VictoriaMetrics/issues/974

------------------

@f41gh7 pls see the test cases in PR. I'd appreciate your review and feel free to suggest additional test cases we may want to check.

codecov[bot]: # [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=h1) Report
> Merging [#975](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=desc) (e2929b9) into [master](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/88ac4dfc0720225fb2ba46c3f7d797566fc99678?el=desc) (88ac4df) will **decrease** coverage by `0.16%`.
> The diff coverage is `66.66%`.

[![Impacted file tree graph](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/graphs/tree.svg?width=650&height=150&src=pr&token=YND8dYVcXn)](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #975      +/-   ##
==========================================
- Coverage   60.72%   60.56%   -0.17%     
==========================================
  Files         202      202              
  Lines       22887    22894       +7     
==========================================
- Hits        13899    13866      -33     
- Misses       7664     7706      +42     
+ Partials     1324     1322       -2     
```


| [Impacted Files](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [app/vmalert/notifier/alert.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvbm90aWZpZXIvYWxlcnQuZ28=) | `47.22% <33.33%> (-1.35%)` | :arrow_down: |
| [app/vmalert/alerting.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvYWxlcnRpbmcuZ28=) | `77.22% <75.00%> (+0.69%)` | :arrow_up: |
| [lib/mergeset/block\_stream\_reader.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-bGliL21lcmdlc2V0L2Jsb2NrX3N0cmVhbV9yZWFkZXIuZ28=) | `49.30% <0.00%> (-13.20%)` | :arrow_down: |
| [app/vmselect/promql/aggr\_incremental.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-YXBwL3Ztc2VsZWN0L3Byb21xbC9hZ2dyX2luY3JlbWVudGFsLmdv) | `82.55% <0.00%> (-5.43%)` | :arrow_down: |
| [lib/mergeset/table.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-bGliL21lcmdlc2V0L3RhYmxlLmdv) | `64.82% <0.00%> (-1.23%)` | :arrow_down: |
| [lib/storage/index\_db.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-bGliL3N0b3JhZ2UvaW5kZXhfZGIuZ28=) | `63.96% <0.00%> (-0.11%)` | :arrow_down: |
| [app/vmalert/remotewrite/remotewrite.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvcmVtb3Rld3JpdGUvcmVtb3Rld3JpdGUuZ28=) | `60.37% <0.00%> (+3.77%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=footer). Last update [88ac4df...e2929b9](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/975?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
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/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 {


Test patch:
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