Flag to add active query tracker. #5555

metonymic-smokey · 2022-07-30T15:02:26Z

Signed-off-by: Aditi Ahuja ahuja.aditi@gmail.com
Fixes #5521

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

Changes

Added a new flag which creates a query.active file in the filepath specified by the flag.

Verification

yeya24

Love this change. LGTM.

Fixes part of #5521

For this pr, I feel it is good to fix 5521. If you plan to also add the UI part of the active queries I would say let's do it in another PR.

Let's also fix docs and add a changelog entry

metonymic-smokey · 2022-07-30T17:23:47Z

Thanks @yeya24 :)
This breaks the E2E tests and I have a feeling it's due to the fact that they're run in containers. This seems to be an existing issue - prometheus/prometheus#5976

Looking into how I can fix this failing test.
Thanks!

Dockerfile

cmd/thanos/query.go

saswatamcode

Thanks for awesome work! This would be awesome to have! 🌟

Some tiny suggestions to fix CI!

saswatamcode · 2022-07-31T12:54:25Z

test/e2e/e2ethanos/services.go

@@ -345,6 +345,9 @@ func (q *QuerierBuilder) collectArgs() ([]string, error) {

 		args = append(args, "--store.sd-files="+filepath.Join(q.InternalDir(), "filesd.yaml"))
 	}
+
+	args = append(args, "--query.active-query-path="+q.InternalDir())


So this breaks a test, specifically the TestQueryCompatibilityWithPreInfoAPI e2e test, reason being that we use specific images for the test cases on that one here. The new -query.active-query-path flag won't exist on any image but the latest, so this cannot be an always-enabled option. 🙂

Maybe you can address @GiedriusS's comment and make the flag optional, and add a new WithActiveQueryTracker method on QuerierBuilder and use that instead in a dedicated e2e test which checks for this file?

So from my attempts at an E2E test, I think the same issues that plagued this PR during CI will occur during an E2E test. The issue I'm referring to is related to container permissions and seems to be a known issue: prometheus/prometheus#5976.
Is there any alternative way I could try?

After making this flag optional in E2E and add WithActiveQueryTracker then you should be good.

I've pushed a draft of the test I created and I have added these but I still face an issue.

Yes, what I think you could do is maybe execute a command, to change permissions for the data dir, as mentioned in this comment! 🙂

It might be a bit too difficult to make it work in an e2e setup, so I think it would be fine to skip an e2e test for this. But maybe you can document this issue in Querier docs instead? So users are aware of the implications of using this! 🙂

docs/components/query.md

yeya24 · 2022-08-01T18:18:45Z

@metonymic-smokey I think this feature is ready for merge now. Can you please mark it as ready for review?

metonymic-smokey · 2022-08-02T02:48:48Z

@yeya24 marked it as ready, but I think I need to add an E2E test right?

yeya24 · 2022-08-02T03:09:45Z

Yeah, please add it.

metonymic-smokey · 2022-08-03T12:37:17Z

@GiedriusS @yeya24 pls take a final look at this when you're free :)

docs/components/query.md

GiedriusS

We should also pass the parameter here. That code creates different engines for different lookback deltas depending on the parameters. Thus, I suggest creating subdirectories like raw, 5m and 1h for the different engines in the provided directory. Maybe you could even move the creation of the ActiveQueryTracker to that function because you will possibly need to create multiple trackers

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

saswatamcode

Awesome work! LGTM!

cmd/thanos/query.go

yeya24 · 2022-08-05T05:27:46Z

cmd/thanos/query.go

+			resActiveQueryDir := filepath.Join(activeQueryDir, getActiveQueryDirBasedOnResolution(r))
+			newEngineOpts.ActiveQueryTracker = promql.NewActiveQueryTracker(resActiveQueryDir, maxConcurrentQueries, logger)
+		} else {
+			newEngineOpts.ActiveQueryTracker = eo.ActiveQueryTracker


Do we need this else block? If activeQueryDir is empty string then the ActiveQueryTracker is just nil if I understand correctly

@yeya24 in the current implementation, this line explicitly adds eo.ActiveQueryTracker

thanos/cmd/thanos/query.go

Line 761 in 99239df

ActiveQueryTracker: eo.ActiveQueryTracker,

.
I have attempted to retain that when I added the else.

Let's remove it to avoid confusion.

Or you could add a comment about it is nil.

Thanks, done.
If possible could you please retrigger the CI tests once again?
Thanks!

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

yeya24

LGTM. Thanks for the great work!

pull-request-size bot added the size/XS label Jul 30, 2022

yeya24 previously approved these changes Jul 30, 2022

View reviewed changes

metonymic-smokey marked this pull request as ready for review July 30, 2022 16:47

metonymic-smokey dismissed yeya24’s stale review via 9e11c61 July 30, 2022 16:57

pull-request-size bot added size/M and removed size/XS labels Jul 30, 2022

metonymic-smokey changed the title ~~Flag to add file to track active queries.~~ Flag to add active query tracker. Jul 30, 2022

metonymic-smokey marked this pull request as draft July 30, 2022 17:10

sonatype-lift bot reviewed Jul 31, 2022

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

metonymic-smokey force-pushed the promql branch 4 times, most recently from 954d3bb to 9763d25 Compare July 31, 2022 08:11

GiedriusS reviewed Jul 31, 2022

View reviewed changes

cmd/thanos/query.go Outdated Show resolved Hide resolved

saswatamcode previously approved these changes Jul 31, 2022

View reviewed changes

metonymic-smokey dismissed saswatamcode’s stale review via 118d32f July 31, 2022 13:49

metonymic-smokey force-pushed the promql branch 4 times, most recently from d2fdcf7 to 9efc71e Compare August 1, 2022 14:44

pull-request-size bot added size/S and removed size/M labels Aug 1, 2022

metonymic-smokey marked this pull request as ready for review August 2, 2022 02:47

metonymic-smokey force-pushed the promql branch from 9efc71e to 1fa3848 Compare August 2, 2022 02:48

metonymic-smokey force-pushed the promql branch from 1fa3848 to b19e03e Compare August 2, 2022 15:45

pull-request-size bot removed the size/S label Aug 2, 2022

metonymic-smokey force-pushed the promql branch from b19e03e to 2e0348e Compare August 3, 2022 02:56

pull-request-size bot added size/S and removed size/M labels Aug 3, 2022

metonymic-smokey force-pushed the promql branch from 2e0348e to a3cb06f Compare August 3, 2022 12:36

GiedriusS reviewed Aug 3, 2022

View reviewed changes

docs/components/query.md Outdated Show resolved Hide resolved

GiedriusS reviewed Aug 3, 2022

View reviewed changes

metonymic-smokey added 5 commits August 3, 2022 21:23

draft: added new flag for query tracking

8ab78bf

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

updated docs

da4cf10

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

updated CHANGELOG

f4824ae

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

review fixes

5fb5343

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

doc fixes

bda234f

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

metonymic-smokey force-pushed the promql branch from a3cb06f to aac92ec Compare August 4, 2022 12:00

pull-request-size bot added size/M and removed size/S labels Aug 4, 2022

review fixes

140fec1

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

metonymic-smokey force-pushed the promql branch from aac92ec to 140fec1 Compare August 4, 2022 12:27

metonymic-smokey requested review from GiedriusS, yeya24 and saswatamcode August 4, 2022 12:59

saswatamcode previously approved these changes Aug 4, 2022

View reviewed changes

GiedriusS reviewed Aug 4, 2022

View reviewed changes

cmd/thanos/query.go Outdated Show resolved Hide resolved

GiedriusS reviewed Aug 4, 2022

View reviewed changes

cmd/thanos/query.go Outdated Show resolved Hide resolved

metonymic-smokey dismissed saswatamcode’s stale review via 8b292ec August 4, 2022 14:43

metonymic-smokey force-pushed the promql branch from 8b292ec to b692b30 Compare August 4, 2022 14:45

yeya24 reviewed Aug 5, 2022

View reviewed changes

minor fixes

42913d7

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

metonymic-smokey force-pushed the promql branch from 59cd803 to 42913d7 Compare August 6, 2022 04:16

yeya24 approved these changes Aug 6, 2022

View reviewed changes

yeya24 merged commit 79a6335 into thanos-io:main Aug 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flag to add active query tracker. #5555

Flag to add active query tracker. #5555

metonymic-smokey commented Jul 30, 2022 •

edited

Loading

yeya24 left a comment •

edited

Loading

metonymic-smokey commented Jul 30, 2022

saswatamcode left a comment

saswatamcode Jul 31, 2022

metonymic-smokey Aug 2, 2022

yeya24 Aug 2, 2022

metonymic-smokey Aug 2, 2022

saswatamcode Aug 2, 2022

saswatamcode Aug 2, 2022

yeya24 commented Aug 1, 2022

metonymic-smokey commented Aug 2, 2022

yeya24 commented Aug 2, 2022

metonymic-smokey commented Aug 3, 2022

GiedriusS left a comment

saswatamcode left a comment

yeya24 Aug 5, 2022

metonymic-smokey Aug 5, 2022

yeya24 Aug 5, 2022

yeya24 Aug 5, 2022

metonymic-smokey Aug 6, 2022

yeya24 left a comment

Flag to add active query tracker. #5555

Flag to add active query tracker. #5555

Conversation

metonymic-smokey commented Jul 30, 2022 • edited Loading

Changes

Verification

yeya24 left a comment • edited Loading

Choose a reason for hiding this comment

metonymic-smokey commented Jul 30, 2022

saswatamcode 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

yeya24 commented Aug 1, 2022

metonymic-smokey commented Aug 2, 2022

yeya24 commented Aug 2, 2022

metonymic-smokey commented Aug 3, 2022

GiedriusS left a comment

Choose a reason for hiding this comment

saswatamcode 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

yeya24 left a comment

Choose a reason for hiding this comment

metonymic-smokey commented Jul 30, 2022 •

edited

Loading

yeya24 left a comment •

edited

Loading