Add ChunksIterator method to Series interface. #5882

bwplotka · 2019-08-13T13:23:15Z

Continuation from: prometheus-junkyard/tsdb#665

Changes:

Adjusts Iterator Interface of Series e.g Seek
Reuse single interface instead of 3 same ones spread across 3 packages.
Refactored tests
Added ChunkIterator to iterate over chunks if requested by the client.

bwplotka · 2019-08-13T16:53:55Z

tsdb/querier.go

-	// Err returns the current error.
-	Err() error
+func (s *chunkSeries) ChunkIterator() ChunkIterator {
+	// TODO(bwplotka): This is wrong. We probably have to be strict in terms of min/maxt and tombstones.


TODO/TO decide:

When we want to apply tombstones in ChunkIterator? :/

TBH it might be the most efficient to pass intervals upstream e.g in streamed remote read protocol as well (???)

cc @brian-brazil

Two options:

A) Naive: Rewrite chunk on the fly applying mint,maxt and dranges. It is must have for Select interface which assumes things are scoped to given time range and tombstones are tolerated.
B) Ignore mint,maxt level. Maybe allow ChunkIterator to return deleted intervals as well. This probably means extending the remote read chunked protocol here for passing tombstones intervals as well -> let client side to apply intervals.. However, we leak "deleted" metrics. If they were deleted because of GDPR or something, this is kind of wrong (:

I think overall we need to do A here. Thoughts @brian-brazil @tomwilkie @codesome ?

Definitely A, for general data security we don't want to be returning deleted data. It'd also push that complexity onto all readers, and they'd probably forget it.

It'd also push that complexity onto all readers, and they'd probably forget it.

You would NOT put it to readers you mean?

Makes sense to me.. Let's do it then here.

bwplotka · 2019-08-13T17:07:45Z

Addressed all comments from the previous PR: prometheus-junkyard/tsdb#665 PTAL @brian-brazil @krasi-georgiev

brian-brazil · 2019-08-14T11:33:33Z

tsdb/head.go

@@ -1617,6 +1617,8 @@ func newMemSeries(lset labels.Labels, id uint64, chunkRange int64) *memSeries {
 		ref:        id,
 		chunkRange: chunkRange,
 		nextAt:     math.MinInt64,
+		// This is to make sure iterator.At without advancing returns default values.


It's not valid to call At without advancing.

Maybe calling At before Next or Seek should cause iterator.Error
In memSafeIterator add a var initialized and when false memSafeIterator.At returns default values and causes memSafeIterator.Err.

Either way the logic should be in the iterator itself.

krasi-georgiev

so many iterators now, might run out of names soon 😄

krasi-georgiev · 2019-08-13T13:26:55Z

tsdb/CHANGELOG.md

@@ -1,5 +1,8 @@
 ## master / unreleased

+- [CHANGE] `chunks.MergeOverlappingChunks` moved to `tsdb.MergeOverlappingChunks`
+- [CHANGE] `Series` interface allows return chunk iterator that allows iterating over encoded chunks.


do you think it is worth it to still keep track of these?
Is it helpful to you or the cortex team?
cc @tomwilkie

I mean do we still need a CHANGELOG file at all? How would it be useful?

krasi-georgiev · 2019-08-14T14:33:26Z

tsdb/querier.go

@@ -1175,6 +1358,19 @@ func (it *deletedIterator) At() (int64, float64) {
 	return it.it.At()
 }

+func (it *deletedIterator) Seek(t int64) bool {
+	if atT, _ := it.At(); t >= atT {
+		return false


Seek should return true when t==atT

If possible you can add the TestDeletedIterator to TestSeriesIterators and delete the TestDeletedIterator test or add a seek test to TestDeletedIterator

krasi-georgiev · 2019-08-14T15:14:10Z

tsdb/querier.go

+// ChunkIterator iterates over the chunk of a time series.
+type ChunkIterator interface {
+	// Seek advances the iterator forward to the given timestamp.
+	// It advances to the chunk with min time at t or first chunk with min time after t.


Add a behavior description similar to the chunk.Iterator?
I mean what hapens in the different scenarios when requested time is the same or smaller than the current At.

krasi-georgiev · 2019-08-14T15:18:03Z

tsdb/querier_test.go

+	var r []tsdbutil.Sample
+	if tc.seek != 0 {
+		testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek))
+		testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.


Suggested change

testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.

testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.

testutil.Equals(t, false, it.Seek(tc.seek-1)) // Seek in the past should be noop and return false.

krasi-georgiev · 2019-08-14T15:20:07Z

tsdb/querier_test.go

+	var r []chunks.Meta
+	if tc.seek != 0 {
+		testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek))
+		testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.


Suggested change

testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.

testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.

testutil.Equals(t, false, it.Seek(tc.seek-1)) // Seek in the past should be noop and return false.

krasi-georgiev · 2019-08-15T11:15:23Z

tsdb/querier.go

 	}
 }

 func (it *verticalMergeSeriesIterator) Seek(t int64) bool {
+	if it.initialized && it.curT >= t {
+		return true


hm this doesn't look right. This would return true when it.curT is 150 and the requested time is 100

I thought we want the following behaviour:
Iterator with minT:100, maxT:200, curT:150

Seek(150) - returns true and the Seek is noop, doesn't advance.
Seek(100) - returns false and is noop.
Seek(160) - returns true and advances the iterator to 160 or the next available timestamp.
Seek(210) - returns false and exhausts the iterator.
When it.Err is not nil return false in all cases.

Maybe putting an example like this in the iterator description would make things a bir more clear for what is expected in the different scenarios.

krasi-georgiev · 2019-08-15T11:32:44Z

tsdb/querier.go

 	if t > it.maxt {
+		// Exhaust iterator.
+		it.i = len(it.chunks)


maybe it is just my style, but I would think it will be easier to follow with an explicit var it.exhausted = true. Can leave as is if you prefer.

krasi-georgiev · 2019-08-15T11:37:45Z

tsdb/querier.go

-			return false
-		}
-		return true
+		return t <= it.maxt
 	}
 	if err := it.cur.Err(); err != nil {


Suggested change

if err := it.cur.Err(); err != nil {

if it.cur.Err() != nil {

krasi-georgiev · 2019-08-15T11:39:06Z

tsdb/querier.go

@@ -940,7 +957,7 @@ func (it *chainedSeriesIterator) Next() bool {
 	if it.cur.Next() {
 		return true
 	}
-	if err := it.cur.Err(); err != nil {
+	if it.cur.Err() != nil {


I completely forgot we can do that 🥇 nice reminder, thanks!

krasi-georgiev · 2019-08-15T11:44:46Z

tsdb/querier_test.go

@@ -1053,8 +1304,9 @@ func TestChunkSeriesIterator_DoubleSeek(t *testing.T) {
 	}

 	res := newChunkSeriesIterator(chkMetas, nil, 2, 8)
-	testutil.Assert(t, res.Seek(1) == true, "")
-	testutil.Assert(t, res.Seek(2) == true, "")
+	testutil.Assert(t, res.Seek(1), "")


don't think we even need this test anymore as we can cover it in TestSeriesIterators

bwplotka · 2019-11-28T12:42:26Z

Need to get back to this soon (:

bwplotka · 2020-02-03T20:40:35Z

Getting back to this, expect changes soon: WIP.

@brian-brazil @codesome do you know why we have two places with storage interfaces?
tsdb/Querier have some: https://github.com/prometheus/prometheus/blob/master/tsdb/querier.go#L33
And storage has almost the same: https://github.com/prometheus/prometheus/blob/master/storage/interface.go

Since we merged TSDB with Prometheus, do you think it makes sense to unify those together?

brian-brazil · 2020-02-03T21:13:43Z

It's historical, but we probably want to think about the impact on the remote storage code too.

bwplotka · 2020-02-05T09:42:34Z

We can literally remove and deduplicate lot's of iterators.. expect big PR 😢

Should be ready for review EOW

This is part of #5882 that can be done to simplify things. All todos I added will be fixed in follow up PRs. ## Changes * querier.Querier, querier.SeriesSet, and querier.Series interfaces merged with storage interface.go. All imports that. * querier.SeriesIterator replaced by chunkenc.Iterator * Added chunkenc.Iterator.Seek method and tests for xor implementation (?) * Since we properly handle SelectParams for Select methods I adjusted min max based on that. This should help in terms of performance for queries with functions like offset. * added Seek to deletedIterator and test. No logic was changed, only different source of abstractions, so no need for benchmarks. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

This is part of #5882 that can be done to simplify things. All todos I added will be fixed in follow up PRs. ## Changes * querier.Querier, querier.Appender, querier.SeriesSet, and querier.Series interfaces merged with storage interface.go. All imports that. * querier.SeriesIterator replaced by chunkenc.Iterator * Added chunkenc.Iterator.Seek method and tests for xor implementation (?) * Since we properly handle SelectParams for Select methods I adjusted min max based on that. This should help in terms of performance for queries with functions like offset. * added Seek to deletedIterator and test. * storage/tsdb was removed as it was only a unnecessary glue with incompatible structs. No logic was changed, only different source of abstractions, so no need for benchmarks. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

This is part of #5882 that can be done to simplify things. All todos I added will be fixed in follow up PRs. * querier.Querier, querier.Appender, querier.SeriesSet, and querier.Series interfaces merged with storage interface.go. All imports that. * querier.SeriesIterator replaced by chunkenc.Iterator * Added chunkenc.Iterator.Seek method and tests for xor implementation (?) * Since we properly handle SelectParams for Select methods I adjusted min max based on that. This should help in terms of performance for queries with functions like offset. * added Seek to deletedIterator and test. * storage/tsdb was removed as it was only a unnecessary glue with incompatible structs. No logic was changed, only different source of abstractions, so no need for benchmarks. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-02-18T17:10:07Z

In heavy progress.

…terface. * Added Seek to chunks.Iterator interface for iterating over chunks. * Mock, NewTestSeries, SampleSeriesIterator and ChunkSeriesIterator are now available from storage package and reuses instead of being recreated in many places. NewConcreteSeries was created to replace concreteSeries. * Added simple `verticalMergeChunkIterator` implementation - no tests so far. Reusing same vertical merging strategy in both compact and querier. * Refactored TestSeriesIterator. * Simplified; merged seek and non seek cases together. Added explicit min/max only for chunk series iterator, where it is relevant. * Adjusted all seek implementation to match edge case requirement (double seek, failed seek + next). Added more tests for old iterators; pulled Free's changes from the PR #329 Fixed series iterator tests. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

krasi-georgiev

I think it is ok,

The Labeled interface name is strange, but I also can't think of a better one

krasi-georgiev · 2020-02-27T13:07:54Z

storage/remote/codec.go

+		for isNext {
+			chk := iter.At()
+
+			if chk.Chunk == nil {


Instead of this I would expect that iter.Next() would return false and the error would be in iter.Err()

krasi-georgiev · 2020-02-27T13:24:07Z

storage/generic.go

+	return s.(SeriesSet), w, err
+}
+func (q *querierAdapter) SelectSorted(params *SelectParams, matchers ...*labels.Matcher) (SeriesSet, Warnings, error) {
+	s, w, err := q.SelectSorted(params, matchers...)


recursion by mistake? the same below

Suggested change

s, w, err := q.SelectSorted(params, matchers...)

s, w, err := q.genericQuerier.SelectSorted(params, matchers...)

krasi-georgiev · 2020-02-27T13:33:31Z

storage/interface.go

-	// LabelValues returns all potential values for a label name.
-	// It is not safe to use the strings beyond the lifefime of the querier.
-	LabelValues(name string) ([]string, Warnings, error)
+// ChunkedQuerier provides querying access over time series' chunks of a fixed


Why not ChunksQuerier?

Suggested change

// ChunkedQuerier provides querying access over time series' chunks of a fixed

// ChunksQuerier provides querying access over time series' chunks of a fixed

for context I would also like to see a small comment on where it can be used - in this case for remote read chunks streaming.

krasi-georgiev · 2020-02-27T14:24:16Z

tsdb/chunks/chunks.go

+	//// It advances to the chunk with min time at t or first chunk with min time after t.
+	//Seek(t int64) bool
+	// At returns the current meta.
+	// It depends on implementation if the chunk is populated or not.


why is that? Why not just return non empty chunks?

I don't think you know what I mean here. Populated is if you have only Ref or if you have actually Bytes of chunk there (:

krasi-georgiev · 2020-02-27T14:33:11Z

storage/series.go

+	ChunkIteratorFn  func() chunks.Iterator
+}
+
+func NewTestSeries(lset labels.Labels, samples ...[]tsdbutil.Sample) Series {


this is only for testing right? so why not put in a test package or a test file?

Because we want to reuse this across all unit test, even those not in this package.

krasi-georgiev · 2020-02-27T14:57:02Z

storage/generic.go

+
+type genericSeriesSet interface {
+	Next() bool
+	At() Labeled


Suggested change

At() Labeled

At() Series

Nope, we need Labelled, but also because it's actually genericSeries ;p

bwplotka · 2020-02-28T17:03:40Z

label querier?

Labeled

I like labelled as it is "something" that has labels (:

This is part of prometheus#5882 that can be done to simplify things. All todos I added will be fixed in follow up PRs. * querier.Querier, querier.Appender, querier.SeriesSet, and querier.Series interfaces merged with storage interface.go. All imports that. * querier.SeriesIterator replaced by chunkenc.Iterator * Added chunkenc.Iterator.Seek method and tests for xor implementation (?) * Since we properly handle SelectParams for Select methods I adjusted min max based on that. This should help in terms of performance for queries with functions like offset. * added Seek to deletedIterator and test. * storage/tsdb was removed as it was only a unnecessary glue with incompatible structs. No logic was changed, only different source of abstractions, so no need for benchmarks. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

This is part of prometheus#5882 that can be done to simplify things. All todos I added will be fixed in follow up PRs. * querier.Querier, querier.Appender, querier.SeriesSet, and querier.Series interfaces merged with storage interface.go. All imports that. * querier.SeriesIterator replaced by chunkenc.Iterator * Added chunkenc.Iterator.Seek method and tests for xor implementation (?) * Since we properly handle SelectParams for Select methods I adjusted min max based on that. This should help in terms of performance for queries with functions like offset. * added Seek to deletedIterator and test. * storage/tsdb was removed as it was only a unnecessary glue with incompatible structs. No logic was changed, only different source of abstractions, so no need for benchmarks. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: blalov <boiskila@gmail.com>

I found during work on #5882 that we do so many repetitions because of this, for not good reason. I think I found a good balance between convenience and readability with just one method. Smaller the interface = better. Also I don't know what TestSelectSorted was testing, but now it's testing sorting. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

…uirement for remote read to sort response. This is technically BREAKING CHANGE, but it was like this from the beginning: I just notice that we rely in Prometheus on remote read being sorted. This is because we use selected data from remote reads in MergeSeriesSet which rely on sorting. I found during work on #5882 that we do so many repetitions because of this, for not good reason. I think I found a good balance between convenience and readability with just one method. Smaller the interface = better. Also I don't know what TestSelectSorted was testing, but now it's testing sorting. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka · 2020-03-16T12:42:56Z

OK, let's close this in favor of clean version of same thing: #6990

…uirement for remote read to sort response. This is technically BREAKING CHANGE, but it was like this from the beginning: I just notice that we rely in Prometheus on remote read being sorted. This is because we use selected data from remote reads in MergeSeriesSet which rely on sorting. I found during work on prometheus#5882 that we do so many repetitions because of this, for not good reason. I think I found a good balance between convenience and readability with just one method. Smaller the interface = better. Also I don't know what TestSelectSorted was testing, but now it's testing sorting. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka requested review from brian-brazil, krasi-georgiev and codesome August 13, 2019 13:23

bwplotka force-pushed the add-chunks-to-queriers-iterator-tsdb branch 2 times, most recently from f460245 to 1999ce8 Compare August 13, 2019 16:51

bwplotka marked this pull request as ready for review August 13, 2019 16:52

bwplotka commented Aug 13, 2019

View reviewed changes

brian-brazil reviewed Aug 14, 2019

View reviewed changes

krasi-georgiev reviewed Aug 15, 2019

View reviewed changes

bwplotka mentioned this pull request Aug 19, 2019

Add streaming to Remote Read endpoint #4517

Closed

krasi-georgiev self-assigned this Aug 22, 2019

YaoZengzeng mentioned this pull request Aug 29, 2019

Allow Prometheus to use ReadRequest_STREAMED_XOR_CHUNKS when calling remote read API. #5926

Closed

bwplotka mentioned this pull request Nov 28, 2019

Remote read in Store API has a high overhead thanos-io/thanos#1774

Closed

bwplotka force-pushed the add-chunks-to-queriers-iterator-tsdb branch from dbe23b0 to 17c05f2 Compare February 3, 2020 20:37

bwplotka force-pushed the add-chunks-to-queriers-iterator-tsdb branch from 17c05f2 to ad7d8e4 Compare February 4, 2020 19:49

bwplotka mentioned this pull request Feb 6, 2020

Import TSDB directly, without adapter; Unify Iterators, Querier, Series interfaces. #6777

Merged

bwplotka force-pushed the add-chunks-to-queriers-iterator-tsdb branch from ad7d8e4 to ad0106b Compare February 18, 2020 17:10

This was referenced Feb 18, 2020

Don't forget about lack of MaxConcurreny in PromQL the next Prometheus dep update cortexproject/cortex#2045

Closed

Compact: Offline deduplication thanos-io/thanos#1276

Closed

bwplotka force-pushed the add-chunks-to-queriers-iterator-tsdb branch from ad0106b to c530b4b Compare February 26, 2020 20:55

krasi-georgiev reviewed Feb 27, 2020

View reviewed changes

bwplotka mentioned this pull request Mar 12, 2020

storage: Removed SelectSorted method while maintaining logic; Simplified interface. #6971

Merged

bwplotka closed this Mar 16, 2020

bwplotka deleted the add-chunks-to-queriers-iterator-tsdb branch March 16, 2020 12:43

roidelapluie mentioned this pull request Mar 20, 2020

Backport #6971 to release-2.17 #7018

Merged

	testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.
	testutil.Equals(t, tc.seekSuccess, it.Seek(tc.seek)) // Next one should be noop.
	testutil.Equals(t, false, it.Seek(tc.seek-1)) // Seek in the past should be noop and return false.

	if err := it.cur.Err(); err != nil {
	if it.cur.Err() != nil {

	s, w, err := q.SelectSorted(params, matchers...)
	s, w, err := q.genericQuerier.SelectSorted(params, matchers...)

	// ChunkedQuerier provides querying access over time series' chunks of a fixed
	// ChunksQuerier provides querying access over time series' chunks of a fixed

Add ChunksIterator method to Series interface. #5882

Add ChunksIterator method to Series interface. #5882

Conversation

bwplotka commented Aug 13, 2019

Changes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev left a comment

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev Aug 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka commented Nov 28, 2019

Uh oh!

bwplotka commented Feb 3, 2020

Uh oh!

brian-brazil commented Feb 3, 2020

Uh oh!

bwplotka commented Feb 5, 2020

Uh oh!

bwplotka commented Feb 18, 2020

Uh oh!

krasi-georgiev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasi-georgiev Aug 13, 2019 •

edited

Loading

krasi-georgiev Aug 15, 2019 •

edited

Loading