Prometheus: Make sure "Min Step" has precedence (#115941)

* set minStep value as final step value when set explicitly. * enhance it with tests * improve function readability * a bit more improvement for readability
2026-01-15 00:50:10 +01:00
parent cddc4776ef
commit 7ae2eed876
4 changed files with 826 additions and 364 deletions
@@ -92,7 +92,6 @@ const (
 )

 // Internal interval and range variables with {} syntax
-// Repetitive code, we should have functionality to unify these
 const (
 	varIntervalAlt       = "${__interval}"
 	varIntervalMsAlt     = "${__interval_ms}"
@@ -112,8 +111,16 @@ const (
 	UnknownQueryType  TimeSeriesQueryType = "unknown"
 )

+// safeResolution is the maximum number of data points to prevent excessive resolution.
+// This ensures queries don't exceed reasonable data point limits, improving performance
+// and preventing potential memory issues. The value of 11000 provides a good balance
+// between resolution and performance for most use cases.
 var safeResolution = 11000

+// rateIntervalMultiplier is the minimum multiplier for rate interval calculation.
+// Rate intervals should be at least 4x the scrape interval to ensure accurate rate calculations.
+const rateIntervalMultiplier = 4
+
 // QueryModel includes both the common and specific values
 // NOTE: this struct may have issues when decoding JSON that requires the special handling
 // registered in https://github.com/grafana/grafana-plugin-sdk-go/blob/v0.228.0/experimental/apis/data/v0alpha1/query.go#L298
@@ -154,7 +161,7 @@ type Query struct {
 // may be either a string or DataSourceRef
 type internalQueryModel struct {
 	PrometheusQueryProperties `json:",inline"`
-	//sdkapi.CommonQueryProperties `json:",inline"`
+	// sdkapi.CommonQueryProperties `json:",inline"`
 	IntervalMS float64 `json:"intervalMs,omitempty"`

 	// The following properties may be part of the request payload, however they are not saved in panel JSON
@@ -272,44 +279,121 @@ func (query *Query) TimeRange() TimeRange {
 	}
 }

+// isRateIntervalVariable checks if the interval string is a rate interval variable
+// ($__rate_interval, ${__rate_interval}, $__rate_interval_ms, or ${__rate_interval_ms})
+func isRateIntervalVariable(interval string) bool {
+	return interval == varRateInterval ||
+		interval == varRateIntervalAlt ||
+		interval == varRateIntervalMs ||
+		interval == varRateIntervalMsAlt
+}
+
+// replaceVariable replaces both $__variable and ${__variable} formats in the expression
+func replaceVariable(expr, dollarFormat, altFormat, replacement string) string {
+	expr = strings.ReplaceAll(expr, dollarFormat, replacement)
+	expr = strings.ReplaceAll(expr, altFormat, replacement)
+	return expr
+}
+
+// isManualIntervalOverride checks if the interval is a manually specified non-variable value
+// that should override the calculated interval
+func isManualIntervalOverride(interval string) bool {
+	return interval != "" &&
+		interval != varInterval &&
+		interval != varIntervalAlt &&
+		interval != varIntervalMs &&
+		interval != varIntervalMsAlt
+}
+
+// maxDuration returns the maximum of two durations
+func maxDuration(a, b time.Duration) time.Duration {
+	if a > b {
+		return a
+	}
+	return b
+}
+
+// normalizeIntervalFactor ensures intervalFactor is at least 1
+func normalizeIntervalFactor(factor int64) int64 {
+	if factor == 0 {
+		return 1
+	}
+	return factor
+}
+
+// calculatePrometheusInterval calculates the optimal step interval for a Prometheus query.
+//
+// The function determines the query step interval by considering multiple factors:
+//   - The minimum step specified in the query (queryInterval)
+//   - The data source scrape interval (dsScrapeInterval)
+//   - The requested interval in milliseconds (intervalMs)
+//   - The time range and maximum data points from the query
+//   - The interval factor multiplier
+//
+// Special handling:
+//   - Variable intervals ($__interval, $__rate_interval, etc.) are replaced with calculated values
+//   - Rate interval variables ($__rate_interval, ${__rate_interval}) use calculateRateInterval for proper rate() function support
+//   - Manual interval overrides (non-variable strings) take precedence over calculated values
+//   - The final interval ensures safe resolution limits are not exceeded
+//
+// Parameters:
+//   - queryInterval: The minimum step interval string (may contain variables like $__interval or $__rate_interval)
+//   - dsScrapeInterval: The data source scrape interval (e.g., "15s", "30s")
+//   - intervalMs: The requested interval in milliseconds
+//   - intervalFactor: Multiplier for the calculated interval (defaults to 1 if 0)
+//   - query: The backend data query containing time range and max data points
+//   - intervalCalculator: Calculator for determining optimal intervals
+//
+// Returns:
+//   - The calculated step interval as a time.Duration
+//   - An error if the interval cannot be calculated (e.g., invalid interval string)
 func calculatePrometheusInterval(
 	queryInterval, dsScrapeInterval string,
 	intervalMs, intervalFactor int64,
 	query backend.DataQuery,
 	intervalCalculator intervalv2.Calculator,
 ) (time.Duration, error) {
-	// we need to compare the original query model after it is overwritten below to variables so that we can
-	// calculate the rateInterval if it is equal to $__rate_interval or ${__rate_interval}
+	// Preserve the original interval for later comparison, as it may be modified below
 	originalQueryInterval := queryInterval

-	// If we are using variable for interval/step, we will replace it with calculated interval
+	// If we are using a variable for minStep, replace it with empty string
+	// so that the interval calculation proceeds with the default logic
 	if isVariableInterval(queryInterval) {
 		queryInterval = ""
 	}

+	// Get the minimum interval from various sources (dsScrapeInterval, queryInterval, intervalMs)
 	minInterval, err := gtime.GetIntervalFrom(dsScrapeInterval, queryInterval, intervalMs, 15*time.Second)
 	if err != nil {
 		return time.Duration(0), err
 	}
+
+	// Calculate the optimal interval based on time range and max data points
 	calculatedInterval := intervalCalculator.Calculate(query.TimeRange, minInterval, query.MaxDataPoints)
+	// Calculate the safe interval to prevent too many data points
 	safeInterval := intervalCalculator.CalculateSafeInterval(query.TimeRange, int64(safeResolution))

-	adjustedInterval := safeInterval.Value
-	if calculatedInterval.Value > safeInterval.Value {
-		adjustedInterval = calculatedInterval.Value
-	}
+	// Use the larger of calculated or safe interval to ensure we don't exceed resolution limits
+	adjustedInterval := maxDuration(calculatedInterval.Value, safeInterval.Value)

-	// here is where we compare for $__rate_interval or ${__rate_interval}
-	if originalQueryInterval == varRateInterval || originalQueryInterval == varRateIntervalAlt {
+	// Handle rate interval variables: these require special calculation
+	if isRateIntervalVariable(originalQueryInterval) {
 		// Rate interval is final and is not affected by resolution
 		return calculateRateInterval(adjustedInterval, dsScrapeInterval), nil
-	} else {
-		queryIntervalFactor := intervalFactor
-		if queryIntervalFactor == 0 {
-			queryIntervalFactor = 1
-		}
-		return time.Duration(int64(adjustedInterval) * queryIntervalFactor), nil
 	}
+
+	// Handle manual interval override: if user specified a non-variable interval,
+	// it takes precedence over calculated values
+	if isManualIntervalOverride(originalQueryInterval) {
+		if parsedInterval, err := gtime.ParseIntervalStringToTimeDuration(originalQueryInterval); err == nil {
+			return parsedInterval, nil
+		}
+		// If parsing fails, fall through to calculated interval with factor
+	}
+
+	// Apply interval factor to the adjusted interval
+	normalizedFactor := normalizeIntervalFactor(intervalFactor)
+	return time.Duration(int64(adjustedInterval) * normalizedFactor), nil
 }

 // calculateRateInterval calculates the $__rate_interval value
@@ -331,7 +415,8 @@ func calculateRateInterval(
 		return time.Duration(0)
 	}

-	rateInterval := time.Duration(int64(math.Max(float64(queryInterval+scrapeIntervalDuration), float64(4)*float64(scrapeIntervalDuration))))
+	minRateInterval := rateIntervalMultiplier * scrapeIntervalDuration
+	rateInterval := maxDuration(queryInterval+scrapeIntervalDuration, minRateInterval)
 	return rateInterval
 }

@@ -366,34 +451,33 @@ func InterpolateVariables(
 		rateInterval = calculateRateInterval(queryInterval, requestedMinStep)
 	}

-	expr = strings.ReplaceAll(expr, varIntervalMs, strconv.FormatInt(int64(calculatedStep/time.Millisecond), 10))
-	expr = strings.ReplaceAll(expr, varInterval, gtime.FormatInterval(calculatedStep))
-	expr = strings.ReplaceAll(expr, varRangeMs, strconv.FormatInt(rangeMs, 10))
-	expr = strings.ReplaceAll(expr, varRangeS, strconv.FormatInt(rangeSRounded, 10))
-	expr = strings.ReplaceAll(expr, varRange, strconv.FormatInt(rangeSRounded, 10)+"s")
-	expr = strings.ReplaceAll(expr, varRateIntervalMs, strconv.FormatInt(int64(rateInterval/time.Millisecond), 10))
-	expr = strings.ReplaceAll(expr, varRateInterval, rateInterval.String())
+	// Replace interval variables (both $__var and ${__var} formats)
+	expr = replaceVariable(expr, varIntervalMs, varIntervalMsAlt, strconv.FormatInt(int64(calculatedStep/time.Millisecond), 10))
+	expr = replaceVariable(expr, varInterval, varIntervalAlt, gtime.FormatInterval(calculatedStep))
+
+	// Replace range variables (both $__var and ${__var} formats)
+	expr = replaceVariable(expr, varRangeMs, varRangeMsAlt, strconv.FormatInt(rangeMs, 10))
+	expr = replaceVariable(expr, varRangeS, varRangeSAlt, strconv.FormatInt(rangeSRounded, 10))
+	expr = replaceVariable(expr, varRange, varRangeAlt, strconv.FormatInt(rangeSRounded, 10)+"s")
+
+	// Replace rate interval variables (both $__var and ${__var} formats)
+	expr = replaceVariable(expr, varRateIntervalMs, varRateIntervalMsAlt, strconv.FormatInt(int64(rateInterval/time.Millisecond), 10))
+	expr = replaceVariable(expr, varRateInterval, varRateIntervalAlt, rateInterval.String())

-	// Repetitive code, we should have functionality to unify these
-	expr = strings.ReplaceAll(expr, varIntervalMsAlt, strconv.FormatInt(int64(calculatedStep/time.Millisecond), 10))
-	expr = strings.ReplaceAll(expr, varIntervalAlt, gtime.FormatInterval(calculatedStep))
-	expr = strings.ReplaceAll(expr, varRangeMsAlt, strconv.FormatInt(rangeMs, 10))
-	expr = strings.ReplaceAll(expr, varRangeSAlt, strconv.FormatInt(rangeSRounded, 10))
-	expr = strings.ReplaceAll(expr, varRangeAlt, strconv.FormatInt(rangeSRounded, 10)+"s")
-	expr = strings.ReplaceAll(expr, varRateIntervalMsAlt, strconv.FormatInt(int64(rateInterval/time.Millisecond), 10))
-	expr = strings.ReplaceAll(expr, varRateIntervalAlt, rateInterval.String())
 	return expr
 }

+// isVariableInterval checks if the interval string is a variable interval
+// (any of $__interval, ${__interval}, $__interval_ms, ${__interval_ms}, $__rate_interval, ${__rate_interval}, etc.)
 func isVariableInterval(interval string) bool {
-	if interval == varInterval || interval == varIntervalMs || interval == varRateInterval || interval == varRateIntervalMs {
-		return true
-	}
-	// Repetitive code, we should have functionality to unify these
-	if interval == varIntervalAlt || interval == varIntervalMsAlt || interval == varRateIntervalAlt || interval == varRateIntervalMsAlt {
-		return true
-	}
-	return false
+	return interval == varInterval ||
+		interval == varIntervalAlt ||
+		interval == varIntervalMs ||
+		interval == varIntervalMsAlt ||
+		interval == varRateInterval ||
+		interval == varRateIntervalAlt ||
+		interval == varRateIntervalMs ||
+		interval == varRateIntervalMsAlt
 }

 // AlignTimeRange aligns query range to step and handles the time offset.
@@ -410,7 +494,7 @@ func AlignTimeRange(t time.Time, step time.Duration, offset int64) time.Time {
 //go:embed query.types.json
 var f embed.FS

-// QueryTypeDefinitionsJSON returns the query type definitions
+// QueryTypeDefinitionListJSON returns the query type definitions
 func QueryTypeDefinitionListJSON() (json.RawMessage, error) {
 	return f.ReadFile("query.types.json")
 }