Refactor Cortex querier to use Queryable interface directly #2060

pstibrany · 2020-01-31T11:47:03Z

This PR changes Cortex querier to use Queryable interface as its primary interface it builds on. Previously, Cortex querier was tied to Cortex chunks store, but this has problems when using it with TSDB blocks: Blocks Querier had to reencode TSDB chunks to Cortex chunks, and also ingester streaming support (PR #1845) would need to do the same.

Chunk-based queryables are still optimized in the way they were before (in mergeSeriesSets). The requirement is that they return series that implement new SeriesWithChunks interface, which gives new mergeSeriesSets method an option to use optimizations.

This PR also removes some duplicity in the code. First by merging ingesterQueryable (used when streaming ingester flag is set) into distributorQuerier, which now simply has a flag that decides whether streaming should be used or not. Second, logic from unifiedChunkQuerier was moved into querier (merging from multiple chunks-based stores), most of the code was already there anyway.

This PR has a follow-up PR that modifies BlocksQuerier to avoid TSDB -> Cortex chunks conversion.

Checklist

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

pracucci

Thanks @pstibrany for this refactoring! Looking at this refactoring it's quite clear how much we need it, to make the read path easier to maintain and evolve (ie. see the struggles we're experiencing with the blocks storage). I left few comments here and there, but the design looks good.

pkg/querier/chunk_store_queryable.go

pkg/querier/chunk_store_queryable_test.go

pkg/querier/distributor_queryable.go

pkg/querier/querier.go

pracucci · 2020-02-03T08:30:29Z

pkg/querier/querier.go

 	}
-	return metricsToSeriesSet(ms), nil, nil
+
+	otherSets = append(otherSets, chunksSet)


The input sets of NewMergeSeriesSet must be sorted. How can we guarantee that appending otherSets and chunksSet together is still sorted?

It's series in individual sets that must be sorted in order for merging to work properly.

Series in chunk-based sets are sorted (guaranteed by partitionChunks).

Non-chunk-based sets that we use are:

values from ingester when not using streaming – these are returned via newConcreteSeriesSet, which sorts the series by label,

after Blocks querier: don't reencode TSDB chunks to Cortex chunks during querying #2061 – series set returned by blocks storage, fortunately it also guarantees order of the series.

pkg/querier/querier.go

pkg/querier/distributor_queryable.go

pstibrany · 2020-02-03T10:11:59Z

Thank you Marco for your review. I believe I've addressed all your comments now.

bboreham

I had some comments, mostly about naming and style.
Would like to see the benchmark results before and after, to check it hasn't introduced a performance regression. Particularly in light of #1195

pkg/querier/chunk_store_queryable.go

pkg/querier/distributor_queryable.go

bboreham · 2020-02-03T10:26:02Z

pkg/querier/distributor_queryable.go

-	if sp != nil {
-		mint = sp.Start
-		maxt = sp.End
+	// Kludge: Prometheus passes nil SelectParams if it is doing a 'series' operation,


Can we clarify who is kludging what? I think it's important that Cortex is relying on a behaviour of Prometheus, to make a decision that helps minimise effort at runtime.

I copied this comment from querier type, which had direct reference to distributor previously.

Any suggestion on how to improve/change this comment? It seems clear to me.

Maybe we can just remove the Kludge:? :) All in all, we're just describing how Prometheus works and why we handle the special case of sp == nil.

I don't see this documented anywhere else, so to me Kludge: indicates that this is possibly a hack that can break :)

pkg/querier/querier.go

pkg/util/merger.go

pkg/querier/chunk_store_queryable_test.go

pkg/querier/lazy_querier.go

pkg/querier/querier.go

pstibrany · 2020-02-03T11:02:21Z

Would like to see the benchmark results before and after, to check it hasn't introduced a performance regression. Particularly in light of #1195

Thanks Bryan for your review. Here are benchmarks of this PR against master: https://gist.github.com/pstibrany/6418ad45e9c982a9e10950c4ad8f12a8

Problem with this benchmark is that many tests don't run long enough to reasonably compare them. I will try to get a better one. (And also that I run them on a rented shared virtual host to avoid blocking my main machine for a long time :-()

pkg/querier/querier.go

pstibrany · 2020-02-03T11:47:58Z

I've rerun benchmarks with -test.run '^$' -test.bench '^Bench.*' -test.v -test.benchmem -test.benchtime=10s parameters.

https://gist.github.com/pstibrany/96b1509429714605bf59382640bc6032

pracucci

I tripled checked this PR and LGTM. I left a couple of nits, but nothing is blocking.

pracucci · 2020-02-05T11:09:32Z

pkg/querier/distributor_queryable.go

-	if sp != nil {
-		mint = sp.Start
-		maxt = sp.End
+	// Kludge: Prometheus passes nil SelectParams if it is doing a 'series' operation,


Maybe we can just remove the Kludge:? :) All in all, we're just describing how Prometheus works and why we handle the special case of sp == nil.

pkg/querier/querier.go

thorfour

LGTM Thanks for doing this work!

This makes it easier to use non-chunk-based queryables. On the other hand, chunk-based queryables are still optimized in the way they were before (in mergeSeriesSets). Merged ingester_streaming_queryable.go into distributor_queryable.go. Merged unified_querier.go into querier.go. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

…erier. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

…s methods. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pstibrany mentioned this pull request Jan 31, 2020

Blocks querier: don't reencode TSDB chunks to Cortex chunks during querying #2061

Merged

3 tasks

pstibrany marked this pull request as ready for review February 3, 2020 07:33

pracucci self-requested a review February 3, 2020 07:36

pracucci reviewed Feb 3, 2020

View reviewed changes

bboreham reviewed Feb 3, 2020

View reviewed changes

pracucci reviewed Feb 3, 2020

View reviewed changes

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

pkg/querier/querier.go Show resolved Hide resolved

pracucci approved these changes Feb 5, 2020

View reviewed changes

bboreham approved these changes Feb 6, 2020

View reviewed changes

thorfour approved these changes Feb 6, 2020

View reviewed changes

pstibrany mentioned this pull request Feb 10, 2020

goimports -local github.com/cortexproject/cortex -w pkg/ cmd/ tools/ #2106

Merged

pstibrany added 15 commits February 17, 2020 11:20

Fix missing labels.

c6fda3e

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Removed unused method.

af4ec9a

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Added test for sorting series by partitionChunks.

b4bf3c2

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Review feedback.

fd4601f

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Added comment about why series-requests are handled by distributor qu…

69f5ad1

…erier. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Don't wait for results to arrive if context is done.

dfdc474

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Don't call At() again for first series.

38a1dd3

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

More review feedback.

e826402

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Added comment

1a80366

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Improve comment.

f9328b9

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Rename parameters to NewQueryable.

377fe7b

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Include empty set with error. Unify checks in seriesSetWithFirstSerie…

594459c

…s methods. Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Fix imports ordering.

02146a3

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Fix compactor test.

4161761

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pracucci merged commit b81d808 into cortexproject:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Cortex querier to use Queryable interface directly #2060

Refactor Cortex querier to use Queryable interface directly #2060

pstibrany commented Jan 31, 2020

pracucci left a comment

pracucci Feb 3, 2020

pstibrany Feb 3, 2020 •

edited

Loading

pstibrany commented Feb 3, 2020

bboreham left a comment

bboreham Feb 3, 2020

pstibrany Feb 3, 2020

pracucci Feb 5, 2020

pstibrany Feb 6, 2020

pstibrany commented Feb 3, 2020 •

edited

Loading

pstibrany commented Feb 3, 2020

pracucci left a comment

pracucci Feb 5, 2020

thorfour left a comment

Refactor Cortex querier to use Queryable interface directly #2060

Refactor Cortex querier to use Queryable interface directly #2060

Conversation

pstibrany commented Jan 31, 2020

pracucci left a comment

Choose a reason for hiding this comment

pracucci Feb 3, 2020

Choose a reason for hiding this comment

pstibrany Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

pstibrany commented Feb 3, 2020

bboreham left a comment

Choose a reason for hiding this comment

bboreham Feb 3, 2020

Choose a reason for hiding this comment

pstibrany Feb 3, 2020

Choose a reason for hiding this comment

pracucci Feb 5, 2020

Choose a reason for hiding this comment

pstibrany Feb 6, 2020

Choose a reason for hiding this comment

pstibrany commented Feb 3, 2020 • edited Loading

pstibrany commented Feb 3, 2020

pracucci left a comment

Choose a reason for hiding this comment

pracucci Feb 5, 2020

Choose a reason for hiding this comment

thorfour left a comment

Choose a reason for hiding this comment

pstibrany Feb 3, 2020 •

edited

Loading

pstibrany commented Feb 3, 2020 •

edited

Loading