Implement metadata api query params #6681
Conversation
SungJin1212
force-pushed
the
Implement-metadata-api-query-params
branch
4 times, most recently
from
9895799 to
3279526
Compare
pkg/querier/metadata_handler.go
Outdated
| } | ||
|
|
||
| // MetadataHandler returns metric metadata held by Cortex for a given tenant. | ||
| // It is kept and returned as a set. | ||
| func MetadataHandler(m MetadataQuerier) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| resp, err := m.MetricsMetadata(r.Context()) | ||
| limit := -1 | ||
| if s := r.FormValue("limit"); s != "" { |
There was a problem hiding this comment.
These two parts are copy/paste - maybe worth converting into a func?
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Implement-metadata-api-query-params
branch
from
3279526 to
7a34129
Compare
| mm.mtx.RLock() | ||
| defer mm.mtx.RUnlock() | ||
| r := make([]*cortexpb.MetricMetadata, 0, len(mm.metricToMetadata)) | ||
| if req.Limit == 0 { |
There was a problem hiding this comment.
This change is not backwards/forwards compatiable right?
For example, during a deployment, querier and ingesters can be using different version of Cortex. If querier is on an older version and ingesters on a newer version, then querier will invoke the ingester rpc without passing a limit.
In this case, ingesters will return empty metadata. Ideally we want the 0 value to be treated as limit is disabled.
There was a problem hiding this comment.
That's a good point. We should put this behind a feature flag to do 2 phase rollout
There was a problem hiding this comment.
Thanks for letting me know. I will add flags and an e2e test for that scenario.
There was a problem hiding this comment.
Ideally we want the 0 value to be treated as limit is disabled.
prometheus/prometheus#12862 There is an issue here.
There was a problem hiding this comment.
I added a feature flag here #6744.
| mm.mtx.RLock() | ||
| defer mm.mtx.RUnlock() | ||
| r := make([]*cortexpb.MetricMetadata, 0, len(mm.metricToMetadata)) | ||
| if req.Limit == 0 { |
There was a problem hiding this comment.
That's a good point. We should put this behind a feature flag to do 2 phase rollout
| Limit: int64(limit), | ||
| LimitPerMetric: int64(limitPerMetric), | ||
| Metric: metric, | ||
| } |
There was a problem hiding this comment.
If limit is set to 0, do we still need to send the request to Ingester? There is no result anyway
This PR implements the Prometheus metadata API query parameters (
limit,limit_per_metric, andmetric) on the Cortex to allow user to limit metadata to return.Which issue(s) this PR fixes:
Fixes #6679
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]