Skip to content

fix inconsistent histogram chunk iterators #13659

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

Closed
wants to merge 5 commits into from
Closed

fix inconsistent histogram chunk iterators #13659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
107bca1
fix: clarify histogram chunk iterator interface
krajorama Feb 28, 2024
5ed612c
fix: update some histogram chunk iterators to fullfil the interface
krajorama Feb 28, 2024
bdf8e3f
Add unit test to verify chunkenc.Iterator interface
krajorama Feb 28, 2024
fe7fc65
Refactor test into its own package
krajorama Feb 28, 2024
e64784d
Split TestListSeriesIteratorCopiesHistograms along type to be simpler
krajorama Feb 28, 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
8 changes: 6 additions & 2 deletions promql/value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,8 +468,12 @@ func (ssi *storageSeriesIterator) AtHistogram(*histogram.Histogram) (int64, *his
panic(errors.New("storageSeriesIterator: AtHistogram not supported"))
}

func (ssi *storageSeriesIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
return ssi.currT, ssi.currH
func (ssi *storageSeriesIterator) AtFloatHistogram(fh *histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
if fh == nil {
return ssi.currT, ssi.currH.Copy()
}
ssi.currH.CopyTo(fh)
return ssi.currT, fh
}

func (ssi *storageSeriesIterator) AtT() int64 {
Expand Down
13 changes: 13 additions & 0 deletions promql/value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import (
"github.com/stretchr/testify/require"

"github.com/prometheus/prometheus/model/labels"
"github.com/prometheus/prometheus/tsdb/testutil"
"github.com/prometheus/prometheus/tsdb/tsdbutil"
)

func TestVector_ContainsSameLabelset(t *testing.T) {
Expand Down Expand Up @@ -108,3 +110,14 @@ func TestMatrix_ContainsSameLabelset(t *testing.T) {
})
}
}

func TestStorageSeriesIterator_CopiesHistograms(t *testing.T) {
h := tsdbutil.GenerateTestFloatHistogram(0)
series := Series{
Metric: labels.FromStrings("__name__", "test"),
Floats: nil,
Histograms: []HPoint{{T: 0, H: h}},
}
it := newStorageSeriesIterator(series)
require.NoError(t, testutil.VerifyChunkIteratorCopiesFloatHistograms(h, it))
}
16 changes: 12 additions & 4 deletions storage/series.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,22 @@ func (it *listSeriesIterator) At() (int64, float64) {
return s.T(), s.F()
}

func (it *listSeriesIterator) AtHistogram(*histogram.Histogram) (int64, *histogram.Histogram) {
func (it *listSeriesIterator) AtHistogram(h *histogram.Histogram) (int64, *histogram.Histogram) {
s := it.samples.Get(it.idx)
return s.T(), s.H()
if h == nil {
return s.T(), s.H().Copy()
}
s.H().CopyTo(h)
return s.T(), h
}

func (it *listSeriesIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
func (it *listSeriesIterator) AtFloatHistogram(fh *histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
s := it.samples.Get(it.idx)
return s.T(), s.FH()
if fh == nil {
return s.T(), s.FH().Copy()
}
s.FH().CopyTo(fh)
return s.T(), fh
}

func (it *listSeriesIterator) AtT() int64 {
Expand Down
18 changes: 18 additions & 0 deletions storage/series_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"github.com/prometheus/prometheus/model/value"
"github.com/prometheus/prometheus/tsdb/chunkenc"
"github.com/prometheus/prometheus/tsdb/chunks"
"github.com/prometheus/prometheus/tsdb/testutil"
"github.com/prometheus/prometheus/tsdb/tsdbutil"
)

func TestListSeriesIterator(t *testing.T) {
Expand Down Expand Up @@ -124,6 +126,22 @@ func TestChunkSeriesSetToSeriesSet(t *testing.T) {
}
}

func TestListSeriesIteratorCopiesHistograms(t *testing.T) {
h := tsdbutil.GenerateTestHistogram(0)
it := NewListSeriesIterator(samples{
hSample{0, h},
})
require.NoError(t, testutil.VerifyChunkIteratorCopiesHistograms(h, it))
}

func TestListSeriesIteratorCopiesFloatHistograms(t *testing.T) {
fh := tsdbutil.GenerateTestFloatHistogram(0)
it := NewListSeriesIterator(samples{
fhSample{1, fh},
})
require.NoError(t, testutil.VerifyChunkIteratorCopiesFloatHistograms(fh, it))
}

type histogramTest struct {
samples []chunks.Sample
expectedCounterResetHeaders []chunkenc.CounterResetHeader
Expand Down
8 changes: 4 additions & 4 deletions tsdb/chunkenc/chunk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,18 +131,18 @@ type Iterator interface {
// At returns the current timestamp/value pair if the value is a float.
// Before the iterator has advanced, the behaviour is unspecified.
At() (int64, float64)
// AtHistogram returns the current timestamp/value pair if the value is a
// AtHistogram returns a copy(!) of the current timestamp/value pair if the value is a
// histogram with integer counts. Before the iterator has advanced, the behaviour
// is unspecified.
// The method accepts an optional Histogram object which will be
// The method accepts an optional Histogram object which may be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t your PR enforce that the optional Histogram object will be reused?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not checking if the value is reused and also I see some examples of especially mock iterators that always return new objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation is then ambiguous. As a caller of it.AtHistogram(h), I want to know whether the object corresponding to the pointer I passed has actually been reused or not, without having to go through all the implementations of the interface.
I think that we should use will as before, even because this way we kind of force new implementations to respect the contract: if they don't reuse the given object, and if something bad happens, then it is their fault.
Related to the mocked iterators, I don't know if they are so relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I can also just update the mocks as well to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although there's one non mock that needs rework then: func (c *concreteSeriesIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {

// reused when not nil. Otherwise, a new Histogram object will be allocated.
AtHistogram(*histogram.Histogram) (int64, *histogram.Histogram)
// AtFloatHistogram returns the current timestamp/value pair if the
// AtFloatHistogram returns a copy(!) of the current timestamp/value pair if the
// value is a histogram with floating-point counts. It also works if the
// value is a histogram with integer counts, in which case a
// FloatHistogram copy of the histogram is returned. Before the iterator
// has advanced, the behaviour is unspecified.
// The method accepts an optional FloatHistogram object which will be
// The method accepts an optional FloatHistogram object which may be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t your PR enforce that the optional FloatHistogram object will be reused?

// reused when not nil. Otherwise, a new FloatHistogram object will be allocated.
AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram)
// AtT returns the current timestamp.
Expand Down
4 changes: 2 additions & 2 deletions tsdb/querier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,11 +776,11 @@ func (it *mockSampleIterator) At() (int64, float64) {
}

func (it *mockSampleIterator) AtHistogram(*histogram.Histogram) (int64, *histogram.Histogram) {
return it.s[it.idx].T(), it.s[it.idx].H()
return it.s[it.idx].T(), it.s[it.idx].H().Copy()
}

func (it *mockSampleIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
return it.s[it.idx].T(), it.s[it.idx].FH()
return it.s[it.idx].T(), it.s[it.idx].FH().Copy()
}

func (it *mockSampleIterator) AtT() int64 {
Expand Down
73 changes: 73 additions & 0 deletions tsdb/testutil/histogram.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// Copyright 2024 The Prometheus Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package testutil

import (
"fmt"

"github.com/prometheus/prometheus/model/histogram"
"github.com/prometheus/prometheus/tsdb/chunkenc"
)

// Test helper to ensure that the iterator copies histograms when iterating.
// The helper will call it.Next() and check if the next value is a histogram.
func VerifyChunkIteratorCopiesHistograms(h *histogram.Histogram, it chunkenc.Iterator) error {
originalCount := h.Count
if it.Next() != chunkenc.ValHistogram {
return fmt.Errorf("expected histogram value type")
}
// Get a copy.
_, hCopy := it.AtHistogram(nil)
// Copy into a new histogram.
hCopy2 := &histogram.Histogram{}
_, hCopy2 = it.AtHistogram(hCopy2)

// Change the original
h.Count = originalCount + 1

// Ensure that the copy is not affected.
if hCopy.Count != originalCount {
return fmt.Errorf("copied histogram count should not change when original is modified")
}
if hCopy2.Count != originalCount {
return fmt.Errorf("copied to histogram count should not change when original is modified")
}
return nil
}

// Test helper to ensure that the iterator copies float histograms when iterating.
// The helper will call it.Next() and check if the next value is a float histogram.
func VerifyChunkIteratorCopiesFloatHistograms(fh *histogram.FloatHistogram, it chunkenc.Iterator) error {
originalCount := fh.Count
if it.Next() != chunkenc.ValFloatHistogram {
return fmt.Errorf("expected float histogram value type")
}
// Get a copy.
_, hCopy := it.AtFloatHistogram(nil)
// Copy into a new histogram.
hCopy2 := &histogram.FloatHistogram{}
_, hCopy2 = it.AtFloatHistogram(hCopy2)

// Change the original
fh.Count = originalCount + 1

// Ensure that the copy is not affected.
if hCopy.Count != originalCount {
return fmt.Errorf("copied histogram count should not change when original is modified")
}
if hCopy2.Count != originalCount {
return fmt.Errorf("copied to histogram count should not change when original is modified")
}
return nil
}