query: introduce dynamic lookback interval by krya-kryak · Pull Request #3277 · thanos-io/thanos

krya-kryak · 2020-10-05T22:50:25Z

Closes #2608
This allows queries with large step to make use of downsampled data.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Added CLI option for query

Verification

This patch was tested with 0.15 branch (minor conflict solved manually), works as expected

Signed-off-by: Vladimir Kononov krya-kryak@users.noreply.github.com

Closes thanos-io#2608 This allows queries with large step to make use of downsampled data. Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

bwplotka

Great job! Thanks for this 👍

Overall to me, it makes sense, just some implementation suggestion etc. 🤗

NOTE: Let's reduce amount of engines we create - can we create just 3 engines (number of resolution we have) and just create promql.Engine wrapper that routes to correct one based on maxSourceResolution? 🤔

cmd/thanos/query.go

bwplotka · 2020-10-08T15:58:45Z

cmd/thanos/query.go

+		totalEngines int
+		newEngine    = func(ld time.Duration) *promql.Engine {
+			var r prometheus.Registerer = reg
+			if dynamicLookbackDelta {


Oh wow 😱 Makes sense, wonder if it's doable to contribute to Promtheus some tweak to allow us to avoid creating 3 engines.

Added a TODO for now

bwplotka · 2020-10-08T16:04:32Z

cmd/thanos/query.go

+			return defaultEvaluationInterval.Milliseconds()
+		}
+		totalEngines int
+		newEngine    = func(ld time.Duration) *promql.Engine {


I am a bit confused, do we need two functions? What about engineFunc? I guess you wanted to have large number of args in function, but it might be cleaner to embed engineFunc here, not embedding this function in engineFunc. WDYT?

You're absolutely right, having two functions is confusing. Got some coffee, merged engineFunc and newEngine into single engineFactory. IMO things got a lot more unerstandable. I hope this isn't just coffee speaking.

bwplotka · 2020-10-08T16:08:19Z

cmd/thanos/query.go

+	for i, d := range deltas[1:] {
+		if ld < d {
+			// Convert delta from milliseconds to time.Duration (nanoseconds).
+			engines[i+1] = newEngine(time.Duration(d * 1_000_000))


This looks weird and should fail our lints: 1_000_000 - what about using timestamp.Time(...)

replaced with standard time.Duration(d) * time.Millisecond. Prometheus'es timestamp seems unhelpful here, as it's Duration we need, not Time

cmd/thanos/query.go

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

krya-kryak · 2020-10-09T00:58:35Z

NOTE: Let's reduce amount of engines we create - can we create just 3 engines (number of resolution we have) and just create promql.Engine wrapper that routes to correct one based on maxSourceResolution? 🤔

Well, this seems to be more or less similar to how it is done already - we create at most three engines, but choose correct one with an anonymous function call (note pkg/api/query change) instead of a wrapper. It seemed like a less intrusive way to implement, please correct me if I'm wrong.

bwplotka

One important question that came to my mind before going full for this solution (:

cmd/thanos/query.go

bwplotka · 2020-10-15T23:46:25Z

Wait, which one is the current fix / better? (: #3267 or this one?

krya-kryak · 2020-10-16T09:03:01Z

Wait, which one is the current fix / better? (: #3267 or this one?

I really prefer this one over #3267 for several reasons:

It doesn't mess with metrics staleness as much
It's less heavy on resources
It doesn't introduce rounding errors to query results

I'll just close that one.

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

bwplotka

Awesome! Some ideas for simplifying the code still, but otherwise LGTM (:

cmd/thanos/query.go

bwplotka · 2020-11-06T14:00:35Z

cmd/thanos/query.go

+		totalEngines++
+		return wr
+	}
+	rawEngine := newEngine(promql.EngineOpts{


Can we remove this and init all engines in exactly the same way (no matter if raw or not raw) (:

It might simplify the code

Thanks for the suggestion. Done.

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

bwplotka

LGTM! Thanks 👍

Sorry for delayed review 🤗

bwplotka · 2020-11-06T15:48:56Z

cmd/thanos/query.go

+		ld      = eo.LookbackDelta.Milliseconds()
+	)
+	wrapReg := func(engineNum int) prometheus.Registerer {
+		return extprom.WrapRegistererWith(map[string]string{"engine": strconv.Itoa(engineNum)}, eo.Reg)


I would just inline this (:

Not a blocker (:

* query: introduce dynamic lookback interval Closes thanos-io#2608 This allows queries with large step to make use of downsampled data. Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Fix minor checks Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Append changelog Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Add missing copyright Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Use pre-defined downsampling resolution constatns Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Use dynamic lookback delta by default Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Rename defaultEngine to rawEngine Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Merge engineFunc and newEngine into single engineFactory Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Remove query.dynamic-lookback-delta from docs Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Rename test Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> * Review fixes Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com> Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Oghenebrume50 <raphlbrume@gmail.com>

krya-kryak mentioned this pull request Oct 5, 2020

query: Staleness problem #2608

Closed

krya-kryak added 3 commits October 6, 2020 02:08

query: introduce dynamic lookback interval

75de5c9

Closes thanos-io#2608 This allows queries with large step to make use of downsampled data. Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Fix minor checks

71a61eb

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Append changelog

5262d98

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

krya-kryak force-pushed the dynamic-lookback-delta branch from ec0651e to 5262d98 Compare October 5, 2020 23:08

krya-kryak added 2 commits October 6, 2020 02:14

Add missing copyright

6935ece

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Use pre-defined downsampling resolution constatns

2824f45

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

bwplotka reviewed Oct 8, 2020

View reviewed changes

krya-kryak and others added 2 commits October 8, 2020 19:21

Use dynamic lookback delta by default

b491083

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Rename defaultEngine to rawEngine

29ea24e

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

krya-kryak force-pushed the dynamic-lookback-delta branch from 2d31bfe to 29ea24e Compare October 8, 2020 16:24

krya-kryak added 3 commits October 9, 2020 03:08

Merge engineFunc and newEngine into single engineFactory

7413ba5

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Remove query.dynamic-lookback-delta from docs

22acb87

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

Rename test

cfbcd46

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

bwplotka reviewed Oct 15, 2020

View reviewed changes

cmd/thanos/query.go Show resolved Hide resolved

krya-kryak mentioned this pull request Oct 16, 2020

query: chain interpolating iterator (#2608) #3267

Closed

krya-kryak requested a review from bwplotka October 23, 2020 13:09

Merge branch 'master' into dynamic-lookback-delta

8daedb0

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

krya-kryak force-pushed the dynamic-lookback-delta branch from 0cd7f56 to 8daedb0 Compare November 2, 2020 11:03

Merge branch 'master' into dynamic-lookback-delta

d4a3db6

bwplotka reviewed Nov 6, 2020

View reviewed changes

Review fixes

886ddd8

Signed-off-by: Vladimir Kononov <krya-kryak@users.noreply.github.com>

krya-kryak force-pushed the dynamic-lookback-delta branch from dda5b24 to 886ddd8 Compare November 6, 2020 15:03

bwplotka approved these changes Nov 6, 2020

View reviewed changes

bwplotka merged commit f6e5d59 into thanos-io:master Nov 6, 2020

krya-kryak deleted the dynamic-lookback-delta branch November 10, 2020 10:55

Conversation

krya-kryak commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

krya-kryak commented Oct 9, 2020

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka commented Oct 15, 2020

Uh oh!

krya-kryak commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

krya-kryak commented Oct 5, 2020 •

edited

Loading

krya-kryak commented Oct 16, 2020 •

edited

Loading