Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest label name and values based on current query vector selector #202

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Suggest label name and values based on current query vector selector #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b95da1d
Suggest label name and values based on query selection context
robskillington Dec 15, 2020
851d8db
Rename selection to currLabelsSelected for better readability
robskillington Dec 15, 2020
874ed16
Fix lint
robskillington Dec 15, 2020
61dfceb
Use label matchers insteads of label set so can use correct matcher type
robskillington Dec 15, 2020
c625d6c
Make sure metric name label matcher only added once
robskillington Dec 16, 2020
7bd0541
Avoid nil ptr for empty label matcher on a vector selector
robskillington Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/prometheus/prometheus v1.8.2-0.20200507164740-ecee9c8abfd1
github.com/rakyll/statik v0.1.7
github.com/sahilm/fuzzy v0.1.0
github.com/stretchr/testify v1.5.1
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c
)
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,7 @@ github.com/prometheus/procfs v0.2.0 h1:wH4vA7pcjKuZzjF7lM8awk4fnuJO6idemZXoKnULU
github.com/prometheus/procfs v0.2.0/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4OA4YeYWdaU=
github.com/prometheus/prometheus v1.8.2-0.20200507164740-ecee9c8abfd1 h1:Oh/bmW9DXCbMeAZbxMmt2wuY6Q4cD0IIbR6vJP3kdHg=
github.com/prometheus/prometheus v1.8.2-0.20200507164740-ecee9c8abfd1/go.mod h1:S5n0C6tSgdnwWshBUceRx5G1OsjLv/EeZ9t3wIfEtsY=
github.com/prometheus/prometheus v2.5.0+incompatible h1:7QPitgO2kOFG8ecuRn9O/4L9+10He72rVRJvMXrE9Hg=
github.com/rakyll/statik v0.1.7 h1:OF3QCZUuyPxuGEP7B4ypUa7sB/iHtqOTDYZXGM8KOdQ=
github.com/rakyll/statik v0.1.7/go.mod h1:AlZONWzMtEnMs7W4e/1LURLiI49pIMmp6V9Unghqrcc=
github.com/rcrowley/go-metrics v0.0.0-20181016184325-3113b8401b8a/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=
Expand Down Expand Up @@ -688,6 +689,7 @@ github.com/streadway/amqp v0.0.0-20190827072141-edfb9018d271/go.mod h1:AZpEONHx3
github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
github.com/stretchr/testify v1.2.0/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
Expand Down
93 changes: 79 additions & 14 deletions langserver/completion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ import (
"strings"

"github.com/pkg/errors"

"github.com/prometheus-community/promql-langserver/internal/vendored/go-tools/lsp/protocol"
"github.com/prometheus-community/promql-langserver/langserver/cache"
"github.com/prometheus/common/model"
"github.com/prometheus/prometheus/pkg/labels"
promql "github.com/prometheus/prometheus/promql/parser"
"github.com/prometheus/prometheus/util/strutil"
"github.com/sahilm/fuzzy"
Expand Down Expand Up @@ -273,10 +274,7 @@ func (s *server) completeLabels(ctx context.Context, completions *[]protocol.Com
}
}

vs, ok := location.Node.(*promql.VectorSelector)
if !ok {
vs = nil
}
vs, _ := depthFirstVectorSelector(location.Node)

item.Pos += offset

Expand All @@ -294,24 +292,67 @@ func (s *server) completeLabels(ctx context.Context, completions *[]protocol.Com

if isValue && lastLabel != "" {
loc.Node = &item
return s.completeLabelValue(ctx, completions, &loc, lastLabel)
return s.completeLabelValue(ctx, completions, &loc, lastLabel, vs)
}

if item.Typ == promql.EQL || item.Typ == promql.NEQ {
loc.Node = &promql.Item{Pos: item.Pos + promql.Pos(len(item.Val))}
return s.completeLabelValue(ctx, completions, &loc, lastLabel)
return s.completeLabelValue(ctx, completions, &loc, lastLabel, vs)
}

return nil
}

func depthFirstVectorSelector(node promql.Node) (*promql.VectorSelector, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be nice to have a tail recursive instead

if vs, ok := node.(*promql.VectorSelector); ok {
return vs, true
}

for _, child := range promql.Children(node) {
if vs, ok := depthFirstVectorSelector(child); ok {
return vs, true
}
}

return nil, false
}

func matchersFromVectorSelector(vs *promql.VectorSelector) ([]*labels.Matcher, error) {
if vs == nil {
return nil, nil
}
var matchers []*labels.Matcher
if vs.Name != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
}

m, err := labels.NewMatcher(labels.MatchEqual, model.MetricNameLabel, vs.Name)
if err != nil {
return nil, err
}
matchers = append(matchers, m)
}
for _, m := range vs.LabelMatchers {
if m == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be more consistent like that, since it has exactly the same effect

if m == nil || m.Name == model.MetricNameLabel {
}

// Sometimes the label matcher is nil.
continue
}
if m.Name == model.MetricNameLabel {
// Only add the metric name label once, sometimes this is set
// just as name, sometimes as both name and as a matcher.
continue
}
matchers = append(matchers, m)
}
return matchers, nil
}

func (s *server) completeLabel(ctx context.Context, completions *[]protocol.CompletionItem, location *cache.Location, vs *promql.VectorSelector) error {
metricName := ""
labelName := location.Node.(*promql.Item).Val

if vs != nil {
metricName = vs.Name
matchers, err := matchersFromVectorSelector(vs)
if err != nil {
return err
}
allNames, err := s.metadataService.LabelNames(ctx, metricName)

allNames, err := s.metadataService.LabelNames(ctx, matchers)
if err != nil {
// nolint: errcheck
s.client.LogMessage(s.lifetime, &protocol.LogMessageParams{
Expand All @@ -326,7 +367,6 @@ func (s *server) completeLabel(ctx context.Context, completions *[]protocol.Comp
return err
}

labelName := location.Node.(*promql.Item).Val
OUTER:
for _, match := range getMatches(labelName, allNames) {
// Skip labels that already have matchers
Expand All @@ -353,8 +393,33 @@ OUTER:
}

// nolint: funlen
func (s *server) completeLabelValue(ctx context.Context, completions *[]protocol.CompletionItem, location *cache.Location, labelName string) error {
labelValues, err := s.metadataService.LabelValues(ctx, labelName)
func (s *server) completeLabelValue(
ctx context.Context,
completions *[]protocol.CompletionItem,
location *cache.Location,
labelName string,
vs *promql.VectorSelector,
) error {
// Current selection from selector if not nil.
matchers, err := matchersFromVectorSelector(vs)
if err != nil {
return err
}

// Delete the current label from matchers if present, it is incomplete and
// we are trying to complete values for it.
if len(matchers) != 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting way of optimizing filtering here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

filtering := matchers[:]
matchers = matchers[:0]
for _, m := range filtering {
if m.Name == labelName {
continue // Filter out.
}
matchers = append(matchers, m)
}
}

labelValues, err := s.metadataService.LabelValues(ctx, labelName, matchers)
if err != nil {
// nolint: errcheck
s.client.LogMessage(s.lifetime, &protocol.LogMessageParams{
Expand Down
Loading