Correctly handle empty labels from alert templates. (#5845)

Fixes https://github.com/prometheus/common/issues/36

Move logic handling this into the labels package,
so all the cases are handled in one place and we're
less likely to have this come up again.

Signed-off-by: Brian Brazil <brian.brazil@robustperception.io>

pkg/labels/labels.go

@@ -285,20 +285,26 @@ type Builder struct {
 	add  []Label
 }
 
-// NewBuilder returns a new LabelsBuilder
+// NewBuilder returns a new LabelsBuilder.
 func NewBuilder(base Labels) *Builder {
-	return &Builder{
+	b := &Builder{
-		base: base,
+		del: make([]string, 0, 5),
-		del:  make([]string, 0, 5),
+		add: make([]Label, 0, 5),
-		add:  make([]Label, 0, 5),
 	}
+	b.Reset(base)
+	return b
 }
 
-// Reset clears all current state for the builder
+// Reset clears all current state for the builder.
 func (b *Builder) Reset(base Labels) {
 	b.base = base
 	b.del = b.del[:0]
 	b.add = b.add[:0]
+	for _, l := range b.base {
+		if l.Value == "" {
+			b.del = append(b.del, l.Name)
+		}
+	}
 }
 
 // Del deletes the label of the given name.
@@ -316,6 +322,10 @@ func (b *Builder) Del(ns ...string) *Builder {
 
 // Set the name/value pair as a label.
 func (b *Builder) Set(n, v string) *Builder {
+	if v == "" {
+		// Empty labels are the same as missing labels.
+		return b.Del(n)
+	}
 	for i, a := range b.add {
 		if a.Name == n {
 			b.add[i].Value = v

rules/alerting_test.go

@@ -49,8 +49,7 @@ func TestAlertingRuleLabelsUpdate(t *testing.T) {
 	testutil.Ok(t, err)
 	defer suite.Close()
 
-	err = suite.Run()
+	testutil.Ok(t, suite.Run())
-	testutil.Ok(t, err)
 
 	expr, err := promql.ParseExpr(`http_requests < 100`)
 	testutil.Ok(t, err)
@@ -152,8 +151,7 @@ func TestAlertingRuleExternalLabelsInTemplate(t *testing.T) {
 	testutil.Ok(t, err)
 	defer suite.Close()
 
-	err = suite.Run()
+	testutil.Ok(t, suite.Run())
-	testutil.Ok(t, err)
 
 	expr, err := promql.ParseExpr(`http_requests < 100`)
 	testutil.Ok(t, err)
@@ -236,3 +234,58 @@ func TestAlertingRuleExternalLabelsInTemplate(t *testing.T) {
 
 	testutil.Equals(t, result, filteredRes)
 }
+
+func TestAlertingRuleEmptyLabelFromTemplate(t *testing.T) {
+	suite, err := promql.NewTest(t, `
+		load 1m
+			http_requests{job="app-server", instance="0"}	75 85 70 70
+	`)
+	testutil.Ok(t, err)
+	defer suite.Close()
+
+	testutil.Ok(t, suite.Run())
+
+	expr, err := promql.ParseExpr(`http_requests < 100`)
+	testutil.Ok(t, err)
+
+	rule := NewAlertingRule(
+		"EmptyLabel",
+		expr,
+		time.Minute,
+		labels.FromStrings("empty_label", ""),
+		nil,
+		nil,
+		true, log.NewNopLogger(),
+	)
+	result := promql.Vector{
+		{
+			Metric: labels.FromStrings(
+				"__name__", "ALERTS",
+				"alertname", "EmptyLabel",
+				"alertstate", "pending",
+				"instance", "0",
+				"job", "app-server",
+			),
+			Point: promql.Point{V: 1},
+		},
+	}
+
+	evalTime := time.Unix(0, 0)
+	result[0].Point.T = timestamp.FromTime(evalTime)
+
+	var filteredRes promql.Vector // After removing 'ALERTS_FOR_STATE' samples.
+	res, err := rule.Eval(
+		suite.Context(), evalTime, EngineQueryFunc(suite.QueryEngine(), suite.Storage()), nil,
+	)
+	testutil.Ok(t, err)
+	for _, smpl := range res {
+		smplName := smpl.Metric.Get("__name__")
+		if smplName == "ALERTS" {
+			filteredRes = append(filteredRes, smpl)
+		} else {
+			// If not 'ALERTS', it has to be 'ALERTS_FOR_STATE'.
+			testutil.Equals(t, smplName, "ALERTS_FOR_STATE")
+		}
+	}
+	testutil.Equals(t, result, filteredRes)
+}

rules/recording.go

@@ -88,11 +88,7 @@ func (rule *RecordingRule) Eval(ctx context.Context, ts time.Time, query QueryFu
 		lb.Set(labels.MetricName, rule.name)
 
 		for _, l := range rule.labels {
-			if l.Value == "" {
+			lb.Set(l.Name, l.Value)
-				lb.Del(l.Name)
-			} else {
-				lb.Set(l.Name, l.Value)
-			}
 		}
 
 		sample.Metric = lb.Labels()

scrape/scrape.go

@@ -449,20 +449,16 @@ func mutateSampleLabels(lset labels.Labels, target *Target, honor bool, rc []*re
 		}
 	} else {
 		for _, l := range target.Labels() {
-			lv := lset.Get(l.Name)
+			// existingValue will be empty if l.Name doesn't exist.
-			if lv != "" {
+			existingValue := lset.Get(l.Name)
-				lb.Set(model.ExportedLabelPrefix+l.Name, lv)
+			// Because setting a label with an empty value is a no-op,
-			}
+			// this will only create the prefixed label if necessary.
+			lb.Set(model.ExportedLabelPrefix+l.Name, existingValue)
+			// It is now safe to set the target label.
 			lb.Set(l.Name, l.Value)
 		}
 	}
 
-	for _, l := range lb.Labels() {
-		if l.Value == "" {
-			lb.Del(l.Name)
-		}
-	}
-
 	res := lb.Labels()
 
 	if len(rc) > 0 {
@@ -476,10 +472,7 @@ func mutateReportSampleLabels(lset labels.Labels, target *Target) labels.Labels
 	lb := labels.NewBuilder(lset)
 
 	for _, l := range target.Labels() {
-		lv := lset.Get(l.Name)
+		lb.Set(model.ExportedLabelPrefix+l.Name, lset.Get(l.Name))
-		if lv != "" {
-			lb.Set(model.ExportedLabelPrefix+l.Name, lv)
-		}
 		lb.Set(l.Name, l.Value)
 	}
 

scrape/scrape_test.go

@@ -85,7 +85,7 @@ func TestDroppedTargetsList(t *testing.T) {
 			},
 		}
 		sp, _                  = newScrapePool(cfg, app, 0, nil)
-		expectedLabelSetString = "{__address__=\"127.0.0.1:9090\", __metrics_path__=\"\", __scheme__=\"\", job=\"dropMe\"}"
+		expectedLabelSetString = "{__address__=\"127.0.0.1:9090\", job=\"dropMe\"}"
 		expectedLength         = 1
 	)
 	sp.Sync(tgs)