From 39f559d22b0610fbe7a7ebe6da33e48de480cf03 Mon Sep 17 00:00:00 2001
From: Roman Khavronenko <roman@victoriametrics.com>
Date: Thu, 29 Sep 2022 18:22:50 +0200
Subject: [PATCH] vmalert: allow using extra labels in annotations (#3181)

According to Ruler specification, only labels returned within time series
should be available for use in annotations.

For long time, vmalert didn't respect this rule. And in PR
https://github.com/VictoriaMetrics/VictoriaMetrics/pull/2403
this was fixed for the sake of compatibility. However, this resulted
into users confusion, as they expected all configured and extra labels
to be available - https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3013

This fix allows to use extra labels in Annotations. But in the case of conflicts
the original labels (extracted from time series) are preferred.

Signed-off-by: hagen1778 <roman@victoriametrics.com>

Signed-off-by: hagen1778 <roman@victoriametrics.com>
---
 app/vmalert/alerting.go                       | 23 +++++++++++++++----
 app/vmalert/alerting_test.go                  | 20 +++++++++++++---
 .../config/testdata/rules/rules2-good.rules   |  5 ++--
 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/app/vmalert/alerting.go b/app/vmalert/alerting.go
index 89b35deb35..db6b24f3e8 100644
--- a/app/vmalert/alerting.go
+++ b/app/vmalert/alerting.go
@@ -165,11 +165,15 @@ func (ar *AlertingRule) logDebugf(at time.Time, a *notifier.Alert, format string
 }
 
 type labelSet struct {
-	// origin labels from series
-	// used for templating
+	// origin labels extracted from received time series
+	// plus extra labels (group labels, service labels like alertNameLabel).
+	// in case of conflicts, origin labels from time series preferred.
+	// used for templating annotations
 	origin map[string]string
-	// processed labels with additional data
-	// used as Alert labels
+	// processed labels includes origin labels
+	// plus extra labels (group labels, service labels like alertNameLabel).
+	// in case of conflicts, extra labels are preferred.
+	// used as labels attached to notifier.Alert and ALERTS series written to remote storage.
 	processed map[string]string
 }
 
@@ -177,7 +181,7 @@ type labelSet struct {
 // to labelSet which contains original and processed labels.
 func (ar *AlertingRule) toLabels(m datasource.Metric, qFn templates.QueryFn) (*labelSet, error) {
 	ls := &labelSet{
-		origin:    make(map[string]string, len(m.Labels)),
+		origin:    make(map[string]string),
 		processed: make(map[string]string),
 	}
 	for _, l := range m.Labels {
@@ -199,14 +203,23 @@ func (ar *AlertingRule) toLabels(m datasource.Metric, qFn templates.QueryFn) (*l
 	}
 	for k, v := range extraLabels {
 		ls.processed[k] = v
+		if _, ok := ls.origin[k]; !ok {
+			ls.origin[k] = v
+		}
 	}
 
 	// set additional labels to identify group and rule name
 	if ar.Name != "" {
 		ls.processed[alertNameLabel] = ar.Name
+		if _, ok := ls.origin[alertNameLabel]; !ok {
+			ls.origin[alertNameLabel] = ar.Name
+		}
 	}
 	if !*disableAlertGroupLabel && ar.GroupName != "" {
 		ls.processed[alertGroupNameLabel] = ar.GroupName
+		if _, ok := ls.origin[alertGroupNameLabel]; !ok {
+			ls.origin[alertGroupNameLabel] = ar.GroupName
+		}
 	}
 	return ls, nil
 }
diff --git a/app/vmalert/alerting_test.go b/app/vmalert/alerting_test.go
index a222b824ea..89607116b8 100644
--- a/app/vmalert/alerting_test.go
+++ b/app/vmalert/alerting_test.go
@@ -700,14 +700,26 @@ func TestAlertingRule_Template(t *testing.T) {
 		expAlerts map[uint64]*notifier.Alert
 	}{
 		{
-			newTestRuleWithLabels("common", "region", "east"),
+			&AlertingRule{
+				Name: "common",
+				Labels: map[string]string{
+					"region": "east",
+				},
+				Annotations: map[string]string{
+					"summary": `{{ $labels.alertname }}: Too high connection number for "{{ $labels.instance }}"`,
+				},
+				alerts: make(map[uint64]*notifier.Alert),
+				state:  newRuleState(),
+			},
 			[]datasource.Metric{
 				metricWithValueAndLabels(t, 1, "instance", "foo"),
 				metricWithValueAndLabels(t, 1, "instance", "bar"),
 			},
 			map[uint64]*notifier.Alert{
 				hash(map[string]string{alertNameLabel: "common", "region": "east", "instance": "foo"}): {
-					Annotations: map[string]string{},
+					Annotations: map[string]string{
+						"summary": `common: Too high connection number for "foo"`,
+					},
 					Labels: map[string]string{
 						alertNameLabel: "common",
 						"region":       "east",
@@ -715,7 +727,9 @@ func TestAlertingRule_Template(t *testing.T) {
 					},
 				},
 				hash(map[string]string{alertNameLabel: "common", "region": "east", "instance": "bar"}): {
-					Annotations: map[string]string{},
+					Annotations: map[string]string{
+						"summary": `common: Too high connection number for "bar"`,
+					},
 					Labels: map[string]string{
 						alertNameLabel: "common",
 						"region":       "east",
diff --git a/app/vmalert/config/testdata/rules/rules2-good.rules b/app/vmalert/config/testdata/rules/rules2-good.rules
index 615a085afa..545a42d555 100644
--- a/app/vmalert/config/testdata/rules/rules2-good.rules
+++ b/app/vmalert/config/testdata/rules/rules2-good.rules
@@ -9,11 +9,12 @@ groups:
       denyPartialResponse: ["true"]
     rules:
       - alert: Conns
-        expr: sum(vm_tcplistener_conns) by(instance) > 1
+        expr: vm_tcplistener_conns > 0
         for: 3m
         debug: true
         annotations:
-          summary: Too high connection number for {{$labels.instance}}
+          labels: "Available labels: {{ $labels }}"
+          summary: Too high connection number for {{ $labels.instance }}
             {{ with printf "sum(vm_tcplistener_conns{instance=%q})" .Labels.instance | query }}
               {{ . | first | value }}
             {{ end }}