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] Special case particular Prom matchers #2479

Merged
merged 5 commits into from
Jul 23, 2020
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
14 changes: 7 additions & 7 deletions scripts/docker-integration-tests/query_fanout/warning.sh
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,10 @@ function test_range_query {
}

function test_search {
start=$(date -d "$(date +%Y-%m-%dT%H:%M:%SZ) -1 minute" +%Y-%m-%dT%H:%M:%SZ)
end=$(date -d "$(date +%Y-%m-%dT%H:%M:%SZ) +1 minute" +%Y-%m-%dT%H:%M:%SZ)

s=$(( $(date +%s) - 60 ))
start=$(date -r $s +%Y-%m-%dT%H:%M:%SZ)
e=$(( $(date +%s) + 60 ))
end=$(date -r $e +%Y-%m-%dT%H:%M:%SZ)
curl -D headers -X POST 0.0.0.0:7201/search -d '{
"start": "'$start'",
"end": "'$end'",
Expand Down Expand Up @@ -255,14 +256,14 @@ function test_fanout_warning_search {
# write 5 metrics to cluster a
write_metrics coordinator-cluster-a 5
# unlimited query against cluster a has no header
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 15
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 1000
# limited query against cluster a has header
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 4 max_fetch_series_limit_applied

# write 10 metrics to cluster b
write_metrics coordinator-cluster-b 10
# unlimited query against cluster a has no header
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 16
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 1000
# remote limited query against cluster a has header
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 4 max_fetch_series_limit_applied
}
Expand Down Expand Up @@ -357,7 +358,7 @@ function test_fanout_warning_missing_zone {
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_range_query 9 14 max_fetch_series_limit_applied,remote_store_cluster-c_fetch_data_error

METRIC_NAME=$SEARCH_NAME
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 16 remote_store_cluster-c_fetch_data_error
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 1000 remote_store_cluster-c_fetch_data_error
ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_search 4 max_fetch_series_limit_applied,remote_store_cluster-c_fetch_data_error

ATTEMPTS=3 TIMEOUT=1 retry_with_backoff test_labels 100 remote_store_cluster-c_fetch_data_error
Expand Down Expand Up @@ -391,4 +392,3 @@ function test_fanout_warnings {
test_fanout_warning_graphite
test_fanout_warning_missing_zone
}

5 changes: 3 additions & 2 deletions src/query/api/v1/handler/prom/read_instant.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,11 @@ func parseTime(s string) (time.Time, error) {
// Upstream issue: https://github.com/golang/go/issues/20555
switch s {
case minTimeFormatted:
return minTime, nil
return time.Time{}, nil
case maxTimeFormatted:
return maxTime, nil
return time.Now(), nil
}

return time.Time{}, fmt.Errorf("cannot parse %q to a valid timestamp", s)
}

Expand Down
107 changes: 95 additions & 12 deletions src/query/storage/index.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
package storage

import (
"bytes"
"fmt"

"github.com/m3db/m3/src/dbnode/storage/index"
Expand All @@ -31,8 +30,10 @@ import (
"github.com/m3db/m3/src/x/ident"
)

var (
dotStar = []byte(".*")
const (
dot = byte('.')
plus = byte('+')
star = byte('*')
)

// FromM3IdentToMetric converts an M3 ident metric to a coordinator metric.
Expand Down Expand Up @@ -119,6 +120,18 @@ func FetchQueryToM3Query(

// Optimization for single matcher case.
if len(matchers) == 1 {
specialCase := isSpecialCaseMatcher(matchers[0])
if specialCase.skip {
// NB: only matcher has no effect; this is synonymous to an AllQuery.
return index.Query{
Query: idx.NewAllQuery(),
}, nil
}

if specialCase.isSpecial {
return index.Query{Query: specialCase.query}, nil
}

q, err := matcherToQuery(matchers[0])
if err != nil {
return index.Query{}, err
Expand All @@ -127,59 +140,129 @@ func FetchQueryToM3Query(
return index.Query{Query: q}, nil
}

idxQueries := make([]idx.Query, len(matchers))
var err error
for i, matcher := range matchers {
idxQueries[i], err = matcherToQuery(matcher)
idxQueries := make([]idx.Query, 0, len(matchers))
for _, matcher := range matchers {
specialCase := isSpecialCaseMatcher(matcher)
if specialCase.skip {
continue
}

if specialCase.isSpecial {
idxQueries = append(idxQueries, specialCase.query)
continue
}

q, err := matcherToQuery(matcher)
if err != nil {
return index.Query{}, err
}

idxQueries = append(idxQueries, q)
}

q := idx.NewConjunctionQuery(idxQueries...)

return index.Query{Query: q}, nil
}

type specialCase struct {
query idx.Query
isSpecial bool
skip bool
}

func isSpecialCaseMatcher(matcher models.Matcher) specialCase {
if len(matcher.Value) == 0 {
if matcher.Type == models.MatchNotRegexp ||
matcher.Type == models.MatchNotEqual {
query := idx.NewFieldQuery(matcher.Name)
return specialCase{query: query, isSpecial: true}
}

if matcher.Type == models.MatchRegexp ||
matcher.Type == models.MatchEqual {
query := idx.NewNegationQuery(idx.NewFieldQuery(matcher.Name))
return specialCase{query: query, isSpecial: true}
}

return specialCase{}
}

// NB: no special case for regex / not regex here.
isNegatedRegex := matcher.Type == models.MatchNotRegexp
isRegex := matcher.Type == models.MatchRegexp
if !isNegatedRegex && !isRegex {
return specialCase{}
}

if len(matcher.Value) != 2 && matcher.Value[0] != dot {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you think about the case when we have "^" and "$"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

never mind, we sanitize at a layer above 👍

return specialCase{}
}

if matcher.Value[1] == star {
if isNegatedRegex {
// NB: This should match no results.
query := idx.NewNegationQuery(idx.NewAllQuery())
return specialCase{query: query, isSpecial: true}
}

// NB: this matcher should not affect query results.
return specialCase{skip: true}
}

if matcher.Value[1] == plus {
query := idx.NewFieldQuery(matcher.Name)
if isNegatedRegex {
query = idx.NewNegationQuery(query)
}

return specialCase{query: query, isSpecial: true}
}

return specialCase{}
}

func matcherToQuery(matcher models.Matcher) (idx.Query, error) {
negate := false
switch matcher.Type {
// Support for Regexp types
case models.MatchNotRegexp:
negate = true
fallthrough

case models.MatchRegexp:
var (
query idx.Query
err error
)
if bytes.Equal(dotStar, matcher.Value) {
query = idx.NewFieldQuery(matcher.Name)
} else {
query, err = idx.NewRegexpQuery(matcher.Name, matcher.Value)
}
query, err = idx.NewRegexpQuery(matcher.Name, matcher.Value)
if err != nil {
return idx.Query{}, err
}

if negate {
query = idx.NewNegationQuery(query)
}

return query, nil

// Support exact matches
case models.MatchNotEqual:
negate = true
fallthrough

case models.MatchEqual:
query := idx.NewTermQuery(matcher.Name, matcher.Value)
if negate {
query = idx.NewNegationQuery(query)
}

return query, nil

case models.MatchNotField:
negate = true
fallthrough

case models.MatchField:
query := idx.NewFieldQuery(matcher.Name)
if negate {
Expand Down
17 changes: 14 additions & 3 deletions src/query/storage/index_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ func TestFetchQueryToM3Query(t *testing.T) {
},
},
{
name: "regexp match -> field",
expected: "field(t1)",
name: "regexp match dot star -> field",
expected: "all()",
matchers: models.Matchers{
{
Type: models.MatchRegexp,
Expand All @@ -122,6 +122,17 @@ func TestFetchQueryToM3Query(t *testing.T) {
},
},
},
{
name: "regexp match dot plus -> field",
expected: "field(t1)",
matchers: models.Matchers{
{
Type: models.MatchRegexp,
Name: []byte("t1"),
Value: []byte(".+"),
},
},
},
{
name: "regexp match negated",
expected: "negation(regexp(t1, v1))",
Expand All @@ -135,7 +146,7 @@ func TestFetchQueryToM3Query(t *testing.T) {
},
{
name: "regexp match negated",
expected: "negation(field(t1))",
expected: "negation(all())",
matchers: models.Matchers{
{
Type: models.MatchNotRegexp,
Expand Down
43 changes: 43 additions & 0 deletions src/query/test/compatibility/testdata/selectors.test
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,46 @@ eval instant at 0s http_requests{foo!~"bar", job="api-server"}
eval instant at 0s http_requests{foo!~"bar", job="api-server", instance="1", x!="y", z="", group!=""}
http_requests{job="api-server", instance="1", group="production"} 0
http_requests{job="api-server", instance="1", group="canary"} 0

# check special casing for existing label
eval instant at 0s http_requests{job="", instance="0", group="production"}

eval instant at 0s http_requests{job!="", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{job=~"", instance="0", group="production"}

eval instant at 0s http_requests{job!~"", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{job=~".+", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{job=~".*", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{job!~".+", instance="0", group="production"}

eval instant at 0s http_requests{job!~".*", instance="0", group="production"}

# check special casing for non-existent label
eval instant at 0s http_requests{foo="", job="api-server", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{foo!="", job="api-server", instance="0", group="production"}

eval instant at 0s http_requests{foo=~"", job="api-server", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{foo!~"", job="api-server", instance="0", group="production"}

eval instant at 0s http_requests{foo=~".+", job="api-server", instance="0", group="production"}

eval instant at 0s http_requests{foo=~".*", job="api-server", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{foo!~".+", job="api-server", instance="0", group="production"}
http_requests{job="api-server", instance="0", group="production"} 0

eval instant at 0s http_requests{foo!~".*", job="api-server", instance="0", group="production"}