fix inconsistent histogram chunk iterators #13659
Conversation
The interface definition wasn't strict enough of what we expect of the iterators. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
In particular storageSeriesIterator was causing an issue when the engine would read without copy from it here: https://github.com/prometheus/prometheus/blob/4408f898bc592b570aa0a173c228e4fe2f284474/promql/engine.go#L2203 And then put that pointer into a pool and reuse. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Only makes sense when the iterator works on data that stores pointers directly. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| // 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 |
There was a problem hiding this comment.
Doesn’t your PR enforce that the optional Histogram object will be reused?
There was a problem hiding this comment.
I'm not checking if the value is reused and also I see some examples of especially mock iterators that always return new objects.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds reasonable. I can also just update the mocks as well to be sure.
There was a problem hiding this comment.
Although there's one non mock that needs rework then: func (c *concreteSeriesIterator) AtFloatHistogram(*histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {
| // 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 |
There was a problem hiding this comment.
Doesn’t your PR enforce that the optional FloatHistogram object will be reused?
Yes, I've reviewed all, but could use a second pair of eyes for sure. |
Otherwise there's a cyclic dependency via tsdbutil Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
From comment by @zenador Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
I have double-checked it, and I confirm that all implementations of |
|
Note : does not affect web api performance because there the histograms are directly accessed prometheus/web/api/v1/json_codec.go Line 168 in 4408f89 |
|
We've discussed in IRL and this goes deep into the rabbit hole of iterator optimizations, we'll do a different fix for the problem that is explained in the description. |
|
Closing until we figure out which direction to go in. |
tsdb: fix inconsistent histogram chunk iterators
The PromQL engine has an implicit requirement that chunk iterators return copies of native histograms. For example here.
Some iterators do not follow this, so this PR updates the interface definition to be more explicit and fixes some iterators.
Caught when
go test -racefailed on TestInstantQuerySplittingCorrectness in Mimir. Here we are seeding multiple engines with the same series, so reading through storageSeriesIterator put the histogram pointers into two separate pools for reuse here.