store add touch series limit #3509

lisuo3389 · 2020-11-27T05:12:52Z

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

Changes

Add touch series limit

Verification

Signed-off-by: lisuo <[email protected]>

bwplotka

LGTM 🤗

Small nit! Thanks, amazing work and good idea 🚀

bwplotka · 2020-11-27T10:33:32Z

cmd/thanos/store.go

@@ -67,6 +67,9 @@ func registerStore(app *extkingpin.App) {
 	maxSampleCount := cmd.Flag("store.grpc.series-sample-limit",
 		"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()
+	maxTouchSeriesCount := cmd.Flag("store.grpc.touch-series-limit",


Amazing. What I miss here is the definition of "touched" (:

Is this applied before fetching Series ID and chunk metas?
Is this applied while fetchink chunk metas and only if there is chunk matching time range?

This will allow understanding this better for users (: I see from code it's the former and that's ok for now, however, hopefully, we can improve and touch less series here with this work: #3512

yes, Is this before fetching Series ID and chunk.

I changed maxTouchSeriesCount to maxTouchedSeriesCount

bwplotka · 2020-11-27T10:34:06Z

pkg/store/bucket.go

@@ -693,6 +703,11 @@ func blockSeries(
 		return storepb.EmptySeriesSet(), indexr.stats, nil
 	}

+	// Reserve seriesLimiter


Suggested change

// Reserve seriesLimiter

// Reserve series seriesLimiter.

bwplotka · 2020-11-27T10:46:35Z

cc @yeya24 as we touch this code

Signed-off-by: lisuo <[email protected]>

cmd/thanos/store.go

yeya24 · 2020-11-30T14:37:47Z

pkg/store/bucket.go

@@ -190,6 +191,10 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics {
 		Name: "thanos_bucket_store_queries_dropped_total",
 		Help: "Number of queries that were dropped due to the sample limit.",
 	})
+	m.queriesSeriesDropped = promauto.With(reg).NewCounter(prometheus.CounterOpts{
+		Name: "thanos_bucket_store_queries_series_dropped_total",
+		Help: "Number of queries that were dropped due to the series limit.",


As I can see that if we want to add more metrics for the limit, why we just reuse thanos_bucket_store_queries_dropped_total metric? We can make it a CounterVec type and add a label reason for the reason of query drop such as series, samples, chunks, etc.

If modify thanos_bucket_store_queries_dropped_total, it will affect ChunksLimiterFactory. Cortex depending it.

No, the factory interface ChunksLimiterFactory won't be changed. The only change would be making thanos_bucket_store_queries_dropped_total a CounterVec type and adding one more label to it.

When initializing chunksLimiter you just need to do something like below so that's the same.

chunksLimiter = s.chunksLimiterFactory(s.metrics.queriesDropped.WithLabelValues("chunks"))

It is also good to keep the current one because chunks and series are two dimensions. We can change it later if needed.

Yes, you are right

Nice! Is it still todo?

I like this idea (:

Okay, let me modify it.

Signed-off-by: lisuo <[email protected]>

yeya24

LGTM

pkg/store/limiter.go

Signed-off-by: lisuo <[email protected]>

bwplotka

LGTM, just @yeya24 comment still not addressed.

We can do that in next Pr though

bwplotka · 2020-12-01T09:49:38Z

pkg/store/bucket.go

@@ -190,6 +191,10 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics {
 		Name: "thanos_bucket_store_queries_dropped_total",
 		Help: "Number of queries that were dropped due to the sample limit.",
 	})
+	m.queriesSeriesDropped = promauto.With(reg).NewCounter(prometheus.CounterOpts{
+		Name: "thanos_bucket_store_queries_series_dropped_total",
+		Help: "Number of queries that were dropped due to the series limit.",


Nice! Is it still todo?

bwplotka · 2020-12-01T09:50:05Z

pkg/store/bucket.go

@@ -190,6 +191,10 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics {
 		Name: "thanos_bucket_store_queries_dropped_total",
 		Help: "Number of queries that were dropped due to the sample limit.",
 	})
+	m.queriesSeriesDropped = promauto.With(reg).NewCounter(prometheus.CounterOpts{
+		Name: "thanos_bucket_store_queries_series_dropped_total",
+		Help: "Number of queries that were dropped due to the series limit.",


I like this idea (:

…r` to `CouterVec` Signed-off-by: lisuo <[email protected]>

yeya24

LGTM

Signed-off-by: Giedrius Statkevičius <[email protected]>

GiedriusS

Thank you! LGTM, clean code. I have solved the conflicts for CHANGELOG.md. Will merge on green ❤️

GiedriusS

It seems like the CI failure is related to the changes. I was able to reproduce the segfault locally as well. Care looking into it?

Signed-off-by: lisuo <[email protected]>

GiedriusS

Thank you for the quick fix!

lisuo added 3 commits November 27, 2020 14:09

add touch series limit

ac33d66

Signed-off-by: lisuo <[email protected]>

update CHANGELOG.md

1cc998e

Signed-off-by: lisuo <[email protected]>

fix go-lint error

854c2d0

Signed-off-by: lisuo <[email protected]>

lisuo3389 force-pushed the series-limit branch from c980321 to 854c2d0 Compare November 27, 2020 06:09

bwplotka approved these changes Nov 27, 2020

View reviewed changes

update flag name maxTouchSeriesCount to maxTouchedSeriesCount

f7ab48d

Signed-off-by: lisuo <[email protected]>

yeya24 reviewed Nov 30, 2020

View reviewed changes

Fix flag store.grpc.touched-series-limit touch to touched

7ee6c38

Signed-off-by: lisuo <[email protected]>

yeya24 approved these changes Dec 1, 2020

View reviewed changes

pkg/store/limiter.go Show resolved Hide resolved

Add comment to SeriesLimiterFactory

b4bc7e7

Signed-off-by: lisuo <[email protected]>

bwplotka approved these changes Dec 1, 2020

View reviewed changes

Update thanos_bucket_store_queries_dropped_total metric type `Coute…

06dbfcf

…r` to `CouterVec` Signed-off-by: lisuo <[email protected]>

yeya24 approved these changes Dec 3, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into HEAD

bf87c0f

Signed-off-by: Giedrius Statkevičius <[email protected]>

GiedriusS approved these changes Dec 21, 2020

View reviewed changes

GiedriusS requested changes Dec 21, 2020

View reviewed changes

lisuo3389 force-pushed the series-limit branch 2 times, most recently from 6633358 to bf87c0f Compare December 28, 2020 09:08

fix unit test

928cf15

Signed-off-by: lisuo <[email protected]>

GiedriusS approved these changes Dec 28, 2020

View reviewed changes

GiedriusS merged commit 0eb4800 into thanos-io:master Dec 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store add touch series limit #3509

store add touch series limit #3509

lisuo3389 commented Nov 27, 2020 •

edited

Loading

bwplotka left a comment

bwplotka Nov 27, 2020

lisuo3389 Nov 28, 2020

lisuo3389 Nov 28, 2020

bwplotka Nov 27, 2020

bwplotka commented Nov 27, 2020

yeya24 Nov 30, 2020 •

edited

Loading

lisuo3389 Dec 1, 2020

yeya24 Dec 1, 2020

lisuo3389 Dec 1, 2020

bwplotka Dec 1, 2020

bwplotka Dec 1, 2020

lisuo3389 Dec 1, 2020

yeya24 left a comment

bwplotka left a comment

bwplotka Dec 1, 2020

bwplotka Dec 1, 2020

yeya24 left a comment

GiedriusS left a comment

GiedriusS left a comment

GiedriusS left a comment

store add touch series limit #3509

store add touch series limit #3509

Conversation

lisuo3389 commented Nov 27, 2020 • edited Loading

Changes

Verification

bwplotka 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

bwplotka commented Nov 27, 2020

yeya24 Nov 30, 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

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

bwplotka left a 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

GiedriusS left a comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

lisuo3389 commented Nov 27, 2020 •

edited

Loading

yeya24 Nov 30, 2020 •

edited

Loading