proposal: A new API for the discovery of metric names, labels and values#74
proposal: A new API for the discovery of metric names, labels and values#74tcp13equals2 wants to merge 17 commits intoprometheus:mainfrom
Conversation
10e6a1a to
2e650d8
Compare
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | | ||
| | `include_metadata` | bool | No | false | Request metric metadata (units, type, description). | |
There was a problem hiding this comment.
Do we also need a metdadata_limit to control how many metadata records we return per metric name?
There was a problem hiding this comment.
I would be careful to not leak the metadata storage details, which we are changing in Prometheus.
Especially limit leaks this a lot (e.g. even now Prometheus does not use "records" model to work with metadata API, only internal caches).
Thus, maybe include or not is enough to not leak things further?
FYI: The general direction we go is:
- No metadata records anymore.
- Always-on metadata. Currently possible with type and unit labels, but eventually attached on export protocols from scrape cache (metadata: Move caching from scrapeCache.metadata to head.series prometheus#17619)
- Type and unit being part of the metric identity (problems like OM 2.0: Handle different type and unit for the same metric family (related to __type__ and __unit__ labels) OpenMetrics#280) and general Otel compatibility.
I'd love if new APIs do not need to change once we arrive in that future (:
There was a problem hiding this comment.
hey @bwplotka - thanks for the extra information here.
Do you have a suggestion of what specific fields we could safely expose in an API like this related to metadata?
| "name": "activity_tracker_failed_total", | ||
| "frequency": 1003, | ||
| "cardinality": 10, | ||
| "type": "counter", |
There was a problem hiding this comment.
Do we need a collection of metadata to allow for multiple metadata records per metric name?
There was a problem hiding this comment.
I think it's a really well written proposal! Please see my suggestions though.
Also wondering: Could /search/metric_names have a match[] parameter? Users might want to filter metric names by existing label matchers (e.g., "metric names where job=prometheus").
charleskorn
left a comment
There was a problem hiding this comment.
I've focused on the first endpoint below, but I believe my comments apply to the other two endpoints as well.
| * **Limited matching capabilities**: `match[]` supports regex matching of label values, but does not support fuzzy or typo-tolerant matching that discovery UIs require (e.g., user types container.memory and expects container_memory). | ||
| * **Unordered `limit` results**: `limit` returns an arbitrary subset of values rather than returning the most relevant or most frequent values, which causes drilldown UIs to display incomplete or unrepresentative results. | ||
| * **No metadata about further results**: responses do not indicate whether `limit` truncated the result set or how many total values exist, preventing the UI from making informed choices (show “more” affordance, refine query, etc.). | ||
| * **No pagination/cursors**: there is no offset/cursor mechanism; UIs cannot page through thousands of label values without fetching very large responses that may time out or overload the server. |
There was a problem hiding this comment.
[nit] This seems to imply a particular solution, and also overlaps with the last point. Perhaps something like "large response payloads" and "slow time-to-first-result" would better describe the problems?
There was a problem hiding this comment.
Can we remove "No pagination/cursors" as a limitation? In my opinion this is a feature, and should remain a feature of all future solutions.
I have never understood what UI would be usable with thousands of results to page through.
| * the existing Prometheus API labels/values parameter set is re-used in each endpoint, with additional parameters introduced to deliver the desired functionality | ||
| * the response format allows for enriched data to be added to each metric name, label name or label value record - rather than just a collection of strings | ||
| * the response is NDJSON (`application/x-ndjson`), allowing for a streamed chunked encoding response |
There was a problem hiding this comment.
Given all of these attributes, it's not clear to me why the existing endpoints can't be extended to support the desired functionality. For example, the existing endpoints could return results as-is, but if an Accept: application/x-ndjson header is included, then the richer streaming response format is used.
There was a problem hiding this comment.
I tried to address this in the Alternatives section - 2. Can we extend/enhance the existing Prometheus endpoints?
I think the main argument for not extending the existing endpoints is the change to the shape of the API responses.
The existing label/values endpoints return a simple collection of strings. Whereas the new endpoints propose to return a collection of objects - allowing for the additional decorations/enrichments to be added.
Although we could return a the different shape depending on the Accept header, I believe it is bad practice for an API to return different raw data based on the Accept. This header should change the serialisation format (ie json, xml etc) but the actual data returned should not be different.
Arguably this is where a version could be introduced to the request (as a header or path param) which can then be used for validly differentiating different data responses.
Aside from the Accept header causing different data to be returned, there is also the case of metric names. In this proposal the metric names search can return enrichments for the metadata, cardinality and frequency. If this was added in as a special case of the existing values endpoint for the __name__ label then we would be returning additional attributes which are not all available on other labels.
Although this could be implemented it does make the API have a number of caveats. Various parameters and response attributes can only be used/expected on specific label names.
| | `batch_size` | int >= 0 | No | 100 | The desired number of records per batch. | | ||
| | `cursor` | string | No | | Request the next page of results. | |
There was a problem hiding this comment.
Are "batch" and "page" synonyms here? Or are they different things?
There was a problem hiding this comment.
A fair question ... no they are not different things, but I choose that wording to make the cursor / page concept severable.
If we do support a page / cursor concept, then a page and batch are equivalent.
If we do not support page / cursor, and just support a batch_size and limit then it makes more sense that this is a batch being returned.
There was a problem hiding this comment.
I think I'm missing something sorry - could you explain the difference between batch size and the limit?
Is the batch size the number of results to return for this page, and the limit the overall number of results to return? (eg. limit is 300, batch size is 50, so even if the search matches 1000 items, at most the response will be 6 pages of 50 items each)
There was a problem hiding this comment.
Yes, exactly.
And if we are not doing pagination the same example applies. We stream 6 batches of 50 items each in the NDJSON response. The intent is that the lower the batch size the faster the first batch should appear to the client.
There was a problem hiding this comment.
We had big debate about paging. Rules API uses tokens. Is there a away we could be consistent with that approach? Both naming (cursor->token) and semantics?
https://prometheus.io/docs/prometheus/latest/querying/api/#rules
There was a problem hiding this comment.
Also true on server side work for every page/token vs client side buffering consequence https://github.com/prometheus/proposals/pull/74/changes#r2761467377
There was a problem hiding this comment.
I have adjusted the proposal to use next_token to align better with the rules API - c114ce4
| | `end` | rfc3339 / unix_timestamp | No | | As per the existing labels/values endpoint. | | ||
| | `limit` | int >= 0 | No | 100 | The maximum number of results to return after any ordering has been applied. | | ||
| | `batch_size` | int >= 0 | No | 100 | The desired number of records per batch. | | ||
| | `cursor` | string | No | | Request the next page of results. | |
There was a problem hiding this comment.
How will this pagination work in practice? My concern is that with the sorting and fuzzy matching behaviour, the server would need to recompute the entire result just to return a single page of results. If there are many pages of results, then this makes a heavy request even heavier (eg. if there are 20 pages of results, this is already a heavy request, but then the server would need to compute that 20 times to return all the data, making that heavy request even worse.)
There was a problem hiding this comment.
Yes - agreed. We have raised this with the web engineers and asked them to confirm the priority / importance of having pagination. If possible this will be dropped from the proposal.
If we do have to support this, we may need to consider some sort of server side pagination state / snapshot. ie a full dataset is cached which subsequent page requests are served from to avoid the scenario you mention.
There was a problem hiding this comment.
Another option: the server streams the whole response (ie. without pagination), and the client either sets a limit, or closes the connection when it doesn't want any more data.
| * **cardinality** - metric names are sorted by their cardinality. | ||
| * **frequency** - metric names are sorted by their frequency of use. |
There was a problem hiding this comment.
What's the difference between frequency and cardinality?
There was a problem hiding this comment.
I suspect you could define these better then me :)
- cardinality - number of distinct time series - the number of unique label-value combinations for this metric name
- frequency - number of samples which have this metric name within the given time period
Thanks @aknuds1 for your feedback and suggestions. I have updated to include this suggestion. |
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com> Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
a4a5507 to
73ea349
Compare
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
|
Thank you @charleskorn for you feedback and suggestions! |
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | | ||
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | |
There was a problem hiding this comment.
What's "cardinality" and "frequency"?
There was a problem hiding this comment.
+1, I'm curious. How this should work in Prometheus
There was a problem hiding this comment.
Could leverage TSDB status API, maybe for cardinality? I'm not sure we track usage frequency of metrics this way though
There was a problem hiding this comment.
I have added clarification on cardinality & frequency - 8d99298
bwplotka
left a comment
There was a problem hiding this comment.
Leaving some questions.
Mostly around how feasible it is for OSS Prometheus, Thanos, Cortex and non-Grafana vendors to implement. I think we are close, but let's double check those things.
If there are pieces that are specific to Mimir, I'd suggest adding those parameters (extra ones) for Mimir only implementation, but leave the formal spec smaller for wider audience.
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | | ||
| | `include_metadata` | bool | No | false | Request metric metadata (units, type, description). | |
There was a problem hiding this comment.
I would be careful to not leak the metadata storage details, which we are changing in Prometheus.
Especially limit leaks this a lot (e.g. even now Prometheus does not use "records" model to work with metadata API, only internal caches).
Thus, maybe include or not is enough to not leak things further?
FYI: The general direction we go is:
- No metadata records anymore.
- Always-on metadata. Currently possible with type and unit labels, but eventually attached on export protocols from scrape cache (metadata: Move caching from scrapeCache.metadata to head.series prometheus#17619)
- Type and unit being part of the metric identity (problems like OM 2.0: Handle different type and unit for the same metric family (related to __type__ and __unit__ labels) OpenMetrics#280) and general Otel compatibility.
I'd love if new APIs do not need to change once we arrive in that future (:
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | | ||
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | |
There was a problem hiding this comment.
+1, I'm curious. How this should work in Prometheus
| | Name | Type | Required | Default | Description | | ||
| |-----------------------|-----------------------------------------|----------|---------|------------------------------------------------------------------------------| | ||
| | `match[]` | string / selectors | No | | Series selector - as per existing labels/values endpoints. | | ||
| | `search` | string | No | | The search string to be used for matching metric names. | |
There was a problem hiding this comment.
| | `search` | string | No | | The search string to be used for matching metric names. | | |
| | `fuzz_search` | string | No | | The search string to be used for matching metric names. | |
Just to be clear?
There was a problem hiding this comment.
It's going to be fun to scale fuzzy queries like this BTW - on single Prometheus it's fine, but I wonder how do you plan to scale this on Mimir/Thanos/Cortex etc.
cc @GiedriusS
There was a problem hiding this comment.
Re changing search --> fuzz_search - I think search is more correct. The fuzzy search can be toggled on/off (or the threshold changed) via the fuzz_threshold param. If the fuzzy search is disabled by this threshold then fuzz_search would not make sense.
Just a note on the fuzzy search implementation - these can be deterministic when comparing to a given string.
if JaroWinkler(label_name, search_string) > threshold {
// match
}
From a little bit of research I think that a Jaro-Winkler is going to be more efficient then a Levenshtein match.
But agree it will be interesting to see the performance impact of this.
| | `batch_size` | int >= 0 | No | 100 | The desired number of records per batch. | | ||
| | `cursor` | string | No | | Request the next page of results. | |
There was a problem hiding this comment.
We had big debate about paging. Rules API uses tokens. Is there a away we could be consistent with that approach? Both naming (cursor->token) and semantics?
https://prometheus.io/docs/prometheus/latest/querying/api/#rules
| | `batch_size` | int >= 0 | No | 100 | The desired number of records per batch. | | ||
| | `cursor` | string | No | | Request the next page of results. | |
There was a problem hiding this comment.
Also true on server side work for every page/token vs client side buffering consequence https://github.com/prometheus/proposals/pull/74/changes#r2761467377
saswatamcode
left a comment
There was a problem hiding this comment.
Thanks for this! Some comments/questions,
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | | ||
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | |
There was a problem hiding this comment.
Could leverage TSDB status API, maybe for cardinality? I'm not sure we track usage frequency of metrics this way though
|
|
||
| The existing response format returns collections of strings which does not support additional record enrichment. | ||
|
|
||
| Although the existing endpoints could be adapted to (optionally) return streaming/batched results, apply filtering and sorting, support pagination etc - these would all be significant internal functional changes to these endpoints. |
There was a problem hiding this comment.
Just wondering, what the cost of this route will be (maybe with v2/ set of same APIs)?
The new v2 endpoints could still be split by label values and names, but support streaming and cursor/batching.
And we have a new special search endpoint exclusively meant for fuzzy matching labels?
That way, we don't have to put all new behaviors behind just one endpoint? But instead, create a new set of logically split but more frontend-friendly ones?
There was a problem hiding this comment.
Creating a v2 of the existing labels/values APIs which support streaming/pagination would definitely be a step forward and would address some of the identified limitations.
From what I understand of the Grafana web developer perspective, they are looking to make fewer API calls to stitch together the data they need. So having to call multiple endpoints is not their preferred approach.
There is also their other requests related to sorting ... in which case we still end up with a new special endpoint or 2 which does both filtering and sorting - and the client side still needs to make separate calls for cardinality, metadata etc.
| |-----------------------|-----------------------------------------|----------|---------|------------------------------------------------------------------------------| | ||
| | `match[]` | string / selectors | No | | Series selector - as per existing labels/values endpoints. | | ||
| | `search` | string | No | | The search string to be used for matching metric names. | | ||
| | `fuzz_threshold` | int [0..100] | No | 100 | Set the fuzzy match threshold. | |
There was a problem hiding this comment.
This seems relevant to how fuzz matching works but this param seems not very user friendly. Do we need to expose this in the API?
There was a problem hiding this comment.
The idea with exposing this is that it allows a client side application to control the level of fuzz matches.
ie imagine a UI where you enter a search string and only a few matches come back. Rather then try to refine your search string you could change a slider on the fuzz search sensitivity to allow for more matches to be found.
Do you have a suggestion for a better name for this param?
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | | ||
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | | ||
| | `include_cardinality` | bool | No | false | Request metric cardinality. | |
There was a problem hiding this comment.
Is it better to have a new set of APIs for cardinality? I do think we need some sort of dedicated cardinality APIs in Prometheus.
I am afraid that this single API does too many things and it might get overloaded. Is there any specific usecase to have frequency and cardinality here?
There was a problem hiding this comment.
The request from the Grafana web developers was to be able to make fewer API calls to stitch together the data they need for a given page/panel feature. Cardinality was one of those features they requested to have in a single call.
| | `case_sensitive` | bool | No | true | Toggle case sensitivity in string matching. | | ||
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | | ||
| | `sort_dir` | asc / dsc | No | asc | Request the ordering of the sort. Only valid when `sort_by` is set. | | ||
| | `include_frequency` | bool | No | false | Request metric frequency. | |
There was a problem hiding this comment.
Is frequency and cardinality same thing for metric name?
There was a problem hiding this comment.
No, they can be different. I added a definition of these into the proposal to provide more clarity.
| | `search` | string | No | | The search string to be used for matching metric names. | | ||
| | `fuzz_threshold` | int [0..100] | No | 100 | Set the fuzzy match threshold. | | ||
| | `case_sensitive` | bool | No | true | Toggle case sensitivity in string matching. | | ||
| | `sort_by` | alpha / cardinality / frequency / score | No | | Request how matching metrics should be sorted in the response. | |
There was a problem hiding this comment.
alpha - order by alphanumerics. So return the metric names, label names or label values in alphabetical order
score - order by match relevance. This is specific for auto-complete scenarios where we want to return the matches in an order. ie we would probably return matches with the longest matching prefix first, then the longest matching contains, and then by higher fuzz matching scores. The specific algorithm would need to be agreed, but I would assume there is an existing one we can leverage for this purpose.
|
|
||
| ##### Example of NDJSON batched result set - no include_* flags set | ||
|
|
||
| ```ndjson |
There was a problem hiding this comment.
I assume the existing Prometheus response format would apply here, including those warnings and infos?
There was a problem hiding this comment.
It would not as it would not be NDJson
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
Signed-off-by: Andrew Hall <andrew.hall@grafana.com>
I added a section around how we could support specific extensions for different implementations - so if there is anything here which is too Mimir specific we could move it into here. See 9f43fdd |
|
Thank you @yeya24 @bwplotka @saswatamcode @GiedriusS for all your commentary. Please keep calling out areas of concern or alternative suggestions! |
roidelapluie
left a comment
There was a problem hiding this comment.
I have read the issue and the comments, added a few.
I feel like it should be decided if we still want to support pagination or not, which is one of the main blockers I see in my view. Let's clarify this.
I also wonder if we should discuss in the proposal the API's that will need to be implemented. Do we want new functions added to the Querier ? What would that look like? Or do we need a new interface?
|
|
||
| For the purposes of this document, the following definitions apply; | ||
|
|
||
| * **frequency** — number of occurrences across a dataset within the queried time range. |
There was a problem hiding this comment.
occurences of what? Samples ? Non-stale samples? labels? metrics? timestamps?
|
|
||
| ***start/end*** | ||
|
|
||
| It is proposed that these could have default values which align to a reasonable look-back period. ie last 24 hours |
There was a problem hiding this comment.
Can we adjust default look-back to something a lot less expensive (ideally to fall into the WAL)? e.g. 1h; to match also the default time range of the prometheus UI?
|
|
||
| A value of 100 disables any fuzz matching. | ||
|
|
||
| The fuzzy search could be a Jaro-Winkler or Levenshtein match. |
There was a problem hiding this comment.
We could define this as parameter to be future proof. With a default to Jaro-Winkler. Strongly suggesting:
- That all implementations support Jaro Wrinkler
- That supported fuzz algorithms are provided via the existing
/featuresAPI endpoint.
| ***sort_by*** | ||
|
|
||
| * **alpha** - metric names are sorted by alphabetical order. | ||
| * **cardinality** - metric names are sorted by their cardinality. |
There was a problem hiding this comment.
Should cardinality and frequency be moved explicltly to the extensions?
No description provided.