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.

Sign up for GitHub

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

Jump to bottom

store: add downloaded bytes limit #5801

Merged
merged 8 commits into from
Oct 27, 2022
Merged

store: add downloaded bytes limit #5801

merged 8 commits into from
Oct 27, 2022

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Oct 20, 2022 •
edited
Loading

Changes

Adds the ability to limit downloaded bytes in Thanos Store for LabelNames/LabelValues/Series. This is needed to make capacity planning easier and because current limits aren't that effective since to calculate series/chunks limit, we first need to download all matching postings.

Closes #5733.

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

Verification

E2E tests.

@GiedriusS
store: add downloaded bytes limit
d8fba06 
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
store: add bytesLimiter to LabelNames, LabelValues
2b33305 
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
test: add e2e test for new limiter
ef85b17 
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
*: update CHANGELOG/etc
d50ec5a 
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS GiedriusS force-pushed the add_downloaded_bytes_limit branch 4 times, most recently from c973e01 to 9382158 Compare October 20, 2022 12:04
@GiedriusS
e2e: hard fail on no error
29b449e 
We always expect an error here.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS GiedriusS force-pushed the add_downloaded_bytes_limit branch from 9382158 to 29b449e Compare October 20, 2022 12:20
@douglascamata
Copy link
Contributor

@GiedriusS do we already have a metric with the size of the postings being fetched? Without having access to this information the limit's value can only be set based on guesses instead of needs.

fpetkovski
fpetkovski reviewed Oct 20, 2022
View reviewed changes
pkg/store/bucket.go Outdated
@@ -2132,6 +2159,17 @@ func (r *bucketIndexReader) fetchPostings(ctx context.Context, keys []labels.Lab
return uint64(ptrs[i].ptr.Start), uint64(ptrs[i].ptr.End)
})

if bytesLimiter != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about having a nopLimiter to avoid nil checks?

Copy link
Member Author

@GiedriusS GiedriusS Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I thought it would be cleaner to add checks instead of passing NewBytesLimiterFactory(0)(nil) everywhere 😄 let me do this change

Copy link
Contributor

Choose a reason for hiding this comment

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

If we make the bytesLimiter an interface, then we don't need to pass the factory around right?

Copy link
Member Author

Choose a reason for hiding this comment

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

But it is an interface already, no? I don't get your point. A nopLimiter is the same as NewBytesLimiterFactory(0)() so I'm not sure what is the benefit of having nopLimiter. Problem is that you have to pass some kind of limiter in all places in all tests even though it is irrelevant. The purpose of this if is to avoid having to copy/paste that. Whether it is nopLimiter or NewBytesLimiterFactory(0)() it doesn't matter. Either way, let me make this change

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought of a better idea - since it will be pretty much necessary to pass something here, I've made it into a "normal" argument to NewBucketStore. With this, it won't be needed to add a bunch of ifs everywhere.

@GiedriusS
Copy link
Member Author

@GiedriusS do we already have a metric with the size of the postings being fetched? Without having access to this information the limit's value can only be set based on guesses instead of needs.

One could take a look at thanos_bucket_store_series_data_size_touched_bytes_sum or thanos_bucket_store_series_data_size_fetched_bytes_sum to have a rough guess. In our own small fork of Thanos, we've added a histogram that shows how many postings were downloaded per Series() call. We have a problem with cardinality so in our case it even exceeds 4GiB and I'm not sure what's the limit for the buckets of that histogram. I guess users could take a look at the aforementioned metrics to see what the limit could be? Do you see value in upstreaming vinted@7ca3496 ?

@douglascamata
Copy link
Contributor

@GiedriusS gotcha. I think there is value in upstreaming vinted@7ca3496 indeed. It would help a lot in setting values for the size of the postings fetched. <3

@GiedriusS
Merge remote-tracking branch 'origin/main' into add_downloaded_bytes_…
703ae31 
…limit

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS
CHANGELOG: fix & improve clarity
59de426 
Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@GiedriusS GiedriusS force-pushed the add_downloaded_bytes_limit branch from ca34d36 to 59de426 Compare October 26, 2022 06:49
@GiedriusS
Copy link
Member Author

CircleCI is broken for some reason 😢

@GiedriusS GiedriusS closed this Oct 26, 2022
@GiedriusS GiedriusS reopened this Oct 26, 2022
@douglascamata
Copy link
Contributor 

error: cannot lock ref 'refs/remotes/origin/pull/5801': 'refs/remotes/origin/pull' exists; cannot create 'refs/remotes/origin/pull/5801'
From github.com:thanos-io/thanos
 ! [new ref]           refs/pull/5801/head -> origin/pull/5801  (unable to update local ref)

I have no idea what this is. 🤯

fpetkovski
fpetkovski approved these changes Oct 26, 2022
View reviewed changes
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@GiedriusS GiedriusS merged commit 931f4e7 into thanos-io:main Oct 27, 2022
@GiedriusS GiedriusS deleted the add_downloaded_bytes_limit branch October 27, 2022 11:51
@matej-g
Copy link
Collaborator

matej-g commented Oct 28, 2022

Looks like the tests might be flaky? https://github.com/thanos-io/thanos/actions/runs/3344323501/jobs/5538543830#step:5:1879

@GiedriusS
Copy link
Member Author

Thanks for the report, I will try to look into that as soon as possible.

GiedriusS added a commit to vinted/thanos that referenced this pull request Oct 31, 2022
@GiedriusS
store: add downloaded bytes limit (thanos-io#5801)
5c5c62c 
* store: add downloaded bytes limit

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* store: add bytesLimiter to LabelNames, LabelValues

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* test: add e2e test for new limiter

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* *: update CHANGELOG/etc

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* e2e: hard fail on no error

We always expect an error here.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* CHANGELOG: fix & improve clarity

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
@yeya24 yeya24 mentioned this pull request Feb 27, 2023
Merged
3 tasks
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request May 18, 2023
@GiedriusS
store: add downloaded bytes limit (thanos-io#5801)
636b66e 
* store: add downloaded bytes limit

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* store: add bytesLimiter to LabelNames, LabelValues

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* test: add e2e test for new limiter

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* *: update CHANGELOG/etc

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* e2e: hard fail on no error

We always expect an error here.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

* CHANGELOG: fix & improve clarity

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpetkovski fpetkovski fpetkovski approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Series limits are not effective in Thanos Store
4 participants
@GiedriusS @douglascamata @matej-g @fpetkovski