tsdb: check for context cancel before regex matching postings #14096

krajorama · 2024-05-14T05:47:53Z

Regex matching can be heavy if the regex takes a lot of cycles to evaluate and we can get stuck evaluating postings for a long time without this fix.

This PR adds a missing unit test for checking that the index reader honors context cancel when doing r.traversePostingOffsets iteration.

Regex matching can be heavy if the regex takes a lot of cycles to evaluate and we can get stuck evaluating postings for a long time without this fix. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama · 2024-05-14T07:14:36Z

We could probably have more sophisticated generated tests, but these show the intent at least.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

tsdb/index/index_test.go

tsdb/index/postings_test.go

tsdb/querier_test.go

pracucci

LGTM (modulo a couple of nits)

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

jesusvazquez

Left a small question

tsdb/index/index.go

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

tsdb/index/index_test.go

bwplotka

Thanks, LGTM! (modulo @pracucci comments)

tsdb/index/index.go

colega · 2024-05-14T13:14:12Z

tsdb/index/index.go

+	// checkContextEveryNIterations is used in some tight loops to check if the context is done.
+	checkContextEveryNIterations = 100


I think this should be 1024

I feel like 100 is too often. How often do we want to check? I'd say every 10ms or even 100ms would be enough. For 10ms, this would mean that each check takes 100 microseconds, and that sounds like a lot (do we have numbers?)

Can we make it a power of two like 1024? We don't really care if it's every 1000 or every 1024 (or 100 vs 128), but it's way cheaper for the CPU to do the latter operation. (I did the benchmarks, and there's a noticeable difference when switching to 128. Interestingly, same difference as checking count == count>>7<<7 but for some reason, checking count & (1<<7-1) == 0 is less performant)

I'm also happy to follow up later with a separate PR and benchmarks that would support this comment.

Agreed that checking every 100ms is enough.
We didn't have timings; I guessed that 100 was right.
The case that prompted this PR had a 7KB regexp with a 40-deep stack.
Also the CPU was throttled because it was well over its request.

I don't have a favorite in this race, we've seen 1000 be effective in a test we did. @bboreham are you ok changing to 1024?

Given that we put out the fire in our own infra, I would like to see one timing of one comparable case before changing this.

Let's merge with 100 and I'll bring proper benchmarks to update the value later.

Follow up: #14118

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

grafana/mimir-prometheus@cf682ab Relevant PRs: prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Followup to prometheus#14096 Unfortunately the previous PR introduced this bug by not releasing the lock before returning. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> (cherry picked from commit 307567a)

* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> (cherry picked from commit 307567a) Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>

…heus#14096) * tsdb: check for context cancel before regex matching postings Regex matching can be heavy if the regex takes a lot of cycles to evaluate and we can get stuck evaluating postings for a long time without this fix. The constant checkContextEveryNIterations=100 may be changed later. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>

Followup to prometheus#14096 Unfortunately the previous PR introduced this bug by not releasing the lock before returning. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>

Quote label name in matchers when needed When the label name of a matcher contains non-standard characters, like a dot, or starts with a digit, it should be quoted. If it's not quoted, then `VectorSelector.String()` isn't a valid PromQL. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com> Optimize Matcher.String() Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com> Use bytes.Buffer from stack buf in Matcher.String() Also removed the growing until there's a benchmark for that. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com> tsdb: check for context cancel before regex matching postings (prometheus#14096) * tsdb: check for context cancel before regex matching postings Regex matching can be heavy if the regex takes a lot of cycles to evaluate and we can get stuck evaluating postings for a long time without this fix. The constant checkContextEveryNIterations=100 may be changed later. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com> tsdb/index/postings: fix missing lock unlock Followup to prometheus#14096 Unfortunately the previous PR introduced this bug by not releasing the lock before returning. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com> fix: rebase bugfix: allow opting-out of multi-cluster setups Allow users to opt-out of the multi-cluster setup for Prometheus dashboard, in environments where it isn't applicable. Refer: prometheus#13180. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com> Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>

Follow up on prometheus#14096 As promised, I bring a benchmark, which shows a very small improvement if context is checked every 128 iterations of label instead of every 100. It's much easier for a computer to check modulo 128 than modulo 100. This is a very small 0-2% improvement but I'd say this is one of the hottest paths of the app so this is still relevant. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Follow up on #14096 As promised, I bring a benchmark, which shows a very small improvement if context is checked every 128 iterations of label instead of every 100. It's much easier for a computer to check modulo 128 than modulo 100. This is a very small 0-2% improvement but I'd say this is one of the hottest paths of the app so this is still relevant. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

tsdb: check for context cancel before regex matching postings

1631e4e

Regex matching can be heavy if the regex takes a lot of cycles to evaluate and we can get stuck evaluating postings for a long time without this fix. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama requested a review from jesusvazquez as a code owner May 14, 2024 05:47

krajorama marked this pull request as draft May 14, 2024 05:50

This comment was marked as outdated.

Sign in to view

Add unit tests

a6d7a9a

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama marked this pull request as ready for review May 14, 2024 07:08

Use correct atomic pkg

ec5de75

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama added 2 commits May 14, 2024 09:18

Actually use MockContext as base for MockContextErrAfter

ce063ae

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Delete unused code

e6f1af3

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

pracucci reviewed May 14, 2024

View reviewed changes

tsdb/index/index_test.go Outdated Show resolved Hide resolved

tsdb/index/postings_test.go Outdated Show resolved Hide resolved

tsdb/querier_test.go Outdated Show resolved Hide resolved

tsdb/querier_test.go Outdated Show resolved Hide resolved

pracucci approved these changes May 14, 2024

View reviewed changes

Be specific with the required number of calls

c8d1aec

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

jesusvazquez reviewed May 14, 2024

View reviewed changes

tsdb/index/index.go Show resolved Hide resolved

krajorama added 3 commits May 14, 2024 10:18

Be even more specific and explain

c1791bb

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Missed this change in previous commit

55b7232

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

Make 100 a const

d6f9ff7

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

pracucci approved these changes May 14, 2024

View reviewed changes

tsdb/index/index_test.go Outdated Show resolved Hide resolved

bwplotka approved these changes May 14, 2024

View reviewed changes

tsdb/index/index.go Show resolved Hide resolved

colega reviewed May 14, 2024

View reviewed changes

Make test independent from value of checkContextEveryNIterations

b1d6d39

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

krajorama merged commit fdaafdb into prometheus:main May 15, 2024
25 checks passed

krajorama mentioned this pull request May 15, 2024

merge upstream at fdaafdb041 grafana/mimir-prometheus#634

Merged

krajorama mentioned this pull request May 15, 2024

Update mimir-prometheus at e5b85c151ba8 grafana/mimir#8141

Merged

krajorama mentioned this pull request May 15, 2024

tsdb/index/postings: fix missing lock unlock #14103

Merged

colega mentioned this pull request May 17, 2024

Check context every 128 labels instead of 100 #14118

Merged

gotjosh mentioned this pull request May 30, 2024

Merge upstream prometheus/prometheus at 37b408c grafana/mimir-prometheus#639

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsdb: check for context cancel before regex matching postings #14096

tsdb: check for context cancel before regex matching postings #14096

krajorama commented May 14, 2024 •

edited

This comment was marked as outdated.

krajorama commented May 14, 2024

pracucci left a comment

jesusvazquez left a comment

bwplotka left a comment

colega May 14, 2024

colega May 14, 2024

bboreham May 14, 2024

krajorama May 14, 2024

bboreham May 14, 2024

colega May 14, 2024

colega May 17, 2024 •

edited

		// checkContextEveryNIterations is used in some tight loops to check if the context is done.
		checkContextEveryNIterations = 100

tsdb: check for context cancel before regex matching postings #14096

tsdb: check for context cancel before regex matching postings #14096

Conversation

krajorama commented May 14, 2024 • edited

This comment was marked as outdated.

krajorama commented May 14, 2024

pracucci left a comment

Choose a reason for hiding this comment

jesusvazquez left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

colega May 14, 2024

Choose a reason for hiding this comment

colega May 14, 2024

Choose a reason for hiding this comment

bboreham May 14, 2024

Choose a reason for hiding this comment

krajorama May 14, 2024

Choose a reason for hiding this comment

bboreham May 14, 2024

Choose a reason for hiding this comment

colega May 14, 2024

Choose a reason for hiding this comment

colega May 17, 2024 • edited

Choose a reason for hiding this comment

krajorama commented May 14, 2024 •

edited

colega May 17, 2024 •

edited