Make maxLimit configurable per tenant for the /cardinality/label_names and /cardinality/label_values endpoints.
#11456
Conversation
Co-authored-by: Taylor C <[email protected]>
Co-authored-by: Taylor C <[email protected]>
|
Hi @narqo @pracucci , could you please take a look at this PR? This PR is based on the feedback from #11025 (review) |
|
@narqo @pracucci @dimitarvdimitrov Any chance of a review on this? We're eager to get this change out so we can proceed with our cardinality work internally. |
|
@narqo @pracucci @dimitarvdimitrov Bump. We'd be happy to get a review of this ❤️ |
|
@narqo @npazosmendez @56quarters Bump. Any chance of a review on this? |
pkg/util/validation/limits.go
Outdated
| @@ -203,6 +203,7 @@ type Limits struct { | |||
| CardinalityAnalysisEnabled bool `yaml:"cardinality_analysis_enabled" json:"cardinality_analysis_enabled"` | |||
| LabelNamesAndValuesResultsMaxSizeBytes int `yaml:"label_names_and_values_results_max_size_bytes" json:"label_names_and_values_results_max_size_bytes"` | |||
| LabelValuesMaxCardinalityLabelNamesPerRequest int `yaml:"label_values_max_cardinality_label_names_per_request" json:"label_values_max_cardinality_label_names_per_request"` | |||
| CardinalityAPIMaxSeriesLimit int `yaml:"cardinality_api_max_series_limit" json:"cardinality_api_max_series_limit"` | |||
There was a problem hiding this comment.
Can you add an experimental tag here so that we can make changes before finalizing this setting?
pkg/util/validation/limits.go
Outdated
| @@ -203,6 +203,7 @@ type Limits struct { | |||
| CardinalityAnalysisEnabled bool `yaml:"cardinality_analysis_enabled" json:"cardinality_analysis_enabled"` | |||
| LabelNamesAndValuesResultsMaxSizeBytes int `yaml:"label_names_and_values_results_max_size_bytes" json:"label_names_and_values_results_max_size_bytes"` | |||
| LabelValuesMaxCardinalityLabelNamesPerRequest int `yaml:"label_values_max_cardinality_label_names_per_request" json:"label_values_max_cardinality_label_names_per_request"` | |||
| CardinalityAPIMaxSeriesLimit int `yaml:"cardinality_api_max_series_limit" json:"cardinality_api_max_series_limit"` | |||
There was a problem hiding this comment.
The limit isn't really limiting the number of series returned. Can you change the setting (and CLI flag, and methods) to cardinality_analysis_max_results.
| } | ||
|
|
||
| // DecodeLabelValuesRequestFromValuesWithUser is like DecodeLabelValuesRequestFromValues but also accepts the userID and overrides. | ||
| func DecodeLabelValuesRequestFromValuesWithUser(values url.Values, userID string, overrides *validation.Overrides) (*LabelValuesRequest, error) { |
There was a problem hiding this comment.
here can we just take a limit instead of accepting the overrides? the limit we take will be the upper bound of the limit in the request. Same with DecodeLabelValuesRequest
|
you also need to run |
What this PR does
This PR introduces a new configurable parameter
cardinality_api_max_series_limitto make the maximum number of series returned by cardinality API requests (/cardinality/label_namesand/cardinality/label_values) configurable on a per-tenant basis.The changes include:
CardinalityAPIMaxSeriesLimitto theLimitsstruct in the cardinality sectionquerier.cardinality-api-max-series-limitwith a default value of 500 (which was the previous hard-coded value)CardinalityAPIMaxSeriesLimitto theOverridesstructextractLimitfunction to use the tenant-specific limit instead of the hard-coded valueThis PR is based on the feedback from #11025 (review)
Which issue(s) this PR fixes or relates to
Fixes #10328
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.