Implement max series limit in distributor for /series API
#4683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
harry671003
changed the title
/series API
alanprot
reviewed
Mar 22, 2022
pkg/querier/distributor_queryable.go
Outdated
| @@ -97,10 +98,22 @@ func (q *distributorQuerier) Select(_ bool, sp *storage.SelectHints, matchers .. | |||
| // See: https://github.com/prometheus/prometheus/pull/8050 | |||
| if sp != nil && sp.Func == "series" { | |||
| ms, err := q.distributor.MetricsForLabelMatchers(ctx, model.Time(q.mint), model.Time(q.maxt), matchers...) | |||
| queryLimiter := limiter.QueryLimiterFromContextWithFallback(ctx) | |||
There was a problem hiding this comment.
I think this change is easier if we do inside the MetricsForLabelMatchers:
and use: queryLimiter.AddSeries(cortexpb.FromMetricsToLabelAdapters(m))
Another possible memory optimization on this code would be to not wait all resps and dedup those resps as soon as it arrive inside the ForReplicationSet
There was a problem hiding this comment.
Thanks for the feedback.
harry671003
force-pushed
the
distributor-series-limit
branch
from
f48f299 to
3ddad82
Compare
harry671003
force-pushed
the
distributor-series-limit
branch
5 times, most recently
from
03956af to
0edc672
Compare
|
LGTM |
alanprot
approved these changes
Apr 4, 2022
alvinlin123
reviewed
Apr 4, 2022
| queryLimiter: limiter.NewQueryLimiter(0, 0, 0), | ||
| expectedErr: nil, | ||
| }, | ||
| "should return err if series limit is exhausted": { |
There was a problem hiding this comment.
Can we add a test where:
- There is 5 ingesters
- Replication factor is 5
- There is exactly 1 time series replicated across the 5 ingesters
- Set the max time series to 1
In this case we shouldn't hit the limit. The purpose of this test is to ensure that the limit is applied against de-duped time series. I know the QueryLimiter does de-dupe for you, but it's good to ensure the same from distributor perspective.
There was a problem hiding this comment.
Thanks for reviewing. The replication factor is 3 by default. I added a test to cover the case where one series is queried with limit is set to 1.
| if err := queryLimiter.AddSeries(cortexpb.FromMetricsToLabelAdapters(m)); err != nil { | ||
| return nil, err | ||
| } | ||
| mutex.Lock() |
There was a problem hiding this comment.
For this would you be able to write some go test -bench (or use existing one if any) to and post before and after?
There was a problem hiding this comment.
I added a benchmark test BenchmarkDistributor_MetricsForLabelMatchers.
Without the locking:
❯ go test -run=^$ -bench ^BenchmarkDistributor_MetricsForLabelMatchers$ github.com/cortexproject/cortex/pkg/distributor -count 10
goos: darwin
goarch: amd64
pkg: github.com/cortexproject/cortex/pkg/distributor
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 775 1553236 ns/op 758952 B/op 8580 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 724 1551052 ns/op 759031 B/op 8581 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 898 1281153 ns/op 759056 B/op 8581 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1064 1145841 ns/op 758964 B/op 8580 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1102 1074635 ns/op 759169 B/op 8581 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1123 1068547 ns/op 759050 B/op 8580 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1108 1080608 ns/op 759064 B/op 8581 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1129 1052474 ns/op 758646 B/op 8578 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1129 1123325 ns/op 758817 B/op 8579 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 1084 1360482 ns/op 758859 B/op 8580 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/distributor 14.253s
With the locking
❯ go test -run=^$ -bench ^BenchmarkDistributor_MetricsForLabelMatchers$ github.com/cortexproject/cortex/pkg/distributor -count 10
goos: darwin
goarch: amd64
pkg: github.com/cortexproject/cortex/pkg/distributor
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 847 1519934 ns/op 870517 B/op 9174 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 804 1789515 ns/op 870984 B/op 9178 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 542 2008873 ns/op 871077 B/op 9179 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 715 1493901 ns/op 871080 B/op 9179 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 793 1619484 ns/op 871042 B/op 9179 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 712 1502533 ns/op 870682 B/op 9176 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 872 1594814 ns/op 870655 B/op 9176 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 652 1608638 ns/op 870481 B/op 9175 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 806 1452676 ns/op 870680 B/op 9176 allocs/op
BenchmarkDistributor_MetricsForLabelMatchers/get_series_within_limits-8 681 1634886 ns/op 870830 B/op 9177 allocs/op
PASS
ok github.com/cortexproject/cortex/pkg/distributor 15.305s
I didn't see any significant differences.
harry671003
force-pushed
the
distributor-series-limit
branch
2 times, most recently
from
f44d52e to
eef0643
Compare
harry671003
force-pushed
the
distributor-series-limit
branch
from
eef0643 to
5292564
Compare
Signed-off-by: 🌲 Harry 🌊 John 🏔 <[email protected]>
harry671003
force-pushed
the
distributor-series-limit
branch
from
5292564 to
7f5b13b
Compare
alvinlin123
approved these changes
Apr 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: 🌲 Harry 🌊 John 🏔 [email protected]
What this PR does:
This PR enforces the
max_fetched_series_per_querylimit for/api/v1/seriesAPI.Which issue(s) this PR fixes:
Fixes #4667
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]