From b8bcaef14da44eb384e8a07655da35614dc97467 Mon Sep 17 00:00:00 2001 From: Fiona Liao Date: Wed, 29 Nov 2023 10:39:12 +0000 Subject: [PATCH] Fix histogram append errors (#13201) * Fix histogram append errors We should check counterReset condition rather than okToAppend because if there's a counter reset, okToAppend is always set to false. Signed-off-by: Fiona Liao --- tsdb/chunkenc/float_histogram.go | 6 ++-- tsdb/chunkenc/float_histogram_test.go | 48 +++++++++++++++++++++++++++ tsdb/chunkenc/histogram.go | 6 ++-- tsdb/chunkenc/histogram_test.go | 48 +++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 6 deletions(-) diff --git a/tsdb/chunkenc/float_histogram.go b/tsdb/chunkenc/float_histogram.go index dd35b9cae4..3d76cdf65f 100644 --- a/tsdb/chunkenc/float_histogram.go +++ b/tsdb/chunkenc/float_histogram.go @@ -605,10 +605,10 @@ func (a *FloatHistogramAppender) AppendFloatHistogram(prev *FloatHistogramAppend pForwardInserts, nForwardInserts, okToAppend, counterReset := a.appendable(h) if !okToAppend || counterReset { if appendOnly { - if !okToAppend { - return nil, false, a, fmt.Errorf("float histogram schema change") + if counterReset { + return nil, false, a, fmt.Errorf("float histogram counter reset") } - return nil, false, a, fmt.Errorf("float histogram counter reset") + return nil, false, a, fmt.Errorf("float histogram schema change") } newChunk := NewFloatHistogramChunk() app, err := newChunk.Appender() diff --git a/tsdb/chunkenc/float_histogram_test.go b/tsdb/chunkenc/float_histogram_test.go index a54125fdcb..33a4bbf1cf 100644 --- a/tsdb/chunkenc/float_histogram_test.go +++ b/tsdb/chunkenc/float_histogram_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "github.com/prometheus/prometheus/model/histogram" + "github.com/prometheus/prometheus/tsdb/tsdbutil" ) type floatResult struct { @@ -928,3 +929,50 @@ func TestFloatHistogramChunkAppendableGauge(t *testing.T) { assertRecodedFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType) } } + +func TestFloatHistogramAppendOnlyErrors(t *testing.T) { + t.Run("schema change error", func(t *testing.T) { + c := Chunk(NewFloatHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + + h := tsdbutil.GenerateTestFloatHistogram(0) + var isRecoded bool + c, isRecoded, app, err = app.AppendFloatHistogram(nil, 1, h, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.NoError(t, err) + + // Add erroring histogram. + h2 := h.Copy() + h2.Schema++ + c, isRecoded, _, err = app.AppendFloatHistogram(nil, 2, h2, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.EqualError(t, err, "float histogram schema change") + }) + t.Run("counter reset error", func(t *testing.T) { + c := Chunk(NewFloatHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + + h := tsdbutil.GenerateTestFloatHistogram(0) + var isRecoded bool + c, isRecoded, app, err = app.AppendFloatHistogram(nil, 1, h, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.NoError(t, err) + + // Add erroring histogram. + h2 := h.Copy() + h2.CounterResetHint = histogram.CounterReset + c, isRecoded, _, err = app.AppendFloatHistogram(nil, 2, h2, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.EqualError(t, err, "float histogram counter reset") + }) +} diff --git a/tsdb/chunkenc/histogram.go b/tsdb/chunkenc/histogram.go index 7c6f07f1f2..ac84e7a1ef 100644 --- a/tsdb/chunkenc/histogram.go +++ b/tsdb/chunkenc/histogram.go @@ -640,10 +640,10 @@ func (a *HistogramAppender) AppendHistogram(prev *HistogramAppender, t int64, h pForwardInserts, nForwardInserts, okToAppend, counterReset := a.appendable(h) if !okToAppend || counterReset { if appendOnly { - if !okToAppend { - return nil, false, a, fmt.Errorf("histogram schema change") + if counterReset { + return nil, false, a, fmt.Errorf("histogram counter reset") } - return nil, false, a, fmt.Errorf("histogram counter reset") + return nil, false, a, fmt.Errorf("histogram schema change") } newChunk := NewHistogramChunk() app, err := newChunk.Appender() diff --git a/tsdb/chunkenc/histogram_test.go b/tsdb/chunkenc/histogram_test.go index 25e415da76..3b6efb811c 100644 --- a/tsdb/chunkenc/histogram_test.go +++ b/tsdb/chunkenc/histogram_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "github.com/prometheus/prometheus/model/histogram" + "github.com/prometheus/prometheus/tsdb/tsdbutil" ) type result struct { @@ -1129,3 +1130,50 @@ func TestHistogramChunkAppendableGauge(t *testing.T) { require.Equal(t, GaugeType, c.(*HistogramChunk).GetCounterResetHeader()) } } + +func TestHistogramAppendOnlyErrors(t *testing.T) { + t.Run("schema change error", func(t *testing.T) { + c := Chunk(NewHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + + h := tsdbutil.GenerateTestHistogram(0) + var isRecoded bool + c, isRecoded, app, err = app.AppendHistogram(nil, 1, h, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.NoError(t, err) + + // Add erroring histogram. + h2 := h.Copy() + h2.Schema++ + c, isRecoded, _, err = app.AppendHistogram(nil, 2, h2, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.EqualError(t, err, "histogram schema change") + }) + t.Run("counter reset error", func(t *testing.T) { + c := Chunk(NewHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + + h := tsdbutil.GenerateTestHistogram(0) + var isRecoded bool + c, isRecoded, app, err = app.AppendHistogram(nil, 1, h, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.NoError(t, err) + + // Add erroring histogram. + h2 := h.Copy() + h2.CounterResetHint = histogram.CounterReset + c, isRecoded, _, err = app.AppendHistogram(nil, 2, h2, true) + require.Nil(t, c) + require.False(t, isRecoded) + require.EqualError(t, err, "histogram counter reset") + }) +}