From cc2c2dba6703ad6cf6af154f5e549753fcb53ad5 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Mon, 16 Mar 2020 18:28:34 -0400 Subject: [PATCH 1/3] Optimize filter queries such as `{label="foo"} |~ "" |~ ".*" |= ""` doesn't contains a filter. The query is not really rewritten and the stringer func still returns the correct original query. However the `Filter` func of the `Expr` returns nil which what is being checked when splitting queries in the frontend and also what is being used in the memchunk. see https://github.com/grafana/loki/blob/master/pkg/querier/queryrange/roundtrip.go#L81 and https://github.com/grafana/loki/blob/master/pkg/chunkenc/memchunk.go#L614 Fixes #1792 Signed-off-by: Cyril Tovena --- pkg/logql/ast.go | 5 ++++- pkg/logql/ast_test.go | 5 +++++ pkg/logql/filter.go | 31 ++++++++++++++++++++++++++++--- pkg/logql/filter_test.go | 35 ++++++++++++++++++++++++++++++++++- 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/pkg/logql/ast.go b/pkg/logql/ast.go index b7c606deeddf5..efd6c2f522223 100644 --- a/pkg/logql/ast.go +++ b/pkg/logql/ast.go @@ -131,7 +131,10 @@ func (e *filterExpr) Filter() (LineFilter, error) { if err != nil { return nil, err } - return newAndFilter(nextFilter, f), nil + f = newAndFilter(nextFilter, f) + } + if f == TrueFilter { + return nil, nil } return f, nil } diff --git a/pkg/logql/ast_test.go b/pkg/logql/ast_test.go index 4148df27f055a..511e384199629 100644 --- a/pkg/logql/ast_test.go +++ b/pkg/logql/ast_test.go @@ -20,6 +20,11 @@ func Test_logSelectorExpr_String(t *testing.T) { {`{foo="bar", bar!="baz"}`, false}, {`{foo="bar", bar!="baz"} != "bip" !~ ".+bop"`, true}, {`{foo="bar"} |= "baz" |~ "blip" != "flip" !~ "flap"`, true}, + {`{foo="bar", bar!="baz"} |= ""`, false}, + {`{foo="bar", bar!="baz"} |~ ""`, false}, + {`{foo="bar", bar!="baz"} |~ ".*"`, false}, + {`{foo="bar", bar!="baz"} |= "" |= ""`, false}, + {`{foo="bar", bar!="baz"} |~ "" |= "" |~ ".*"`, false}, } for _, tt := range tests { diff --git a/pkg/logql/filter.go b/pkg/logql/filter.go index 228fd0958827d..76ddefb3d6ae0 100644 --- a/pkg/logql/filter.go +++ b/pkg/logql/filter.go @@ -21,6 +21,13 @@ func (f LineFilterFunc) Filter(line []byte) bool { return f(line) } +type trueFilter struct{} + +func (trueFilter) Filter(_ []byte) bool { return true } + +// TrueFilter is a filter that returns and matches all log lines whatever their content. +var TrueFilter = &trueFilter{} + type notFilter struct { LineFilter } @@ -46,6 +53,9 @@ type andFilter struct { // newAndFilter creates a new filter which matches only if left and right matches. func newAndFilter(left LineFilter, right LineFilter) LineFilter { + if (right == TrueFilter || right == nil) && (left == TrueFilter || left == nil) { + return TrueFilter + } return andFilter{ left: left, right: right, @@ -63,6 +73,9 @@ type orFilter struct { // newOrFilter creates a new filter which matches only if left or right matches. func newOrFilter(left LineFilter, right LineFilter) LineFilter { + if (right == TrueFilter || right == nil) && (left == TrueFilter || left == nil) { + return TrueFilter + } return orFilter{ left: left, right: right, @@ -113,6 +126,13 @@ func (l containsFilter) String() string { return string(l) } +func newContainsFilter(match string) LineFilter { + if match == "" { + return TrueFilter + } + return containsFilter(match) +} + // newFilter creates a new line filter from a match string and type. func newFilter(match string, mt labels.MatchType) (LineFilter, error) { switch mt { @@ -121,10 +141,9 @@ func newFilter(match string, mt labels.MatchType) (LineFilter, error) { case labels.MatchNotRegexp: return parseRegexpFilter(match, false) case labels.MatchEqual: - return containsFilter(match), nil + return newContainsFilter(match), nil case labels.MatchNotEqual: - return newNotFilter(containsFilter(match)), nil - + return newNotFilter(newContainsFilter(match)), nil default: return nil, fmt.Errorf("unknown matcher: %v", match) } @@ -163,6 +182,12 @@ func simplify(reg *syntax.Regexp) (LineFilter, bool) { return simplify(reg) case syntax.OpLiteral: return containsFilter(string(reg.Rune)), true + case syntax.OpStar: + if reg.Sub[0].Op == syntax.OpAnyCharNotNL { + return TrueFilter, true + } + case syntax.OpEmptyMatch: + return TrueFilter, true } return nil, false } diff --git a/pkg/logql/filter_test.go b/pkg/logql/filter_test.go index ac47e6bfe9c19..8013f184f8b35 100644 --- a/pkg/logql/filter_test.go +++ b/pkg/logql/filter_test.go @@ -48,12 +48,13 @@ func Test_SimplifiedRegex(t *testing.T) { {"f.*|foobar.*|.*buzz", true, newOrFilter(newOrFilter(containsFilter("f"), containsFilter("foobar")), containsFilter("buzz")), true}, // parsed as ((f(?-s:.)*)|foobar(?-s:.)*)|(?-s:.)*buzz {"((f.*)|foobar.*)|.*buzz", true, newOrFilter(newOrFilter(containsFilter("f"), containsFilter("foobar")), containsFilter("buzz")), true}, + {".*", true, TrueFilter, true}, + {"", true, TrueFilter, true}, // regex we are not supporting. {"[a-z]+foo", false, nil, false}, {".+foo", false, nil, false}, {".*fo.*o", false, nil, false}, - {".*", false, nil, false}, {`\d`, false, nil, false}, {`\sfoo`, false, nil, false}, {`foo?`, false, nil, false}, @@ -89,6 +90,38 @@ func Test_SimplifiedRegex(t *testing.T) { } } +func Test_TrueFilter(t *testing.T) { + for _, test := range []struct { + name string + f LineFilter + expectTrue bool + }{ + {"empty match", newContainsFilter(""), true}, + {"not empty match", newNotFilter(newContainsFilter("")), false}, + {"match", newContainsFilter("foo"), false}, + {"empty match and", newAndFilter(newContainsFilter(""), newContainsFilter("")), true}, + {"empty match or", newOrFilter(newContainsFilter(""), newContainsFilter("")), true}, + {"nil right and", newAndFilter(newContainsFilter(""), nil), true}, + {"nil left or", newOrFilter(nil, newContainsFilter("")), true}, + {"nil right and not empty", newAndFilter(newContainsFilter("foo"), nil), false}, + {"nil left or not empty", newOrFilter(nil, newContainsFilter("foo")), false}, + {"nil both and", newAndFilter(nil, nil), true}, + {"nil both or", newOrFilter(nil, nil), true}, + {"empty match and chained", newAndFilter(newContainsFilter(""), newAndFilter(newContainsFilter(""), newAndFilter(newContainsFilter(""), newContainsFilter("")))), true}, + {"empty match or chained", newOrFilter(newContainsFilter(""), newOrFilter(newContainsFilter(""), newOrFilter(newContainsFilter(""), newContainsFilter("")))), true}, + {"empty match and", newNotFilter(newAndFilter(newContainsFilter(""), newContainsFilter(""))), false}, + {"empty match or", newNotFilter(newOrFilter(newContainsFilter(""), newContainsFilter(""))), false}, + } { + t.Run(test.name, func(t *testing.T) { + if test.expectTrue { + require.Equal(t, TrueFilter, test.f) + } else { + require.NotEqual(t, TrueFilter, test.f) + } + }) + } +} + func Benchmark_LineFilter(b *testing.B) { b.ReportAllocs() logline := `level=bar ts=2020-02-22T14:57:59.398312973Z caller=logging.go:44 traceID=2107b6b551458908 msg="GET /buzz (200) 4.599635ms` From e109841b986b2d1346ed63049a5fc50a3b19236f Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Mon, 16 Mar 2020 18:36:02 -0400 Subject: [PATCH 2/3] Add missing tests with combination of empty or regex. Signed-off-by: Cyril Tovena --- pkg/logql/filter_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/logql/filter_test.go b/pkg/logql/filter_test.go index 8013f184f8b35..d25a5729c19d3 100644 --- a/pkg/logql/filter_test.go +++ b/pkg/logql/filter_test.go @@ -49,6 +49,8 @@ func Test_SimplifiedRegex(t *testing.T) { // parsed as ((f(?-s:.)*)|foobar(?-s:.)*)|(?-s:.)*buzz {"((f.*)|foobar.*)|.*buzz", true, newOrFilter(newOrFilter(containsFilter("f"), containsFilter("foobar")), containsFilter("buzz")), true}, {".*", true, TrueFilter, true}, + {".*|.*", true, TrueFilter, true}, + {".*||||", true, TrueFilter, true}, {"", true, TrueFilter, true}, // regex we are not supporting. From 7dd1a6559efcabf82d8bf8db3c44e50b6c47c497 Mon Sep 17 00:00:00 2001 From: Cyril Tovena Date: Tue, 17 Mar 2020 17:04:51 -0400 Subject: [PATCH 3/3] Review feedback. Signed-off-by: Cyril Tovena --- pkg/logql/filter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/logql/filter.go b/pkg/logql/filter.go index 76ddefb3d6ae0..a6a0f5046a97a 100644 --- a/pkg/logql/filter.go +++ b/pkg/logql/filter.go @@ -26,7 +26,7 @@ type trueFilter struct{} func (trueFilter) Filter(_ []byte) bool { return true } // TrueFilter is a filter that returns and matches all log lines whatever their content. -var TrueFilter = &trueFilter{} +var TrueFilter = trueFilter{} type notFilter struct { LineFilter