-
Notifications
You must be signed in to change notification settings - Fork 455
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.
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
[query] Add tag completion HTTP endpoint onto query #1175
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1175 +/- ##
========================================
- Coverage 71.1% 71% -0.1%
========================================
Files 737 734 -3
Lines 61854 61684 -170
========================================
- Hits 43994 43848 -146
Misses 15011 15011
+ Partials 2849 2825 -24
Continue to review full report at Codecov.
|
FilterNameTags: [][]byte{nameBytes}, | ||
TagMatchers: models.Matchers{ | ||
models.Matcher{ | ||
Type: models.MatchEqual, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatchRegexp ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prom supports exact matchers for this endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, making it MatchEqual will fail the Matches() call
src/query/models/matcher.go
Outdated
if len(ss) > 2 { | ||
return Matcher{}, errors.New("invalid arg length for matcher") | ||
} | ||
fmt.Println(l) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove ?
src/query/models/matcher.go
Outdated
|
||
if l == 1 { | ||
return Matcher{ | ||
Type: MatchEqual, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit weird since you can in theory also have match not equal, match regex, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the TODO; this is a bit annoying to do since it requires query parsing on our side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to leave this until we add in m3ql parsing since we'll be able to use the query parsing from that codebase here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can have MatchRegex in prom too!
if b.nameOnly { | ||
for name := range b.tagBuilders { | ||
result = append(result, CompletedTag{Name: []byte(name)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, did it break any test now ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was only added in this PR; once tests were added it failed a few
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
FilterNameTags: [][]byte{nameBytes}, | ||
TagMatchers: models.Matchers{ | ||
models.Matcher{ | ||
Type: models.MatchEqual, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed, making it MatchEqual will fail the Matches() call
src/query/models/matcher.go
Outdated
|
||
if l == 1 { | ||
return Matcher{ | ||
Type: MatchEqual, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can have MatchRegex in prom too!
src/query/models/matcher.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
fmt.Println(matcher) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove ?
if b.nameOnly { | ||
for name := range b.tagBuilders { | ||
result = append(result, CompletedTag{Name: []byte(name)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
src/query/models/matcher.go
Outdated
// TODO: make this more robust, handle types other than MatchEqual | ||
func matcherFromString(s string) (Matcher, error) { | ||
ss := strings.Split(s, ":") | ||
l := len(ss) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call it length ?
src/query/models/matcher.go
Outdated
func matcherFromString(s string) (Matcher, error) { | ||
ss := strings.Split(s, ":") | ||
l := len(ss) | ||
if len(ss) > 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u already have a variable for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to bump by characters per commit metrics 😛
Adds the endpoint to query which will allow tag completion