Merge pull request #12357 from krajorama/fix-histogram-appendable-emptybucket

fix HistogramAppender.appendable segfault

tsdb/chunkenc/float_histogram.go

@@ -358,9 +358,9 @@ func counterResetInAnyFloatBucket(oldBuckets []xorValue, newBuckets []float64, o
 
 		if oldIdx <= newIdx {
 			// Moving ahead old bucket and span by 1 index.
-			if oldInsideSpanIdx == oldSpans[oldSpanSliceIdx].Length-1 {
+			if oldInsideSpanIdx+1 >= oldSpans[oldSpanSliceIdx].Length {
 				// Current span is over.
-				oldSpanSliceIdx++
+				oldSpanSliceIdx = nextNonEmptySpanSliceIdx(oldSpanSliceIdx, oldSpans)
 				oldInsideSpanIdx = 0
 				if oldSpanSliceIdx >= len(oldSpans) {
 					// All old spans are over.
@@ -377,9 +377,9 @@ func counterResetInAnyFloatBucket(oldBuckets []xorValue, newBuckets []float64, o
 
 		if oldIdx > newIdx {
 			// Moving ahead new bucket and span by 1 index.
-			if newInsideSpanIdx == newSpans[newSpanSliceIdx].Length-1 {
+			if newInsideSpanIdx+1 >= newSpans[newSpanSliceIdx].Length {
 				// Current span is over.
-				newSpanSliceIdx++
+				newSpanSliceIdx = nextNonEmptySpanSliceIdx(newSpanSliceIdx, newSpans)
 				newInsideSpanIdx = 0
 				if newSpanSliceIdx >= len(newSpans) {
 					// All new spans are over.

tsdb/chunkenc/float_histogram_test.go

@@ -365,6 +365,64 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
 	}
 }
 
+func TestFloatHistogramChunkAppendableWithEmptySpan(t *testing.T) {
+	h1 := &histogram.FloatHistogram{
+		Schema:        0,
+		Count:         21,
+		Sum:           1234.5,
+		ZeroThreshold: 0.001,
+		ZeroCount:     4,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 4},
+			{Offset: 0, Length: 0},
+			{Offset: 0, Length: 3},
+		},
+		PositiveBuckets: []float64{1, 2, 1, 1, 1, 1, 1},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 4},
+			{Offset: 2, Length: 0},
+			{Offset: 2, Length: 3},
+		},
+		NegativeBuckets: []float64{1, 2, 1, 2, 2, 2, 2},
+	}
+	h2 := &histogram.FloatHistogram{
+		Schema:        0,
+		Count:         37,
+		Sum:           2345.6,
+		ZeroThreshold: 0.001,
+		ZeroCount:     5,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 4},
+			{Offset: 0, Length: 0},
+			{Offset: 0, Length: 3},
+		},
+		PositiveBuckets: []float64{1, 3, 1, 2, 1, 1, 1},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 4},
+			{Offset: 2, Length: 0},
+			{Offset: 2, Length: 3},
+		},
+		NegativeBuckets: []float64{1, 4, 2, 7, 5, 5, 2},
+	}
+
+	c := Chunk(NewFloatHistogramChunk())
+
+	// Create fresh appender and add the first histogram.
+	app, err := c.Appender()
+	require.NoError(t, err)
+	require.Equal(t, 0, c.NumSamples())
+
+	app.AppendFloatHistogram(1, h1)
+	require.Equal(t, 1, c.NumSamples())
+	hApp, _ := app.(*FloatHistogramAppender)
+
+	pI, nI, okToAppend, counterReset := hApp.Appendable(h2)
+	require.Empty(t, pI)
+	require.Empty(t, nI)
+	require.True(t, okToAppend)
+	require.False(t, counterReset)
+}
+
 func TestFloatHistogramChunkAppendableGauge(t *testing.T) {
 	c := Chunk(NewFloatHistogramChunk())
 

tsdb/chunkenc/histogram.go

@@ -386,9 +386,9 @@ func counterResetInAnyBucket(oldBuckets, newBuckets []int64, oldSpans, newSpans
 
 		if oldIdx <= newIdx {
 			// Moving ahead old bucket and span by 1 index.
-			if oldInsideSpanIdx == oldSpans[oldSpanSliceIdx].Length-1 {
+			if oldInsideSpanIdx+1 >= oldSpans[oldSpanSliceIdx].Length {
 				// Current span is over.
-				oldSpanSliceIdx++
+				oldSpanSliceIdx = nextNonEmptySpanSliceIdx(oldSpanSliceIdx, oldSpans)
 				oldInsideSpanIdx = 0
 				if oldSpanSliceIdx >= len(oldSpans) {
 					// All old spans are over.
@@ -405,9 +405,9 @@ func counterResetInAnyBucket(oldBuckets, newBuckets []int64, oldSpans, newSpans
 
 		if oldIdx > newIdx {
 			// Moving ahead new bucket and span by 1 index.
-			if newInsideSpanIdx == newSpans[newSpanSliceIdx].Length-1 {
+			if newInsideSpanIdx+1 >= newSpans[newSpanSliceIdx].Length {
 				// Current span is over.
-				newSpanSliceIdx++
+				newSpanSliceIdx = nextNonEmptySpanSliceIdx(newSpanSliceIdx, newSpans)
 				newInsideSpanIdx = 0
 				if newSpanSliceIdx >= len(newSpans) {
 					// All new spans are over.

tsdb/chunkenc/histogram_meta.go

@@ -487,3 +487,10 @@ func counterResetHint(crh CounterResetHeader, numRead uint16) histogram.CounterR
 		return histogram.UnknownCounterReset
 	}
 }
+
+// Handle pathological case of empty span when advancing span idx.
+func nextNonEmptySpanSliceIdx(idx int, spans []histogram.Span) (newIdx int) {
+	for idx++; idx < len(spans) && spans[idx].Length == 0; idx++ {
+	}
+	return idx
+}

tsdb/chunkenc/histogram_test.go

@@ -14,6 +14,7 @@
 package chunkenc
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -387,6 +388,64 @@ func TestHistogramChunkAppendable(t *testing.T) {
 	}
 }
 
+func TestHistogramChunkAppendableWithEmptySpan(t *testing.T) {
+	h1 := &histogram.Histogram{
+		Schema:        0,
+		Count:         21,
+		Sum:           1234.5,
+		ZeroThreshold: 0.001,
+		ZeroCount:     4,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 4},
+			{Offset: 0, Length: 0},
+			{Offset: 0, Length: 3},
+		},
+		PositiveBuckets: []int64{1, 1, -1, 0, 0, 0, 0},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 4},
+			{Offset: 2, Length: 0},
+			{Offset: 2, Length: 3},
+		},
+		NegativeBuckets: []int64{1, 1, -1, 1, 0, 0, 0},
+	}
+	h2 := &histogram.Histogram{
+		Schema:        0,
+		Count:         37,
+		Sum:           2345.6,
+		ZeroThreshold: 0.001,
+		ZeroCount:     5,
+		PositiveSpans: []histogram.Span{
+			{Offset: 0, Length: 4},
+			{Offset: 0, Length: 0},
+			{Offset: 0, Length: 3},
+		},
+		PositiveBuckets: []int64{1, 2, -2, 1, -1, 0, 0},
+		NegativeSpans: []histogram.Span{
+			{Offset: 1, Length: 4},
+			{Offset: 2, Length: 0},
+			{Offset: 2, Length: 3},
+		},
+		NegativeBuckets: []int64{1, 3, -2, 5, -2, 0, -3},
+	}
+
+	c := Chunk(NewHistogramChunk())
+
+	// Create fresh appender and add the first histogram.
+	app, err := c.Appender()
+	require.NoError(t, err)
+	require.Equal(t, 0, c.NumSamples())
+
+	app.AppendHistogram(1, h1)
+	require.Equal(t, 1, c.NumSamples())
+	hApp, _ := app.(*HistogramAppender)
+
+	pI, nI, okToAppend, counterReset := hApp.Appendable(h2)
+	require.Empty(t, pI)
+	require.Empty(t, nI)
+	require.True(t, okToAppend)
+	require.False(t, counterReset)
+}
+
 func TestAtFloatHistogram(t *testing.T) {
 	input := []histogram.Histogram{
 		{
@@ -514,6 +573,10 @@ func TestAtFloatHistogram(t *testing.T) {
 	app, err := chk.Appender()
 	require.NoError(t, err)
 	for i := range input {
+		if i > 0 {
+			_, _, okToAppend, _ := app.(*HistogramAppender).Appendable(&input[i])
+			require.True(t, okToAppend, fmt.Sprintf("idx: %d", i))
+		}
 		app.AppendHistogram(int64(i), &input[i])
 	}
 	it := chk.Iterator(nil)