Emit error when querying native histograms with extended functions (#322

) * Emit error when querying native histograms with extended functions We saw panics on our setup because users tried to use the extended functions with native histograms, which led to rates calculated with only one sample in some windows. In this PR I am introducing a safeguard against this type of situation and emiting a warn when users try to query native histograms with extended functions. Signed-off-by: Pedro Tanaka <[email protected]> Fixing test case Signed-off-by: Pedro Tanaka <[email protected]> removing dead code Signed-off-by: Pedro Tanaka <[email protected]> Return error instead of warn Signed-off-by: Pedro Tanaka <[email protected]> fixing linting Signed-off-by: Pedro Tanaka <[email protected]> * Update execution/scan/functions.go Co-authored-by: Filip Petkovski <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]> * Removing query context Signed-off-by: Pedro Tanaka <[email protected]> --------- Signed-off-by: Pedro Tanaka <[email protected]> Co-authored-by: Filip Petkovski <[email protected]>
thanos-io · Oct 30, 2023 · dfb4b4f · dfb4b4f
1 parent 4517c0d
commit dfb4b4f
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 2 deletions.
diff --git a/engine/engine_test.go b/engine/engine_test.go
@@ -16,6 +16,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/efficientgo/core/errors"
 	"github.com/efficientgo/core/testutil"
 	"go.uber.org/goleak"
@@ -2138,6 +2140,44 @@ func TestDisabledXFunction(t *testing.T) {
 	}
 }
 
+func TestXFunctionsWithNativeHistograms(t *testing.T) {
+	defaultQueryTime := time.Unix(50, 0)
+
+	expr := "sum(xincrease(native_histogram_series[50s]))"
+
+	// Negative offset and at modifier are enabled by default
+	// since Prometheus v2.33.0, so we also enable them.
+	opts := promql.EngineOpts{
+		Timeout:              1 * time.Hour,
+		MaxSamples:           1e10,
+		EnableNegativeOffset: true,
+		EnableAtModifier:     true,
+	}
+
+	lStorage := teststorage.New(t)
+	defer lStorage.Close()
+
+	app := lStorage.Appender(context.TODO())
+	testutil.Ok(t, generateFloatHistogramSeries(app, 3000, false))
+	testutil.Ok(t, app.Commit())
+
+	optimizers := logicalplan.AllOptimizers
+
+	ctx := context.Background()
+	newEngine := engine.New(engine.Opts{
+		EngineOpts:        opts,
+		DisableFallback:   true,
+		LogicalOptimizers: optimizers,
+		EnableXFunctions:  true,
+	})
+	query, err := newEngine.NewInstantQuery(ctx, lStorage, nil, expr, defaultQueryTime)
+	testutil.Ok(t, err)
+	defer query.Close()
+
+	engineResult := query.Exec(ctx)
+	require.Error(t, engineResult.Err)
+}
+
 func TestXFunctionsWhenDisabled(t *testing.T) {
 	var (
 		query = "xincrease(http_requests[50s])"

diff --git a/execution/scan/functions.go b/execution/scan/functions.go
@@ -381,6 +381,11 @@ func extendedRate(samples []Sample, isCounter, isRate bool, stepTime int64, sele
 // points[0] to be a histogram. It returns nil if any other Point in points is
 // not a histogram.
 func histogramRate(points []Sample, isCounter bool) *histogram.FloatHistogram {
+	// Calculating a rate on a single sample is not defined.
+	if len(points) < 2 {
+		return nil
+	}
+
 	prev := points[0].H // We already know that this is a histogram.
 	last := points[len(points)-1].H
 	if last == nil {

diff --git a/execution/scan/matrix_selector.go b/execution/scan/matrix_selector.go
@@ -9,13 +9,13 @@ import (
 	"sync"
 	"time"
 
+	"github.com/efficientgo/core/errors"
 	"github.com/prometheus/prometheus/model/labels"
 	"github.com/prometheus/prometheus/model/value"
+	"github.com/prometheus/prometheus/promql/parser"
 	"github.com/prometheus/prometheus/storage"
 	"github.com/prometheus/prometheus/tsdb/chunkenc"
 
-	"github.com/prometheus/prometheus/promql/parser"
-
 	"github.com/thanos-io/promql-engine/execution/function"
 	"github.com/thanos-io/promql-engine/execution/model"
 	engstore "github.com/thanos-io/promql-engine/execution/storage"
@@ -60,6 +60,8 @@ type matrixSelector struct {
 	model.OperatorTelemetry
 }
 
+var ErrNativeHistogramsNotSupported = errors.New("native histograms are not supported in extended range functions")
+
 // NewMatrixSelector creates operator which selects vector of series over time.
 func NewMatrixSelector(
 	pool *model.VectorPool,
@@ -353,6 +355,7 @@ loop:
 // TODO(fpetkovski): Add max samples limit.
 func selectExtPoints(it *storage.BufferedSeriesIterator, mint, maxt int64, out []Sample, extLookbackDelta int64, metricAppearedTs **int64) ([]Sample, error) {
 	extMint := mint - extLookbackDelta
+	selectsNativeHistograms := false
 
 	if len(out) > 0 && out[len(out)-1].T >= mint {
 		// There is an overlap between previous and current ranges, retain common
@@ -396,6 +399,7 @@ loop:
 		case chunkenc.ValNone:
 			break loop
 		case chunkenc.ValHistogram, chunkenc.ValFloatHistogram:
+			selectsNativeHistograms = true
 			t, fh := buf.AtFloatHistogram()
 			if value.IsStaleNaN(fh.Sum) {
 				continue loop
@@ -431,6 +435,7 @@ loop:
 	// The sought sample might also be in the range.
 	switch soughtValueType {
 	case chunkenc.ValHistogram, chunkenc.ValFloatHistogram:
+		selectsNativeHistograms = true
 		t, fh := it.AtFloatHistogram()
 		if t == maxt && !value.IsStaleNaN(fh.Sum) {
 			if *metricAppearedTs == nil {
@@ -448,5 +453,9 @@ loop:
 		}
 	}
 
+	if selectsNativeHistograms {
+		return nil, ErrNativeHistogramsNotSupported
+	}
+
 	return out, nil
 }