From 54b9537a76acaf148874f45860e59d5a085129f5 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin 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 | 1 + 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/app/vmselect/promql/transform.go b/app/vmselect/promql/transform.go index 279d2e6c3b..5397751739 100644 --- a/app/vmselect/promql/transform.go +++ b/app/vmselect/promql/transform.go @@ -552,33 +552,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 { @@ -590,13 +589,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 0261353ce3..6a4b29d01b 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 a35d55dfd6..ea7a554886 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -39,6 +39,7 @@ created by v1.90.0 or newer versions. The solution is to upgrade to v1.90.0 or n * BUGFIX: [vmbackup](https://docs.victoriametrics.com/vmbackup.html): fix snapshot not being deleted in case of error during backup. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/2055). * BUGFIX: allow using dashes and dots in environment variables names referred in config files via `%{ENV-VAR.SYNTAX}`. See [these docs](https://docs.victoriametrics.com/#environment-variables) and [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3999). * BUGFIX: return back query performance scalability on hosts with big number of CPU cores. The scalability has been reduced in [v1.86.0](https://docs.victoriametrics.com/CHANGELOG.html#v1860). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3966). +* 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.89.1](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.89.1)