Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] Improve OTTL parser error reporting #23840

Merged

Conversation

evan-bradley
Copy link
Contributor

Description:

I was playing with an erroneous config like the following and noticed that the error reporting was confusing:

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

@evan-bradley
Copy link
Contributor Author

We can improve message formatting in follow-up PRs by having components using OTTL attempt to parse all statements and print a formatted message using the multierr error.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, it brings the error handling in line with how the Execute functions work. Please handle CI failures.

@github-actions github-actions bot added processor/filter Filter processor processor/transform Transform processor labels Jun 30, 2023
@evan-bradley evan-bradley merged commit a0883a0 into open-telemetry:main Jul 12, 2023
88 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 12, 2023
evan-bradley added a commit that referenced this pull request Jul 14, 2023
**Description:**

Follow up from
#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 "<EOF>" (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 <evan-bradley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/ottl processor/filter Filter processor processor/transform Transform processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants