From 9b28f76c02c18f7479d10e4b6a95a21467fd85d6 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 14 Jul 2023 16:47:50 -0400 Subject: [PATCH] [processor/transform] Report all parsing errors (#24245) **Description:** Follow up from https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/23840 to report all errors that occur during parsing. The error isn't pretty, but this way users know all syntax errors in their OTTL statements up front. I played around with trying to format this to make it more readable, but wasn't able to come up with anything that was satisfactory. Example config and resulting error: ```yaml processors: transform: trace_statements: - context: span statements: - ParseJSON("...") - false - set[[]] metric_statements: - context: datapoint statements: - set(attributes, attributes) - true - set( ``` ``` Error: invalid configuration: processors::transform: unable to parse OTTL statement "ParseJSON(\"...\")": editor names must start with a lowercase letter but got 'ParseJSON'; unable to parse OTTL statement "0": statement has invalid syntax: 1:1: unexpected token "0"; unable to parse OTTL statement "set[[]]": statement has invalid syntax: 1:4: unexpected token "[" (expected "(" (Value ("," Value)*)? ")" Key*); unable to parse OTTL statement "1": statement has invalid syntax: 1:1: unexpected token "1"; unable to parse OTTL statement "set(": statement has invalid syntax: 1:5: unexpected token "" (expected ")" Key*) ``` I aggregated the errors both in the `Config` struct and on processor startup, but realistically are highly unlikely to get an error from the parser on startup because we've already validated the statements. I updated it just so we're consistent between validating the config and reporting errors from the OTTL parser. --------- Co-authored-by: Evan Bradley --- .chloggen/tp-report-all-parsing-errors.yaml | 20 +++++++++++++++++++ processor/transformprocessor/config.go | 13 +++++++----- processor/transformprocessor/config_test.go | 14 ++++++++++++- processor/transformprocessor/go.mod | 2 +- .../internal/logs/processor.go | 8 +++++++- .../internal/metrics/processor.go | 8 +++++++- .../internal/traces/processor.go | 8 +++++++- .../transformprocessor/testdata/config.yaml | 17 ++++++++++++++++ 8 files changed, 80 insertions(+), 10 deletions(-) create mode 100755 .chloggen/tp-report-all-parsing-errors.yaml diff --git a/.chloggen/tp-report-all-parsing-errors.yaml b/.chloggen/tp-report-all-parsing-errors.yaml new file mode 100755 index 000000000000..e3677b67b583 --- /dev/null +++ b/.chloggen/tp-report-all-parsing-errors.yaml @@ -0,0 +1,20 @@ +# 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: processor/transform + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Report all errors from parsing OTTL statements + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [24245] + +# (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: diff --git a/processor/transformprocessor/config.go b/processor/transformprocessor/config.go index 172d7ed90e84..c81700201aad 100644 --- a/processor/transformprocessor/config.go +++ b/processor/transformprocessor/config.go @@ -5,6 +5,7 @@ package transformprocessor // import "github.com/open-telemetry/opentelemetry-co import ( "go.opentelemetry.io/collector/component" + "go.uber.org/multierr" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" @@ -31,6 +32,8 @@ type Config struct { var _ component.Config = (*Config)(nil) func (c *Config) Validate() error { + var errors error + if len(c.TraceStatements) > 0 { pc, err := common.NewTraceParserCollection(component.TelemetrySettings{Logger: zap.NewNop()}, common.WithSpanParser(traces.SpanFunctions()), common.WithSpanEventParser(traces.SpanEventFunctions())) if err != nil { @@ -39,7 +42,7 @@ func (c *Config) Validate() error { for _, cs := range c.TraceStatements { _, err = pc.ParseContextStatements(cs) if err != nil { - return err + errors = multierr.Append(errors, err) } } } @@ -50,9 +53,9 @@ func (c *Config) Validate() error { return err } for _, cs := range c.MetricStatements { - _, err = pc.ParseContextStatements(cs) + _, err := pc.ParseContextStatements(cs) if err != nil { - return err + errors = multierr.Append(errors, err) } } } @@ -65,10 +68,10 @@ func (c *Config) Validate() error { for _, cs := range c.LogStatements { _, err = pc.ParseContextStatements(cs) if err != nil { - return err + errors = multierr.Append(errors, err) } } } - return nil + return errors } diff --git a/processor/transformprocessor/config_test.go b/processor/transformprocessor/config_test.go index add12415f9b0..256f69b9b6b7 100644 --- a/processor/transformprocessor/config_test.go +++ b/processor/transformprocessor/config_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap/confmaptest" + "go.uber.org/multierr" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" "github.com/open-telemetry/opentelemetry-collector-contrib/processor/transformprocessor/internal/common" @@ -22,6 +23,7 @@ func TestLoadConfig(t *testing.T) { tests := []struct { id component.ID expected component.Config + errorLen int }{ { id: component.NewIDWithName(metadata.Type, ""), @@ -108,6 +110,10 @@ func TestLoadConfig(t *testing.T) { { id: component.NewIDWithName(metadata.Type, "unknown_function_log"), }, + { + id: component.NewIDWithName(metadata.Type, "bad_syntax_multi_signal"), + errorLen: 3, + }, } for _, tt := range tests { t.Run(tt.id.String(), func(t *testing.T) { @@ -122,7 +128,13 @@ func TestLoadConfig(t *testing.T) { assert.NoError(t, component.UnmarshalConfig(sub, cfg)) if tt.expected == nil { - assert.Error(t, component.ValidateConfig(cfg)) + err = component.ValidateConfig(cfg) + assert.Error(t, err) + + if tt.errorLen > 0 { + assert.Equal(t, tt.errorLen, len(multierr.Errors(err))) + } + return } assert.NoError(t, component.ValidateConfig(cfg)) diff --git a/processor/transformprocessor/go.mod b/processor/transformprocessor/go.mod index a0b04946809b..14a3ca0d7fac 100644 --- a/processor/transformprocessor/go.mod +++ b/processor/transformprocessor/go.mod @@ -10,6 +10,7 @@ require ( go.opentelemetry.io/collector/consumer v0.81.0 go.opentelemetry.io/collector/pdata v1.0.0-rcv0013 go.opentelemetry.io/collector/processor v0.81.0 + go.uber.org/multierr v1.11.0 go.uber.org/zap v1.24.0 ) @@ -39,7 +40,6 @@ require ( go.opentelemetry.io/otel/metric v1.16.0 // indirect go.opentelemetry.io/otel/trace v1.16.0 // indirect go.uber.org/atomic v1.10.0 // indirect - go.uber.org/multierr v1.11.0 // indirect golang.org/x/exp v0.0.0-20230510235704-dd950f8aeaea // indirect golang.org/x/net v0.12.0 // indirect golang.org/x/sys v0.10.0 // indirect diff --git a/processor/transformprocessor/internal/logs/processor.go b/processor/transformprocessor/internal/logs/processor.go index 2f70f995a9a9..d9502e97762f 100644 --- a/processor/transformprocessor/internal/logs/processor.go +++ b/processor/transformprocessor/internal/logs/processor.go @@ -9,6 +9,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/pdata/plog" + "go.uber.org/multierr" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" @@ -27,14 +28,19 @@ func NewProcessor(contextStatements []common.ContextStatements, errorMode ottl.E } contexts := make([]consumer.Logs, len(contextStatements)) + var errors error for i, cs := range contextStatements { context, err := pc.ParseContextStatements(cs) if err != nil { - return nil, err + errors = multierr.Append(errors, err) } contexts[i] = context } + if errors != nil { + return nil, errors + } + return &Processor{ contexts: contexts, logger: settings.Logger, diff --git a/processor/transformprocessor/internal/metrics/processor.go b/processor/transformprocessor/internal/metrics/processor.go index 15da5273d205..135b1bcbad59 100644 --- a/processor/transformprocessor/internal/metrics/processor.go +++ b/processor/transformprocessor/internal/metrics/processor.go @@ -9,6 +9,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/pdata/pmetric" + "go.uber.org/multierr" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" @@ -27,14 +28,19 @@ func NewProcessor(contextStatements []common.ContextStatements, errorMode ottl.E } contexts := make([]consumer.Metrics, len(contextStatements)) + var errors error for i, cs := range contextStatements { context, err := pc.ParseContextStatements(cs) if err != nil { - return nil, err + errors = multierr.Append(errors, err) } contexts[i] = context } + if errors != nil { + return nil, errors + } + return &Processor{ contexts: contexts, logger: settings.Logger, diff --git a/processor/transformprocessor/internal/traces/processor.go b/processor/transformprocessor/internal/traces/processor.go index 33f58e1374e7..e20c87880ce3 100644 --- a/processor/transformprocessor/internal/traces/processor.go +++ b/processor/transformprocessor/internal/traces/processor.go @@ -9,6 +9,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/pdata/ptrace" + "go.uber.org/multierr" "go.uber.org/zap" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" @@ -27,14 +28,19 @@ func NewProcessor(contextStatements []common.ContextStatements, errorMode ottl.E } contexts := make([]consumer.Traces, len(contextStatements)) + var errors error for i, cs := range contextStatements { context, err := pc.ParseContextStatements(cs) if err != nil { - return nil, err + errors = multierr.Append(errors, err) } contexts[i] = context } + if errors != nil { + return nil, errors + } + return &Processor{ contexts: contexts, logger: settings.Logger, diff --git a/processor/transformprocessor/testdata/config.yaml b/processor/transformprocessor/testdata/config.yaml index f759932e959b..81a097e1098c 100644 --- a/processor/transformprocessor/testdata/config.yaml +++ b/processor/transformprocessor/testdata/config.yaml @@ -52,6 +52,23 @@ transform/bad_syntax_trace: - set(name, "bear" where attributes["http.path"] == "/animal" - keep_keys(attributes, ["http.method", "http.path"]) +transform/bad_syntax_multi_signal: + trace_statements: + - context: span + statements: + - set(name, "bear" where attributes["http.path"] == "/animal" + - keep_keys(attributes, ["http.method", "http.path"]) + metric_statements: + - context: datapoint + statements: + - set(name, "bear" where attributes["http.path"] == "/animal" + - keep_keys(attributes, ["http.method", "http.path"]) + log_statements: + - context: log + statements: + - set(body, "bear" where attributes["http.path"] == "/animal" + - keep_keys(attributes, ["http.method", "http.path"]) + transform/unknown_function_log: log_statements: - context: log