From df99337658765eb936c400b8e7e4cd622ce30701 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Wed, 9 Oct 2024 13:43:49 +0200 Subject: [PATCH 1/4] Add OTTL parser utility to rewrite statements appending missing paths context --- ...-add-statement-context-append-utility.yaml | 27 ++++ pkg/ottl/paths.go | 54 +++++++ pkg/ottl/paths_test.go | 141 ++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 .chloggen/ottl-add-statement-context-append-utility.yaml diff --git a/.chloggen/ottl-add-statement-context-append-utility.yaml b/.chloggen/ottl-add-statement-context-append-utility.yaml new file mode 100644 index 000000000000..3852f6a3edef --- /dev/null +++ b/.chloggen/ottl-add-statement-context-append-utility.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# 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: Add OTTL parser utility to rewrite statements appending missing paths context + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [29017] + +# (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: "The `ottl.Parser[K]` has a new function `AppendStatementPathsContext`" + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] diff --git a/pkg/ottl/paths.go b/pkg/ottl/paths.go index dbb66ee7c994..a7bbed71b898 100644 --- a/pkg/ottl/paths.go +++ b/pkg/ottl/paths.go @@ -2,6 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 package ottl // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" +import ( + "fmt" + "strings" +) // grammarPathVisitor is used to extract all path from a parsedStatement or booleanExpression type grammarPathVisitor struct { @@ -30,3 +34,53 @@ func getBooleanExpressionPaths(be *booleanExpression) []path { be.accept(visitor) return visitor.paths } + +// AppendStatementPathsContext changes the given OTTL statement adding the context name prefix +// to all context-less paths. No modifications are performed for paths which [Path.Context] +// value matches any WithPathContextNames value. +// The context argument must be valid WithPathContextNames value, otherwise an error is returned. +func (p *Parser[K]) AppendStatementPathsContext(context string, statement string) (string, error) { + if _, ok := p.pathContextNames[context]; !ok { + return statement, fmt.Errorf(`unknown context "%s" for parser %T, valid options are: %s`, context, p, p.buildPathContextNamesText("")) + } + parsed, err := parseStatement(statement) + if err != nil { + return "", err + } + paths := getParsedStatementPaths(parsed) + if len(paths) == 0 { + return statement, nil + } + + var missingContextOffsets []int + for _, it := range paths { + if _, ok := p.pathContextNames[it.Context]; !ok { + missingContextOffsets = append(missingContextOffsets, it.Pos.Offset) + } + } + + return writeStatementWithPathsContext(context, statement, missingContextOffsets), nil +} + +func writeStatementWithPathsContext(context string, statement string, offsets []int) string { + if len(offsets) == 0 { + return statement + } + + contextPrefix := context + "." + var sb strings.Builder + sb.Grow(len(statement) + (len(contextPrefix) * len(offsets))) + + left := 0 + for i, offset := range offsets { + sb.WriteString(statement[left:offset]) + sb.WriteString(contextPrefix) + if i+1 >= len(offsets) { + sb.WriteString(statement[offset:]) + } else { + left = offset + } + } + + return sb.String() +} diff --git a/pkg/ottl/paths_test.go b/pkg/ottl/paths_test.go index 9f31dda15718..4122a06832a4 100644 --- a/pkg/ottl/paths_test.go +++ b/pkg/ottl/paths_test.go @@ -4,10 +4,13 @@ package ottl import ( + "context" "testing" "github.com/alecthomas/participle/v2/lexer" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/component/componenttest" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" ) @@ -448,3 +451,141 @@ func Test_getBooleanExpressionPaths(t *testing.T) { paths := getBooleanExpressionPaths(c) require.Equal(t, expected, paths) } + +func Test_AppendStatementPathsContext_InvalidStatement(t *testing.T) { + ps, err := NewParser( + CreateFactoryMap[any](), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any]([]string{"foo", "bar"}), + ) + require.NoError(t, err) + _, err = ps.AppendStatementPathsContext("foo", "this is invalid") + require.ErrorContains(t, err, `statement has invalid syntax`) +} + +func Test_AppendStatementPathsContext_InvalidContext(t *testing.T) { + ps, err := NewParser( + CreateFactoryMap[any](), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any]([]string{"foo", "bar"}), + ) + require.NoError(t, err) + _, err = ps.AppendStatementPathsContext("foobar", "set(foo, 1)") + require.ErrorContains(t, err, `unknown context "foobar" for parser`) +} + +func Test_AppendStatementPathsContext_Success(t *testing.T) { + type mockSetArguments[K any] struct { + Target Setter[K] + Value Getter[K] + } + + mockSetFactory := NewFactory("set", &mockSetArguments[any]{}, func(_ FunctionContext, _ Arguments) (ExprFunc[any], error) { + return func(_ context.Context, _ any) (any, error) { + return nil, nil + }, nil + }) + + tests := []struct { + name string + statement string + context string + pathContextNames []string + expected string + }{ + { + name: "no paths", + statement: `set("foo", 1)`, + context: "bar", + }, + { + name: "single path with context", + statement: `set(span.value, 1)`, + context: "span", + }, + { + name: "single path without context", + statement: "set(value, 1)", + expected: "set(span.value, 1)", + context: "span", + }, + { + name: "single path with context - multiple context names", + statement: "set(span.value, 1)", + pathContextNames: []string{"spanevent", "span"}, + context: "spanevent", + }, + { + name: "multiple paths with the same context", + statement: `set(span.value, 1) where span.attributes["foo"] == "foo" and span.id == 1`, + pathContextNames: []string{"another", "span"}, + context: "another", + }, + { + name: "multiple paths with different contexts", + statement: `set(another.value, 1) where span.attributes["foo"] == "foo" and another.id == 1`, + pathContextNames: []string{"another", "span"}, + context: "another", + }, + { + name: "multiple paths with and without contexts", + statement: `set(value, 1) where span.attributes["foo"] == "foo" and id == 1`, + expected: `set(spanevent.value, 1) where span.attributes["foo"] == "foo" and spanevent.id == 1`, + pathContextNames: []string{"spanevent", "span"}, + context: "spanevent", + }, + { + name: "multiple paths without context", + statement: `set(value, 1) where name == attributes["foo.name"]`, + expected: `set(span.value, 1) where span.name == span.attributes["foo.name"]`, + context: "span", + }, + { + name: "function path parameter without context", + statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, + context: "log", + expected: `set(log.attributes["test"], "pass") where IsMatch(log.name, "operation[AC]")`, + }, + { + name: "function path parameter with context", + statement: `set(attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, + context: "log", + pathContextNames: []string{"log", "resource"}, + expected: `set(log.attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if len(tt.pathContextNames) == 0 { + tt.pathContextNames = append(tt.pathContextNames, tt.context) + } + + ps, err := NewParser( + CreateFactoryMap[any](mockSetFactory), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any](tt.pathContextNames), + ) + + require.NoError(t, err) + require.NotNil(t, ps) + + var expected string + if tt.expected != "" { + expected = tt.expected + } else { + expected = tt.statement + } + + result, err := ps.AppendStatementPathsContext(tt.context, tt.statement) + require.NoError(t, err) + assert.Equal(t, expected, result) + }) + } +} From 63b53b58be87fb42b303f9d23f9279c89f151554 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Wed, 30 Oct 2024 15:39:05 +0100 Subject: [PATCH 2/4] Move AppendStatementPathsContext to parser.go --- pkg/ottl/parser.go | 51 +++++++++++++++ pkg/ottl/parser_test.go | 138 +++++++++++++++++++++++++++++++++++++++ pkg/ottl/paths.go | 54 --------------- pkg/ottl/paths_test.go | 141 ---------------------------------------- 4 files changed, 189 insertions(+), 195 deletions(-) diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index ed8457603f7c..9e5c27f1c40b 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "strings" "github.com/alecthomas/participle/v2" "go.opentelemetry.io/collector/component" @@ -195,6 +196,33 @@ func (p *Parser[K]) ParseCondition(condition string) (*Condition[K], error) { }, nil } +// AppendStatementPathsContext changes the given OTTL statement adding the context name prefix +// to all context-less paths. No modifications are performed for paths which [Path.Context] +// value matches any WithPathContextNames value. +// The context argument must be valid WithPathContextNames value, otherwise an error is returned. +func (p *Parser[K]) AppendStatementPathsContext(context string, statement string) (string, error) { + if _, ok := p.pathContextNames[context]; !ok { + return statement, fmt.Errorf(`unknown context "%s" for parser %T, valid options are: %s`, context, p, p.buildPathContextNamesText("")) + } + parsed, err := parseStatement(statement) + if err != nil { + return "", err + } + paths := getParsedStatementPaths(parsed) + if len(paths) == 0 { + return statement, nil + } + + var missingContextOffsets []int + for _, it := range paths { + if _, ok := p.pathContextNames[it.Context]; !ok { + missingContextOffsets = append(missingContextOffsets, it.Pos.Offset) + } + } + + return writeStatementWithPathsContext(context, statement, missingContextOffsets), nil +} + var parser = newParser[parsedStatement]() var conditionParser = newParser[booleanExpression]() @@ -226,6 +254,29 @@ func parseCondition(raw string) (*booleanExpression, error) { return parsed, nil } +func writeStatementWithPathsContext(context string, statement string, offsets []int) string { + if len(offsets) == 0 { + return statement + } + + contextPrefix := context + "." + var sb strings.Builder + sb.Grow(len(statement) + (len(contextPrefix) * len(offsets))) + + left := 0 + for i, offset := range offsets { + sb.WriteString(statement[left:offset]) + sb.WriteString(contextPrefix) + if i+1 >= len(offsets) { + sb.WriteString(statement[offset:]) + } else { + left = offset + } + } + + return sb.String() +} + // newParser returns a parser that can be used to read a string into a parsedStatement. An error will be returned if the string // is not formatted for the DSL. func newParser[G any]() *participle.Parser[G] { diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index e8bb93af6f9b..ce4659f84df9 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -2714,3 +2714,141 @@ func Test_ConditionSequence_Eval_Error(t *testing.T) { }) } } + +func Test_AppendStatementPathsContext_InvalidStatement(t *testing.T) { + ps, err := NewParser( + CreateFactoryMap[any](), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any]([]string{"foo", "bar"}), + ) + require.NoError(t, err) + _, err = ps.AppendStatementPathsContext("foo", "this is invalid") + require.ErrorContains(t, err, `statement has invalid syntax`) +} + +func Test_AppendStatementPathsContext_InvalidContext(t *testing.T) { + ps, err := NewParser( + CreateFactoryMap[any](), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any]([]string{"foo", "bar"}), + ) + require.NoError(t, err) + _, err = ps.AppendStatementPathsContext("foobar", "set(foo, 1)") + require.ErrorContains(t, err, `unknown context "foobar" for parser`) +} + +func Test_AppendStatementPathsContext_Success(t *testing.T) { + type mockSetArguments[K any] struct { + Target Setter[K] + Value Getter[K] + } + + mockSetFactory := NewFactory("set", &mockSetArguments[any]{}, func(_ FunctionContext, _ Arguments) (ExprFunc[any], error) { + return func(_ context.Context, _ any) (any, error) { + return nil, nil + }, nil + }) + + tests := []struct { + name string + statement string + context string + pathContextNames []string + expected string + }{ + { + name: "no paths", + statement: `set("foo", 1)`, + context: "bar", + }, + { + name: "single path with context", + statement: `set(span.value, 1)`, + context: "span", + }, + { + name: "single path without context", + statement: "set(value, 1)", + expected: "set(span.value, 1)", + context: "span", + }, + { + name: "single path with context - multiple context names", + statement: "set(span.value, 1)", + pathContextNames: []string{"spanevent", "span"}, + context: "spanevent", + }, + { + name: "multiple paths with the same context", + statement: `set(span.value, 1) where span.attributes["foo"] == "foo" and span.id == 1`, + pathContextNames: []string{"another", "span"}, + context: "another", + }, + { + name: "multiple paths with different contexts", + statement: `set(another.value, 1) where span.attributes["foo"] == "foo" and another.id == 1`, + pathContextNames: []string{"another", "span"}, + context: "another", + }, + { + name: "multiple paths with and without contexts", + statement: `set(value, 1) where span.attributes["foo"] == "foo" and id == 1`, + expected: `set(spanevent.value, 1) where span.attributes["foo"] == "foo" and spanevent.id == 1`, + pathContextNames: []string{"spanevent", "span"}, + context: "spanevent", + }, + { + name: "multiple paths without context", + statement: `set(value, 1) where name == attributes["foo.name"]`, + expected: `set(span.value, 1) where span.name == span.attributes["foo.name"]`, + context: "span", + }, + { + name: "function path parameter without context", + statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, + context: "log", + expected: `set(log.attributes["test"], "pass") where IsMatch(log.name, "operation[AC]")`, + }, + { + name: "function path parameter with context", + statement: `set(attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, + context: "log", + pathContextNames: []string{"log", "resource"}, + expected: `set(log.attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if len(tt.pathContextNames) == 0 { + tt.pathContextNames = append(tt.pathContextNames, tt.context) + } + + ps, err := NewParser( + CreateFactoryMap[any](mockSetFactory), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any](tt.pathContextNames), + ) + + require.NoError(t, err) + require.NotNil(t, ps) + + var expected string + if tt.expected != "" { + expected = tt.expected + } else { + expected = tt.statement + } + + result, err := ps.AppendStatementPathsContext(tt.context, tt.statement) + require.NoError(t, err) + assert.Equal(t, expected, result) + }) + } +} diff --git a/pkg/ottl/paths.go b/pkg/ottl/paths.go index a7bbed71b898..dbb66ee7c994 100644 --- a/pkg/ottl/paths.go +++ b/pkg/ottl/paths.go @@ -2,10 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 package ottl // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" -import ( - "fmt" - "strings" -) // grammarPathVisitor is used to extract all path from a parsedStatement or booleanExpression type grammarPathVisitor struct { @@ -34,53 +30,3 @@ func getBooleanExpressionPaths(be *booleanExpression) []path { be.accept(visitor) return visitor.paths } - -// AppendStatementPathsContext changes the given OTTL statement adding the context name prefix -// to all context-less paths. No modifications are performed for paths which [Path.Context] -// value matches any WithPathContextNames value. -// The context argument must be valid WithPathContextNames value, otherwise an error is returned. -func (p *Parser[K]) AppendStatementPathsContext(context string, statement string) (string, error) { - if _, ok := p.pathContextNames[context]; !ok { - return statement, fmt.Errorf(`unknown context "%s" for parser %T, valid options are: %s`, context, p, p.buildPathContextNamesText("")) - } - parsed, err := parseStatement(statement) - if err != nil { - return "", err - } - paths := getParsedStatementPaths(parsed) - if len(paths) == 0 { - return statement, nil - } - - var missingContextOffsets []int - for _, it := range paths { - if _, ok := p.pathContextNames[it.Context]; !ok { - missingContextOffsets = append(missingContextOffsets, it.Pos.Offset) - } - } - - return writeStatementWithPathsContext(context, statement, missingContextOffsets), nil -} - -func writeStatementWithPathsContext(context string, statement string, offsets []int) string { - if len(offsets) == 0 { - return statement - } - - contextPrefix := context + "." - var sb strings.Builder - sb.Grow(len(statement) + (len(contextPrefix) * len(offsets))) - - left := 0 - for i, offset := range offsets { - sb.WriteString(statement[left:offset]) - sb.WriteString(contextPrefix) - if i+1 >= len(offsets) { - sb.WriteString(statement[offset:]) - } else { - left = offset - } - } - - return sb.String() -} diff --git a/pkg/ottl/paths_test.go b/pkg/ottl/paths_test.go index 4122a06832a4..9f31dda15718 100644 --- a/pkg/ottl/paths_test.go +++ b/pkg/ottl/paths_test.go @@ -4,13 +4,10 @@ package ottl import ( - "context" "testing" "github.com/alecthomas/participle/v2/lexer" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/collector/component/componenttest" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" ) @@ -451,141 +448,3 @@ func Test_getBooleanExpressionPaths(t *testing.T) { paths := getBooleanExpressionPaths(c) require.Equal(t, expected, paths) } - -func Test_AppendStatementPathsContext_InvalidStatement(t *testing.T) { - ps, err := NewParser( - CreateFactoryMap[any](), - testParsePath[any], - componenttest.NewNopTelemetrySettings(), - WithEnumParser[any](testParseEnum), - WithPathContextNames[any]([]string{"foo", "bar"}), - ) - require.NoError(t, err) - _, err = ps.AppendStatementPathsContext("foo", "this is invalid") - require.ErrorContains(t, err, `statement has invalid syntax`) -} - -func Test_AppendStatementPathsContext_InvalidContext(t *testing.T) { - ps, err := NewParser( - CreateFactoryMap[any](), - testParsePath[any], - componenttest.NewNopTelemetrySettings(), - WithEnumParser[any](testParseEnum), - WithPathContextNames[any]([]string{"foo", "bar"}), - ) - require.NoError(t, err) - _, err = ps.AppendStatementPathsContext("foobar", "set(foo, 1)") - require.ErrorContains(t, err, `unknown context "foobar" for parser`) -} - -func Test_AppendStatementPathsContext_Success(t *testing.T) { - type mockSetArguments[K any] struct { - Target Setter[K] - Value Getter[K] - } - - mockSetFactory := NewFactory("set", &mockSetArguments[any]{}, func(_ FunctionContext, _ Arguments) (ExprFunc[any], error) { - return func(_ context.Context, _ any) (any, error) { - return nil, nil - }, nil - }) - - tests := []struct { - name string - statement string - context string - pathContextNames []string - expected string - }{ - { - name: "no paths", - statement: `set("foo", 1)`, - context: "bar", - }, - { - name: "single path with context", - statement: `set(span.value, 1)`, - context: "span", - }, - { - name: "single path without context", - statement: "set(value, 1)", - expected: "set(span.value, 1)", - context: "span", - }, - { - name: "single path with context - multiple context names", - statement: "set(span.value, 1)", - pathContextNames: []string{"spanevent", "span"}, - context: "spanevent", - }, - { - name: "multiple paths with the same context", - statement: `set(span.value, 1) where span.attributes["foo"] == "foo" and span.id == 1`, - pathContextNames: []string{"another", "span"}, - context: "another", - }, - { - name: "multiple paths with different contexts", - statement: `set(another.value, 1) where span.attributes["foo"] == "foo" and another.id == 1`, - pathContextNames: []string{"another", "span"}, - context: "another", - }, - { - name: "multiple paths with and without contexts", - statement: `set(value, 1) where span.attributes["foo"] == "foo" and id == 1`, - expected: `set(spanevent.value, 1) where span.attributes["foo"] == "foo" and spanevent.id == 1`, - pathContextNames: []string{"spanevent", "span"}, - context: "spanevent", - }, - { - name: "multiple paths without context", - statement: `set(value, 1) where name == attributes["foo.name"]`, - expected: `set(span.value, 1) where span.name == span.attributes["foo.name"]`, - context: "span", - }, - { - name: "function path parameter without context", - statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, - context: "log", - expected: `set(log.attributes["test"], "pass") where IsMatch(log.name, "operation[AC]")`, - }, - { - name: "function path parameter with context", - statement: `set(attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, - context: "log", - pathContextNames: []string{"log", "resource"}, - expected: `set(log.attributes["test"], "pass") where IsMatch(resource.name, "operation[AC]")`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if len(tt.pathContextNames) == 0 { - tt.pathContextNames = append(tt.pathContextNames, tt.context) - } - - ps, err := NewParser( - CreateFactoryMap[any](mockSetFactory), - testParsePath[any], - componenttest.NewNopTelemetrySettings(), - WithEnumParser[any](testParseEnum), - WithPathContextNames[any](tt.pathContextNames), - ) - - require.NoError(t, err) - require.NotNil(t, ps) - - var expected string - if tt.expected != "" { - expected = tt.expected - } else { - expected = tt.statement - } - - result, err := ps.AppendStatementPathsContext(tt.context, tt.statement) - require.NoError(t, err) - assert.Equal(t, expected, result) - }) - } -} From a027e80d1ad967498d079f3318317e56805a414a Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Wed, 30 Oct 2024 18:40:05 +0100 Subject: [PATCH 3/4] Unexported appendStatementPathsContext and removed extra logic from tests --- pkg/ottl/parser.go | 4 +- pkg/ottl/parser_test.go | 81 ++++++++++++++++++++--------------------- 2 files changed, 42 insertions(+), 43 deletions(-) diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index 9e5c27f1c40b..aec7b1ac9335 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -196,11 +196,11 @@ func (p *Parser[K]) ParseCondition(condition string) (*Condition[K], error) { }, nil } -// AppendStatementPathsContext changes the given OTTL statement adding the context name prefix +// appendStatementPathsContext changes the given OTTL statement adding the context name prefix // to all context-less paths. No modifications are performed for paths which [Path.Context] // value matches any WithPathContextNames value. // The context argument must be valid WithPathContextNames value, otherwise an error is returned. -func (p *Parser[K]) AppendStatementPathsContext(context string, statement string) (string, error) { +func (p *Parser[K]) appendStatementPathsContext(context string, statement string) (string, error) { if _, ok := p.pathContextNames[context]; !ok { return statement, fmt.Errorf(`unknown context "%s" for parser %T, valid options are: %s`, context, p, p.buildPathContextNamesText("")) } diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index ce4659f84df9..4105f7326537 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -2715,7 +2715,7 @@ func Test_ConditionSequence_Eval_Error(t *testing.T) { } } -func Test_AppendStatementPathsContext_InvalidStatement(t *testing.T) { +func Test_appendStatementPathsContext_InvalidStatement(t *testing.T) { ps, err := NewParser( CreateFactoryMap[any](), testParsePath[any], @@ -2724,11 +2724,11 @@ func Test_AppendStatementPathsContext_InvalidStatement(t *testing.T) { WithPathContextNames[any]([]string{"foo", "bar"}), ) require.NoError(t, err) - _, err = ps.AppendStatementPathsContext("foo", "this is invalid") + _, err = ps.appendStatementPathsContext("foo", "this is invalid") require.ErrorContains(t, err, `statement has invalid syntax`) } -func Test_AppendStatementPathsContext_InvalidContext(t *testing.T) { +func Test_appendStatementPathsContext_InvalidContext(t *testing.T) { ps, err := NewParser( CreateFactoryMap[any](), testParsePath[any], @@ -2737,11 +2737,11 @@ func Test_AppendStatementPathsContext_InvalidContext(t *testing.T) { WithPathContextNames[any]([]string{"foo", "bar"}), ) require.NoError(t, err) - _, err = ps.AppendStatementPathsContext("foobar", "set(foo, 1)") + _, err = ps.appendStatementPathsContext("foobar", "set(foo, 1)") require.ErrorContains(t, err, `unknown context "foobar" for parser`) } -func Test_AppendStatementPathsContext_Success(t *testing.T) { +func Test_appendStatementPathsContext_Success(t *testing.T) { type mockSetArguments[K any] struct { Target Setter[K] Value Getter[K] @@ -2761,57 +2761,67 @@ func Test_AppendStatementPathsContext_Success(t *testing.T) { expected string }{ { - name: "no paths", - statement: `set("foo", 1)`, - context: "bar", + name: "no paths", + statement: `set("foo", 1)`, + context: "bar", + pathContextNames: []string{"bar"}, + expected: `set("foo", 1)`, }, { - name: "single path with context", - statement: `set(span.value, 1)`, - context: "span", + name: "single path with context", + statement: `set(span.value, 1)`, + context: "span", + pathContextNames: []string{"span"}, + expected: `set(span.value, 1)`, }, { - name: "single path without context", - statement: "set(value, 1)", - expected: "set(span.value, 1)", - context: "span", + name: "single path without context", + statement: "set(value, 1)", + context: "span", + pathContextNames: []string{"span"}, + expected: "set(span.value, 1)", }, { name: "single path with context - multiple context names", statement: "set(span.value, 1)", - pathContextNames: []string{"spanevent", "span"}, context: "spanevent", + pathContextNames: []string{"spanevent", "span"}, + expected: "set(span.value, 1)", }, { name: "multiple paths with the same context", statement: `set(span.value, 1) where span.attributes["foo"] == "foo" and span.id == 1`, - pathContextNames: []string{"another", "span"}, context: "another", + pathContextNames: []string{"another", "span"}, + expected: `set(span.value, 1) where span.attributes["foo"] == "foo" and span.id == 1`, }, { name: "multiple paths with different contexts", statement: `set(another.value, 1) where span.attributes["foo"] == "foo" and another.id == 1`, - pathContextNames: []string{"another", "span"}, context: "another", + pathContextNames: []string{"another", "span"}, + expected: `set(another.value, 1) where span.attributes["foo"] == "foo" and another.id == 1`, }, { name: "multiple paths with and without contexts", statement: `set(value, 1) where span.attributes["foo"] == "foo" and id == 1`, - expected: `set(spanevent.value, 1) where span.attributes["foo"] == "foo" and spanevent.id == 1`, - pathContextNames: []string{"spanevent", "span"}, context: "spanevent", + pathContextNames: []string{"spanevent", "span"}, + expected: `set(spanevent.value, 1) where span.attributes["foo"] == "foo" and spanevent.id == 1`, }, { - name: "multiple paths without context", - statement: `set(value, 1) where name == attributes["foo.name"]`, - expected: `set(span.value, 1) where span.name == span.attributes["foo.name"]`, - context: "span", + name: "multiple paths without context", + statement: `set(value, 1) where name == attributes["foo.name"]`, + context: "span", + pathContextNames: []string{"span"}, + expected: `set(span.value, 1) where span.name == span.attributes["foo.name"]`, }, { - name: "function path parameter without context", - statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, - context: "log", - expected: `set(log.attributes["test"], "pass") where IsMatch(log.name, "operation[AC]")`, + name: "function path parameter without context", + statement: `set(attributes["test"], "pass") where IsMatch(name, "operation[AC]")`, + context: "log", + pathContextNames: []string{"log"}, + expected: `set(log.attributes["test"], "pass") where IsMatch(log.name, "operation[AC]")`, }, { name: "function path parameter with context", @@ -2824,10 +2834,6 @@ func Test_AppendStatementPathsContext_Success(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if len(tt.pathContextNames) == 0 { - tt.pathContextNames = append(tt.pathContextNames, tt.context) - } - ps, err := NewParser( CreateFactoryMap[any](mockSetFactory), testParsePath[any], @@ -2839,16 +2845,9 @@ func Test_AppendStatementPathsContext_Success(t *testing.T) { require.NoError(t, err) require.NotNil(t, ps) - var expected string - if tt.expected != "" { - expected = tt.expected - } else { - expected = tt.statement - } - - result, err := ps.AppendStatementPathsContext(tt.context, tt.statement) + result, err := ps.appendStatementPathsContext(tt.context, tt.statement) require.NoError(t, err) - assert.Equal(t, expected, result) + assert.Equal(t, tt.expected, result) }) } } From 8225cec3d62f1ce7cc73f6e57d34e5fd31faf4ab Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Thu, 31 Oct 2024 13:08:58 +0100 Subject: [PATCH 4/4] Removed change log file, renamed function and moved if out of the loop --- ...-add-statement-context-append-utility.yaml | 27 ------------------- pkg/ottl/parser.go | 26 +++++++++--------- pkg/ottl/parser_test.go | 12 ++++----- 3 files changed, 20 insertions(+), 45 deletions(-) delete mode 100644 .chloggen/ottl-add-statement-context-append-utility.yaml diff --git a/.chloggen/ottl-add-statement-context-append-utility.yaml b/.chloggen/ottl-add-statement-context-append-utility.yaml deleted file mode 100644 index 3852f6a3edef..000000000000 --- a/.chloggen/ottl-add-statement-context-append-utility.yaml +++ /dev/null @@ -1,27 +0,0 @@ -# Use this changelog template to create an entry for release notes. - -# 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: Add OTTL parser utility to rewrite statements appending missing paths context - -# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. -issues: [29017] - -# (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: "The `ottl.Parser[K]` has a new function `AppendStatementPathsContext`" - -# If your change doesn't affect end users or the exported elements of any package, -# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. -# Optional: The change log or logs in which this entry should be included. -# e.g. '[user]' or '[user, api]' -# Include 'user' if the change is relevant to end users. -# Include 'api' if there is a change to a library API. -# Default: '[user]' -change_logs: [api] diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index aec7b1ac9335..f16f0e3b0fbb 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "sort" "strings" "github.com/alecthomas/participle/v2" @@ -196,11 +197,11 @@ func (p *Parser[K]) ParseCondition(condition string) (*Condition[K], error) { }, nil } -// appendStatementPathsContext changes the given OTTL statement adding the context name prefix +// prependContextToStatementPaths changes the given OTTL statement adding the context name prefix // to all context-less paths. No modifications are performed for paths which [Path.Context] // value matches any WithPathContextNames value. // The context argument must be valid WithPathContextNames value, otherwise an error is returned. -func (p *Parser[K]) appendStatementPathsContext(context string, statement string) (string, error) { +func (p *Parser[K]) prependContextToStatementPaths(context string, statement string) (string, error) { if _, ok := p.pathContextNames[context]; !ok { return statement, fmt.Errorf(`unknown context "%s" for parser %T, valid options are: %s`, context, p, p.buildPathContextNamesText("")) } @@ -220,7 +221,7 @@ func (p *Parser[K]) appendStatementPathsContext(context string, statement string } } - return writeStatementWithPathsContext(context, statement, missingContextOffsets), nil + return insertContextIntoStatementOffsets(context, statement, missingContextOffsets) } var parser = newParser[parsedStatement]() @@ -254,27 +255,28 @@ func parseCondition(raw string) (*booleanExpression, error) { return parsed, nil } -func writeStatementWithPathsContext(context string, statement string, offsets []int) string { +func insertContextIntoStatementOffsets(context string, statement string, offsets []int) (string, error) { if len(offsets) == 0 { - return statement + return statement, nil } contextPrefix := context + "." var sb strings.Builder sb.Grow(len(statement) + (len(contextPrefix) * len(offsets))) + sort.Ints(offsets) left := 0 - for i, offset := range offsets { + for _, offset := range offsets { + if offset < 0 || offset > len(statement) { + return statement, fmt.Errorf(`failed to insert context "%s" into statement "%s": offset %d is out of range`, context, statement, offset) + } sb.WriteString(statement[left:offset]) sb.WriteString(contextPrefix) - if i+1 >= len(offsets) { - sb.WriteString(statement[offset:]) - } else { - left = offset - } + left = offset } + sb.WriteString(statement[left:]) - return sb.String() + return sb.String(), nil } // newParser returns a parser that can be used to read a string into a parsedStatement. An error will be returned if the string diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 4105f7326537..9e2e09a10e5f 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -2715,7 +2715,7 @@ func Test_ConditionSequence_Eval_Error(t *testing.T) { } } -func Test_appendStatementPathsContext_InvalidStatement(t *testing.T) { +func Test_prependContextToStatementPaths_InvalidStatement(t *testing.T) { ps, err := NewParser( CreateFactoryMap[any](), testParsePath[any], @@ -2724,11 +2724,11 @@ func Test_appendStatementPathsContext_InvalidStatement(t *testing.T) { WithPathContextNames[any]([]string{"foo", "bar"}), ) require.NoError(t, err) - _, err = ps.appendStatementPathsContext("foo", "this is invalid") + _, err = ps.prependContextToStatementPaths("foo", "this is invalid") require.ErrorContains(t, err, `statement has invalid syntax`) } -func Test_appendStatementPathsContext_InvalidContext(t *testing.T) { +func Test_prependContextToStatementPaths_InvalidContext(t *testing.T) { ps, err := NewParser( CreateFactoryMap[any](), testParsePath[any], @@ -2737,11 +2737,11 @@ func Test_appendStatementPathsContext_InvalidContext(t *testing.T) { WithPathContextNames[any]([]string{"foo", "bar"}), ) require.NoError(t, err) - _, err = ps.appendStatementPathsContext("foobar", "set(foo, 1)") + _, err = ps.prependContextToStatementPaths("foobar", "set(foo, 1)") require.ErrorContains(t, err, `unknown context "foobar" for parser`) } -func Test_appendStatementPathsContext_Success(t *testing.T) { +func Test_prependContextToStatementPaths_Success(t *testing.T) { type mockSetArguments[K any] struct { Target Setter[K] Value Getter[K] @@ -2845,7 +2845,7 @@ func Test_appendStatementPathsContext_Success(t *testing.T) { require.NoError(t, err) require.NotNil(t, ps) - result, err := ps.appendStatementPathsContext(tt.context, tt.statement) + result, err := ps.prependContextToStatementPaths(tt.context, tt.statement) require.NoError(t, err) assert.Equal(t, tt.expected, result) })