Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark posting group lazy if it has a lot of keys #7961

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 5, 2024

Signed-off-by: Ben Ye [email protected]

Fixes #7956

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

Changes

  • Introduce a new Store Gateway flag store.posting-group-max-keys to mark a posting group as lazy if it tries to fetch too many keys. 0 disables the limit. This is only effective if lazy expanded posting is enabled.
  • Introduce a new metric thanos_bucket_store_lazy_expanded_posting_groups_total to track total number of posting groups that are marked to lazy for different reasons

Verification

  • Updated unit tests

@yeya24 yeya24 changed the title mark posting group lazy if it has a lot of add keys Mark posting group lazy if it has a lot of keys Dec 5, 2024
Signed-off-by: Ben Ye <[email protected]>
@@ -204,6 +205,9 @@ func (sc *storeConfig) registerFlag(cmd extkingpin.FlagClause) {
cmd.Flag("store.enable-lazy-expanded-postings", "If true, Store Gateway will estimate postings size and try to lazily expand postings if it downloads less data than expanding all postings.").
Default("false").BoolVar(&sc.lazyExpandedPostingsEnabled)

cmd.Flag("store.posting-group-max-keys", "Mark posting group as lazy if it fetches more keys than the configured number. Only valid if lazy expanded posting is enabled. 0 disables the limit.").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could the user know what the appropriate number here should be? Could we calculate & use some pre-defined threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question... For us, we actually added some additional logs in our fork to log cardinalities for those queries that triggered lazy posting optimization. So that we have insights about the size. But logging those information in Thanos might be too expensive.

Do you think it would help if we introduce a native histogram to track posting group total posting sizes and number of keys?

I thought about making it a percentage value based on your block size. For example, 1% of your number of series, or 10% of your total label pairs but I couldn't find a good way to define that. Percentage will make the threshold very small for small blocks even though they don't cause any impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GiedriusS One idea we had is to mark lazy if number of keys is much larger than the max possible series for the query (or smallest cardinality posting group).

number of keys / max_series_to_match > R

For a query, if the matcher with the smallest posting could match 1000 series. Then it can have at most 1000 different values for a single label. If the current posting group matches 100K keys it means we match at most 1K values and throw away 99K keys. Do you think this is a good metric? We can start with threshold like 100 and tune it accordingly

@@ -250,6 +250,14 @@ Flags:
The maximum series allowed for a single Series
request. The Series call fails if this limit is
exceeded. 0 means no limit.
--store.posting-group-max-key-series-ratio=100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could also add that one should look at thanos_bucket_store_lazy_expanded_posting_groups_total if they want to understand whether this value needs to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in latest commit!

Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
@yeya24
Copy link
Contributor Author

yeya24 commented Dec 9, 2024

PTAL @GiedriusS, thanks for the review!

@yeya24 yeya24 merged commit 51c7dcd into thanos-io:main Dec 10, 2024
22 checks passed
@yeya24 yeya24 deleted the skip-posting-group-too-many-keys branch December 10, 2024 17:19
MichaHoffmann pushed a commit that referenced this pull request Dec 16, 2024
* mark posting group lazy if it has a lot of add keys

Signed-off-by: Ben Ye <[email protected]>

* update docs

Signed-off-by: Ben Ye <[email protected]>

* rename labels

Signed-off-by: Ben Ye <[email protected]>

* changelog

Signed-off-by: Ben Ye <[email protected]>

* change to use max key series ratio

Signed-off-by: Ben Ye <[email protected]>

* update docs

Signed-off-by: Ben Ye <[email protected]>

* mention metrics

Signed-off-by: Ben Ye <[email protected]>

* update docs

Signed-off-by: Ben Ye <[email protected]>

---------

Signed-off-by: Ben Ye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store Gateway: Lazy posting should mark posting group as lazy if adding too many keys
2 participants