From 1c18b3760406d26910c8262c4fbfb86d02e3cd80 Mon Sep 17 00:00:00 2001
From: Aliaksandr Valialkin <valyala@victoriametrics.com>
Date: Mon, 27 Mar 2023 18:02:26 -0700
Subject: [PATCH] app/vmselect/promql: follow-up for
 79e1c6a6fc2a5030b81ea77a0629eaeffa8ee5a9

- Document the fix at docs/CHANGELOG.md
- Add tests with multiple adjancent zero buckets
- Simplify the fix a bit

Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/296
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4021
---
 app/vmselect/promql/transform.go      | 45 +++++++++++----------------
 app/vmselect/promql/transform_test.go | 21 +++++++++++++
 docs/CHANGELOG.md                     |  2 ++
 3 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/app/vmselect/promql/transform.go b/app/vmselect/promql/transform.go
index bcebe46dd..7cb7766b3 100644
--- a/app/vmselect/promql/transform.go
+++ b/app/vmselect/promql/transform.go
@@ -548,33 +548,32 @@ func vmrangeBucketsToLE(tss []*timeseries) []*timeseries {
 		sort.Slice(xss, func(i, j int) bool { return xss[i].end < xss[j].end })
 		xssNew := make([]x, 0, len(xss)+2)
 		var xsPrev x
-		hasNonEmpty := false
 		uniqTs := make(map[string]*timeseries, len(xss))
 		for _, xs := range xss {
 			ts := xs.ts
 			if isZeroTS(ts) {
-				xsPrev = xs
+				// Skip buckets with zero values - they will be merged into a single bucket
+				// when the next non-zero bucket appears.
 
-				if uniqTs[xs.endStr] == nil {
-					uniqTs[xs.endStr] = xs.ts
-					xssNew = append(xssNew, x{
-						endStr: xs.endStr,
-						end:    xs.end,
-						ts:     copyTS(ts, xs.endStr),
-					})
-				}
+				// Do not store xs in xsPrev in order to properly create `le` time series
+				// for zero buckets.
+				// See https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4021
 				continue
 			}
-
-			hasNonEmpty = true
-			if xs.start != xsPrev.end && uniqTs[xs.startStr] == nil {
-				uniqTs[xs.startStr] = xs.ts
-				xssNew = append(xssNew, x{
-					endStr: xs.startStr,
-					end:    xs.start,
-					ts:     copyTS(ts, xs.startStr),
-				})
+			if xs.start != xsPrev.end {
+				// There is a gap between the previous bucket and the current bucket
+				// or the previous bucket is skipped because it was zero.
+				// Fill it with a time series with le=xs.start.
+				if uniqTs[xs.startStr] == nil {
+					uniqTs[xs.startStr] = xs.ts
+					xssNew = append(xssNew, x{
+						endStr: xs.startStr,
+						end:    xs.start,
+						ts:     copyTS(ts, xs.startStr),
+					})
+				}
 			}
+			// Convert the current time series to a time series with le=xs.end
 			ts.MetricName.AddTag("le", xs.endStr)
 			prevTs := uniqTs[xs.endStr]
 			if prevTs != nil {
@@ -586,13 +585,7 @@ func vmrangeBucketsToLE(tss []*timeseries) []*timeseries {
 			}
 			xsPrev = xs
 		}
-
-		if !hasNonEmpty {
-			xssNew = []x{}
-			continue
-		}
-
-		if !math.IsInf(xsPrev.end, 1) && !isZeroTS(xsPrev.ts) {
+		if xsPrev.ts != nil && !math.IsInf(xsPrev.end, 1) && !isZeroTS(xsPrev.ts) {
 			xssNew = append(xssNew, x{
 				endStr: "+Inf",
 				end:    math.Inf(1),
diff --git a/app/vmselect/promql/transform_test.go b/app/vmselect/promql/transform_test.go
index 0261353ce..6a4b29d01 100644
--- a/app/vmselect/promql/transform_test.go
+++ b/app/vmselect/promql/transform_test.go
@@ -87,6 +87,27 @@ foo{le="8.799e+05"} 5 123
 foo{le="+Inf"} 5 123`,
 	)
 
+	// Multiple adjacent empty vmrange bucket
+	f(
+		`foo{vmrange="7.743e+05...8.799e+05"} 5 123
+foo{vmrange="6.813e+05...7.743e+05"} 0 123
+foo{vmrange="5.813e+05...6.813e+05"} 0 123
+`,
+		`foo{le="7.743e+05"} 0 123
+foo{le="8.799e+05"} 5 123
+foo{le="+Inf"} 5 123`,
+	)
+	f(
+		`foo{vmrange="8.799e+05...9.813e+05"} 0 123
+foo{vmrange="7.743e+05...8.799e+05"} 5 123
+foo{vmrange="6.813e+05...7.743e+05"} 0 123
+foo{vmrange="5.813e+05...6.813e+05"} 0 123
+`,
+		`foo{le="7.743e+05"} 0 123
+foo{le="8.799e+05"} 5 123
+foo{le="+Inf"} 5 123`,
+	)
+
 	// Multiple non-empty vmrange buckets
 	f(
 		`foo{vmrange="4.084e+02...4.642e+02"} 2 123
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 60b1fb48b..98b269721 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -15,6 +15,8 @@ The following tip changes can be tested by building VictoriaMetrics components f
 
 ## v1.87.x long-time support release (LTS)
 
+* BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): properly convert [VictoriaMetrics historgram buckets](https://valyala.medium.com/improving-histogram-usability-for-prometheus-and-grafana-bc7e5df0e350) to Prometheus histogram buckets when VictoriaMetrics histogram contain zero buckets. Previously these buckets were ignored, and this could lead to missing Prometheus histogram buckets after the conversion. Thanks to @zklapow for [the fix](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/4021).
+
 ## [v1.87.4](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.87.4)
 
 Released at 2023-03-25