From 36e0897f0f6cc456915860d7f3ac57fb60ecf6cb Mon Sep 17 00:00:00 2001
From: Neeraj Gartia <80708727+NeerajGartia21@users.noreply.github.com>
Date: Thu, 28 Nov 2024 01:35:08 +0530
Subject: [PATCH] [BUGFIX] PromQL: Fix behaviour of `changes()` for mix of
 histograms and floats (#15469)

PromQL: Fix behaviour of changes() for mix of histograms and floats

---------

Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
---
 promql/functions.go                       | 51 +++++++++++++++++++----
 promql/promqltest/testdata/functions.test | 20 ++++++++-
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/promql/functions.go b/promql/functions.go
index cadac19b4..83834afe5 100644
--- a/promql/functions.go
+++ b/promql/functions.go
@@ -1429,20 +1429,53 @@ func funcResets(vals []parser.Value, args parser.Expressions, enh *EvalNodeHelpe
 // === changes(Matrix parser.ValueTypeMatrix) (Vector, Annotations) ===
 func funcChanges(vals []parser.Value, args parser.Expressions, enh *EvalNodeHelper) (Vector, annotations.Annotations) {
 	floats := vals[0].(Matrix)[0].Floats
+	histograms := vals[0].(Matrix)[0].Histograms
 	changes := 0
-
-	if len(floats) == 0 {
-		// TODO(beorn7): Only histogram values, still need to add support.
+	if len(floats) == 0 && len(histograms) == 0 {
 		return enh.Out, nil
 	}
 
-	prev := floats[0].F
-	for _, sample := range floats[1:] {
-		current := sample.F
-		if current != prev && !(math.IsNaN(current) && math.IsNaN(prev)) {
-			changes++
+	var prevSample, curSample Sample
+	for iFloat, iHistogram := 0, 0; iFloat < len(floats) || iHistogram < len(histograms); {
+		switch {
+		// Process a float sample if no histogram sample remains or its timestamp is earlier.
+		// Process a histogram sample if no float sample remains or its timestamp is earlier.
+		case iHistogram >= len(histograms) || iFloat < len(floats) && floats[iFloat].T < histograms[iHistogram].T:
+			{
+				curSample.F = floats[iFloat].F
+				curSample.H = nil
+				iFloat++
+			}
+		case iFloat >= len(floats) || iHistogram < len(histograms) && floats[iFloat].T > histograms[iHistogram].T:
+			{
+				curSample.H = histograms[iHistogram].H
+				iHistogram++
+			}
 		}
-		prev = current
+		// Skip the comparison for the first sample, just initialize prevSample.
+		if iFloat+iHistogram == 1 {
+			prevSample = curSample
+			continue
+		}
+		switch {
+		case prevSample.H == nil && curSample.H == nil:
+			{
+				if curSample.F != prevSample.F && !(math.IsNaN(curSample.F) && math.IsNaN(prevSample.F)) {
+					changes++
+				}
+			}
+		case prevSample.H != nil && curSample.H == nil, prevSample.H == nil && curSample.H != nil:
+			{
+				changes++
+			}
+		case prevSample.H != nil && curSample.H != nil:
+			{
+				if !curSample.H.Equals(prevSample.H) {
+					changes++
+				}
+			}
+		}
+		prevSample = curSample
 	}
 
 	return append(enh.Out, Sample{F: float64(changes)}), nil
diff --git a/promql/promqltest/testdata/functions.test b/promql/promqltest/testdata/functions.test
index 1c832c89d..c8a2ea34a 100644
--- a/promql/promqltest/testdata/functions.test
+++ b/promql/promqltest/testdata/functions.test
@@ -2,7 +2,10 @@
 load 5m
 	http_requests{path="/foo"}	1 2 3 0 1 0 0 1 2 0
 	http_requests{path="/bar"}	1 2 3 4 5 1 2 3 4 5
-	http_requests{path="/biz"}	0 0 0 0 0 1 1 1 1 1
+	http_requests{path="/biz"}  0 0 0 0 0 1 1 1 1 1
+    http_requests_histogram{path="/foo"}    {{schema:0 sum:1 count:1}}x9
+    http_requests_histogram{path="/bar"}    0 0 0 0 0 0 0 0 {{schema:0 sum:1 count:1}} {{schema:0 sum:1 count:1}}
+    http_requests_histogram{path="/biz"}    0 1 0 2 0 3 0 {{schema:0 sum:1 count:1}} {{schema:0 sum:2 count:2}} {{schema:0 sum:1 count:1}}
 
 # Tests for resets().
 eval instant at 50m resets(http_requests[5m])
@@ -69,6 +72,21 @@ eval instant at 50m changes((http_requests[50m]))
 
 eval instant at 50m changes(nonexistent_metric[50m])
 
+# Test for mix of floats and histograms.
+# Because of bug #14172 we are not able to test more complex cases like below:
+# 0 1 2 {{schema:0 sum:1 count:1}} 3 {{schema:0 sum:2 count:2}} 4 {{schema:0 sum:3 count:3}}.
+eval instant at 50m changes(http_requests_histogram[5m])
+
+eval instant at 50m changes(http_requests_histogram[6m])
+    {path="/foo"} 0
+    {path="/bar"} 0
+    {path="/biz"} 0
+
+eval instant at 50m changes(http_requests_histogram[60m])
+    {path="/foo"} 0
+    {path="/bar"} 1
+    {path="/biz"} 9
+
 clear
 
 load 5m