querier: bugfix - data gaps when switching iterators #3010

krasi-georgiev · 2020-08-10T11:23:36Z

closes: #3001

Promql has a default 5m look back delta so if the penalty causes the
duration between the samples to be greater than 5m this will show as
gaps in the visualisation and when using the count function returns a
lower number then expected.

For example:
time is in mins

series 1

itA  t1 0.1  t3 0.3
itB  t1 0.1  t3 0.3 t4 0.4  t9 0.9

it.penB = 2 * (ta - it.lastT), 2 * (3 - 1) = 4

merged t1 0.1  t3 0.3  t9 0.9

t4 is missing because the it.penB is 4 so it skips it.

When the time between t3 0.3 and t9 0.9 is more then 5min(the default look
back) so when visualizing this it shows as gaps in the graph.

When the penalty is resolution based the seek for itB always advances NO
more then the requested time so doesn’t skip samples when switching
between iterators.

UPDATE: step based penalty wouldn't work when using promql functions. Adjusting the look back solves the problem so PR is updated to add it as a configurable flag.

bwplotka

Nice, can we make it buildable AND add test case you were reproing? Without that - hard to prove it works. (:

pkg/api/query/v1.go

pkg/query/iter.go

CHANGELOG.md

pkg/query/querier_test.go

pkg/query/iter.go

pkg/query/querier_test.go

bwplotka

OK, this is critical path so let's take it carefully. I am not fan of changing all the tests and carefully adjusted test cases.

Please be aware that we planned to have this algo stabilized to ensure we can run offline deduplication safely. With this we just start from scratch as we changed a bit - plus we change unit tests. This is quite risky.

I checked all and I don't think step based dedup will work ):

As commented below:

Let's take following query: sum_over_time(my_metric[1h]) for 4d with step 1h. Let's say we have 15s scrape interval of this metric and 2 replicas with rollout in between. So let's say:

a replica samples (timestamps after start time): 0, 1, 2, 3,..... 59m58s, 59m59s, 1h, <rollout take 10m>, 1h20m, 1h20m1s, 1h20m2s...
b replica samples 1, 2, 3, 4, ..... every second always

Now our algo will choose replica a first and add step penalty (1h). At the last sample before rollout we will make penalty of B = step (1h), so next iteration we have:

1h20m form a, and 2h from b. This is a gap that could be totally avoided because b has that data. This will be highly visible on PromQL result despite the large step, as certain queries care about each samples - this has to be reliable ):

cc @brian-brazil if you have time.

Let's also do research on how others are doing it, but I believe the algo should be scrape interval-based - we need to assume it based on seen samples. I don't see any other way right now.

Curious if we could team up with promxy creator @jacksontj - we are in open source after all 🤗
We definitely work on something similar, he has some interesting approach to parallel queries on leafs: https://github.com/jacksontj/promxy/blob/master/pkg/proxystorage/proxy.go#L179 but cannot find any deduplication specifics.

CHANGELOG.md

pkg/query/iter.go

pkg/query/querier_test.go

pkg/query/iter.go

brian-brazil · 2020-08-24T11:45:23Z

I checked all and I don't think step based dedup will work ):

As a matter of general principle, I agree. Due to races etc. you're going to need to look at 2-3 intervals minimum.

I believe the algo should be scrape interval-based - we need to assume it based on seen samples. I don't see any other way right now.

Indeed, and there will always have to be a gap left for safety when you switch replicas, otherwise you're going to have issues with counters.

krasi-georgiev · 2020-08-27T11:13:19Z

Turns out the deduplication alg works as expected and the problem is that in our case the data was sent at an interval of 4.5min and the dedup alg uses 2xdelta so 9min which is more then the default lookBackDelta in promql(5min).

Changed the PR to make the look back configurable.

bwplotka

Thanks, LGTM, just minor nit in terms of description, also let's fix changelog.

And please remove fixes stuff - it does not fix any of the issues mentioned I think.

bwplotka · 2020-08-27T12:41:03Z

cmd/thanos/query.go

@@ -70,6 +70,8 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application) {
 	maxConcurrentQueries := cmd.Flag("query.max-concurrent", "Maximum number of queries processed concurrently by query node.").
 		Default("20").Int()

+	lookbackDelta := cmd.Flag("query.lookback-delta", "The maximum lookback duration for retrieving metrics during expression evaluations.").Duration()


Can we describe it better? It's not maximum but actually lookback delta right? And also we should explain what it means for PromQL or link to docs 🤗

Added some more details, let me know what you think.

Unfortunately, there are official Prom docs about this flag.

CHANGELOG.md

bwplotka

Still not addressed comments ):

CHANGELOG.md

kakkoyun · 2020-09-01T09:28:32Z

@krasi-georgiev We can include this in the next release candidate, so could you baed ut on release-0.15 branch?

bwplotka

One suggestion, otherwise LGTM! 💪

bwplotka · 2020-09-01T10:48:29Z

CHANGELOG.md

@@ -16,6 +16,7 @@ We use *breaking :warning:* word for marking changes that are not backward compa
 * [#3095](https://github.com/thanos-io/thanos/pull/3095) Rule: update manager when all rule files are removed.
 * [#3098](https://github.com/thanos-io/thanos/pull/3098) ui: Fix Block Viewer for Compactor and Store
 * [#3105](https://github.com/thanos-io/thanos/pull/3105) Query: Fix overwriting maxSourceResolution when auto downsampling is enabled.
+* [#3010](https://github.com/thanos-io/thanos/pull/3010) Querier: Added a flag to override the default look back delta in promql. The flag should be set to at least 2 times the slowest scrape interval.


Let's not forget to fix this on @kakkoyun PR (:

cmd/thanos/query.go

docs/components/query.md

…om> Date: Thu Aug 27 14:10:19 2020 +0300 make the promql lookBack configurable with a flag Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

kakkoyun

LGTM 🥇

…om> Date: Thu Aug 27 14:10:19 2020 +0300 (thanos-io#3010) Make the PromQL lookBack configurable with a flag Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

…om> Date: Thu Aug 27 14:10:19 2020 +0300 (thanos-io#3010) Make the PromQL lookBack configurable with a flag Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Made sure old sse S3 option if specified, produces error. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Cut v0.15.0-rc.0 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * tsdbstore: Optimized response framing if iterator finished. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Apply suggestions from code review Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Rule: update manager when all rule files are removed (#3095) This bug was already fixed in #2615 but it got lost when we merged #2200. Co-authored-by: johncming <johncming@yahoo.com> Co-authored-by: Lili Cosic <cosiclili@gmail.com> Signed-off-by: Simon Pasquier <spasquie@redhat.com> Co-authored-by: johncming <johncming@yahoo.com> Co-authored-by: Lili Cosic <cosiclili@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * ui: Fix Block Viewer for Compactor and Store (#3098) * ui: Fix Block viewer for Compactor and Store Signed-off-by: Prem Kumar <prmsrswt@gmail.com> * Add an entry in CHANGELOG.md Signed-off-by: Prem Kumar <prmsrswt@gmail.com> * ui: react: Fixed tests for Blocks Signed-off-by: Prem Kumar <prmsrswt@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * querier: Fix overwriting maxSourceResolution when auto downsampling is enabled (#3105) * Fix overwriting maxSourceResolution when auto downsampling is enabled Signed-off-by: Ben Ye <yb532204897@gmail.com> * add changelog Signed-off-by: Ben Ye <yb532204897@gmail.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Author: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Date: Thu Aug 27 14:10:19 2020 +0300 (#3010) Make the PromQL lookBack configurable with a flag Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Cut release v0.15.0-rc.1 (#3103) * Cut release v0.15.0-rc.1 Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Address review issues Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Fix changelog Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Cut release v0.15.0 (#3129) Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Minor fix Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> * Address review comments Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Co-authored-by: Simon Pasquier <spasquie@redhat.com> Co-authored-by: johncming <johncming@yahoo.com> Co-authored-by: Lili Cosic <cosiclili@gmail.com> Co-authored-by: Prem Kumar <prmsrswt@gmail.com> Co-authored-by: Ben Ye <yb532204897@gmail.com>

Behaviour of this changed in thanos-io/thanos#3010 -- this now generates an error, as it probably should, but we sent back a series with no datapoints in it previously.

krasi-georgiev changed the title ~~bigfix - data gaps when switching iterators~~ querier: bugfix - data gaps when switching iterators Aug 10, 2020

krasi-georgiev force-pushed the qyerying-dedup-penalty-with-res-step branch 4 times, most recently from 83b1a92 to 2c6d15d Compare August 10, 2020 12:00

bwplotka requested changes Aug 10, 2020

View reviewed changes

pkg/api/query/v1.go Outdated Show resolved Hide resolved

pkg/query/iter.go Outdated Show resolved Hide resolved

krasi-georgiev force-pushed the qyerying-dedup-penalty-with-res-step branch from 53ccdba to 6c06810 Compare August 10, 2020 23:08

kakkoyun reviewed Aug 11, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

krasi-georgiev mentioned this pull request Aug 13, 2020

query: invalid rate/irate with deduplication for most recent values #2890

Closed

krasi-georgiev force-pushed the qyerying-dedup-penalty-with-res-step branch from 8fb0fe0 to db2eb42 Compare August 21, 2020 10:36