Skip to content

Commit

Permalink
Cherry-pick: Always return unknown hint for first sample in non-gauge…
Browse files Browse the repository at this point in the history 
… histogram chunk (#764)

* Always return unknown hint for first sample in non-gauge histogram chunk (#15343)

Always return unknown hint for first sample in non-gauge histogram chunk

---------

Signed-off-by: Fiona Liao <fiona.liao@grafana.com>
Co-authored-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
  • Loading branch information
@fionaliao @krajorama
fionaliao and krajorama authored Nov 25, 2024
1 parent 8809a3e commit 978684a
Show file tree
Hide file tree
Showing 7 changed files with 490 additions and 107 deletions.
43 changes: 30 additions & 13 deletions tsdb/chunkenc/float_histogram_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,27 @@ func TestFirstFloatHistogramExplicitCounterReset(t *testing.T) {
tests := map[string]struct {
hint histogram.CounterResetHint
expHeader CounterResetHeader
expHint histogram.CounterResetHint
}{
"CounterReset": {
hint: histogram.CounterReset,
expHeader: CounterReset,
expHint: histogram.UnknownCounterReset,
},
"NotCounterReset": {
hint: histogram.NotCounterReset,
expHeader: UnknownCounterReset,
expHint: histogram.UnknownCounterReset,
},
"UnknownCounterReset": {
hint: histogram.UnknownCounterReset,
expHeader: UnknownCounterReset,
expHint: histogram.UnknownCounterReset,
},
"Gauge": {
hint: histogram.GaugeType,
expHeader: GaugeType,
expHint: histogram.GaugeType,
},
}
for name, test := range tests {
Expand All @@ -63,6 +68,7 @@ func TestFirstFloatHistogramExplicitCounterReset(t *testing.T) {
require.False(t, recoded)
require.Equal(t, app, newApp)
require.Equal(t, test.expHeader, chk.GetCounterResetHeader())
assertFirstFloatHistogramSampleHint(t, chk, test.expHint)
})
}
}
Expand Down Expand Up @@ -338,7 +344,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
_, _, _, _, ok, _ := hApp.appendable(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset, histogram.UnknownCounterReset)
}

{ // Zero threshold change.
Expand All @@ -348,7 +354,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
_, _, _, _, ok, _ := hApp.appendable(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, UnknownCounterReset, histogram.UnknownCounterReset)
}

{ // New histogram that has more buckets.
Expand Down Expand Up @@ -397,7 +403,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.False(t, ok) // Need to cut a new chunk.
require.True(t, cr)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // New histogram that has buckets missing but the buckets missing were empty.
Expand Down Expand Up @@ -480,7 +486,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.False(t, ok) // Need to cut a new chunk.
require.True(t, cr)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // New histogram that has a counter reset while new buckets were added.
Expand All @@ -503,7 +509,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.False(t, ok) // Need to cut a new chunk.
require.True(t, cr)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{
Expand Down Expand Up @@ -532,15 +538,15 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.False(t, ok) // Need to cut a new chunk.
require.True(t, cr)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // New histogram that has an explicit counter reset.
c, hApp, ts, h1 := setup(eh)
h2 := h1.Copy()
h2.CounterResetHint = histogram.CounterReset

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // Start new chunk explicitly, and append a new histogram that is considered appendable to the previous chunk.
Expand All @@ -557,6 +563,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.Equal(t, app, newApp)
assertSampleCount(t, nextChunk, 1, ValFloatHistogram)
require.Equal(t, NotCounterReset, nextChunk.GetCounterResetHeader())
assertFirstFloatHistogramSampleHint(t, nextChunk, histogram.UnknownCounterReset)
}

{ // Start new chunk explicitly, and append a new histogram that is not considered appendable to the previous chunk.
Expand All @@ -574,6 +581,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.Equal(t, app, newApp)
assertSampleCount(t, nextChunk, 1, ValFloatHistogram)
require.Equal(t, CounterReset, nextChunk.GetCounterResetHeader())
assertFirstFloatHistogramSampleHint(t, nextChunk, histogram.UnknownCounterReset)
}

{ // Start new chunk explicitly, and append a new histogram that would need recoding if we added it to the chunk.
Expand All @@ -600,6 +608,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
require.Equal(t, app, newApp)
assertSampleCount(t, nextChunk, 1, ValFloatHistogram)
require.Equal(t, NotCounterReset, nextChunk.GetCounterResetHeader())
assertFirstFloatHistogramSampleHint(t, nextChunk, histogram.UnknownCounterReset)
}

{
Expand Down Expand Up @@ -664,7 +673,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
_, _, _, _, ok, _ := hApp.appendable(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // Custom buckets, change only in custom bounds.
Expand All @@ -674,7 +683,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
_, _, _, _, ok, _ := hApp.appendable(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, CounterReset, histogram.UnknownCounterReset)
}

{ // Custom buckets, with more buckets.
Expand Down Expand Up @@ -705,7 +714,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) {
}
}

func assertNewFloatHistogramChunkOnAppend(t *testing.T, oldChunk Chunk, hApp *FloatHistogramAppender, ts int64, h *histogram.FloatHistogram, expectHeader CounterResetHeader) {
func assertNewFloatHistogramChunkOnAppend(t *testing.T, oldChunk Chunk, hApp *FloatHistogramAppender, ts int64, h *histogram.FloatHistogram, expectHeader CounterResetHeader, expectHint histogram.CounterResetHint) {
oldChunkBytes := oldChunk.Bytes()
newChunk, recoded, newAppender, err := hApp.AppendFloatHistogram(nil, ts, h, false)
require.Equal(t, oldChunkBytes, oldChunk.Bytes()) // Sanity check that previous chunk is untouched.
Expand All @@ -717,6 +726,7 @@ func assertNewFloatHistogramChunkOnAppend(t *testing.T, oldChunk Chunk, hApp *Fl
require.NotNil(t, newAppender)
require.NotEqual(t, hApp, newAppender)
assertSampleCount(t, newChunk, 1, ValFloatHistogram)
assertFirstFloatHistogramSampleHint(t, newChunk, expectHint)
}

func assertNoNewFloatHistogramChunkOnAppend(t *testing.T, oldChunk Chunk, hApp *FloatHistogramAppender, ts int64, h *histogram.FloatHistogram, expectHeader CounterResetHeader) {
Expand Down Expand Up @@ -1023,7 +1033,7 @@ func TestFloatHistogramChunkAppendableGauge(t *testing.T) {
_, _, _, _, _, _, ok := hApp.appendableGauge(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType, histogram.GaugeType)
}

{ // Zero threshold change.
Expand All @@ -1033,7 +1043,7 @@ func TestFloatHistogramChunkAppendableGauge(t *testing.T) {
_, _, _, _, _, _, ok := hApp.appendableGauge(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType, histogram.GaugeType)
}

{ // New histogram that has more buckets.
Expand Down Expand Up @@ -1208,7 +1218,7 @@ func TestFloatHistogramChunkAppendableGauge(t *testing.T) {
_, _, _, _, _, _, ok := hApp.appendableGauge(h2)
require.False(t, ok)

assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType)
assertNewFloatHistogramChunkOnAppend(t, c, hApp, ts+1, h2, GaugeType, histogram.GaugeType)
}

{ // Custom buckets, with more buckets.
Expand Down Expand Up @@ -1357,3 +1367,10 @@ func TestFloatHistogramUniqueSpansAfterNext(t *testing.T) {
require.NotSame(t, &rh1.PositiveSpans[0], &rh2.PositiveSpans[0], "PositiveSpans should be unique between histograms")
require.NotSame(t, &rh1.NegativeSpans[0], &rh2.NegativeSpans[0], "NegativeSpans should be unique between histograms")
}

func assertFirstFloatHistogramSampleHint(t *testing.T, chunk Chunk, expected histogram.CounterResetHint) {
it := chunk.Iterator(nil)
require.Equal(t, ValFloatHistogram, it.Next())
_, v := it.AtFloatHistogram(nil)
require.Equal(t, expected, v.CounterResetHint)
}
22 changes: 6 additions & 16 deletions tsdb/chunkenc/histogram_meta.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,26 +558,16 @@ func counterResetHint(crh CounterResetHeader, numRead uint16) histogram.CounterR
// In a counter histogram chunk, there will not be any counter
// resets after the first histogram.
return histogram.NotCounterReset
case crh == CounterReset:
// If the chunk was started because of a counter reset, we can
// safely return that hint. This histogram always has to be
// treated as a counter reset.
return histogram.CounterReset
default:
// Sadly, we have to return "unknown" as the hint for all other
// cases, even if we know that the chunk was started without a
// cases, even if we know that the chunk was started with or without a
// counter reset. But we cannot be sure that the previous chunk
// still exists in the TSDB, so we conservatively return
// "unknown". On the bright side, this case should be relatively
// rare.
// still exists in the TSDB, or if the previous chunk was added later
// by out of order or backfill, so we conservatively return "unknown".
//
// TODO(beorn7): Nevertheless, if the current chunk is in the
// middle of a block (not the first chunk in the block for this
// series), it's probably safe to assume that the previous chunk
// will exist in the TSDB for as long as the current chunk
// exist, and we could safely return
// "histogram.NotCounterReset". This needs some more work and
// might not be worth the effort and/or risk. To be vetted...
// TODO: If we can detect whether the previous and current chunk are
// actually consecutive then we could trust its hint:
// https://github.com/prometheus/prometheus/issues/15346.
return histogram.UnknownCounterReset
}
}
Expand Down
Loading

0 comments on commit 978684a

Please sign in to comment.