From 277f2850854a95221b2a7da329f0dab07c4beede Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:11:56 +0200 Subject: [PATCH 1/5] Change the OTTL grammar to support expressing statements context via path names --- ...ttl_statements_context_change_grammar.yaml | 27 ++++ pkg/ottl/contexts/internal/path.go | 5 + pkg/ottl/expression.go | 2 +- pkg/ottl/functions.go | 78 ++++++++++- pkg/ottl/functions_test.go | 127 +++++++++++++++++- pkg/ottl/grammar.go | 3 +- pkg/ottl/parser.go | 36 ++++- pkg/ottl/parser_test.go | 60 ++++++--- 8 files changed, 307 insertions(+), 31 deletions(-) create mode 100644 .chloggen/ottl_statements_context_change_grammar.yaml diff --git a/.chloggen/ottl_statements_context_change_grammar.yaml b/.chloggen/ottl_statements_context_change_grammar.yaml new file mode 100644 index 000000000000..0fc6ecace81e --- /dev/null +++ b/.chloggen/ottl_statements_context_change_grammar.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: "Change the OTTL grammar to support expressing statements context via path names" + +# 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: + +# 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/contexts/internal/path.go b/pkg/ottl/contexts/internal/path.go index c7d9d802b664..954d14329646 100644 --- a/pkg/ottl/contexts/internal/path.go +++ b/pkg/ottl/contexts/internal/path.go @@ -12,6 +12,7 @@ import ( var _ ottl.Path[any] = &TestPath[any]{} type TestPath[K any] struct { + C string N string KeySlice []ottl.Key[K] NextPath *TestPath[K] @@ -21,6 +22,10 @@ func (p *TestPath[K]) Name() string { return p.N } +func (p *TestPath[K]) Context() string { + return p.C +} + func (p *TestPath[K]) Next() ottl.Path[K] { if p.NextPath == nil { return nil diff --git a/pkg/ottl/expression.go b/pkg/ottl/expression.go index c1cb833c66ce..4483d59e93c5 100644 --- a/pkg/ottl/expression.go +++ b/pkg/ottl/expression.go @@ -715,7 +715,7 @@ func (p *Parser[K]) newGetter(val value) (Getter[K], error) { return &literal[K]{value: *i}, nil } if eL.Path != nil { - np, err := newPath[K](eL.Path.Fields) + np, err := p.newPath(eL.Path) if err != nil { return nil, err } diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index 9c6ccc226aaf..3f289d3301c2 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -22,9 +22,15 @@ type Enum int64 type EnumSymbol string -func buildOriginalText(fields []field) string { +func buildOriginalText(path *path) string { var builder strings.Builder - for i, f := range fields { + if path.Context != "" { + builder.WriteString(path.Context) + if len(path.Fields) > 0 { + builder.WriteString(".") + } + } + for i, f := range path.Fields { builder.WriteString(f.Name) if len(f.Keys) > 0 { for _, k := range f.Keys { @@ -38,21 +44,29 @@ func buildOriginalText(fields []field) string { builder.WriteString("]") } } - if i != len(fields)-1 { + if i != len(path.Fields)-1 { builder.WriteString(".") } } return builder.String() } -func newPath[K any](fields []field) (*basePath[K], error) { +func (p *Parser[K]) newPath(path *path) (*basePath[K], error) { + fields := path.Fields if len(fields) == 0 { return nil, fmt.Errorf("cannot make a path from zero fields") } - originalText := buildOriginalText(fields) + + pathContext, fields, err := p.parsePathContext(path) + if err != nil { + return nil, err + } + + originalText := buildOriginalText(path) var current *basePath[K] for i := len(fields) - 1; i >= 0; i-- { current = &basePath[K]{ + context: pathContext, name: fields[i].Name, keys: newKeys[K](fields[i].Keys), nextPath: current, @@ -64,10 +78,57 @@ func newPath[K any](fields []field) (*basePath[K], error) { return current, nil } +func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { + fields := path.Fields + var pathContext string + + if path.Context != "" { + // nil or empty pathContextNames means the Parser isn't handling the grammar path's + // context yet, so it falls back to the previous behavior with the path.Context value + // as the first path's segment. + if p.pathContextNames == nil || len(p.pathContextNames) == 0 { + fields = append([]field{{Name: path.Context}}, fields...) + } else if _, ok := p.pathContextNames[path.Context]; !ok { + if p.validatePathContextNames { + return pathContext, fields, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of [%[3]v]`, buildOriginalText(path), path.Context, p.buildPathContextNamesText("")) + } + fields = append([]field{{Name: path.Context}}, fields...) + } else { + pathContext = path.Context + } + } else if p.validatePathContextNames { + originalText := buildOriginalText(path) + return pathContext, fields, fmt.Errorf(`missing context name for path "%s", valid options are [%v]`, originalText, p.buildPathContextNamesText(originalText)) + } + + return pathContext, fields, nil +} + +func (p *Parser[K]) buildPathContextNamesText(path string) string { + var builder strings.Builder + var suffix string + if path != "" { + suffix = "." + path + } + + i := 0 + for ctx := range p.pathContextNames { + builder.WriteString(fmt.Sprintf(`"%s%s"`, ctx, suffix)) + if i != len(p.pathContextNames)-1 { + builder.WriteString(", ") + } + i++ + } + return builder.String() +} + // Path represents a chain of path parts in an OTTL statement, such as `body.string`. // A Path has a name, and potentially a set of keys. // If the path in the OTTL statement contains multiple parts (separated by a dot (`.`)), then the Path will have a pointer to the next Path. type Path[K any] interface { + // Context is the OTTL context name of this Path. + Context() string + // Name is the name of this segment of the path. Name() string @@ -86,6 +147,7 @@ type Path[K any] interface { var _ Path[any] = &basePath[any]{} type basePath[K any] struct { + context string name string keys []Key[K] nextPath *basePath[K] @@ -94,6 +156,10 @@ type basePath[K any] struct { originalText string } +func (p *basePath[K]) Context() string { + return p.context +} + func (p *basePath[K]) Name() string { return p.name } @@ -412,7 +478,7 @@ func (p *Parser[K]) buildArg(argVal value, argType reflect.Type) (any, error) { if argVal.Literal == nil || argVal.Literal.Path == nil { return nil, fmt.Errorf("must be a path") } - np, err := newPath[K](argVal.Literal.Path.Fields) + np, err := p.newPath(argVal.Literal.Path) if err != nil { return nil, err } diff --git a/pkg/ottl/functions_test.go b/pkg/ottl/functions_test.go index b2210c36654b..cfcdbddc4c56 100644 --- a/pkg/ottl/functions_test.go +++ b/pkg/ottl/functions_test.go @@ -2230,6 +2230,14 @@ func Test_basePath_Name(t *testing.T) { assert.Equal(t, "test", n) } +func Test_basePath_Context(t *testing.T) { + bp := basePath[any]{ + context: "log", + } + n := bp.Context() + assert.Equal(t, "log", n) +} + func Test_basePath_Next(t *testing.T) { bp := basePath[any]{ nextPath: &basePath[any]{}, @@ -2352,6 +2360,13 @@ func Test_basePath_NextWithIsComplete(t *testing.T) { } func Test_newPath(t *testing.T) { + ps, _ := NewParser[any]( + defaultFunctionsForTests(), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + ) + fields := []field{ { Name: "body", @@ -2365,7 +2380,8 @@ func Test_newPath(t *testing.T) { }, }, } - np, err := newPath[any](fields) + + np, err := ps.newPath(&path{Fields: fields}) assert.NoError(t, err) p := Path[any](np) assert.Equal(t, "body", p.Name()) @@ -2384,6 +2400,115 @@ func Test_newPath(t *testing.T) { assert.Nil(t, i) } +func Test_newPath_PathContext(t *testing.T) { + tests := []struct { + name string + pathContext string + pathContextNames []string + validationEnabled bool + contextParsedAsField bool + expectedError bool + }{ + { + name: "without context", + pathContextNames: []string{"log"}, + }, + { + name: "with context", + pathContext: "log", + pathContextNames: []string{"log"}, + }, + { + name: "with multiple contexts", + pathContext: "span", + pathContextNames: []string{"log", "span"}, + }, + { + name: "with unknown context validation disabled", + pathContext: "span", + pathContextNames: []string{"log"}, + validationEnabled: false, + contextParsedAsField: true, + }, + { + name: "with unknown context validation enabled", + pathContext: "span", + pathContextNames: []string{"log"}, + validationEnabled: true, + expectedError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ps, _ := NewParser[any]( + defaultFunctionsForTests(), + testParsePath[any], + componenttest.NewNopTelemetrySettings(), + WithEnumParser[any](testParseEnum), + WithPathContextNames[any](tt.pathContextNames), + WithPathContextNameValidation[any](tt.validationEnabled), + ) + + gp := &path{ + Context: tt.pathContext, + Fields: []field{ + { + Name: "body", + }, + { + Name: "string", + Keys: []key{ + { + String: ottltest.Strp("key"), + }, + }, + }, + }} + + np, err := ps.newPath(gp) + if tt.expectedError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + p := Path[any](np) + if tt.contextParsedAsField { + assert.Equal(t, tt.pathContext, p.Name()) + assert.Equal(t, "", p.Context()) + assert.Nil(t, p.Keys()) + p = p.Next() + } + var bodyStringFuncValue string + if tt.pathContext != "" { + bodyStringFuncValue = fmt.Sprintf("%s.body.string[key]", tt.pathContext) + } else { + bodyStringFuncValue = "body.string[key]" + } + assert.Equal(t, "body", p.Name()) + assert.Nil(t, p.Keys()) + assert.Equal(t, bodyStringFuncValue, p.String()) + if !tt.contextParsedAsField { + assert.Equal(t, tt.pathContext, p.Context()) + } + p = p.Next() + assert.Equal(t, "string", p.Name()) + assert.Equal(t, bodyStringFuncValue, p.String()) + if !tt.contextParsedAsField { + assert.Equal(t, tt.pathContext, p.Context()) + } + assert.Nil(t, p.Next()) + assert.Equal(t, 1, len(p.Keys())) + v, err := p.Keys()[0].String(context.Background(), struct{}{}) + assert.NoError(t, err) + assert.Equal(t, "key", *v) + i, err := p.Keys()[0].Int(context.Background(), struct{}{}) + assert.NoError(t, err) + assert.Nil(t, i) + }) + } +} + func Test_baseKey_String(t *testing.T) { bp := baseKey[any]{ s: ottltest.Strp("test"), diff --git a/pkg/ottl/grammar.go b/pkg/ottl/grammar.go index 04352a6b7a8e..8c410fb51cc9 100644 --- a/pkg/ottl/grammar.go +++ b/pkg/ottl/grammar.go @@ -252,7 +252,8 @@ func (v *value) checkForCustomError() error { // path represents a telemetry path mathExpression. type path struct { - Fields []field `parser:"@@ ( '.' @@ )*"` + Context string `parser:"(@Lowercase '.')?"` + Fields []field `parser:"@@ ( '.' @@ )*"` } // field is an item within a path. diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index f8d5ab7dabc8..158dcf331e78 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -63,10 +63,12 @@ func (c *Condition[K]) Eval(ctx context.Context, tCtx K) (bool, error) { // Parser provides the means to parse OTTL StatementSequence and Conditions given a specific set of functions, // a PathExpressionParser, and an EnumParser. type Parser[K any] struct { - functions map[string]Factory[K] - pathParser PathExpressionParser[K] - enumParser EnumParser - telemetrySettings component.TelemetrySettings + functions map[string]Factory[K] + pathParser PathExpressionParser[K] + enumParser EnumParser + telemetrySettings component.TelemetrySettings + pathContextNames map[string]bool + validatePathContextNames bool } func NewParser[K any]( @@ -100,6 +102,32 @@ func WithEnumParser[K any](parser EnumParser) Option[K] { } } +// WithPathContextNameValidation turns the validation mode on/off for the Path's contexts. +// When enabled, and WithPathContextNames is configured, it requires all Path's to be prefixed +// with a valid context name, matching at least one configured WithPathContextNames value. +func WithPathContextNameValidation[K any](enabled bool) Option[K] { + return func(p *Parser[K]) { + p.validatePathContextNames = enabled + } +} + +// WithPathContextNames sets the context names to be considered when parsing a Path value. +// When this option is empty or nil, all Path segments are considered fields, and the +// Path.Context value is always empty. +// When this option is configured, and the first path segment is not present in this context +// names list, it either parses the whole Path as fields, without a Path.Context value, +// or results into an error, depending on the WithPathContextNameValidation config. +func WithPathContextNames[K any](contexts []string) Option[K] { + return func(p *Parser[K]) { + pathContextNames := make(map[string]bool, len(contexts)) + for _, ctx := range contexts { + pathContextNames[ctx] = true + } + + p.pathContextNames = pathContextNames + } +} + // ParseStatements parses string statements into ottl.Statement objects ready for execution. // Returns a slice of statements and a nil error on successful parsing. // If parsing fails, returns nil and a joined error containing each error per failed statement. diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 679ceddd1872..dddb07d6cb30 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -111,10 +111,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "bear", Fields: []field{ - { - Name: "bear", - }, { Name: "honey", }, @@ -144,10 +142,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "foo", Fields: []field{ - { - Name: "foo", - }, { Name: "attributes", Keys: []key{ @@ -174,6 +170,42 @@ func Test_parse(t *testing.T) { WhereClause: nil, }, }, + { + name: "single field segment", + statement: `set(attributes["bar"], "dog")`, + expected: &parsedStatement{ + Editor: editor{ + Function: "set", + Arguments: []argument{ + { + Value: value{ + Literal: &mathExprLiteral{ + Path: &path{ + Context: "", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + }, + }, + }, + }, + }, + { + Value: value{ + String: ottltest.Strp("dog"), + }, + }, + }, + }, + WhereClause: nil, + }, + }, { name: "Converter parameters (All Uppercase)", statement: `replace_pattern(attributes["message"], "device=*", attributes["device_name"], SHA256)`, @@ -311,10 +343,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "foo", Fields: []field{ - { - Name: "foo", - }, { Name: "bar", Keys: []key{ @@ -367,10 +397,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "foo", Fields: []field{ - { - Name: "foo", - }, { Name: "attributes", Keys: []key{ @@ -430,10 +458,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "foo", Fields: []field{ - { - Name: "foo", - }, { Name: "attributes", Keys: []key{ @@ -493,10 +519,8 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "foo", Fields: []field{ - { - Name: "foo", - }, { Name: "attributes", Keys: []key{ From a2c8ab836fd4cc4c649eda9c7cd14ac9ee78519c Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:39:47 +0200 Subject: [PATCH 2/5] Fix failing test after branch update, adding more tests cases and nitpicks --- pkg/ottl/functions.go | 8 +++----- pkg/ottl/functions_test.go | 20 ++++++++++++++++---- pkg/ottl/parser_test.go | 4 +--- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index 3f289d3301c2..8221fb21c060 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -52,8 +52,7 @@ func buildOriginalText(path *path) string { } func (p *Parser[K]) newPath(path *path) (*basePath[K], error) { - fields := path.Fields - if len(fields) == 0 { + if len(path.Fields) == 0 { return nil, fmt.Errorf("cannot make a path from zero fields") } @@ -81,7 +80,6 @@ func (p *Parser[K]) newPath(path *path) (*basePath[K], error) { func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { fields := path.Fields var pathContext string - if path.Context != "" { // nil or empty pathContextNames means the Parser isn't handling the grammar path's // context yet, so it falls back to the previous behavior with the path.Context value @@ -90,7 +88,7 @@ func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { fields = append([]field{{Name: path.Context}}, fields...) } else if _, ok := p.pathContextNames[path.Context]; !ok { if p.validatePathContextNames { - return pathContext, fields, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of [%[3]v]`, buildOriginalText(path), path.Context, p.buildPathContextNamesText("")) + return "", nil, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of: %s`, path.Context, buildOriginalText(path), p.buildPathContextNamesText("")) } fields = append([]field{{Name: path.Context}}, fields...) } else { @@ -98,7 +96,7 @@ func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { } } else if p.validatePathContextNames { originalText := buildOriginalText(path) - return pathContext, fields, fmt.Errorf(`missing context name for path "%s", valid options are [%v]`, originalText, p.buildPathContextNamesText(originalText)) + return "", nil, fmt.Errorf(`missing context name for path "%s", valid options are: %s`, originalText, p.buildPathContextNamesText(originalText)) } return pathContext, fields, nil diff --git a/pkg/ottl/functions_test.go b/pkg/ottl/functions_test.go index cfcdbddc4c56..7e10f3021ef3 100644 --- a/pkg/ottl/functions_test.go +++ b/pkg/ottl/functions_test.go @@ -2400,7 +2400,7 @@ func Test_newPath(t *testing.T) { assert.Nil(t, i) } -func Test_newPath_PathContext(t *testing.T) { +func Test_newPath_WithPathContextNames(t *testing.T) { tests := []struct { name string pathContext string @@ -2410,7 +2410,7 @@ func Test_newPath_PathContext(t *testing.T) { expectedError bool }{ { - name: "without context", + name: "with no context", pathContextNames: []string{"log"}, }, { @@ -2424,19 +2424,31 @@ func Test_newPath_PathContext(t *testing.T) { pathContextNames: []string{"log", "span"}, }, { - name: "with unknown context validation disabled", + name: "with invalid context and validation disabled", pathContext: "span", pathContextNames: []string{"log"}, validationEnabled: false, contextParsedAsField: true, }, { - name: "with unknown context validation enabled", + name: "with invalid context and validation enabled", pathContext: "span", pathContextNames: []string{"log"}, validationEnabled: true, expectedError: true, }, + { + name: "with valid context and validation enabled", + pathContext: "spanevent", + pathContextNames: []string{"spanevent"}, + validationEnabled: true, + }, + { + name: "with no context and validation enabled", + pathContextNames: []string{"spanevent"}, + validationEnabled: true, + expectedError: true, + }, } for _, tt := range tests { diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 1ed7457c2615..4db8c47c820f 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -212,10 +212,8 @@ func Test_parse(t *testing.T) { Value: &value{ Literal: &mathExprLiteral{ Path: &path{ + Context: "bear", Fields: []field{ - { - Name: "bear", - }, { Name: "honey", }, From 0552371aa049e056e8eba42f9cde9ecda9814aeb Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 30 Aug 2024 17:24:48 +0200 Subject: [PATCH 3/5] Removed `WithPathContextNameValidation` option --- pkg/ottl/functions.go | 25 ++++++--------- pkg/ottl/functions_test.go | 62 ++++++++++++++------------------------ pkg/ottl/parser.go | 25 +++++---------- 3 files changed, 39 insertions(+), 73 deletions(-) diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index 8221fb21c060..54943f33bf32 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -78,28 +78,23 @@ func (p *Parser[K]) newPath(path *path) (*basePath[K], error) { } func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { - fields := path.Fields - var pathContext string + hasPathContextNames := len(p.pathContextNames) > 0 if path.Context != "" { - // nil or empty pathContextNames means the Parser isn't handling the grammar path's - // context yet, so it falls back to the previous behavior with the path.Context value - // as the first path's segment. - if p.pathContextNames == nil || len(p.pathContextNames) == 0 { - fields = append([]field{{Name: path.Context}}, fields...) + // no pathContextNames means the Parser isn't handling the grammar path's context yet, + // so it falls back to the previous behavior with the path.Context value as the first + // path's segment. + if !hasPathContextNames { + return "", append([]field{{Name: path.Context}}, path.Fields...), nil } else if _, ok := p.pathContextNames[path.Context]; !ok { - if p.validatePathContextNames { - return "", nil, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of: %s`, path.Context, buildOriginalText(path), p.buildPathContextNamesText("")) - } - fields = append([]field{{Name: path.Context}}, fields...) - } else { - pathContext = path.Context + return "", path.Fields, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of: %s`, path.Context, buildOriginalText(path), p.buildPathContextNamesText("")) } - } else if p.validatePathContextNames { + return path.Context, path.Fields, nil + } else if hasPathContextNames { originalText := buildOriginalText(path) return "", nil, fmt.Errorf(`missing context name for path "%s", valid options are: %s`, originalText, p.buildPathContextNamesText(originalText)) } - return pathContext, fields, nil + return "", path.Fields, nil } func (p *Parser[K]) buildPathContextNamesText(path string) string { diff --git a/pkg/ottl/functions_test.go b/pkg/ottl/functions_test.go index 7e10f3021ef3..b072afd43c41 100644 --- a/pkg/ottl/functions_test.go +++ b/pkg/ottl/functions_test.go @@ -2402,52 +2402,34 @@ func Test_newPath(t *testing.T) { func Test_newPath_WithPathContextNames(t *testing.T) { tests := []struct { - name string - pathContext string - pathContextNames []string - validationEnabled bool - contextParsedAsField bool - expectedError bool + name string + pathContext string + pathContextNames []string + expectedError string }{ { - name: "with no context", + name: "with no path context", pathContextNames: []string{"log"}, + expectedError: `missing context name for path "body.string[key]", valid options are: "log.body.string[key]"`, }, { - name: "with context", + name: "with no path context and configuration", + }, + { + name: "with valid path context", pathContext: "log", pathContextNames: []string{"log"}, }, { - name: "with multiple contexts", + name: "with invalid path context", pathContext: "span", - pathContextNames: []string{"log", "span"}, - }, - { - name: "with invalid context and validation disabled", - pathContext: "span", - pathContextNames: []string{"log"}, - validationEnabled: false, - contextParsedAsField: true, - }, - { - name: "with invalid context and validation enabled", - pathContext: "span", - pathContextNames: []string{"log"}, - validationEnabled: true, - expectedError: true, - }, - { - name: "with valid context and validation enabled", - pathContext: "spanevent", - pathContextNames: []string{"spanevent"}, - validationEnabled: true, + pathContextNames: []string{"log"}, + expectedError: `context "span" from path "span.body.string[key]" is not valid, it must be replaced by one of: "log"`, }, { - name: "with no context and validation enabled", - pathContextNames: []string{"spanevent"}, - validationEnabled: true, - expectedError: true, + name: "with multiple configured contexts", + pathContext: "span", + pathContextNames: []string{"log", "span"}, }, } @@ -2459,7 +2441,6 @@ func Test_newPath_WithPathContextNames(t *testing.T) { componenttest.NewNopTelemetrySettings(), WithEnumParser[any](testParseEnum), WithPathContextNames[any](tt.pathContextNames), - WithPathContextNameValidation[any](tt.validationEnabled), ) gp := &path{ @@ -2479,13 +2460,14 @@ func Test_newPath_WithPathContextNames(t *testing.T) { }} np, err := ps.newPath(gp) - if tt.expectedError { - assert.Error(t, err) + if tt.expectedError != "" { + assert.Error(t, err, tt.expectedError) return } assert.NoError(t, err) p := Path[any](np) - if tt.contextParsedAsField { + contextParsedAsField := len(tt.pathContextNames) == 0 && tt.pathContext != "" + if contextParsedAsField { assert.Equal(t, tt.pathContext, p.Name()) assert.Equal(t, "", p.Context()) assert.Nil(t, p.Keys()) @@ -2500,13 +2482,13 @@ func Test_newPath_WithPathContextNames(t *testing.T) { assert.Equal(t, "body", p.Name()) assert.Nil(t, p.Keys()) assert.Equal(t, bodyStringFuncValue, p.String()) - if !tt.contextParsedAsField { + if !contextParsedAsField { assert.Equal(t, tt.pathContext, p.Context()) } p = p.Next() assert.Equal(t, "string", p.Name()) assert.Equal(t, bodyStringFuncValue, p.String()) - if !tt.contextParsedAsField { + if !contextParsedAsField { assert.Equal(t, tt.pathContext, p.Context()) } assert.Nil(t, p.Next()) diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index 158dcf331e78..a8c3381895ca 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -63,12 +63,11 @@ func (c *Condition[K]) Eval(ctx context.Context, tCtx K) (bool, error) { // Parser provides the means to parse OTTL StatementSequence and Conditions given a specific set of functions, // a PathExpressionParser, and an EnumParser. type Parser[K any] struct { - functions map[string]Factory[K] - pathParser PathExpressionParser[K] - enumParser EnumParser - telemetrySettings component.TelemetrySettings - pathContextNames map[string]bool - validatePathContextNames bool + functions map[string]Factory[K] + pathParser PathExpressionParser[K] + enumParser EnumParser + telemetrySettings component.TelemetrySettings + pathContextNames map[string]bool } func NewParser[K any]( @@ -102,21 +101,11 @@ func WithEnumParser[K any](parser EnumParser) Option[K] { } } -// WithPathContextNameValidation turns the validation mode on/off for the Path's contexts. -// When enabled, and WithPathContextNames is configured, it requires all Path's to be prefixed -// with a valid context name, matching at least one configured WithPathContextNames value. -func WithPathContextNameValidation[K any](enabled bool) Option[K] { - return func(p *Parser[K]) { - p.validatePathContextNames = enabled - } -} - // WithPathContextNames sets the context names to be considered when parsing a Path value. // When this option is empty or nil, all Path segments are considered fields, and the // Path.Context value is always empty. -// When this option is configured, and the first path segment is not present in this context -// names list, it either parses the whole Path as fields, without a Path.Context value, -// or results into an error, depending on the WithPathContextNameValidation config. +// When this option is configured, and the path's context is empty or is not present in +// this context names list, it results into an error. func WithPathContextNames[K any](contexts []string) Option[K] { return func(p *Parser[K]) { pathContextNames := make(map[string]bool, len(contexts)) From ee02ebd8d51dff18ca42c1afcf0ab05ccad8ed34 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 30 Aug 2024 17:40:26 +0200 Subject: [PATCH 4/5] Fix test to use assert.Len (testifylint) --- pkg/ottl/functions_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/ottl/functions_test.go b/pkg/ottl/functions_test.go index b072afd43c41..0f28e62be0c6 100644 --- a/pkg/ottl/functions_test.go +++ b/pkg/ottl/functions_test.go @@ -2391,7 +2391,7 @@ func Test_newPath(t *testing.T) { assert.Equal(t, "string", p.Name()) assert.Equal(t, "body.string[key]", p.String()) assert.Nil(t, p.Next()) - assert.Equal(t, 1, len(p.Keys())) + assert.Len(t, p.Keys(), 1) v, err := p.Keys()[0].String(context.Background(), struct{}{}) assert.NoError(t, err) assert.Equal(t, "key", *v) @@ -2492,7 +2492,7 @@ func Test_newPath_WithPathContextNames(t *testing.T) { assert.Equal(t, tt.pathContext, p.Context()) } assert.Nil(t, p.Next()) - assert.Equal(t, 1, len(p.Keys())) + assert.Len(t, p.Keys(), 1) v, err := p.Keys()[0].String(context.Background(), struct{}{}) assert.NoError(t, err) assert.Equal(t, "key", *v) @@ -2534,7 +2534,7 @@ func Test_newKey(t *testing.T) { } ks := newKeys[any](keys) - assert.Equal(t, 2, len(ks)) + assert.Len(t, ks, 2) s, err := ks[0].String(context.Background(), nil) assert.NoError(t, err) From e04e2a32db985f9c0cab023d1ad5f1f9aeae82d7 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 6 Sep 2024 18:28:00 +0200 Subject: [PATCH 5/5] Applied PR suggestions, changed pathContextNames set type, and added "possibly" on the error message --- .chloggen/ottl_statements_context_change_grammar.yaml | 4 ++-- pkg/ottl/functions.go | 11 ++++++++--- pkg/ottl/parser.go | 6 +++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/.chloggen/ottl_statements_context_change_grammar.yaml b/.chloggen/ottl_statements_context_change_grammar.yaml index 0fc6ecace81e..ba582e9ac615 100644 --- a/.chloggen/ottl_statements_context_change_grammar.yaml +++ b/.chloggen/ottl_statements_context_change_grammar.yaml @@ -1,7 +1,7 @@ # Use this changelog template to create an entry for release notes. # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: enhancement +change_type: breaking # The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) component: pkg/ottl @@ -15,7 +15,7 @@ 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: +subtext: "The `ottl.Path` interface requires a new method: `Context() string`" # 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. diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index 54943f33bf32..251e79587b75 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -85,13 +85,18 @@ func (p *Parser[K]) parsePathContext(path *path) (string, []field, error) { // path's segment. if !hasPathContextNames { return "", append([]field{{Name: path.Context}}, path.Fields...), nil - } else if _, ok := p.pathContextNames[path.Context]; !ok { + } + + if _, ok := p.pathContextNames[path.Context]; !ok { return "", path.Fields, fmt.Errorf(`context "%s" from path "%s" is not valid, it must be replaced by one of: %s`, path.Context, buildOriginalText(path), p.buildPathContextNamesText("")) } + return path.Context, path.Fields, nil - } else if hasPathContextNames { + } + + if hasPathContextNames { originalText := buildOriginalText(path) - return "", nil, fmt.Errorf(`missing context name for path "%s", valid options are: %s`, originalText, p.buildPathContextNamesText(originalText)) + return "", nil, fmt.Errorf(`missing context name for path "%s", possibly valid options are: %s`, originalText, p.buildPathContextNamesText(originalText)) } return "", path.Fields, nil diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go index a8c3381895ca..0f196af032df 100644 --- a/pkg/ottl/parser.go +++ b/pkg/ottl/parser.go @@ -67,7 +67,7 @@ type Parser[K any] struct { pathParser PathExpressionParser[K] enumParser EnumParser telemetrySettings component.TelemetrySettings - pathContextNames map[string]bool + pathContextNames map[string]struct{} } func NewParser[K any]( @@ -108,9 +108,9 @@ func WithEnumParser[K any](parser EnumParser) Option[K] { // this context names list, it results into an error. func WithPathContextNames[K any](contexts []string) Option[K] { return func(p *Parser[K]) { - pathContextNames := make(map[string]bool, len(contexts)) + pathContextNames := make(map[string]struct{}, len(contexts)) for _, ctx := range contexts { - pathContextNames[ctx] = true + pathContextNames[ctx] = struct{}{} } p.pathContextNames = pathContextNames