From e9b4391ed03e1ee2ee4772d20450f0b356c4a5a9 Mon Sep 17 00:00:00 2001 From: Martin Disibio Date: Wed, 7 Dec 2022 13:07:22 -0500 Subject: [PATCH] Tweak traceql parsing to correctly parse binary operations without space (#1941) * Tweak traceql parsing to correctly parse some binary operations without space * changelog --- CHANGELOG.md | 1 + pkg/traceql/lexer.go | 16 ++++++++----- pkg/traceql/lexer_test.go | 6 +++++ pkg/traceql/parse_test.go | 49 +++++++++++++++++++++++++-------------- 4 files changed, 48 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6498f173890..a45bc92cf5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,7 @@ Internal types are updated to use `scope` instead of `instrumentation_library`. * [BUGFIX] tempo-mixin: tweak dashboards to support metrics without `cluster` label present [#1913](https://github.com/grafana/tempo/pull/1913) (@kvrhdn) * [ENHANCEMENT] New tenant dashboard [#1901](https://github.com/grafana/tempo/pull/1901) (@mapno) * [BUGFIX] Fix docker-compose examples not running on Apple M1 hardware [#1920](https://github.com/grafana/tempo/pull/1920) (@stoewer) +* [BUGFIX] Fix traceql parsing of most binary operations to not require spacing [#1939](https://github.com/grafana/tempo/pull/1941) (@mdisibio) ## v1.5.0 / 2022-08-17 diff --git a/pkg/traceql/lexer.go b/pkg/traceql/lexer.go index 313692c7350..7e3eeed14c5 100644 --- a/pkg/traceql/lexer.go +++ b/pkg/traceql/lexer.go @@ -224,12 +224,16 @@ func isDurationRune(r rune) bool { } func isAttributeRune(r rune) bool { - return !unicode.IsSpace(r) && - r != scanner.EOF && - r != '(' && - r != ')' && - r != '}' && - r != '{' + if unicode.IsSpace(r) { + return false + } + + switch r { + case scanner.EOF, '{', '}', '(', ')', '=', '~', '!', '<', '>', '&', '|', '^': + return false + default: + return true + } } func startsAttribute(tok int) bool { diff --git a/pkg/traceql/lexer_test.go b/pkg/traceql/lexer_test.go index 87506335505..a066c97999b 100644 --- a/pkg/traceql/lexer_test.go +++ b/pkg/traceql/lexer_test.go @@ -134,3 +134,9 @@ func testLexer(t *testing.T, tcs []lexerTestCase) { }) } } + +func BenchmarkIsAttributeRune(b *testing.B) { + for i := 0; i < b.N; i++ { + isAttributeRune('=') + } +} diff --git a/pkg/traceql/parse_test.go b/pkg/traceql/parse_test.go index 3ea662921ba..843520b7271 100644 --- a/pkg/traceql/parse_test.go +++ b/pkg/traceql/parse_test.go @@ -1,6 +1,7 @@ package traceql import ( + "strings" "testing" "time" @@ -646,36 +647,48 @@ func TestSpansetFilterStatics(t *testing.T) { func TestSpansetFilterOperators(t *testing.T) { tests := []struct { - in string - err error - expected FieldExpression + in string + expected FieldExpression + alsoTestWithoutSpace bool }{ {in: "{ .a + .b }", expected: newBinaryOperation(OpAdd, NewAttribute("a"), NewAttribute("b"))}, {in: "{ .a - .b }", expected: newBinaryOperation(OpSub, NewAttribute("a"), NewAttribute("b"))}, {in: "{ .a / .b }", expected: newBinaryOperation(OpDiv, NewAttribute("a"), NewAttribute("b"))}, {in: "{ .a % .b }", expected: newBinaryOperation(OpMod, NewAttribute("a"), NewAttribute("b"))}, {in: "{ .a * .b }", expected: newBinaryOperation(OpMult, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a = .b }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a != .b }", expected: newBinaryOperation(OpNotEqual, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a =~ .b }", expected: newBinaryOperation(OpRegex, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a !~ .b }", expected: newBinaryOperation(OpNotRegex, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a > .b }", expected: newBinaryOperation(OpGreater, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a >= .b }", expected: newBinaryOperation(OpGreaterEqual, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a < .b }", expected: newBinaryOperation(OpLess, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a <= .b }", expected: newBinaryOperation(OpLessEqual, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a ^ .b }", expected: newBinaryOperation(OpPower, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a && .b }", expected: newBinaryOperation(OpAnd, NewAttribute("a"), NewAttribute("b"))}, - {in: "{ .a || .b }", expected: newBinaryOperation(OpOr, NewAttribute("a"), NewAttribute("b"))}, + {in: "{ .a = .b }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a != .b }", expected: newBinaryOperation(OpNotEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a =~ .b }", expected: newBinaryOperation(OpRegex, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a !~ .b }", expected: newBinaryOperation(OpNotRegex, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a > .b }", expected: newBinaryOperation(OpGreater, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a >= .b }", expected: newBinaryOperation(OpGreaterEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a < .b }", expected: newBinaryOperation(OpLess, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a <= .b }", expected: newBinaryOperation(OpLessEqual, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a ^ .b }", expected: newBinaryOperation(OpPower, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a && .b }", expected: newBinaryOperation(OpAnd, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, + {in: "{ .a || .b }", expected: newBinaryOperation(OpOr, NewAttribute("a"), NewAttribute("b")), alsoTestWithoutSpace: true}, {in: "{ !.b }", expected: newUnaryOperation(OpNot, NewAttribute("b"))}, {in: "{ -.b }", expected: newUnaryOperation(OpSub, NewAttribute("b"))}, + + // Against statics + {in: "{ .a = `foo` }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticString("foo")), alsoTestWithoutSpace: true}, + {in: "{ .a = 3 }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticInt(3)), alsoTestWithoutSpace: true}, + {in: "{ .a = 3.0 }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticFloat(3)), alsoTestWithoutSpace: true}, + {in: "{ .a = true }", expected: newBinaryOperation(OpEqual, NewAttribute("a"), NewStaticBool(true)), alsoTestWithoutSpace: true}, + } + + test := func(q string, expected FieldExpression) { + actual, err := Parse(q) + require.NoError(t, err, q) + require.Equal(t, &RootExpr{newPipeline(newSpansetFilter(expected))}, actual, q) } for _, tc := range tests { t.Run(tc.in, func(t *testing.T) { - actual, err := Parse(tc.in) - - require.NoError(t, err) - require.Equal(t, &RootExpr{newPipeline(newSpansetFilter(tc.expected))}, actual) + test(tc.in, tc.expected) + if tc.alsoTestWithoutSpace { + test(strings.ReplaceAll(tc.in, " ", ""), tc.expected) + } }) } }