[pkg/ottl] Improve OTTL parser error reporting (open-telemetry#23840)

**Description:** I was playing with an erroneous config like the following and noticed that the error reporting was confusing: ```yaml processors: filter: metrics: metric: - true - false ``` This PR updates the error messages to print the statements next to the error so it's clear which statement caused the error. To speed up feedback cycles when changing multiple statements, I've also configured the parser to attempt to parse each statement so users can see all errors at once. Errors now look like this: ``` Error: invalid configuration: processors::filter: unable to parse OTTL statement "drop() where 1": statement has invalid syntax: 1:15: unexpected token "<EOF>" (expected <opcomparison> Value); unable to parse OTTL statement "drop() where 0": statement has invalid syntax: 1:15: unexpected token "<EOF>" (expected <opcomparison> Value) ``` **Testing:** Unit test Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
movence · Jul 12, 2023 · a0883a0 · a0883a0
1 parent 7239c4a
commit a0883a0
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 37 deletions.
diff --git a/.chloggen/ottl-improve-parsing-errors.yaml b/.chloggen/ottl-improve-parsing-errors.yaml
@@ -0,0 +1,22 @@
+# Use this changelog template to create an entry for release notes.
+# If your change doesn't affect end users, such as a test fix or a tooling change,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+
+# 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: Improve error reporting for errors during statement parsing
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [23840]
+
+# (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: |
+  - Failures are now printed for all statements within a context, and the statements are printed next to the errors.
+  - Erroneous values found during parsing are now quoted in error logs.
diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go
@@ -20,7 +20,7 @@ type Enum int64
 func (p *Parser[K]) newFunctionCall(ed editor) (Expr[K], error) {
 	f, ok := p.functions[ed.Function]
 	if !ok {
-		return Expr[K]{}, fmt.Errorf("undefined function %v", ed.Function)
+		return Expr[K]{}, fmt.Errorf("undefined function %q", ed.Function)
 	}
 	args := f.CreateDefaultArguments()
 
@@ -30,12 +30,12 @@ func (p *Parser[K]) newFunctionCall(ed editor) (Expr[K], error) {
 		// settability requirements. Non-pointer values are not
 		// modifiable through reflection.
 		if reflect.TypeOf(args).Kind() != reflect.Pointer {
-			return Expr[K]{}, fmt.Errorf("factory for %s must return a pointer to an Arguments value in its CreateDefaultArguments method", ed.Function)
+			return Expr[K]{}, fmt.Errorf("factory for %q must return a pointer to an Arguments value in its CreateDefaultArguments method", ed.Function)
 		}
 
 		err := p.buildArgs(ed, reflect.ValueOf(args).Elem())
 		if err != nil {
-			return Expr[K]{}, fmt.Errorf("error while parsing arguments for call to '%v': %w", ed.Function, err)
+			return Expr[K]{}, fmt.Errorf("error while parsing arguments for call to %q: %w", ed.Function, err)
 		}
 	}
 
@@ -61,17 +61,17 @@ func (p *Parser[K]) buildArgs(ed editor, argsVal reflect.Value) error {
 		fieldTag, ok := argsType.Field(i).Tag.Lookup("ottlarg")
 
 		if !ok {
-			return fmt.Errorf("no `ottlarg` struct tag on Arguments field '%s'", argsType.Field(i).Name)
+			return fmt.Errorf("no `ottlarg` struct tag on Arguments field %q", argsType.Field(i).Name)
 		}
 
 		argNum, err := strconv.Atoi(fieldTag)
 
 		if err != nil {
-			return fmt.Errorf("ottlarg struct tag on field '%s' is not a valid integer: %w", argsType.Field(i).Name, err)
+			return fmt.Errorf("ottlarg struct tag on field %q is not a valid integer: %w", argsType.Field(i).Name, err)
 		}
 
 		if argNum < 0 || argNum >= len(ed.Arguments) {
-			return fmt.Errorf("ottlarg struct tag on field '%s' has value %d, but must be between 0 and %d", argsType.Field(i).Name, argNum, len(ed.Arguments))
+			return fmt.Errorf("ottlarg struct tag on field %q has value %d, but must be between 0 and %d", argsType.Field(i).Name, argNum, len(ed.Arguments))
 		}
 
 		argVal := ed.Arguments[argNum]
@@ -167,7 +167,7 @@ func (p *Parser[K]) buildSliceArg(argVal value, argType reflect.Type) (any, erro
 		}
 		return arg, nil
 	default:
-		return nil, fmt.Errorf("unsupported slice type '%s' for function", argType.Elem().Name())
+		return nil, fmt.Errorf("unsupported slice type %q for function", argType.Elem().Name())
 	}
 }
 

diff --git a/pkg/ottl/go.mod b/pkg/ottl/go.mod
@@ -13,6 +13,7 @@ require (
 	go.opentelemetry.io/collector/component v0.81.0
 	go.opentelemetry.io/collector/pdata v1.0.0-rcv0013
 	go.opentelemetry.io/otel/trace v1.16.0
+	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.24.0
 	golang.org/x/exp v0.0.0-20230510235704-dd950f8aeaea
 )
@@ -35,7 +36,6 @@ require (
 	go.opentelemetry.io/otel v1.16.0 // indirect
 	go.opentelemetry.io/otel/metric v1.16.0 // indirect
 	go.uber.org/atomic v1.10.0 // indirect
-	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/net v0.12.0 // indirect
 	golang.org/x/sys v0.10.0 // indirect
 	golang.org/x/text v0.11.0 // indirect

diff --git a/pkg/ottl/parser.go b/pkg/ottl/parser.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/alecthomas/participle/v2"
 	"go.opentelemetry.io/collector/component"
+	"go.uber.org/multierr"
 	"go.uber.org/zap"
 )
 
@@ -96,15 +97,27 @@ func WithEnumParser[K any](parser EnumParser) Option[K] {
 	}
 }
 
+// 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 an empty slice  with a multierr error containing
+// an error per failed statement.
 func (p *Parser[K]) ParseStatements(statements []string) ([]*Statement[K], error) {
 	var parsedStatements []*Statement[K]
+	var parseErr error
+
 	for _, statement := range statements {
 		ps, err := p.ParseStatement(statement)
 		if err != nil {
-			return nil, err
+			parseErr = multierr.Append(parseErr, fmt.Errorf("unable to parse OTTL statement %q: %w", statement, err))
+			continue
 		}
 		parsedStatements = append(parsedStatements, ps)
 	}
+
+	if parseErr != nil {
+		return nil, parseErr
+	}
+
 	return parsedStatements, nil
 }
 
@@ -134,7 +147,7 @@ func parseStatement(raw string) (*parsedStatement, error) {
 	parsed, err := parser.ParseString("", raw)
 
 	if err != nil {
-		return nil, fmt.Errorf("unable to parse OTTL statement: %w", err)
+		return nil, fmt.Errorf("statement has invalid syntax: %w", err)
 	}
 	err = parsed.checkForCustomError()
 	if err != nil {

diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go
@@ -12,6 +12,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"go.opentelemetry.io/collector/component/componenttest"
+	"go.uber.org/multierr"
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest"
 )
@@ -1365,6 +1366,33 @@ func testParseEnum(val *EnumSymbol) (*Enum, error) {
 	return nil, fmt.Errorf("enum symbol not provided")
 }
 
+func Test_ParseStatements_Error(t *testing.T) {
+	statements := []string{
+		`set(`,
+		`set("foo)`,
+		`set(name.)`,
+	}
+
+	p, _ := NewParser(
+		CreateFactoryMap[any](),
+		testParsePath,
+		componenttest.NewNopTelemetrySettings(),
+		WithEnumParser[any](testParseEnum),
+	)
+
+	_, err := p.ParseStatements(statements)
+
+	assert.Error(t, err)
+
+	multiErrs := multierr.Errors(err)
+
+	assert.Len(t, multiErrs, len(statements), "ParseStatements didn't return an error per statement")
+
+	for i, statementErr := range multiErrs {
+		assert.ErrorContains(t, statementErr, fmt.Sprintf("unable to parse OTTL statement %q", statements[i]))
+	}
+}
+
 // This test doesn't validate parser results, simply checks whether the parse succeeds or not.
 // It's a fast way to check a large range of possible syntaxes.
 func Test_parseStatement(t *testing.T) {

diff --git a/processor/filterprocessor/config_test.go b/processor/filterprocessor/config_test.go
@@ -893,24 +893,19 @@ func TestLoadingConfigOTTL(t *testing.T) {
 			errorMessage: "cannot use ottl conditions and include/exclude for logs at the same time",
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_span"),
-			errorMessage: "unable to parse OTTL statement: 1:25: unexpected token \"test\" (expected (<string> | <int>) \"]\")",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_span"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_spanevent"),
-			errorMessage: "unable to parse OTTL statement: 1:25: unexpected token \"test\" (expected (<string> | <int>) \"]\")",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_spanevent"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_metric"),
-			errorMessage: "unable to parse OTTL statement: 1:34: unexpected token \"test\" (expected (<string> | <int>) \"]\")",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_metric"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_datapoint"),
-			errorMessage: "unable to parse OTTL statement: 1:25: unexpected token \"test\" (expected (<string> | <int>) \"]\")",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_datapoint"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_log"),
-			errorMessage: "unable to parse OTTL statement: 1:25: unexpected token \"test\" (expected (<string> | <int>) \"]\")",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_log"),
 		},
 	}
 
@@ -924,7 +919,11 @@ func TestLoadingConfigOTTL(t *testing.T) {
 			require.NoError(t, component.UnmarshalConfig(sub, cfg))
 
 			if tt.expected == nil {
-				assert.EqualError(t, component.ValidateConfig(cfg), tt.errorMessage)
+				if tt.errorMessage != "" {
+					assert.EqualError(t, component.ValidateConfig(cfg), tt.errorMessage)
+				} else {
+					assert.Error(t, component.ValidateConfig(cfg))
+				}
 			} else {
 				assert.NoError(t, component.ValidateConfig(cfg))
 				assert.Equal(t, tt.expected, cfg)

diff --git a/processor/transformprocessor/config_test.go b/processor/transformprocessor/config_test.go
@@ -20,9 +20,8 @@ func TestLoadConfig(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
-		id           component.ID
-		expected     component.Config
-		errorMessage string
+		id       component.ID
+		expected component.Config
 	}{
 		{
 			id: component.NewIDWithName(metadata.Type, ""),
@@ -92,28 +91,22 @@ func TestLoadConfig(t *testing.T) {
 			},
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_trace"),
-			errorMessage: "unable to parse OTTL statement: 1:18: unexpected token \"where\" (expected \")\" Key*)",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_trace"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "unknown_function_trace"),
-			errorMessage: "undefined function not_a_function",
+			id: component.NewIDWithName(metadata.Type, "unknown_function_trace"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_metric"),
-			errorMessage: "unable to parse OTTL statement: 1:18: unexpected token \"where\" (expected \")\" Key*)",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_metric"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "unknown_function_metric"),
-			errorMessage: "undefined function not_a_function",
+			id: component.NewIDWithName(metadata.Type, "unknown_function_metric"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "bad_syntax_log"),
-			errorMessage: "unable to parse OTTL statement: 1:18: unexpected token \"where\" (expected \")\" Key*)",
+			id: component.NewIDWithName(metadata.Type, "bad_syntax_log"),
 		},
 		{
-			id:           component.NewIDWithName(metadata.Type, "unknown_function_log"),
-			errorMessage: "undefined function not_a_function",
+			id: component.NewIDWithName(metadata.Type, "unknown_function_log"),
 		},
 	}
 	for _, tt := range tests {
@@ -129,7 +122,7 @@ func TestLoadConfig(t *testing.T) {
 			assert.NoError(t, component.UnmarshalConfig(sub, cfg))
 
 			if tt.expected == nil {
-				assert.EqualError(t, component.ValidateConfig(cfg), tt.errorMessage)
+				assert.Error(t, component.ValidateConfig(cfg))
 				return
 			}
 			assert.NoError(t, component.ValidateConfig(cfg))