Merge pull request #1585 from cloudflare/tweaks

Fix sorting for range queries

Author: prymitive

internal/checks/alerts_template.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"slices"
 	"strings"
+	"sync"
 	textTemplate "text/template"
 	"text/template/parse"
 	"time"
@@ -35,7 +36,8 @@ var (
 		"{{$externalURL := .ExternalURL}}",
 		"{{$value := .Value}}",
 	}
-	templateDefsLen = len(strings.Join(templateDefs, ""))
+	templateDefsString = strings.Join(templateDefs, "")
+	templateDefsLen    = len(templateDefsString)
 
 	templateFuncMap = textTemplate.FuncMap{
 		"query":              dummyFuncMap,
@@ -67,6 +69,16 @@ var (
 		"externalURL":        dummyFuncMap,
 		"parseDuration":      dummyFuncMap,
 	}
+
+	templatePool = sync.Pool{
+		New: func() any {
+			t := textTemplate.
+				New(TemplateCheckName).
+				Funcs(templateFuncMap).
+				Option("missingkey=zero")
+			return t
+		},
+	}
 )
 
 func dummyFuncMap(q string) string {
@@ -135,7 +147,7 @@ func (c TemplateCheck) Check(ctx context.Context, entry discovery.Entry, _ []dis
 				},
 			})
 		}
-		for _, msg := range checkForValueInLabels(label.Key.Value, label.Value.Value) {
+		for _, msg := range checkForValueInLabels(label.Value.Value) {
 			problems = append(problems, Problem{
 				Anchor: AnchorAfter,
 				Lines: diags.LineRange{
@@ -194,10 +206,10 @@ func (c TemplateCheck) Check(ctx context.Context, entry discovery.Entry, _ []dis
 }
 
 func (c TemplateCheck) checkHumanizeIsNeeded(expr parser.PromQLExpr, ann *parser.YamlKeyValue) (problems []Problem) {
-	if !hasValue(ann.Key.Value, ann.Value.Value) {
+	if !hasValue(ann.Value.Value) {
 		return problems
 	}
-	if hasHumanize(ann.Key.Value, ann.Value.Value) {
+	if hasHumanize(ann.Value.Value) {
 		return problems
 	}
 	vars, aliases, ok := findTemplateVariables(ann.Key.Value, ann.Value.Value)
@@ -295,7 +307,7 @@ func maybeExpandError(err error) error {
 func checkTemplateSyntax(ctx context.Context, name, text string, data any) error {
 	tmpl := promTemplate.NewTemplateExpander(
 		ctx,
-		strings.Join(append(templateDefs, text), ""),
+		templateDefsString+text,
 		name,
 		data,
 		model.Time(timestamp.FromTime(time.Now())),
@@ -316,18 +328,17 @@ func checkTemplateSyntax(ctx context.Context, name, text string, data any) error
 	return nil
 }
 
-func checkForValueInLabels(name, text string) (msgs []string) {
-	t, err := textTemplate.
-		New(name).
-		Funcs(templateFuncMap).
-		Option("missingkey=zero").
-		Parse(strings.Join(append(templateDefs, text), ""))
+func checkForValueInLabels(text string) (msgs []string) {
+	t := templatePool.Get().(*textTemplate.Template)
+	defer templatePool.Put(t)
+
+	tt, err := t.Parse(templateDefsString + text)
 	if err != nil {
 		// no need to double report errors
 		return nil
 	}
-	aliases := aliasesForTemplate(t)
-	for _, node := range t.Root.Nodes {
+	aliases := aliasesForTemplate(tt)
+	for _, node := range tt.Root.Nodes {
 		if v, ok := containsAliasedNode(aliases, node, ".Value"); ok {
 			msg := fmt.Sprintf("Using `%s` in labels will generate a new alert on every value change, move it to annotations.", v)
 			msgs = append(msgs, msg)
@@ -348,17 +359,16 @@ func containsAliasedNode(am aliasMap, node parse.Node, alias string) (string, bo
 	return "", false
 }
 
-func hasValue(name, text string) bool {
-	t, err := textTemplate.
-		New(name).
-		Funcs(templateFuncMap).
-		Option("missingkey=zero").
-		Parse(strings.Join(append(templateDefs, text), ""))
+func hasValue(text string) bool {
+	t := templatePool.Get().(*textTemplate.Template)
+	defer templatePool.Put(t)
+
+	tt, err := t.Parse(templateDefsString + text)
 	if err != nil {
 		// no need to double report errors
 		return false
 	}
-	aliases := aliasesForTemplate(t)
+	aliases := aliasesForTemplate(tt)
 	for _, node := range t.Root.Nodes {
 		if _, ok := containsAliasedNode(aliases, node, ".Value"); ok {
 			return true
@@ -367,19 +377,18 @@ func hasValue(name, text string) bool {
 	return false
 }
 
-func hasHumanize(name, text string) bool {
-	t, err := textTemplate.
-		New(name).
-		Funcs(templateFuncMap).
-		Option("missingkey=zero").
-		Parse(strings.Join(append(templateDefs, text), ""))
+func hasHumanize(text string) bool {
+	t := templatePool.Get().(*textTemplate.Template)
+	defer templatePool.Put(t)
+
+	tt, err := t.Parse(templateDefsString + text)
 	if err != nil {
 		// no need to double report errors
 		return false
 	}
-	aliases := aliasesForTemplate(t)
+	aliases := aliasesForTemplate(tt)
 
-	for _, node := range t.Root.Nodes {
+	for _, node := range tt.Root.Nodes {
 		if _, ok := containsAliasedNode(aliases, node, ".Value"); !ok {
 			continue
 		}
@@ -474,19 +483,18 @@ func getVariables(node parse.Node) (vars []tmplVar) {
 	return vars
 }
 
-func findTemplateVariables(name, text string) (vars []tmplVar, aliases aliasMap, ok bool) {
-	t, err := textTemplate.
-		New(name).
-		Funcs(templateFuncMap).
-		Option("missingkey=zero").
-		Parse(strings.Join(append(templateDefs, text), ""))
+func findTemplateVariables(_, text string) (vars []tmplVar, aliases aliasMap, ok bool) {
+	t := templatePool.Get().(*textTemplate.Template)
+	defer templatePool.Put(t)
+
+	tt, err := t.Parse(templateDefsString + text)
 	if err != nil {
 		// no need to double report errors
 		return vars, aliases, false
 	}
 
 	aliases.aliases = map[string]map[string]struct{}{}
-	for _, node := range t.Root.Nodes {
+	for _, node := range tt.Root.Nodes {
 		getAliases(node, &aliases)
 		vars = append(vars, getVariables(node)...)
 	}

internal/checks/promql_series.go

@@ -260,14 +260,14 @@ func (c SeriesCheck) Check(ctx context.Context, entry discovery.Entry, entries [
 		if err != nil {
 			slog.LogAttrs(ctx, slog.LevelWarn, "Cannot detect Prometheus uptime gaps", slog.Any("err", err), slog.String("name", c.prom.Name()))
 		}
-		if promUptime != nil && promUptime.Series.Ranges.Len() == 0 {
+		if promUptime != nil && len(promUptime.Series.Ranges) == 0 {
 			slog.LogAttrs(ctx, slog.LevelWarn,
 				"No results for Prometheus uptime metric, you might have set uptime config option to a missing metric, please check your config",
 				slog.String("name", c.prom.Name()),
 				slog.String("metric", c.prom.UptimeMetric()),
 			)
 		}
-		if promUptime == nil || promUptime.Series.Ranges.Len() == 0 {
+		if promUptime == nil || len(promUptime.Series.Ranges) == 0 {
 			slog.LogAttrs(ctx, slog.LevelWarn,
 				"Using dummy Prometheus uptime metric results with no gaps",
 				slog.String("name", c.prom.Name()),
@@ -401,7 +401,7 @@ func (c SeriesCheck) Check(ctx context.Context, entry discovery.Entry, entries [
 				continue
 			}
 
-			if trsLabelCount.Series.Ranges.Len() == 1 && len(trsLabelCount.Series.Gaps) == 0 {
+			if len(trsLabelCount.Series.Ranges) == 1 && len(trsLabelCount.Series.Gaps) == 0 {
 				problems = append(problems, Problem{
 					Anchor:   AnchorAfter,
 					Lines:    expr.Value.Pos.Lines(),

internal/config/rule.go

@@ -5,6 +5,7 @@ import (
 	"log/slog"
 	"regexp"
 	"slices"
+	"strings"
 	"time"
 
 	"github.com/cloudflare/pint/internal/checks"
@@ -148,10 +149,9 @@ func (rule Rule) validate() (err error) {
 }
 
 func isDisabledForRule(rule parser.Rule, name string, check checks.RuleChecker, promTags []string) bool {
-	matches := []string{
-		name,
-		check.String(),
-	}
+	matches := make([]string, 0, len(promTags)+2)
+	matches = append(matches, name)
+	matches = append(matches, check.String())
 	for _, tag := range promTags {
 		matches = append(matches, name+"(+"+tag+")")
 	}
@@ -197,9 +197,11 @@ func isEnabled(enabledChecks, disabledChecks []string, rule parser.Rule, name st
 		if c == name || c == check.String() {
 			return false
 		}
-		for _, tag := range promTags {
-			if c == name+"(+"+tag+")" {
-				return false
+		if strings.HasPrefix(c, name) {
+			for _, tag := range promTags {
+				if c == name+"(+"+tag+")" {
+					return false
+				}
 			}
 		}
 	}

internal/parser/utils/source.go

@@ -895,6 +895,7 @@ func parseCall(expr string, n *promParser.Call) (src []Source) {
 }
 
 func parseBinOps(expr string, n *promParser.BinaryExpr) (src []Source) {
+	src = make([]Source, 0, 2)
 	switch {
 	// foo{} + 1
 	// 1 + foo{}

internal/promapi/range.go

@@ -9,7 +9,7 @@ import (
 	"math"
 	"net/http"
 	"net/url"
-	"sort"
+	"slices"
 	"strconv"
 	"sync"
 	"time"
@@ -101,13 +101,13 @@ func (prom *Prometheus) RangeQuery(ctx context.Context, expr string, params Rang
 	lookback := params.Dur()
 	step := params.Step()
 
-	var slices []TimeRange
+	var timeSlices []TimeRange
 	queryStep := (time.Hour * 2).Round(step)
 	if queryStep > lookback {
 		queryStep = lookback
-		slices = append(slices, TimeRange{Start: start, End: end})
+		timeSlices = append(timeSlices, TimeRange{Start: start, End: end})
 	} else {
-		slices = sliceRange(start, end, step, queryStep)
+		timeSlices = sliceRange(start, end, step, queryStep)
 	}
 
 	slog.LogAttrs(ctx, slog.LevelDebug, "Scheduling prometheus range query",
@@ -116,7 +116,7 @@ func (prom *Prometheus) RangeQuery(ctx context.Context, expr string, params Rang
 		slog.String("lookback", output.HumanizeDuration(lookback)),
 		slog.String("step", output.HumanizeDuration(step)),
 		slog.String("slice", output.HumanizeDuration(queryStep)),
-		slog.Int("slices", len(slices)),
+		slog.Int("slices", len(timeSlices)),
 	)
 
 	key := APIPathQueryRange + "\n" + expr + "\n" + params.String()
@@ -129,8 +129,8 @@ func (prom *Prometheus) RangeQuery(ctx context.Context, expr string, params Rang
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	results := make(chan queryResult, len(slices))
-	for _, s := range slices {
+	results := make(chan queryResult, len(timeSlices))
+	for _, s := range timeSlices {
 		query := queryRequest{ // nolint: exhaustruct
 			query: rangeQuery{
 				prom: prom,
@@ -200,7 +200,7 @@ func (prom *Prometheus) RangeQuery(ctx context.Context, expr string, params Rang
 		return nil, QueryError{err: lastErr, msg: decodeError(lastErr)}
 	}
 
-	sort.Stable(merged.Series.Ranges)
+	slices.SortStableFunc(merged.Series.Ranges, CompareMetricTimeRanges)
 
 	slog.LogAttrs(ctx, slog.LevelDebug,
 		"Parsed range response",

internal/promapi/range_normalize.go

@@ -9,51 +9,6 @@ import (
 	"github.com/prometheus/prometheus/model/labels"
 )
 
-func labelValue(ls labels.Labels, name string) (val string, ok bool) {
-	ls.Range(func(l labels.Label) {
-		if l.Name == name {
-			val = l.Value
-			ok = true
-		}
-	})
-	return val, ok
-}
-
-func labelsBefore(ls, o labels.Labels) bool {
-	if ls.Len() < o.Len() {
-		return true
-	}
-	if ls.Len() > o.Len() {
-		return false
-	}
-
-	lns := make([]string, 0, ls.Len()+o.Len())
-	ls.Range(func(l labels.Label) {
-		lns = append(lns, l.Name)
-	})
-	o.Range(func(l labels.Label) {
-		lns = append(lns, l.Name)
-	})
-	slices.Sort(lns)
-	for _, ln := range lns {
-		mlv, ok := labelValue(ls, ln)
-		if !ok {
-			return true
-		}
-		olv, ok := labelValue(o, ln)
-		if !ok {
-			return false
-		}
-		if mlv < olv {
-			return true
-		}
-		if mlv > olv {
-			return false
-		}
-	}
-	return false
-}
-
 type TimeRange struct {
 	Start time.Time
 	End   time.Time
@@ -182,19 +137,11 @@ func (mtr MetricTimeRanges) String() string {
 	return strings.Join(sl, " ** ")
 }
 
-func (mtr MetricTimeRanges) Len() int {
-	return len(mtr)
-}
-
-func (mtr MetricTimeRanges) Swap(i, j int) {
-	mtr[i], mtr[j] = mtr[j], mtr[i]
-}
-
-func (mtr MetricTimeRanges) Less(i, j int) bool {
-	if mtr[i].Fingerprint != mtr[j].Fingerprint {
-		return labelsBefore(mtr[i].Labels, mtr[j].Labels)
+func CompareMetricTimeRanges(a, b MetricTimeRange) int {
+	if a.Fingerprint != b.Fingerprint {
+		return labels.Compare(a.Labels, b.Labels)
 	}
-	return mtr[i].Start.Before(mtr[j].Start)
+	return a.Start.Compare(b.Start)
 }
 
 type SeriesTimeRanges struct {
@@ -241,19 +188,14 @@ func (str *SeriesTimeRanges) FindGaps(baseline SeriesTimeRanges, from, until tim
 // merge [t1:t2] [t2:t3] together.
 // This will sort the source slice.
 func MergeRanges(source MetricTimeRanges, step time.Duration) (MetricTimeRanges, bool) {
-	slices.SortStableFunc(source, func(a, b MetricTimeRange) int {
-		if a.Fingerprint != b.Fingerprint {
-			return labels.Compare(a.Labels, b.Labels)
-		}
-		return a.Start.Compare(b.Start)
-	})
+	slices.SortStableFunc(source, CompareMetricTimeRanges)
 
 	var (
 		ok, hadMerged bool
 		tr            TimeRange
 	)
 
-	merged := make(MetricTimeRanges, 0, len(source)/4)
+	merged := make(MetricTimeRanges, 0, len(source))
 L:
 	for i := range source {
 		for j := len(merged) - 1; j >= 0; j-- {