From 1d7e2f949dcd444c8bcc61acb96ddfc63c13c029 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Fri, 14 Apr 2023 12:28:13 -0600 Subject: [PATCH 1/4] Allow Converter in constant bool expr --- .../ottl-allow-converters-as-constbool.yaml | 16 ++ connector/countconnector/config_test.go | 40 ++-- connector/countconnector/testdata/config.yaml | 40 ++-- internal/filter/filterottl/filter_test.go | 10 +- pkg/ottl/README.md | 7 +- pkg/ottl/boolean_value.go | 40 +++- pkg/ottl/boolean_value_test.go | 200 +++++++++++++++--- pkg/ottl/grammar.go | 7 +- pkg/ottl/parser_test.go | 126 +++++++++-- processor/filterprocessor/README.md | 4 +- processor/filterprocessor/logs_test.go | 6 +- processor/filterprocessor/metrics_test.go | 2 +- processor/filterprocessor/traces_test.go | 2 +- processor/routingprocessor/README.md | 2 +- processor/routingprocessor/config_test.go | 2 +- processor/routingprocessor/logs_test.go | 4 +- .../routingprocessor/testdata/config.yaml | 2 +- .../internal/logs/processor_test.go | 2 +- .../internal/metrics/processor_test.go | 2 +- .../internal/traces/processor_test.go | 2 +- 20 files changed, 393 insertions(+), 123 deletions(-) create mode 100755 .chloggen/ottl-allow-converters-as-constbool.yaml diff --git a/.chloggen/ottl-allow-converters-as-constbool.yaml b/.chloggen/ottl-allow-converters-as-constbool.yaml new file mode 100755 index 000000000000..ddd560930a16 --- /dev/null +++ b/.chloggen/ottl-allow-converters-as-constbool.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: pkg/ottl + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Allow using Converts as constant boolean expressions + +# One or more tracking issues related to the change +issues: [20911] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: This means you don't need to add `== true` after `IsMatch` in OTTL conditions. diff --git a/connector/countconnector/config_test.go b/connector/countconnector/config_test.go index f8d9cdeaa953..c96b9b73c5e0 100644 --- a/connector/countconnector/config_test.go +++ b/connector/countconnector/config_test.go @@ -126,31 +126,31 @@ func TestLoadConfig(t *testing.T) { Spans: map[string]MetricInfo{ "my.span.count": { Description: "My span count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-s") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-s")`}, }, }, SpanEvents: map[string]MetricInfo{ "my.spanevent.count": { Description: "My span event count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-e") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-e")`}, }, }, Metrics: map[string]MetricInfo{ "my.metric.count": { Description: "My metric count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-m") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-m")`}, }, }, DataPoints: map[string]MetricInfo{ "my.datapoint.count": { Description: "My data point count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-d") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-d")`}, }, }, Logs: map[string]MetricInfo{ "my.logrecord.count": { Description: "My log record count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-l") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-l")`}, }, }, }, @@ -162,8 +162,8 @@ func TestLoadConfig(t *testing.T) { "my.span.count": { Description: "My span count.", Conditions: []string{ - `IsMatch(resource.attributes["host.name"], "pod-s") == true`, - `IsMatch(resource.attributes["foo"], "bar-s") == true`, + `IsMatch(resource.attributes["host.name"], "pod-s")`, + `IsMatch(resource.attributes["foo"], "bar-s")`, }, }, }, @@ -171,8 +171,8 @@ func TestLoadConfig(t *testing.T) { "my.spanevent.count": { Description: "My span event count.", Conditions: []string{ - `IsMatch(resource.attributes["host.name"], "pod-e") == true`, - `IsMatch(resource.attributes["foo"], "bar-e") == true`, + `IsMatch(resource.attributes["host.name"], "pod-e")`, + `IsMatch(resource.attributes["foo"], "bar-e")`, }, }, }, @@ -180,8 +180,8 @@ func TestLoadConfig(t *testing.T) { "my.metric.count": { Description: "My metric count.", Conditions: []string{ - `IsMatch(resource.attributes["host.name"], "pod-m") == true`, - `IsMatch(resource.attributes["foo"], "bar-m") == true`, + `IsMatch(resource.attributes["host.name"], "pod-m")`, + `IsMatch(resource.attributes["foo"], "bar-m")`, }, }, }, @@ -189,8 +189,8 @@ func TestLoadConfig(t *testing.T) { "my.datapoint.count": { Description: "My data point count.", Conditions: []string{ - `IsMatch(resource.attributes["host.name"], "pod-d") == true`, - `IsMatch(resource.attributes["foo"], "bar-d") == true`, + `IsMatch(resource.attributes["host.name"], "pod-d")`, + `IsMatch(resource.attributes["foo"], "bar-d")`, }, }, }, @@ -198,8 +198,8 @@ func TestLoadConfig(t *testing.T) { "my.logrecord.count": { Description: "My log record count.", Conditions: []string{ - `IsMatch(resource.attributes["host.name"], "pod-l") == true`, - `IsMatch(resource.attributes["foo"], "bar-l") == true`, + `IsMatch(resource.attributes["host.name"], "pod-l")`, + `IsMatch(resource.attributes["foo"], "bar-l")`, }, }, }, @@ -256,7 +256,7 @@ func TestLoadConfig(t *testing.T) { }, "limited.span.count": { Description: "Limited span count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-s") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-s")`}, Attributes: []AttributeConfig{ { Key: "env", @@ -274,7 +274,7 @@ func TestLoadConfig(t *testing.T) { }, "limited.spanevent.count": { Description: "Limited span event count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-e") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-e")`}, Attributes: []AttributeConfig{ { Key: "env", @@ -291,7 +291,7 @@ func TestLoadConfig(t *testing.T) { Description: "My metric count."}, "limited.metric.count": { Description: "Limited metric count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-m") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-m")`}, }, }, DataPoints: map[string]MetricInfo{ @@ -300,7 +300,7 @@ func TestLoadConfig(t *testing.T) { }, "limited.datapoint.count": { Description: "Limited data point count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-d") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-d")`}, Attributes: []AttributeConfig{ { Key: "env", @@ -318,7 +318,7 @@ func TestLoadConfig(t *testing.T) { }, "limited.logrecord.count": { Description: "Limited log record count.", - Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-l") == true`}, + Conditions: []string{`IsMatch(resource.attributes["host.name"], "pod-l")`}, Attributes: []AttributeConfig{ { Key: "env", diff --git a/connector/countconnector/testdata/config.yaml b/connector/countconnector/testdata/config.yaml index 51accc8b4e48..e81889fd9309 100644 --- a/connector/countconnector/testdata/config.yaml +++ b/connector/countconnector/testdata/config.yaml @@ -36,58 +36,58 @@ my.span.count: description: My span count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-s") == true + - IsMatch(resource.attributes["host.name"], "pod-s") spanevents: my.spanevent.count: description: My span event count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-e") == true + - IsMatch(resource.attributes["host.name"], "pod-e") metrics: my.metric.count: description: My metric count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-m") == true + - IsMatch(resource.attributes["host.name"], "pod-m") datapoints: my.datapoint.count: description: My data point count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-d") == true + - IsMatch(resource.attributes["host.name"], "pod-d") logs: my.logrecord.count: description: My log record count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-l") == true + - IsMatch(resource.attributes["host.name"], "pod-l") count/multiple_condition: spans: my.span.count: description: My span count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-s") == true - - IsMatch(resource.attributes["foo"], "bar-s") == true + - IsMatch(resource.attributes["host.name"], "pod-s") + - IsMatch(resource.attributes["foo"], "bar-s") spanevents: my.spanevent.count: description: My span event count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-e") == true - - IsMatch(resource.attributes["foo"], "bar-e") == true + - IsMatch(resource.attributes["host.name"], "pod-e") + - IsMatch(resource.attributes["foo"], "bar-e") metrics: my.metric.count: description: My metric count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-m") == true - - IsMatch(resource.attributes["foo"], "bar-m") == true + - IsMatch(resource.attributes["host.name"], "pod-m") + - IsMatch(resource.attributes["foo"], "bar-m") datapoints: my.datapoint.count: description: My data point count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-d") == true - - IsMatch(resource.attributes["foo"], "bar-d") == true + - IsMatch(resource.attributes["host.name"], "pod-d") + - IsMatch(resource.attributes["foo"], "bar-d") logs: my.logrecord.count: description: My log record count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-l") == true - - IsMatch(resource.attributes["foo"], "bar-l") == true + - IsMatch(resource.attributes["host.name"], "pod-l") + - IsMatch(resource.attributes["foo"], "bar-l") count/attribute: spans: my.span.count: @@ -120,7 +120,7 @@ limited.span.count: description: Limited span count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-s") == true + - IsMatch(resource.attributes["host.name"], "pod-s") attributes: - key: env - key: component @@ -131,7 +131,7 @@ limited.spanevent.count: description: Limited span event count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-e") == true + - IsMatch(resource.attributes["host.name"], "pod-e") attributes: - key: env - key: component @@ -142,14 +142,14 @@ limited.metric.count: description: Limited metric count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-m") == true + - IsMatch(resource.attributes["host.name"], "pod-m") datapoints: my.datapoint.count: description: My data point count. limited.datapoint.count: description: Limited data point count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-d") == true + - IsMatch(resource.attributes["host.name"], "pod-d") attributes: - key: env - key: component @@ -160,7 +160,7 @@ limited.logrecord.count: description: Limited log record count. conditions: - - IsMatch(resource.attributes["host.name"], "pod-l") == true + - IsMatch(resource.attributes["host.name"], "pod-l") attributes: - key: env - key: component diff --git a/internal/filter/filterottl/filter_test.go b/internal/filter/filterottl/filter_test.go index 3ef62a075fbd..896984c6bc78 100644 --- a/internal/filter/filterottl/filter_test.go +++ b/internal/filter/filterottl/filter_test.go @@ -53,7 +53,7 @@ func Test_NewBoolExprForSpan(t *testing.T) { { name: "With Converter", conditions: []string{ - `IsMatch("test", "pass") == true`, + `IsMatch("test", "pass")`, }, expectedResult: false, }, @@ -95,7 +95,7 @@ func Test_NewBoolExprForSpanEvent(t *testing.T) { { name: "With Converter", conditions: []string{ - `IsMatch("test", "pass") == true`, + `IsMatch("test", "pass")`, }, expectedResult: false, }, @@ -137,7 +137,7 @@ func Test_NewBoolExprForMetric(t *testing.T) { { name: "With Converter", conditions: []string{ - `IsMatch("test", "pass") == true`, + `IsMatch("test", "pass")`, }, expectedResult: false, }, @@ -179,7 +179,7 @@ func Test_NewBoolExprForDataPoint(t *testing.T) { { name: "With Converter", conditions: []string{ - `IsMatch("test", "pass") == true`, + `IsMatch("test", "pass")`, }, expectedResult: false, }, @@ -221,7 +221,7 @@ func Test_NewBoolExprForLog(t *testing.T) { { name: "With Converter", conditions: []string{ - `IsMatch("test", "pass") == true`, + `IsMatch("test", "pass")`, }, expectedResult: false, }, diff --git a/pkg/ottl/README.md b/pkg/ottl/README.md index 118c06b47317..69968c09c309 100644 --- a/pkg/ottl/README.md +++ b/pkg/ottl/README.md @@ -168,6 +168,7 @@ Boolean Expressions can be grouped with parentheses to override evaluation prece Booleans can be either: - A literal boolean value (`true` or `false`). +- A Converter that returns a boolean value (`true` or `false`). - A Comparison, made up of a left Value, an operator, and a right Value. See [Values](#values) for details on what a Value can be. Operators determine how the two Values are compared. @@ -183,8 +184,8 @@ The valid operators are: Booleans can be negated with the `not` keyword such as - `not true` -- `not name == "foo"` -- `not (IsMatch(name, "http_.*") == true and kind > 0)` +- `not name == "foo"` +- `not (IsMatch(name, "http_.*") and kind > 0)` ### Comparison Rules @@ -212,7 +213,7 @@ Examples: - `name == "a name"` - `1 < 2` - `attributes["custom-attr"] != nil` -- `IsMatch(resource.attributes["host.name"], "pod-*") == true` +- `IsMatch(resource.attributes["host.name"], "pod-*")` ## Accessing signal telemetry diff --git a/pkg/ottl/boolean_value.go b/pkg/ottl/boolean_value.go index 9b4fba326d5f..07f5a739c156 100644 --- a/pkg/ottl/boolean_value.go +++ b/pkg/ottl/boolean_value.go @@ -162,10 +162,20 @@ func (p *Parser[K]) newBooleanValueEvaluator(value *booleanValue) (BoolExpr[K], return BoolExpr[K]{}, err } case value.ConstExpr != nil: - if *value.ConstExpr { - boolExpr = BoolExpr[K]{alwaysTrue[K]} - } else { - boolExpr = BoolExpr[K]{alwaysFalse[K]} + switch { + case value.ConstExpr.Boolean != nil: + if *value.ConstExpr.Boolean { + boolExpr = BoolExpr[K]{alwaysTrue[K]} + } else { + boolExpr = BoolExpr[K]{alwaysFalse[K]} + } + case value.ConstExpr.Converter != nil: + boolExpr, err = p.newConverterEvaluator(value) + if err != nil { + return BoolExpr[K]{}, err + } + default: + return BoolExpr[K]{}, fmt.Errorf("unhandled boolean operation %v", value) } case value.SubExpr != nil: boolExpr, err = p.newBoolExpr(value.SubExpr) @@ -181,3 +191,25 @@ func (p *Parser[K]) newBooleanValueEvaluator(value *booleanValue) (BoolExpr[K], } return boolExpr, nil } + +func (p *Parser[K]) newConverterEvaluator(val *booleanValue) (BoolExpr[K], error) { + getter, err := p.newGetter(value{ + Literal: &mathExprLiteral{ + Converter: val.ConstExpr.Converter, + }, + }) + if err != nil { + return BoolExpr[K]{}, err + } + return BoolExpr[K]{func(ctx context.Context, tCtx K) (bool, error) { + result, err := getter.Get(ctx, tCtx) + if err != nil { + return false, err + } + boolResult, ok := result.(bool) + if !ok { + return false, fmt.Errorf("value returned from Converter in constant expression must be bool but got %T", result) + } + return boolResult, nil + }}, nil +} diff --git a/pkg/ottl/boolean_value_test.go b/pkg/ottl/boolean_value_test.go index d31548a01541..7e5648d86cdd 100644 --- a/pkg/ottl/boolean_value_test.go +++ b/pkg/ottl/boolean_value_test.go @@ -163,9 +163,24 @@ func Test_newConditionEvaluator_invalid(t *testing.T) { } } +func True[K any]() (ExprFunc[K], error) { + return func(ctx context.Context, tCtx K) (interface{}, error) { + return true, nil + }, nil +} +func False[K any]() (ExprFunc[K], error) { + return func(ctx context.Context, tCtx K) (interface{}, error) { + return false, nil + }, nil +} + func Test_newBooleanExpressionEvaluator(t *testing.T) { + functions := defaultFunctionsForTests() + functions["True"] = True[any] + functions["False"] = False[any] + p, _ := NewParser[any]( - defaultFunctionsForTests(), + functions, testParsePath, componenttest.NewNopTelemetrySettings(), WithEnumParser[any](testParseEnum), @@ -180,13 +195,17 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -197,13 +216,17 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -214,19 +237,25 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -237,7 +266,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -245,7 +276,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -256,7 +289,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, Right: []*opOrTerm{ @@ -264,7 +299,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -275,7 +312,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, Right: []*opOrTerm{ @@ -283,7 +322,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -294,13 +335,17 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -310,7 +355,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -321,7 +368,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { @@ -330,7 +379,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { SubExpr: &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -338,7 +389,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -354,8 +407,10 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(false), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -364,8 +419,10 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(true), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -392,7 +449,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { @@ -402,7 +461,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { SubExpr: &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -410,7 +471,9 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -426,15 +489,19 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { &booleanExpression{ Left: &term{ Left: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(true), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(false), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -444,8 +511,36 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(true), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, + }, + }, + }, + }, + }, + }, + {"n", true, + &booleanExpression{ + Left: &term{ + Left: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "True", + }, + }, + }, + }, + }, + }, + {"o", false, + &booleanExpression{ + Left: &term{ + Left: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "False", }, }, }, @@ -463,3 +558,42 @@ func Test_newBooleanExpressionEvaluator(t *testing.T) { }) } } + +func Test_newBooleanExpressionEvaluator_invalid(t *testing.T) { + functions := map[string]interface{}{"Hello": hello[interface{}]} + + p, _ := NewParser[any]( + functions, + testParsePath, + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + ) + + tests := []struct { + name string + expr *booleanExpression + }{ + { + name: "Converter doesn't return bool", + expr: &booleanExpression{ + Left: &term{ + Left: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "Hello", + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evaluator, err := p.newBoolExpr(tt.expr) + assert.NoError(t, err) + _, err = evaluator.Eval(context.Background(), nil) + assert.Error(t, err) + }) + } +} diff --git a/pkg/ottl/grammar.go b/pkg/ottl/grammar.go index 0d9a170d0baa..cfd8d3b406f6 100644 --- a/pkg/ottl/grammar.go +++ b/pkg/ottl/grammar.go @@ -43,13 +43,18 @@ func (p *parsedStatement) checkForCustomError() error { return nil } +type constExpr struct { + Boolean *boolean `parser:"( @Boolean"` + Converter *converter `parser:"| @@ )"` +} + // booleanValue represents something that evaluates to a boolean -- // either an equality or inequality, explicit true or false, or // a parenthesized subexpression. type booleanValue struct { Negation *string `parser:"@OpNot?"` Comparison *comparison `parser:"( @@"` - ConstExpr *boolean `parser:"| @Boolean"` + ConstExpr *constExpr `parser:"| @@"` SubExpr *booleanExpression `parser:"| '(' @@ ')' )"` } diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index a697fa42090d..9aae0e6f6edc 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -821,7 +821,9 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }), @@ -831,13 +833,17 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -849,19 +855,25 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -873,7 +885,9 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -881,7 +895,9 @@ func Test_parseWhere(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -893,13 +909,17 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -909,7 +929,9 @@ func Test_parseWhere(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -924,13 +946,17 @@ func Test_parseWhere(t *testing.T) { SubExpr: &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, }, @@ -943,7 +969,9 @@ func Test_parseWhere(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -955,7 +983,9 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, Right: []*opAndBooleanValue{ { @@ -964,7 +994,9 @@ func Test_parseWhere(t *testing.T) { SubExpr: &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -972,7 +1004,9 @@ func Test_parseWhere(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -1091,14 +1125,18 @@ func Test_parseWhere(t *testing.T) { expected: setNameTest(&booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, Right: []*opAndBooleanValue{ { Operator: "and", Value: &booleanValue{ - Negation: ottltest.Strp("not"), - ConstExpr: booleanp(false), + Negation: ottltest.Strp("not"), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -1141,7 +1179,9 @@ func Test_parseWhere(t *testing.T) { SubExpr: &booleanExpression{ Left: &term{ Left: &booleanValue{ - ConstExpr: booleanp(true), + ConstExpr: &constExpr{ + Boolean: booleanp(true), + }, }, }, Right: []*opOrTerm{ @@ -1149,7 +1189,9 @@ func Test_parseWhere(t *testing.T) { Operator: "or", Term: &term{ Left: &booleanValue{ - ConstExpr: booleanp(false), + ConstExpr: &constExpr{ + Boolean: booleanp(false), + }, }, }, }, @@ -1159,6 +1201,46 @@ func Test_parseWhere(t *testing.T) { }, }), }, + { + statement: `True()`, + expected: setNameTest(&booleanExpression{ + Left: &term{ + Left: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "True", + }, + }, + }, + }, + }), + }, + { + statement: `True() and False()`, + expected: setNameTest(&booleanExpression{ + Left: &term{ + Left: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "True", + }, + }, + }, + Right: []*opAndBooleanValue{ + { + Operator: "and", + Value: &booleanValue{ + ConstExpr: &constExpr{ + Converter: &converter{ + Function: "False", + }, + }, + }, + }, + }, + }, + }), + }, } // create a test name that doesn't confuse vscode so we can rerun tests with one click diff --git a/processor/filterprocessor/README.md b/processor/filterprocessor/README.md index 5430e58f467b..897877192979 100644 --- a/processor/filterprocessor/README.md +++ b/processor/filterprocessor/README.md @@ -371,7 +371,7 @@ processors: - 'name == "app_3"' spanevent: - 'attributes["grpc"] == true' - - 'IsMatch(name, ".*grpc.*") == true' + - 'IsMatch(name, ".*grpc.*")' metrics: metric: - 'name == "my.metric" and resource.attributes["my_label"] == "abc123"' @@ -381,7 +381,7 @@ processors: - 'resource.attributes["service.name"] == "my_service_name"' logs: log_record: - - 'IsMatch(body, ".*password.*") == true' + - 'IsMatch(body, ".*password.*")' - 'severity_number < SEVERITY_NUMBER_WARN' ``` diff --git a/processor/filterprocessor/logs_test.go b/processor/filterprocessor/logs_test.go index 1d3b9327d48a..5a790aaf151b 100644 --- a/processor/filterprocessor/logs_test.go +++ b/processor/filterprocessor/logs_test.go @@ -734,7 +734,7 @@ func TestFilterLogProcessorWithOTTL(t *testing.T) { { name: "drop everything by dropping all logs", conditions: []string{ - `IsMatch(body, "operation.*") == true`, + `IsMatch(body, "operation.*")`, }, filterEverything: true, errorMode: ottl.IgnoreError, @@ -742,8 +742,8 @@ func TestFilterLogProcessorWithOTTL(t *testing.T) { { name: "multiple conditions", conditions: []string{ - `IsMatch(body, "wrong name") == true`, - `IsMatch(body, "operation.*") == true`, + `IsMatch(body, "wrong name")`, + `IsMatch(body, "operation.*")`, }, filterEverything: true, errorMode: ottl.IgnoreError, diff --git a/processor/filterprocessor/metrics_test.go b/processor/filterprocessor/metrics_test.go index c11043866311..4e453daa3213 100644 --- a/processor/filterprocessor/metrics_test.go +++ b/processor/filterprocessor/metrics_test.go @@ -547,7 +547,7 @@ func TestFilterMetricProcessorWithOTTL(t *testing.T) { name: "drop everything by dropping all metrics", conditions: MetricFilters{ MetricConditions: []string{ - `IsMatch(name, "operation.*") == true`, + `IsMatch(name, "operation.*")`, }, }, filterEverything: true, diff --git a/processor/filterprocessor/traces_test.go b/processor/filterprocessor/traces_test.go index d3210dce323f..51d8795ee1e2 100644 --- a/processor/filterprocessor/traces_test.go +++ b/processor/filterprocessor/traces_test.go @@ -228,7 +228,7 @@ func TestFilterTraceProcessorWithOTTL(t *testing.T) { name: "drop everything by dropping all spans", conditions: TraceFilters{ SpanConditions: []string{ - `IsMatch(name, "operation.*") == true`, + `IsMatch(name, "operation.*")`, }, }, filterEverything: true, diff --git a/processor/routingprocessor/README.md b/processor/routingprocessor/README.md index 6c561c6c78e0..ace80d9081a3 100644 --- a/processor/routingprocessor/README.md +++ b/processor/routingprocessor/README.md @@ -74,7 +74,7 @@ processors: table: - statement: route() where resource.attributes["X-Tenant"] == "acme" exporters: [jaeger/acme] - - statement: delete_key(resource.attributes, "X-Tenant") where IsMatch(resource.attributes["X-Tenant"], ".*corp") == true + - statement: delete_key(resource.attributes, "X-Tenant") where IsMatch(resource.attributes["X-Tenant"], ".*corp") exporters: [jaeger/ecorp] exporters: diff --git a/processor/routingprocessor/config_test.go b/processor/routingprocessor/config_test.go index 196792f913ff..cb6fb3010d6d 100644 --- a/processor/routingprocessor/config_test.go +++ b/processor/routingprocessor/config_test.go @@ -120,7 +120,7 @@ func TestLoadConfig(t *testing.T) { Exporters: []string{"jaeger/acme"}, }, { - Statement: "delete_key(resource.attributes, \"X-Tenant\") where IsMatch(resource.attributes[\"X-Tenant\"], \".*corp\") == true", + Statement: "delete_key(resource.attributes, \"X-Tenant\") where IsMatch(resource.attributes[\"X-Tenant\"], \".*corp\")", Exporters: []string{"jaeger/ecorp"}, }, }, diff --git a/processor/routingprocessor/logs_test.go b/processor/routingprocessor/logs_test.go index e76a6277ece7..287afe99800a 100644 --- a/processor/routingprocessor/logs_test.go +++ b/processor/routingprocessor/logs_test.go @@ -268,11 +268,11 @@ func TestLogsAreCorrectlySplitPerResourceAttributeWithOTTL(t *testing.T) { DefaultExporters: []string{"otlp"}, Table: []RoutingTableItem{ { - Statement: `route() where IsMatch(resource.attributes["X-Tenant"], ".*acme") == true`, + Statement: `route() where IsMatch(resource.attributes["X-Tenant"], ".*acme")`, Exporters: []string{"otlp/1"}, }, { - Statement: `route() where IsMatch(resource.attributes["X-Tenant"], "_acme") == true`, + Statement: `route() where IsMatch(resource.attributes["X-Tenant"], "_acme")`, Exporters: []string{"otlp/2"}, }, }, diff --git a/processor/routingprocessor/testdata/config.yaml b/processor/routingprocessor/testdata/config.yaml index 10594105d29c..18d42e3e17ff 100644 --- a/processor/routingprocessor/testdata/config.yaml +++ b/processor/routingprocessor/testdata/config.yaml @@ -15,5 +15,5 @@ routing/ottl: table: - statement: route() where resource.attributes["X-Tenant"] == "acme" exporters: [jaeger/acme] - - statement: delete_key(resource.attributes, "X-Tenant") where IsMatch(resource.attributes["X-Tenant"], ".*corp") == true + - statement: delete_key(resource.attributes, "X-Tenant") where IsMatch(resource.attributes["X-Tenant"], ".*corp") exporters: [jaeger/ecorp] diff --git a/processor/transformprocessor/internal/logs/processor_test.go b/processor/transformprocessor/internal/logs/processor_test.go index 6ed1d6ffa276..bc618ece008e 100644 --- a/processor/transformprocessor/internal/logs/processor_test.go +++ b/processor/transformprocessor/internal/logs/processor_test.go @@ -210,7 +210,7 @@ func Test_ProcessLogs_LogContext(t *testing.T) { }, }, { - statement: `set(attributes["test"], "pass") where IsMatch(body, "operation[AC]") == true`, + statement: `set(attributes["test"], "pass") where IsMatch(body, "operation[AC]")`, want: func(td plog.Logs) { td.ResourceLogs().At(0).ScopeLogs().At(0).LogRecords().At(0).Attributes().PutStr("test", "pass") }, diff --git a/processor/transformprocessor/internal/metrics/processor_test.go b/processor/transformprocessor/internal/metrics/processor_test.go index 33a5eb2ab355..3c04ff616719 100644 --- a/processor/transformprocessor/internal/metrics/processor_test.go +++ b/processor/transformprocessor/internal/metrics/processor_test.go @@ -368,7 +368,7 @@ func Test_ProcessMetrics_DataPointContext(t *testing.T) { }, }, { - statements: []string{`set(attributes["test"], "pass") where IsMatch(metric.name, "operation[AC]") == true`}, + statements: []string{`set(attributes["test"], "pass") where IsMatch(metric.name, "operation[AC]")`}, want: func(td pmetric.Metrics) { td.ResourceMetrics().At(0).ScopeMetrics().At(0).Metrics().At(0).Sum().DataPoints().At(0).Attributes().PutStr("test", "pass") td.ResourceMetrics().At(0).ScopeMetrics().At(0).Metrics().At(0).Sum().DataPoints().At(1).Attributes().PutStr("test", "pass") diff --git a/processor/transformprocessor/internal/traces/processor_test.go b/processor/transformprocessor/internal/traces/processor_test.go index ba384c37ee34..370aba41b05c 100644 --- a/processor/transformprocessor/internal/traces/processor_test.go +++ b/processor/transformprocessor/internal/traces/processor_test.go @@ -217,7 +217,7 @@ func Test_ProcessTraces_TraceContext(t *testing.T) { }, }, { - statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]") == true`, + statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, want: func(td ptrace.Traces) { td.ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).Attributes().PutStr("test", "pass") }, From 7c28771f6b0fb3bafddf541f1c96039e79ecf5ae Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 17 Apr 2023 10:47:14 -0600 Subject: [PATCH 2/4] Extract common logic --- pkg/ottl/boolean_value.go | 10 +++------- pkg/ottl/expression.go | 24 ++++++++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/ottl/boolean_value.go b/pkg/ottl/boolean_value.go index 07f5a739c156..14dbf02fd2ad 100644 --- a/pkg/ottl/boolean_value.go +++ b/pkg/ottl/boolean_value.go @@ -170,7 +170,7 @@ func (p *Parser[K]) newBooleanValueEvaluator(value *booleanValue) (BoolExpr[K], boolExpr = BoolExpr[K]{alwaysFalse[K]} } case value.ConstExpr.Converter != nil: - boolExpr, err = p.newConverterEvaluator(value) + boolExpr, err = p.newConverterEvaluator(*value.ConstExpr.Converter) if err != nil { return BoolExpr[K]{}, err } @@ -192,12 +192,8 @@ func (p *Parser[K]) newBooleanValueEvaluator(value *booleanValue) (BoolExpr[K], return boolExpr, nil } -func (p *Parser[K]) newConverterEvaluator(val *booleanValue) (BoolExpr[K], error) { - getter, err := p.newGetter(value{ - Literal: &mathExprLiteral{ - Converter: val.ConstExpr.Converter, - }, - }) +func (p *Parser[K]) newConverterEvaluator(c converter) (BoolExpr[K], error) { + getter, err := p.newGetterFromConverter(c) if err != nil { return BoolExpr[K]{}, err } diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index fa64bbf473f1..0ce98202c86d 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -209,16 +209,7 @@ func (p *Parser[K]) newGetter(val value) (Getter[K], error) { return p.pathParser(eL.Path) } if eL.Converter != nil { - call, err := p.newFunctionCall(invocation{ - Function: eL.Converter.Function, - Arguments: eL.Converter.Arguments, - }) - if err != nil { - return nil, err - } - return &exprGetter[K]{ - expr: call, - }, nil + return p.newGetterFromConverter(*eL.Converter) } } @@ -240,3 +231,16 @@ func (p *Parser[K]) newGetter(val value) (Getter[K], error) { } return p.evaluateMathExpression(val.MathExpression) } + +func (p *Parser[K]) newGetterFromConverter(c converter) (Getter[K], error) { + call, err := p.newFunctionCall(invocation{ + Function: c.Function, + Arguments: c.Arguments, + }) + if err != nil { + return nil, err + } + return &exprGetter[K]{ + expr: call, + }, nil +} From af5ce7c9b29c9a4b314822aa4524d7904e8794ec Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 17 Apr 2023 10:47:41 -0600 Subject: [PATCH 3/4] Update .chloggen/ottl-allow-converters-as-constbool.yaml Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> --- .chloggen/ottl-allow-converters-as-constbool.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/ottl-allow-converters-as-constbool.yaml b/.chloggen/ottl-allow-converters-as-constbool.yaml index ddd560930a16..b8401cb20ef7 100755 --- a/.chloggen/ottl-allow-converters-as-constbool.yaml +++ b/.chloggen/ottl-allow-converters-as-constbool.yaml @@ -5,7 +5,7 @@ change_type: enhancement component: pkg/ottl # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Allow using Converts as constant boolean expressions +note: Allow using Converters as constant boolean expressions # One or more tracking issues related to the change issues: [20911] From e59d17d33a127ad0b8ca18f032fb78a02109e543 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 17 Apr 2023 11:51:07 -0600 Subject: [PATCH 4/4] Fix lint --- pkg/ottl/expression.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index 0ce98202c86d..1bedb60a4eb3 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -233,10 +233,7 @@ func (p *Parser[K]) newGetter(val value) (Getter[K], error) { } func (p *Parser[K]) newGetterFromConverter(c converter) (Getter[K], error) { - call, err := p.newFunctionCall(invocation{ - Function: c.Function, - Arguments: c.Arguments, - }) + call, err := p.newFunctionCall(invocation(c)) if err != nil { return nil, err }