store/bucket: make getFor() work with interleaved resolutions #1146

GiedriusS · 2019-05-15T14:09:49Z

Add interleaved resolutions test for getFor(). Ordinary users could run into such a case when, for example, Thanos Compact would downsample metrics but also the metrics with the better resolution would still be available. Fix this by making getFor() look for blocks "in the middle" as well.

Fix the getDownsamplingParam() interface (and rename it to getDownsamplingParamMillis()) so that it would return a value which will be ready to use by the querier code.

Add unit tests for all of that.

Verification: go test and Grafana dashboards actually started showing all of the data.

Without this, we get max resolutions like 1, 2, 3 which do not mean anything to getFor(). With this, we get proper data from Thanos Store.

Makes a querier via queryableCreator and checks if the maxSourceResolution was passed properly.

domgreen

Looks legit to me :D just nit on naming

pkg/store/bucket.go

Makes it clearer that it's just a temporary variable for the loop.

domgreen

LGTM, tests seem good

Approving without @bwplotka :p

GiedriusS · 2019-05-16T14:31:48Z

Since this touches a critical part of the code I think it would be best to wait for @bwplotka to review this (and, of course, anyone else who wants :P)

bwplotka

Thanks @GiedriusS I think this is our bug found & fixed!

Generally good, but let's revert the previous change #1154 and leave or better document the maxResolution tranformation?

I need to dive into this between thing, it's 1AM and want to ensure we don't miss anything further. For example not sure if divide and conquer recurrency here helps with readability or not.

bwplotka · 2019-05-16T23:03:56Z

pkg/query/querier.go

@@ -55,7 +55,7 @@ type queryable struct {

 // Querier returns a new storage querier against the underlying proxy store API.
 func (q *queryable) Querier(ctx context.Context, mint, maxt int64) (storage.Querier, error) {
-	return newQuerier(ctx, q.logger, mint, maxt, q.replicaLabel, q.proxy, q.deduplicate, int64(q.maxSourceResolution/time.Millisecond), q.partialResponse, q.warningReporter), nil
+	return newQuerier(ctx, q.logger, mint, maxt, q.replicaLabel, q.proxy, q.deduplicate, int64(q.maxSourceResolution), q.partialResponse, q.warningReporter), nil


Quite confusing - hard to tell for me if this makes sense (:

This has the same unit as mint, maxt int64 right? Maybe making the underlying variable maxSourceResolutionMiliseonconds or keeping it time.Duration as long as possible makes sense?

Hm.. am I right that is PR was actually wrong? #1144 And we moved to miliseconds there but we are doing exactly the same here and we missed it? 🤔

If yes, let's remove https://github.com/improbable-eng/thanos/blob/2c4f64b1b96907295a7f8e99d8fd64697f0eb12a/pkg/query/api/v1.go#L227 I think it's wrong. All is in time.Duration so moving to miliseconds there does not make sense. It's here that we transition from duration to integer having arbitrary unit (miliseconds) so I think this should stay and last PR should be reverted.

And yea, master is broken now in terms of downsampling if I am not wrong?

Revert PR:#1154

Perhaps we should move the whole function to return int64 and in a form appropriate to the caller since as we can see it is hard to make a mental connection between such distant places in the code? Because that's what parsing should do IMHO - we shouldn't do more parsing down the road.

And yea, master is broken now in terms of downsampling if I am not wrong?

I think that's a bit of an overstatement if it has never worked properly 😄

Sure but now it does not work even more, so there is regression (:

Anyway, I just want to avoid confusion, sorry for being quite strict here ):

Now once we had no regression we can actually make that work and nice. Yes, I am fine with:

sticking to maxResolutionMilisec int64 name everywhere.

parseResolution returning int64 straight away.

Thoughts? (:

Indeed that's what I've done. Hopefully it is much more clearer now about what kind of units are being used and that no one in the future will run into this problem like me when I was trying to hunt down one problem but accidentally got caught up in this.

pkg/store/bucket.go

bwplotka · 2019-05-16T23:35:30Z

pkg/store/bucket.go

@@ -1030,6 +1032,19 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl
 	if len(bs) == 0 {
 		return s.getFor(mint, maxt, s.resolutions[i])
 	}
+
+	until := len(bs) - 1
+	for j := 0; j < until; j++ {


It's tricky, need to dive more but generally makes sense. Explaining what we are seeing now. Thanks for tests catching this as well!

All of this assumes that blocks within different resolutions are aligned ideally right? Can we comment this somehow?

Also be aware of this: #1104 Hope you algorthim would be nicely extendable I guess.
cc @mjd95

They always should be aligned ideally or on the same timestamps because in other cases we would get a problem known as "overlapping blocks".

This change should be easy to integrate into that PR.

OK, It makes total sense BUT to me left := s.getFor(mint, bs[0].meta.MinTime, s.resolutions[i]) and right are just between 0 and first min and lastMax and maxt, right?

Can we remove this left and right and treat them properly in this loop as between? I think it would simplify flow and algorithm.

The idea sounds good and I played around with it a bit but I doubt that this function can be made more succinct because we have three distinct cases of the append call: at the beginning of the array, between two elements, and at the end. Please correct me if I'm wrong.

Hm, is it really too complex? I will propose something to your PR

Proposed: GiedriusS#1

IMO it's way simpler, what do you think? @GiedriusS

// getFor returns a time-ordered list of blocks that cover date between mint and maxt. // Blocks with the biggest resolution possible but not bigger than the given max resolution are returned. func (s *bucketBlockSet) getFor(mint, maxt, maxResolutionMillis int64) (bs []*bucketBlock) { if mint == maxt { return nil } s.mtx.RLock() defer s.mtx.RUnlock() // Find first matching resolution. i := 0 for ; i < len(s.resolutions) && s.resolutions[i] > maxResolutionMillis; i++ { } // Fill the given interval with the blocks for the current resolution. // Our current resolution might not cover all data, so recursively fill the gaps with higher resolution blocks if there is any. start := mint for _, b := range s.blocks[i] { if b.meta.MaxTime <= mint { continue } if b.meta.MinTime >= maxt { break } if i+1 < len(s.resolutions) { bs = append(bs, s.getFor(start, b.meta.MinTime, s.resolutions[i+1])...) } bs = append(bs, b) start = b.meta.MaxTime } if i+1 < len(s.resolutions) { bs = append(bs, s.getFor(start, maxt, s.resolutions[i+1])...) } return bs }

* Convert parseDownsamplingParam() into parseDownsamplingParamMillis() which properly returns int64 for use in the querier code * Add parseDownsamplingMillis() tests

bwplotka

Thanks 👍 Some suggestions, but overall thanks for spotting this bug.

bwplotka · 2019-05-17T13:52:58Z

pkg/query/api/v1.go

@@ -223,7 +223,7 @@ func (api *API) parseDownsamplingParam(r *http.Request, step time.Duration) (max
 		return 0, &ApiError{errorBadData, errors.Errorf("negative '%s' is not accepted. Try a positive integer", maxSourceResolutionParam)}
 	}

-	return maxSourceResolution, nil
+	return int64(maxSourceResolution / time.Millisecond), nil


bwplotka · 2019-05-17T14:09:41Z

pkg/store/bucket_test.go

+		mint, maxt int64
+		window     int64
+	}
+	input := []resBlock{


What do you think about joinging those together in form of table test? essentiall add those as test cases to TestBucketBlockSet_addGet? might be more consistent and easier to read just "cases" as test itself feels similar TBH?

These ones are a bit special because we are doing a property-based test instead of an ad-hoc one. I have removed the other test cases that I've added before and only left this one in - this already covers the "interleaved resolutions" case.

pkg/store/bucket.go

bwplotka · 2019-05-17T14:13:51Z

pkg/store/bucket.go

@@ -1030,6 +1032,19 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl
 	if len(bs) == 0 {
 		return s.getFor(mint, maxt, s.resolutions[i])
 	}
+
+	until := len(bs) - 1
+	for j := 0; j < until; j++ {


OK, It makes total sense BUT to me left := s.getFor(mint, bs[0].meta.MinTime, s.resolutions[i]) and right are just between 0 and first min and lastMax and maxt, right?

Can we remove this left and right and treat them properly in this loop as between? I think it would simplify flow and algorithm.

This doesn't really matter as the tests show.

Only leave the property tests in place since they catch all of the errors.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka

LGTM for tests, but proposed bit simpler algorithm for implementation, WDYT: GiedriusS#1 @GiedriusS ?

Simplified goFor implementation.

bwplotka

LGTM, thanks for finding and fixing this bug, was so hard to find out this (: The property testing is something awesome to talk about in some Golang meetup!

Giedrius Statkevičius added 2 commits May 15, 2019 16:59

store/bucket_test: add interleaved resolutions test for getFor()

9c94918

store/bucket: include blocks in the middle as well

cd3363c

GiedriusS changed the title ~~store/bucket_test: add interleaved resolutions test for getFor()~~ store/bucket: make getFor() work with interleaved resolutions May 15, 2019

GiedriusS force-pushed the getFor branch from b50f568 to cd3363c Compare May 15, 2019 19:52

Giedrius Statkevičius added 2 commits May 16, 2019 09:53

store/bucket: add test cases with duplicated time ranges

00b2c7c

query/querier: send proper maxSourceResolution

8f22504

Without this, we get max resolutions like 1, 2, 3 which do not mean anything to getFor(). With this, we get proper data from Thanos Store.

GiedriusS requested review from bwplotka, domgreen and povilasv May 16, 2019 08:08

GiedriusS added component: query component: store priority: P0 labels May 16, 2019

GiedriusS changed the title ~~store/bucket: make getFor() work with interleaved resolutions~~ store/bucket: make getFor() work with interleaved resolutions, pass proper resolution from querier May 16, 2019

Giedrius Statkevičius added 3 commits May 16, 2019 11:18

README: add entry

e2c46bf

query/querier_test: add queryableCreator test

342aede

Makes a querier via queryableCreator and checks if the maxSourceResolution was passed properly.

store/bucket: do the iteration without sorting

4e73b2f

domgreen reviewed May 16, 2019

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

store/bucket: bsi->j in loop

fae1929

Makes it clearer that it's just a temporary variable for the loop.

domgreen approved these changes May 16, 2019

View reviewed changes

This was referenced May 16, 2019

query/api: properly pass downsampling param #1144

Merged

Revert "query/api: properly pass downsampling param (#1144)" #1154

Merged

bwplotka requested changes May 16, 2019

View reviewed changes

Giedrius Statkevičius added 3 commits May 17, 2019 11:49

Merge remote-tracking branch 'origin/master' into getFor

bf12553

store/bucket: fix according to review comments

de4b79a

* Convert parseDownsamplingParam() into parseDownsamplingParamMillis() which properly returns int64 for use in the querier code * Add parseDownsamplingMillis() tests

query/querier_test: fix test

c3abb09

GiedriusS changed the title ~~store/bucket: make getFor() work with interleaved resolutions, pass proper resolution from querier~~ store/bucket: make getFor() work with interleaved resolutions May 17, 2019

Giedrius Statkevičius added 3 commits May 17, 2019 13:38

*: clarify everywhere that max source resolution is in millis

f22cffa

*: maxSourceResolutionMillis -> maxResolutionMillis

5e9d0e9

CHANGELOG: update

420ebe0

query/querier_test: fix

e8b3189

GiedriusS requested review from bwplotka and domgreen May 17, 2019 11:14

bwplotka reviewed May 17, 2019

View reviewed changes

Giedrius Statkevičius added 8 commits May 17, 2019 17:36

store/bucket: add gets all data in range property test

bacfd06

store/bucket_test: add production property test

93a764c

store/bucket_test: fix

f4b0a66

store/bucket_test: add always gets property

dac133e

query/querier_test: do not shrink

dbe3727

store/bucket: revert change

6dac954

This doesn't really matter as the tests show.

store/bucket_test: remove more confusion

55aa32d

store/bucket: clean up tests

3fe4217

Only leave the property tests in place since they catch all of the errors.

GiedriusS requested a review from bwplotka May 22, 2019 13:17

bwplotka mentioned this pull request May 27, 2019

ragged chart with Max1hDownsampling #1063

Closed

Simplified goFor implementation.

3bbc6cb

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

bwplotka reviewed May 27, 2019

View reviewed changes

Merge pull request #1 from improbable-eng/getFor

3602678

Simplified goFor implementation.

bwplotka approved these changes May 27, 2019

View reviewed changes

bwplotka merged commit 34ad98e into thanos-io:master May 27, 2019

This was referenced May 30, 2019

No datapoints for queries with a window smaller than 105h with auto-downsampling selected #1127

Closed

query: auto-downsampling causes inaccurate output of metrics (inflated values) #922

Closed

tik-stbuehler mentioned this pull request Jul 9, 2019

query: gaps for 1h downsampled blocks #1316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store/bucket: make getFor() work with interleaved resolutions #1146

store/bucket: make getFor() work with interleaved resolutions #1146

GiedriusS commented May 15, 2019 •

edited

Loading

domgreen left a comment

domgreen left a comment

GiedriusS commented May 16, 2019

bwplotka left a comment

bwplotka May 16, 2019

bwplotka May 16, 2019

bwplotka May 16, 2019

bwplotka May 16, 2019

GiedriusS May 17, 2019

GiedriusS May 17, 2019

bwplotka May 17, 2019

GiedriusS May 17, 2019

bwplotka May 16, 2019

GiedriusS May 17, 2019

bwplotka May 17, 2019 •

edited

Loading

GiedriusS May 18, 2019 •

edited

Loading

bwplotka May 27, 2019

bwplotka May 27, 2019

bwplotka left a comment

bwplotka May 17, 2019

bwplotka May 17, 2019

GiedriusS May 22, 2019 •

edited

Loading

bwplotka May 27, 2019

bwplotka May 17, 2019 •

edited

Loading

bwplotka left a comment

bwplotka left a comment

store/bucket: make getFor() work with interleaved resolutions #1146

store/bucket: make getFor() work with interleaved resolutions #1146

Conversation

GiedriusS commented May 15, 2019 • edited Loading

domgreen left a comment

Choose a reason for hiding this comment

domgreen left a comment

Choose a reason for hiding this comment

GiedriusS commented May 16, 2019

bwplotka 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

bwplotka May 17, 2019 • edited Loading

Choose a reason for hiding this comment

GiedriusS May 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS May 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka May 17, 2019 • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

GiedriusS commented May 15, 2019 •

edited

Loading

bwplotka May 17, 2019 •

edited

Loading

GiedriusS May 18, 2019 •

edited

Loading

GiedriusS May 22, 2019 •

edited

Loading

bwplotka May 17, 2019 •

edited

Loading