From 2880b5055a71f337e2a0f8bfe724faac08c73294 Mon Sep 17 00:00:00 2001
From: Francois Gouteroux <francois.gouteroux@gmail.com>
Date: Thu, 27 Jul 2023 11:12:21 +0200
Subject: [PATCH] fix: parse expression error message based on record or alert
 (#10065)

---
 CHANGELOG.md             |  1 +
 pkg/ruler/compat.go      |  5 +++-
 pkg/ruler/compat_test.go | 59 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7c43d49046..5c92342f16 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -50,6 +50,7 @@
 
 ##### Fixes
 
+* [10065](https://github.com/grafana/loki/pull/10065) **fgouteroux**: Fix the syntax error message when parsing expression rule.
 * [8979](https://github.com/grafana/loki/pull/8979) **slim-bean**: Fix the case where a logs query with start time == end time was returning logs when none should be returned.
 * [9099](https://github.com/grafana/loki/pull/9099) **salvacorts**: Fix the estimated size of chunks when writing a new TSDB file during compaction.
 * [9130](https://github.com/grafana/loki/pull/9130) **salvacorts**: Pass LogQL engine options down to the _split by range_, _sharding_, and _query size limiter_ middlewares.
diff --git a/pkg/ruler/compat.go b/pkg/ruler/compat.go
index daa987ee81..e2557b4c22 100644
--- a/pkg/ruler/compat.go
+++ b/pkg/ruler/compat.go
@@ -249,7 +249,10 @@ func validateRuleNode(r *rulefmt.RuleNode, groupName string) error {
 	if r.Expr.Value == "" {
 		return errors.Errorf("field 'expr' must be set in rule")
 	} else if _, err := syntax.ParseExpr(r.Expr.Value); err != nil {
-		return errors.Wrapf(err, fmt.Sprintf("could not parse expression for record '%s' in group '%s'", r.Record.Value, groupName))
+		if r.Record.Value != "" {
+			return errors.Wrapf(err, fmt.Sprintf("could not parse expression for record '%s' in group '%s'", r.Record.Value, groupName))
+		}
+		return errors.Wrapf(err, fmt.Sprintf("could not parse expression for alert '%s' in group '%s'", r.Alert.Value, groupName))
 	}
 
 	if r.Record.Value != "" {
diff --git a/pkg/ruler/compat_test.go b/pkg/ruler/compat_test.go
index c665e7b0f5..ef109d4d43 100644
--- a/pkg/ruler/compat_test.go
+++ b/pkg/ruler/compat_test.go
@@ -6,8 +6,12 @@ import (
 	"time"
 
 	"github.com/prometheus/prometheus/config"
+	"github.com/prometheus/prometheus/model/rulefmt"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"gopkg.in/yaml.v3"
+
 	"github.com/grafana/loki/pkg/iter"
 	"github.com/grafana/loki/pkg/logql"
 	rulerbase "github.com/grafana/loki/pkg/ruler/base"
@@ -15,6 +19,61 @@ import (
 	"github.com/grafana/loki/pkg/validation"
 )
 
+// TestInvalidRuleGroup tests that a validation error is raised when rule group is invalid
+func TestInvalidRuleGroup(t *testing.T) {
+	ruleGroupValid := rulefmt.RuleGroup{
+		Name: "test",
+		Rules: []rulefmt.RuleNode{
+			{
+				Alert: yaml.Node{Value: "alert-1-name"},
+				Expr:  yaml.Node{Value: "sum by (job) (rate({namespace=~\"test\"} [5m]) > 0)"},
+			},
+			{
+				Alert: yaml.Node{Value: "record-1-name"},
+				Expr:  yaml.Node{Value: "sum by (job) (rate({namespace=~\"test\"} [5m]) > 0)"},
+			},
+		},
+	}
+	require.Nil(t, ValidateGroups(ruleGroupValid))
+
+	ruleGroupInValid := rulefmt.RuleGroup{
+		Name: "test",
+		Rules: []rulefmt.RuleNode{
+			{
+				Alert: yaml.Node{Value: "alert-1-name"},
+				Expr:  yaml.Node{Value: "bad_value"},
+			},
+			{
+				Record: yaml.Node{Value: "record-1-name"},
+				Expr:   yaml.Node{Value: "bad_value"},
+			},
+		},
+	}
+	require.Error(t, ValidateGroups(ruleGroupInValid)[0])
+	require.Error(t, ValidateGroups(ruleGroupInValid)[1])
+}
+
+// TestInvalidRuleExprParsing tests that a validation error is raised when rule expression is invalid
+func TestInvalidRuleExprParsing(t *testing.T) {
+	expectedAlertErrorMsg := "could not parse expression for alert 'alert-1-name' in group 'test': parse error"
+	alertRuleExprInvalid := &rulefmt.RuleNode{
+		Alert: yaml.Node{Value: "alert-1-name"},
+		Expr:  yaml.Node{Value: "bad_value"},
+	}
+
+	alertErr := validateRuleNode(alertRuleExprInvalid, "test")
+	assert.Containsf(t, alertErr.Error(), expectedAlertErrorMsg, "expected error containing '%s', got '%s'", expectedAlertErrorMsg, alertErr)
+
+	expectedRecordErrorMsg := "could not parse expression for record 'record-1-name' in group 'test': parse error"
+	recordRuleExprInvalid := &rulefmt.RuleNode{
+		Record: yaml.Node{Value: "record-1-name"},
+		Expr:   yaml.Node{Value: "bad_value"},
+	}
+
+	recordErr := validateRuleNode(recordRuleExprInvalid, "test")
+	assert.Containsf(t, recordErr.Error(), expectedRecordErrorMsg, "expected error containing '%s', got '%s'", expectedRecordErrorMsg, recordErr)
+}
+
 // TestInvalidRemoteWriteConfig tests that a validation error is raised when config is invalid
 func TestInvalidRemoteWriteConfig(t *testing.T) {
 	// if remote-write is not enabled, validation fails