From 6d5ed2ba3c53574b1d3d22bda65635569a0b96fb Mon Sep 17 00:00:00 2001
From: Cristian Greco <cristian@regolo.cc>
Date: Mon, 6 May 2024 12:44:50 +0200
Subject: [PATCH] mimirtool: use strings replacer in dashboards queries
 analyzer

When parsing queries from grafana dashboards, use a replacer with a
list of variables rather than a regex matcher.

The regex matcher works ok with ranges and subqueries, but is not
working when the variables are used in other parts of the query (e.g.
with `offset`).

This PR switches back to using a list of variables to be replaced,
partially reverting the changes from #6657, but uses a
`strings.Replacer` for better readability.
---
 CHANGELOG.md                          |  1 +
 pkg/mimirtool/analyze/grafana.go      | 29 ++++++++++---------
 pkg/mimirtool/analyze/grafana_test.go | 40 ++++++++++++++++++++++++++-
 3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 372a47e9f79..b7f52dc39a7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -101,6 +101,7 @@
 * [BUGFIX] Fix panic in `loadgen` subcommand. #7629
 * [ENHANCEMENT] `mimirtool promql format`: Format PromQL query with Prometheus' string or pretty-print formatter. #7742
 * [BUGFIX] `mimirtool rules prepare`: do not add aggregation label to `on()` clause if already present in `group_left()` or `group_right()`. #7839
+* [BUGFIX] Analyze Grafana: fix parsing queries with variables. #8062
 
 ### Mimir Continuous Test
 
diff --git a/pkg/mimirtool/analyze/grafana.go b/pkg/mimirtool/analyze/grafana.go
index 0e8176df70c..95592228813 100644
--- a/pkg/mimirtool/analyze/grafana.go
+++ b/pkg/mimirtool/analyze/grafana.go
@@ -8,6 +8,7 @@ package analyze
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/grafana/regexp"
 	"github.com/pkg/errors"
@@ -21,12 +22,20 @@ import (
 )
 
 var (
-	lvRegexp                     = regexp.MustCompile(`(?s)label_values\((.+),.+\)`)
-	lvNoQueryRegexp              = regexp.MustCompile(`(?s)label_values\((.+)\)`)
-	qrRegexp                     = regexp.MustCompile(`(?s)query_result\((.+)\)`)
-	validMetricName              = regexp.MustCompile(`^[a-zA-Z_:][a-zA-Z0-9_:]*$`)
-	variableRangeQueryRangeRegex = regexp.MustCompile(`\[\$?\w+?]`)
-	variableSubqueryRangeRegex   = regexp.MustCompile(`\[\$?\w+:\$?\w+?]`)
+	lvRegexp        = regexp.MustCompile(`(?s)label_values\((.+),.+\)`)
+	lvNoQueryRegexp = regexp.MustCompile(`(?s)label_values\((.+)\)`)
+	qrRegexp        = regexp.MustCompile(`(?s)query_result\((.+)\)`)
+	validMetricName = regexp.MustCompile(`^[a-zA-Z_:][a-zA-Z0-9_:]*$`)
+	replacer        = strings.NewReplacer(
+		"$__interval", "5m",
+		"$interval", "5m",
+		"$resolution", "5s",
+		"$__rate_interval", "15s",
+		"$rate_interval", "15s",
+		"$__range", "1d",
+		"${__range_s:glob}", "30",
+		"${__range_s}", "30",
+	)
 )
 
 type MetricsInGrafana struct {
@@ -205,14 +214,8 @@ func metricsFromPanel(panel minisdk.Panel, metrics map[string]struct{}) []error
 	return parseErrors
 }
 
-func removeVariablesFromRanges(query string) string {
-	query = variableRangeQueryRangeRegex.ReplaceAllLiteralString(query, `[5m]`)
-	query = variableSubqueryRangeRegex.ReplaceAllLiteralString(query, `[5m:1m]`)
-	return query
-}
-
 func parseQuery(query string, metrics map[string]struct{}) error {
-	expr, err := parser.ParseExpr(removeVariablesFromRanges(query))
+	expr, err := parser.ParseExpr(replacer.Replace(query))
 	if err != nil {
 		return err
 	}
diff --git a/pkg/mimirtool/analyze/grafana_test.go b/pkg/mimirtool/analyze/grafana_test.go
index 22ead855bc6..61b232c3b32 100644
--- a/pkg/mimirtool/analyze/grafana_test.go
+++ b/pkg/mimirtool/analyze/grafana_test.go
@@ -147,7 +147,7 @@ func TestMetricsFromTemplating(t *testing.T) {
 		require.Empty(t, metrics)
 	})
 
-	t.Run(`label_values query with no metric selector multiline `, func(t *testing.T) {
+	t.Run(`label_values query with no metric selector multiline`, func(t *testing.T) {
 		metrics := make(map[string]struct{})
 		in := minisdk.Templating{
 			List: []minisdk.TemplateVar{
@@ -166,4 +166,42 @@ func TestMetricsFromTemplating(t *testing.T) {
 		require.Empty(t, errs)
 		require.Empty(t, metrics)
 	})
+
+	t.Run(`query contains a subquery with the $__interval variable`, func(t *testing.T) {
+		metrics := make(map[string]struct{})
+		in := minisdk.Templating{
+			List: []minisdk.TemplateVar{
+				{
+					Name:       "variable",
+					Type:       "query",
+					Datasource: nil,
+					Query:      `increase(varnish_main_threads_failed{job=~"$job",instance=~"$instance"}[$__interval:])`,
+				},
+			},
+		}
+
+		errs := metricsFromTemplating(in, metrics)
+		require.Empty(t, errs)
+		require.Len(t, metrics, 1)
+		require.Equal(t, map[string]struct{}{"varnish_main_threads_failed": {}}, metrics)
+	})
+
+	t.Run(`query uses offset with $__interval variable`, func(t *testing.T) {
+		metrics := make(map[string]struct{})
+		in := minisdk.Templating{
+			List: []minisdk.TemplateVar{
+				{
+					Name:       "variable",
+					Type:       "query",
+					Datasource: nil,
+					Query:      `increase(tomcat_session_processingtime_total{job=~"$job", instance=~"$instance", host=~"$host", context=~"$context"}[$__interval:] offset -$__interval)`,
+				},
+			},
+		}
+
+		errs := metricsFromTemplating(in, metrics)
+		require.Empty(t, errs)
+		require.Len(t, metrics, 1)
+		require.Equal(t, map[string]struct{}{"tomcat_session_processingtime_total": {}}, metrics)
+	})
 }