Limit queried chunks by bytes #3089

mneverov · 2020-08-27T17:05:21Z

Signed-off-by: Max Neverov [email protected]

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

Changes

Add possibility to limit queried chunks by bytes via store.grpc.series-sample-size-limit flag.

bwplotka · 2020-09-22T11:08:21Z

pkg/store/bucket.go

@@ -769,6 +774,10 @@ func blockSeries(
 		return nil, nil, errors.Wrap(err, "preload chunks")
 	}

+	if err := chunksSizeLimiter.Reserve(uint64(chunkr.stats.seriesFetchedSizeSum)); err != nil {


As per your question:

Another question is is it a proper way to limit based on a chunk reader stats? https://github.com/thanos-io/thanos/pull/3089/files#diff-a75f50a9f5bf5b21a862e4e7c6bd1576R777
Or is it better to pass the limiter to the preload function?

That's a good one, it really depends on one thing: Do we want to return partial data if limit is exceeded or fail everything? (:

My take on that is that a user might be surprised when find out that the returned response was partial.
It might make sense to create a config for that and apply for both: limiting by a number of samples and by byte size.

Oh yes, we have warnings for that. Everything with warning is assumed partial - then querier decide if it's error or not with partialResponseStrategy request option (:

mneverov · 2020-09-24T20:02:45Z

To the reviewer:
A new bucket store constructor name suggestions are greatly appreciated.
I reused a chunk reader stats instead of passing the limiter to the preload function because it was refactored out in #2858

pkg/store/bucket.go

pracucci · 2020-10-19T15:00:14Z

cmd/thanos/store.go

@@ -68,6 +68,10 @@ func registerStore(app *extkingpin.App) {
 		"Maximum amount of samples returned via a single Series call. The Series call fails if this limit is exceeded. 0 means no limit. NOTE: For efficiency the limit is internally implemented as 'chunks limit' considering each chunk contains 120 samples (it's the max number of samples each chunk can contain), so the actual number of samples might be lower, even though the maximum could be hit.").
 		Default("0").Uint()

+	maxSampleSize := cmd.Flag("store.grpc.series-sample-size-limit",
+		"Maximum size of samples returned via a single Series call. The Series call fails if this limit is exceeded. 0 means no limit.").


I would clarify what's the unit here. Bytes?

pracucci · 2020-10-19T15:27:06Z

pkg/store/bucket.go

 	)

+	chunksSizeLimiter, err = chunksSizeLimiter.NewWithFailedCounterFrom(chunksLimiter)


Could you clarify why you need NewWithFailedCounterFrom()?

@pracucci thanks for reviewing this.
Chunks limiter and chunks size limiter should have common failedOnce to avoid concurrent update of the failedCounter.

more info on that: store does not know at the moment of ChunksLimit and ChunksSizeLimit creation what metrics will be used. Metrics are the part of the BucketStore. That is why store operates limiter factories and bucket creates new chunk size limiter with the sync.Once shared between limiters. Hope that helps

I think NewWithFailedCounterFrom() is a bit overengineered.

There's no problem updating a metric concurrently, but we may want to distinguish the reason why a query was dropped. An option may be adding a "reason" label to queriesDropped and passing s.metrics.queriesDropped.WithLabelValues("<reason>") to both factories.

Signed-off-by: Max Neverov <[email protected]>

…ne; amend the new config description Signed-off-by: Max Neverov <[email protected]>

stale · 2020-12-26T11:58:46Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

kakkoyun · 2021-01-05T08:50:52Z

Hey @mneverov, shall we reopen this? Do you plan to work on it?

mneverov · 2021-01-05T09:03:07Z

hi @kakkoyun,
I created this PR in August, asked a couple of times for review, did some fixes after review.
I do not know if this still needed. From my point of view someone from the main contributors should check this and merge/reject/close.
Will be happy to work again on this if you, @bwplotka, or @pracucci have comments.

kakkoyun · 2021-01-05T10:39:28Z

@mneverov 👍 Reopening it and passing it to @bwplotka @pracucci. We can either merge if it's good to go or close it again.

pracucci

Sorry for the extreme low review. Overall changes LGTM, but I left a couple of comments.

pracucci · 2021-03-16T12:11:57Z

pkg/store/bucket.go

 	)

+	chunksSizeLimiter, err = chunksSizeLimiter.NewWithFailedCounterFrom(chunksLimiter)


I think NewWithFailedCounterFrom() is a bit overengineered.

There's no problem updating a metric concurrently, but we may want to distinguish the reason why a query was dropped. An option may be adding a "reason" label to queriesDropped and passing s.metrics.queriesDropped.WithLabelValues("<reason>") to both factories.

pracucci · 2021-03-16T12:13:04Z

pkg/store/bucket.go

+
+// NewBucketStoreWithOptions creates a new bucket backed store that implements the store API against
+// an object store bucket. It is optimized to work against high latency backends.
+func NewBucketStoreWithOptions(


@bwplotka What do you think about this? I've mixed feelings about it, just to avoid adding another argument to NewBucketStore(). Maybe we can rollback options in this PR and follow up the discussion about functional options in #3931?

pracucci · 2021-03-16T12:13:21Z

pkg/store/bucket.go

+
+// NewBucketStoreWithOptions creates a new bucket backed store that implements the store API against
+// an object store bucket. It is optimized to work against high latency backends.
+func NewBucketStoreWithOptions(


@bwplotka What do you think about this? I've mixed feelings about it, just to avoid adding another argument to NewBucketStore(). Maybe we can rollback options in this PR and follow up the discussion about functional options in #3931?

mneverov · 2021-03-31T08:14:01Z

hi @pracucci ,
thanks for the review! All comments are valid.
It's been a while since the PR was submitted and I stuck in rebasing master branch. I think it is easier to close this PR and reimplement the functionality on top of updated bucket.go.
Sorry you wasted your time on this, I should have closed it long time ago.

mneverov force-pushed the chunk_byte_limit branch from ec3275b to 0a9d125 Compare August 27, 2020 20:19

mneverov changed the title ~~Add new flag description~~ WIP: Limit queried chunks by bytes Aug 28, 2020

bwplotka reviewed Sep 22, 2020

View reviewed changes

mneverov force-pushed the chunk_byte_limit branch 2 times, most recently from dce3725 to 666479a Compare September 24, 2020 19:45

mneverov changed the title ~~WIP: Limit queried chunks by bytes~~ Limit queried chunks by bytes Sep 24, 2020

mneverov marked this pull request as ready for review September 24, 2020 20:07

mneverov force-pushed the chunk_byte_limit branch from 666479a to b0f792e Compare September 26, 2020 16:21

OGKevin reviewed Sep 30, 2020

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

mneverov force-pushed the chunk_byte_limit branch from b0f792e to ae47944 Compare September 30, 2020 15:31

pracucci reviewed Oct 19, 2020

View reviewed changes

Limit queried chunks by bytes

316caa9

Signed-off-by: Max Neverov <[email protected]>

mneverov force-pushed the chunk_byte_limit branch 2 times, most recently from 0aec4b0 to abdb25f Compare October 25, 2020 18:04

Review fix: rename new bucket store constructor; deprecated the old o…

c24c072

…ne; amend the new config description Signed-off-by: Max Neverov <[email protected]>

mneverov force-pushed the chunk_byte_limit branch from abdb25f to c24c072 Compare October 25, 2020 18:12

pracucci mentioned this pull request Oct 26, 2020

Store: limit queried chunks by bytes #2861

Closed

stale bot added the stale label Dec 26, 2020

stale bot closed this Jan 2, 2021

kakkoyun reopened this Jan 5, 2021

stale bot removed stale labels Jan 5, 2021

Base automatically changed from master to main February 26, 2021 16:30

pracucci reviewed Mar 16, 2021

View reviewed changes

mneverov closed this Mar 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit queried chunks by bytes #3089

Limit queried chunks by bytes #3089

mneverov commented Aug 27, 2020 •

edited

Loading

bwplotka Sep 22, 2020

mneverov Sep 22, 2020

bwplotka Sep 22, 2020

mneverov commented Sep 24, 2020 •

edited

Loading

pracucci Oct 19, 2020

pracucci Oct 19, 2020

mneverov Oct 25, 2020

mneverov Oct 27, 2020

pracucci Mar 16, 2021

stale bot commented Dec 26, 2020

kakkoyun commented Jan 5, 2021

mneverov commented Jan 5, 2021

kakkoyun commented Jan 5, 2021

pracucci left a comment

pracucci Mar 16, 2021

pracucci Mar 16, 2021

pracucci Mar 16, 2021

mneverov commented Mar 31, 2021

		)

		chunksSizeLimiter, err = chunksSizeLimiter.NewWithFailedCounterFrom(chunksLimiter)

Limit queried chunks by bytes #3089

Limit queried chunks by bytes #3089

Conversation

mneverov commented Aug 27, 2020 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mneverov commented Sep 24, 2020 • edited Loading

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

stale bot commented Dec 26, 2020

kakkoyun commented Jan 5, 2021

mneverov commented Jan 5, 2021

kakkoyun commented Jan 5, 2021

pracucci 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

mneverov commented Mar 31, 2021

mneverov commented Aug 27, 2020 •

edited

Loading

mneverov commented Sep 24, 2020 •

edited

Loading