Support histograms in pkg/querier #4376

codesome · 2023-03-06T05:11:49Z

What this PR does

Syncs the changes in pkg/querier (except remote_read, see #4425) from sparsehistogram branch.

In general terms: adds support to further iterators (beyond Batch iterator) for the querier to be able to server PromQL.

This does not turn native histograms on in the read path. We'll do that together with all the e2e test updates.

Related to: #3478
Fixes: #4405

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pkg/querier/iterators/chunk_merge_iterator.go

pkg/querier/querier_test.go

pkg/querier/distributor_queryable_test.go

pkg/querier/querier_test.go

pkg/querier/remote_read.go

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

Allow time series where there are both float and histogram samples. Add test to verify it. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

codesome

Lots of small comments

pkg/querier/iterators/chunk_iterator.go

codesome · 2023-03-07T09:00:44Z

pkg/querier/iterators/chunk_merge_iterator.go

+	if iT == jT {
+		iTyp := (*h)[i].AtType()
+		jTyp := (*h)[j].AtType()
+		return iTyp > jTyp


Worth a comment here. I guess we are assuming that float type has a lower numerical value than histogram type, so we are making use of that to prefer histograms over float.

yes. It's about as arbitrary as the current way of working which can ignore some float values. We're assuming that we pretty much never end up in such situation as the storage will return consistent series.

To illustrate: create two float (XOR) chunks with the same timestamps, but different values. The merge iterator simply ignores one of them.

pkg/querier/iterators/chunk_merge_iterator_test.go

codesome · 2023-03-07T09:14:54Z

pkg/querier/iterators/chunk_merge_iterator_test.go

 			}
 			assert.Equal(t, chunkenc.ValNone, iter.Next())
 		})
 	}
 }

-func TestChunkMergeIteratorSeek(t *testing.T) {
+func TestChunkMergeIteratorMixed(t *testing.T) {


We should also be checking if the result type are right for given time ranges. For example if at timestamp 80 if we were expecting Histogram, and if we get a float, then this test does not catch it.

With how the test case is, it can be as simple as 3 sequential for loops, each expecting a fixed type, and we don't need switch case anymore.

Made something a bit more sophisticated while reusing code

pkg/querier/iterators/chunk_merge_iterator_test.go

codesome · 2023-03-07T12:56:13Z

pkg/querier/querier_test.go

@@ -224,10 +270,90 @@ func TestQuerier_QueryableReturnsChunksOutsideQueriedRange(t *testing.T) {
 	}, m[0].Points)
 }

-func mockTSDB(t *testing.T, mint model.Time, samples int, step, chunkOffset time.Duration, samplesPerChunk int) (storage.Queryable, model.Time) {
+func TestBatchMergeChunks(t *testing.T) {


This test does not have histograms. Was it to test something else or some issue with how GitHub is showing the diff?

The batch merger has its own tests, so I think this is ok, as it needs to just check that the querier can use the batch merger-er.

I've added a comment to explain why this test is there in the first place

pkg/querier/remote_read.go

pkg/querier/timeseries_series_set.go

codesome · 2023-03-07T13:21:48Z

pkg/querier/timeseries_series_set_test.go

+			},
+			Histograms: []mimirpb.Histogram{
+				mimirpb.FromHistogramToHistogramProto(1232, generateTestHistogram(7)),
+				mimirpb.FromFloatHistogramToHistogramProto(1233, generateTestFloatHistogram(8)),
 			},
 		},
 	}

 	it := ts.Iterator(nil)


I feel we could use a slice of expected stuff and use a loop to do the matching to avoid a lot of duplication below. In case of seek, we could make it check against a partial slice. But if you feel it is easier this way, then I have no problem :)

I don't want to reinvent this testcase right now, plus I've added a case where we test two things after Next call, so I'd rather defer this question.

rewrote using test util functions to be more concise

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

The iterator was not doing do same type check as the others. The merge iterator can be used directly in the querier for feeding PromQL so it needs to be able to auto convert histogram to float histogram. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Replace panic with assertion. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Prepare to merge the float and histogram tests and add more cases Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Let the mock TSDB generate samples based on supplied type function Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

…togram with test Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Explain why this test is there. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Write a distributed querier test that involves both float and histogram samples at the same time. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Went to #4425 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

pstibrany · 2023-03-09T09:10:36Z

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

Use the iterator test functions. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

If we cache an integer histogram but then read a float histogram, the returned float histogram is invalid. Fix by using the correct cache. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

pkg/querier/block_test.go

# Conflicts: # pkg/querier/iterators/chunk_iterator.go # pkg/querier/iterators/chunk_iterator_test.go

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov

LGTM, i left mostly nitpicks and one or two minor bugs

pkg/querier/iterators/chunk_iterator.go

pkg/querier/iterators/chunk_merge_iterator_test.go

dimitarvdimitrov · 2023-03-13T11:02:48Z

pkg/querier/iterators/chunk_merge_iterator_test.go

+		if i < 50 {
+			test.RequireIteratorIthFloat(t, i, iter, valueType)
+			return
+		}
+		if i < 125 {
+			test.RequireIteratorIthHistogram(t, i, iter, valueType)
+			return
+		}
+		test.RequireIteratorIthFloatHistogram(t, i, iter, valueType)


do i have this right - because the chunks overlap and the timestamps of their samples are exactly the same, then the samples from the chunk with "more data" takes precedence. Can you maybe leave a comment for this? It looks like TestChunkMergeIteratorMixed is written in the same way.

yes, I'll add a comment

pkg/querier/block_test.go

pkg/querier/querier_test.go

pkg/querier/timeseries_series_set.go

pkg/querier/timeseries_series_set_test.go

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov

LGTM, thank you for addressing my comments!

codesome

I glanced over the non-test files. Just have 2 non-critical comments.

codesome · 2023-03-14T09:42:31Z

pkg/querier/iterators/chunk_iterator.go

+		// use cache as histogram
+		t, h := i.AtHistogram()
+		return t, h.ToFloat()


Are we not using the cache for this case? It will be common to call AtFloatHistogram() on integer histograms (I don't know how often the cache is hit here).

We can cache the float histogram value here as well as the integer histogram

In which case it is safer to set nil to the cached histograms when we set cacheValid as false.

Created issue: #4489

codesome · 2023-03-14T10:59:19Z

pkg/querier/timeseries_series_set.go

+	if h.IsFloatHistogram() {
+		return h.Timestamp, mimirpb.FromHistogramProtoToFloatHistogram(&h)
+	}
+	return h.Timestamp, mimirpb.FromHistogramProtoToHistogram(&h).ToFloat()


Maybe a TODO: We can directly do the conversion to float instead of getting integer histogram first and then converting to float. It will save some computation and allocation. Also for other places where we do it.

Created issue: #4488

This was referenced Mar 6, 2023

Support histograms in pkg/frontend #4377

Merged

Prometheus Native Histogram support phase1 #3478

Closed

codesome force-pushed the codesome/sync-querier branch from f347b74 to 36ddfd5 Compare March 6, 2023 09:54

codesome marked this pull request as ready for review March 6, 2023 10:07

codesome requested a review from a team as a code owner March 6, 2023 10:07

codesome requested a review from krajorama March 6, 2023 10:07

codesome force-pushed the codesome/sync-querier branch from 36ddfd5 to 86c53e2 Compare March 6, 2023 12:20

krajorama reviewed Mar 6, 2023

View reviewed changes

pkg/querier/iterators/chunk_merge_iterator.go Show resolved Hide resolved

pkg/querier/querier_test.go Outdated Show resolved Hide resolved

pkg/querier/querier_test.go Outdated Show resolved Hide resolved

krajorama reviewed Mar 6, 2023

View reviewed changes

pkg/querier/distributor_queryable_test.go Outdated Show resolved Hide resolved

pkg/querier/distributor_queryable_test.go Outdated Show resolved Hide resolved

pkg/querier/querier_test.go Outdated Show resolved Hide resolved

pkg/querier/remote_read.go Outdated Show resolved Hide resolved

krajorama mentioned this pull request Mar 7, 2023

Add test in pkg/querier/distributor_queryable_test.go to check case of float values and histograms in same series #4405

Closed

Support histograms in pkg/querier

f325495

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

codesome force-pushed the codesome/sync-querier branch from 86c53e2 to f325495 Compare March 7, 2023 06:41

Remote read fix histograms

8e05de4

Allow time series where there are both float and histogram samples. Add test to verify it. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

codesome commented Mar 7, 2023

View reviewed changes

krajorama and others added 16 commits March 8, 2023 08:04

Use switch in remote_read and handle default case

e2238b7

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Update pkg/querier/distributor_queryable_test.go

8918aab

querier_test: add float histogram to switches

2cacdaa

Replace panic with assertion. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Let querier_test.go testRangeQuery be more generic

707ff44

Prepare to merge the float and histogram tests and add more cases Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

querier_test.go: merge float and histogram tests

0ca8999

Let the mock TSDB generate samples based on supplied type function Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

query_test.go: add more histogram and mixed tests

267a84c

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

timeseries_series_set should be able to auto convert int to float his…

d21303f

…togram with test Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix possible overflow

0310e1f

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Update comment in chunk_merge_iterator

e37eb1d

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Rework TestChunkMergeIteratorSeek to remove duplications

9bd40ad

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Rework chunk_merge_iterator_test to have less duplication

922f5c3

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Remove comment as it was discussed IRL

392e843

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Add comment on TestBatchMergeChunks

62ed6ff

Explain why this test is there. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix formatting of comment

01c3476

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Make utility function out of asserting on iterator content

256e6c2

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama added 4 commits March 8, 2023 17:03

Fix issue 4405

77854d0

Write a distributed querier test that involves both float and histogram samples at the same time. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Err on the side of caution and do automatic convert in chunk_iterator

1f5d895

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Remove remote read from this PR

1e34e21

Went to #4425 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Merge branch 'main' into codesome/sync-querier

5ceaa09

pstibrany added the release/notified-changelog-cut label Mar 9, 2023

krajorama mentioned this pull request Mar 9, 2023

Extra tests for native histograms in queryStreamSamples and queryStreamChunks #4428

Merged

3 tasks

krajorama added 4 commits March 11, 2023 19:33

Clean up timeseries_series_set_test.go

aa05f49

Use the iterator test functions. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix caching bug in chunk_iterator

d3987fa

If we cache an integer histogram but then read a float histogram, the returned float histogram is invalid. Fix by using the correct cache. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Reintroduce the time cache for heap compare operator in chunk iterator

f82fced

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix missing license

ca05489

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

fayzal-g reviewed Mar 13, 2023

View reviewed changes

pkg/querier/block_test.go Outdated Show resolved Hide resolved

krajorama added 2 commits March 13, 2023 13:28

Merge branch 'main' into codesome/sync-querier

0d5023a

# Conflicts: # pkg/querier/iterators/chunk_iterator.go # pkg/querier/iterators/chunk_iterator_test.go

Add comment to explain +1 in test

90a7999

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov approved these changes Mar 13, 2023

View reviewed changes

krajorama added 7 commits March 13, 2023 16:08

Simplify chunkIterator.AtT and add cache unit test

aa0bd73

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

chunk merge iterator fixups

f6749c7

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Restore missing TestDistributorQuerier_LabelNames

849c931

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Simplify and fix convertToChunks

abb7f41

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Cosmetic fixes in querier_test

a1d9f83

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix typo

d91de8c

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Fix linter fail

215bffa

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

dimitarvdimitrov approved these changes Mar 13, 2023

View reviewed changes

codesome commented Mar 14, 2023

View reviewed changes

This was referenced Mar 14, 2023

Convert remote write sample histogram directly to float histogram where possible #4488

Closed

Cache converted integer to float histogram in chunk_iterator #4489

Closed

krajorama merged commit 8a98ed2 into main Mar 14, 2023

krajorama deleted the codesome/sync-querier branch March 14, 2023 11:17

krajorama mentioned this pull request Mar 16, 2023

Enable ingester and store-gateway to return native histogram samples and chunks to read path #4176

Closed

pracucci mentioned this pull request Dec 4, 2023

Remove deprecated configurations #6673

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support histograms in pkg/querier #4376

Support histograms in pkg/querier #4376

codesome commented Mar 6, 2023 •

edited by krajorama

Loading

codesome left a comment

codesome Mar 7, 2023

krajorama Mar 8, 2023

krajorama Mar 13, 2023

codesome Mar 7, 2023

krajorama Mar 8, 2023

codesome Mar 7, 2023

krajorama Mar 8, 2023

krajorama Mar 8, 2023

codesome Mar 7, 2023

krajorama Mar 8, 2023

krajorama Mar 11, 2023

pstibrany commented Mar 9, 2023

dimitarvdimitrov left a comment

dimitarvdimitrov Mar 13, 2023

krajorama Mar 13, 2023

dimitarvdimitrov left a comment

codesome left a comment

codesome Mar 14, 2023

codesome Mar 14, 2023

codesome Mar 14, 2023

krajorama Mar 14, 2023

codesome Mar 14, 2023

krajorama Mar 14, 2023

Support histograms in pkg/querier #4376

Support histograms in pkg/querier #4376

Conversation

codesome commented Mar 6, 2023 • edited by krajorama Loading

What this PR does

Checklist

codesome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany commented Mar 9, 2023

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

codesome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesome commented Mar 6, 2023 •

edited by krajorama

Loading