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.

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] Fix bug with parsing Graphite find query #1676

Merged
merged 2 commits into from
May 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/query/api/v1/handler/graphite/find_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ func parseFindParamsToQueries(r *http.Request) (

clonedMatchers := make([]models.Matcher, len(matchers))
copy(clonedMatchers, matchers)
// NB: change terminator from `MatchNotRegexp` to `MatchRegexp` to ensure
// NB: change terminator from `MatchNotField` to `MatchField` to ensure
// segments with children are matched.
clonedMatchers[len(clonedMatchers)-1].Type = models.MatchRegexp
clonedMatchers[len(clonedMatchers)-1].Type = models.MatchField
childQuery := &storage.CompleteTagsQuery{
CompleteNameOnly: false,
FilterNameTags: filter,
Expand Down
4 changes: 2 additions & 2 deletions src/query/api/v1/handler/graphite/find_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func setupStorage(ctrl *gomock.Controller) storage.Storage {
matchers: []models.Matcher{
{Type: models.MatchEqual, Name: b("__g0__"), Value: b("foo")},
{Type: models.MatchRegexp, Name: b("__g1__"), Value: b(`b[^\.]*`)},
{Type: models.MatchNotRegexp, Name: b("__g2__"), Value: b(".*")},
{Type: models.MatchNotField, Name: b("__g2__")},
},
}

Expand All @@ -146,7 +146,7 @@ func setupStorage(ctrl *gomock.Controller) storage.Storage {
matchers: []models.Matcher{
{Type: models.MatchEqual, Name: b("__g0__"), Value: b("foo")},
{Type: models.MatchRegexp, Name: b("__g1__"), Value: b(`b[^\.]*`)},
{Type: models.MatchRegexp, Name: b("__g2__"), Value: b(".*")},
{Type: models.MatchField, Name: b("__g2__")},
},
}

Expand Down
146 changes: 76 additions & 70 deletions src/query/generated/proto/rpcpb/query.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/query/generated/proto/rpcpb/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ enum MatcherType {
REGEXP = 2;
NOTREGEXP = 3;
// EXISTS and NOTEXISTS apply only to
// matcher name rather than value
// matcher name rather than value.
EXISTS = 4;
NOTEXISTS = 5;
// ALL supercedes other matcher types
// and does no filtering.
ALL = 6;
}

message FetchResponse {
Expand Down
16 changes: 11 additions & 5 deletions src/query/graphite/storage/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,22 @@ import (
"github.com/m3db/m3/src/query/models"
)

var (
wildcard = []byte(".*")
const (
wildcard = "*"
)

func convertMetricPartToMatcher(
count int,
metric string,
) (models.Matcher, error) {
var matchType models.MatchType
if metric == wildcard {
return models.Matcher{
Type: models.MatchField,
Name: graphite.TagName(count),
}, nil
}

value, isRegex, err := graphite.GlobToRegexPattern(metric)
if err != nil {
return models.Matcher{}, err
Expand All @@ -54,8 +61,7 @@ func convertMetricPartToMatcher(

func matcherTerminator(count int) models.Matcher {
return models.Matcher{
Type: models.MatchNotRegexp,
Name: graphite.TagName(count),
Value: wildcard,
Type: models.MatchNotField,
Name: graphite.TagName(count),
}
}
19 changes: 16 additions & 3 deletions src/query/graphite/storage/converter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ func TestConvertMetricPartToMatcher(t *testing.T) {
}
}

func TestConvertWildcardToMatcher(t *testing.T) {
metric := "*"
for i := 0; i < 100; i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of a weird test -- why not just run this once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to ensure that the metric name is also converted to expected (i.e. __g100__) as well

expected := models.Matcher{
Type: models.MatchField,
Name: graphite.TagName(i),
}

actual, err := convertMetricPartToMatcher(i, metric)
require.NoError(t, err)
assert.Equal(t, expected, actual)
}
}

func TestConvertAlphanumericMetricPartToMatcher(t *testing.T) {
metric := "abcdefg"
expected := models.Matcher{
Expand All @@ -61,9 +75,8 @@ func TestConvertAlphanumericMetricPartToMatcher(t *testing.T) {
func TestMatcherTerminator(t *testing.T) {
for i := 0; i < 100; i++ {
expected := models.Matcher{
Type: models.MatchNotRegexp,
Name: graphite.TagName(i),
Value: []byte(".*"),
Type: models.MatchNotField,
Name: graphite.TagName(i),
}

actual := matcherTerminator(i)
Expand Down
4 changes: 2 additions & 2 deletions src/query/graphite/storage/m3_wrapper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ func TestTranslateQuery(t *testing.T) {
{Type: models.MatchEqual, Name: graphite.TagName(3), Value: []byte("terminator")},
{Type: models.MatchEqual, Name: graphite.TagName(4), Value: []byte("will")},
{Type: models.MatchEqual, Name: graphite.TagName(5), Value: []byte("be")},
{Type: models.MatchRegexp, Name: graphite.TagName(6), Value: []byte(`[^\.]*`)},
{Type: models.MatchField, Name: graphite.TagName(6)},
{Type: models.MatchRegexp, Name: graphite.TagName(7), Value: []byte(`back[^\.]`)},
{Type: models.MatchNotRegexp, Name: graphite.TagName(8), Value: []byte(".*")},
{Type: models.MatchNotField, Name: graphite.TagName(8)},
}

assert.Equal(t, expected, matchers)
Expand Down
20 changes: 4 additions & 16 deletions src/query/models/matcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ func (m MatchType) String() string {
return "=~"
case MatchNotRegexp:
return "!~"
case MatchField:
return "-"
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my understanding, how did you come up with these mappings?

Copy link
Collaborator Author

@arnikola arnikola May 30, 2019

Choose a reason for hiding this comment

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

Made 'em up :p

It's not really a "standard" matcher type, and the only thing this affects is the String() method on MatchTypes so we should be fine here

case MatchNotField:
return "!-"
case MatchAll:
return "*"
default:
Expand Down Expand Up @@ -69,22 +73,6 @@ func (m Matcher) String() string {
return fmt.Sprintf("%s%s%q", m.Name, m.Type, m.Value)
}

// Matches returns whether the matcher matches the given string value.
func (m Matcher) Matches(s []byte) bool {
switch m.Type {
case MatchEqual:
return bytes.Equal(s, m.Value)
case MatchNotEqual:
return !bytes.Equal(s, m.Value)
case MatchRegexp:
return m.re.MatchString(string(s))
case MatchNotRegexp:
return !m.re.MatchString(string(s))
}

panic("labels.Matcher.Matches: invalid match type")
}

// ToTags converts Matchers to Tags
// NB (braskin): this only works for exact matches
func (m Matchers) ToTags(
Expand Down
Loading