Suggest label name and values based on current query vector selector #202
Conversation
There was a problem hiding this comment.
Thanks for the PR!
As far as I could figure it out it does three things:
- Use a significantly better heuristic for filtering label completions. However this heuristic should be documented somewhere, ideally including the cases that aren't covered yet, like
label_replace. - Introduce the
testifyframework and change some tests to use it. This seems fine to me since Prometheus is adoptingtestifyas well. - Include a mock Prometheus server in some tests. This could be a good way to address Integration test for getting metadata from Prometheus #133.
All three of them looks both well implemented and like something that would be a good addition to the language server.
However it would have been way easier to review if it was split into several smaller PRs.
For the future I'd also recommend opening an issue first for large changes, to check whether those changes are actually desired.
|
|
||
| // Delete the current label from matchers if present, it is incomplete and | ||
| // we are trying to complete values for it. | ||
| if len(matchers) != 0 { |
There was a problem hiding this comment.
Interesting way of optimizing filtering here.
There was a problem hiding this comment.
even if it's not possible, !=0 suggests it can be ok for the value -1. But to know it's not possible to have a negative value, you have to know what is the struct of matchers which requires more thought when reading the code.
But if you are using len(matchers) > 0 is quite fast forward.
| metricName string | ||
| metricLabels []*labels.Matcher | ||
| ) | ||
| for _, matcher := range currMatchers { |
There was a problem hiding this comment.
You could replace most parts of this function by constructing a VectorSelector and calling this one:
| } | ||
|
|
||
| // deduplicated is a de-duplicated result set. | ||
| deduplicated := make(map[string]map[string]struct{}) |
There was a problem hiding this comment.
In cases when we only want to know the values of one label, computing all the other stuff seems like busywork.
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func depthFirstVectorSelector(node promql.Node) (*promql.VectorSelector, bool) { |
There was a problem hiding this comment.
I think it will be nice to have a tail recursive instead
| return nil, nil | ||
| } | ||
| var matchers []*labels.Matcher | ||
| if vs.Name != "" { |
There was a problem hiding this comment.
Can you please check the non empty string by using len, it helps to avoid the nasty issue of testing a string " " which can happen just by miss typed a space.
if len(vs.Name) > 0 {
}| matchers = append(matchers, m) | ||
| } | ||
| for _, m := range vs.LabelMatchers { | ||
| if m == nil { |
There was a problem hiding this comment.
would be more consistent like that, since it has exactly the same effect
if m == nil || m.Name == model.MetricNameLabel {
}|
|
||
| // Delete the current label from matchers if present, it is incomplete and | ||
| // we are trying to complete values for it. | ||
| if len(matchers) != 0 { |
There was a problem hiding this comment.
even if it's not possible, !=0 suggests it can be ok for the value -1. But to know it's not possible to have a negative value, you have to know what is the struct of matchers which requires more thought when reading the code.
But if you are using len(matchers) > 0 is quite fast forward.
| @@ -55,32 +57,56 @@ func (c *notCompatibleHTTPClient) AllMetricMetadata(ctx context.Context) (map[st | |||
| return allMetadata, nil | |||
| } | |||
|
|
|||
| func (c *notCompatibleHTTPClient) LabelNames(ctx context.Context, name string) ([]string, error) { | |||
| if len(name) == 0 { | |||
| func (c *notCompatibleHTTPClient) LabelNames( | |||
There was a problem hiding this comment.
I think you can still keep it in one line, there are not so much parameter here
|
Thanks a lot for your comments @slrtbtfs and @Nexucis - I'll work on integrating them, and apologies for not being more clear with the PR on the intention for the PR. Yes the main intent here is to filter away a lot of the label names and/or values that aren't applicable given a partially complete query. A lot of the use cases here end up being centered around you narrowing down a query as you add label selectors and the ability to filter out irrelevant label names and/or values can help a lot to realize what is interesting to group things by or constraint the query further by. Ideally we upstream to Prometheus at some point the ability to aggregate the applicable label names/values given a vector selector without having to match raw time series and aggregate this client side (since for high cardinality data that can cause a lot of overhead exchanging raw time series metadata only to aggregate it client side). |
This adds the context to filter the label names and values down to the subset of timeseries applicable for a query. This also makes this logic applicable for filtering the label name suggestions for a
by ()statement in an aggregate expression.