Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cherry-pick: Always return unknown hint for first sample in non-gauge histogram chunk #764

Merged
merged 2 commits into from
Nov 25, 2024
Merged

Cherry-pick: Always return unknown hint for first sample in non-gauge histogram chunk #764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd5caa5
Always return unknown hint for first sample in non-gauge histogram ch…
fionaliao Nov 12, 2024
e254486
Fix tests
fionaliao Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 30 additions & 13 deletions tsdb/chunkenc/float_histogram_test.go
View file
Open in desktop
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
Open in desktop
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
Loading