Merge pull request #1474 from cloudflare/query/cost

Don't suggest bogus replacements

Author: prymitive

docs/changelog.md

@@ -5,6 +5,8 @@
 ### Fixed
 
 - Fixed false positive reports from [promql/fragile](checks/promql/fragile.md) check - [#1466](https://github.com/cloudflare/pint/issues/1466).
+- Fixed incorrect reports from [query/cost](checks/query/cost.md) when multiple rules with `rate(...)` were present
+  and each had a different rate time window.
 
 ## v0.74.2
 

internal/checks/query_cost.go

@@ -251,6 +251,13 @@ func (c CostCheck) suggestRecordingRules(
 						continue
 					}
 
+					// Don't replace the whole rule, that's usually not what we want.
+					// Useful suggestions should find parts of the query that can be improved.
+					if op.PositionRange().Start == expr.Query.Expr.PositionRange().Start &&
+						op.PositionRange().End == expr.Query.Expr.PositionRange().End {
+						continue
+					}
+
 					sq := c.rewriteRuleFragment(expr.Value.Value, op.PositionRange(), other.Rule.RecordingRule.Record.Value+extra)
 					var details strings.Builder
 					qr, afterSeries, err := c.getQueryCost(ctx, sq)
@@ -414,9 +421,10 @@ func (c CostCheck) isSuggestionFor(src, potential utils.Source, join *utils.Join
 	//   * sum -> rate -> selector
 	//   * rate -> selector
 	// - On same the selector.
-	// - With the same labels possible. All? Only from joins?
+	// - With the same labels possible.
 
 	// Src must have all operations potential does, so skip checks if potential is shorter.
+	// Ideally potential is shorter than src because we're looking to speed up part of the query, not replace it.
 	if len(potential.Operations) > len(src.Operations) {
 		return nil, "", false, false
 	}

internal/checks/query_cost_test.go

@@ -1325,6 +1325,56 @@ func TestCostCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "suggest recording rule / ignore whole rule",
+			content: `- record: sum:foo:rate5m
+  expr: sum(rate(foo_total[5m]))
+`,
+			checker: func(prom *promapi.FailoverGroup) checks.RuleChecker {
+				return checks.NewCostCheck(prom, 100, 100, 0, 0, "check comment", checks.Warning)
+			},
+			prometheus: newSimpleProm,
+			entries: mustParseContent(`
+
+- record: sum:foo:rate15m
+  expr: sum(rate(foo_total[15m]))
+- record: sum:foo:rate30m
+  expr: sum(rate(foo_total[30m]))
+`),
+			mocks: []*prometheusMock{
+				{
+					conds: []requestCondition{
+						requireQueryPath,
+						formCond{key: "query", value: "count(sum(rate(foo_total[5m])))"},
+					},
+					resp: vectorResponse{
+						samples: []*model.Sample{
+							generateSample(map[string]string{}),
+						},
+						stats: promapi.QueryStats{
+							Samples: promapi.QuerySamples{
+								TotalQueryableSamples: 50,
+								PeakSamples:           50,
+							},
+							Timings: promapi.QueryTimings{
+								EvalTotalTime: 10,
+							},
+						},
+					},
+				},
+				{
+					conds: []requestCondition{
+						requireQueryPath,
+						formCond{key: "query", value: checks.BytesPerSampleQuery},
+					},
+					resp: vectorResponse{
+						samples: []*model.Sample{
+							generateSampleWithValue(map[string]string{}, 2048),
+						},
+					},
+				},
+			},
+		},
 	}
 
 	runTests(t, testCases)

internal/checks/query_cost_test.snap

@@ -457,6 +457,11 @@
 
 ---
 
+[TestCostCheck/suggest_recording_rule_/_ignore_whole_rule - 1]
+[]
+
+---
+
 [TestCostCheck/suggest_recording_rule_/_irate_vs_rate - 1]
 - description: suggest recording rule / irate vs rate
   content: |4