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][draft] Update statements to reflect their context #35050

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

edmocosta
Copy link
Contributor

Description:

This PR is a draft implementation of the issue: #29017, and introduces the necessary ottl and transformprocessor changes to make it possible for statements express their context via path names.

Please note that just a few unit tests were added at the moment, it's a WIP PR and the goal is to provide a full picture to the community and discuss the implemented solution. Individuals PRs will spawn from this draft, including extra changes, tests, docs, changelogs, etc.

Changelog


ottl/grammar

  • It now parses the first path segment as the Context name, for example, the path foo.bar.field would be parsed as the following AST example: {Context: "foo", Fields: [{Name: "bar"}, {Name: "field"}]}, instead of {Fields: [{Name: "foo"},{Name: "bar"}, {Name: "field"}]}. This new logic is enabled/disabled by setting the parser option WithPathContextNames. When it's enabled, valid path's context are required.
  • A new field Pos lexer.Position was added to the path object, it's handled/filled by participle lexer automatically, and is used to infer the statements path's context and for the appending missing context utility.

ottl/parser_collection (*new)

This utility is based on the existing transformprocessor's ParserCollection concept and is used to group context's parsers, abstracting the logic to infer the context from the statements, and/or rewriting them adding their paths's context if necessary.

In summary, the general idea is: given the configured contexts, parsers and statements converters, pick the right parser, and transform the parsed result (ottl.Statements[K]) into something common to the parser collection ([R]). For more details, please have a look at the transformprocessor refactoring using this utility.

Given ottl.Parse[K]r's have different type restrictions/generics (e.g. TransformContext), this utility heavily rely on the Go's reflection api, being the exposed interface type-safe, but manipulating untyped objects under the hood.

Considering this code is only executed during the bootstrap, IMO, it should be fine using reflection. I couldn't think about any other solution that would make this utility as generic as it's with reflection. Another approach would be hard-code all the existing ottl contexts's parsers on the collection, having fixed WithParserX functions per context and generating a direct dependency on the ottl/contexts package.

Suggestions are very welcome!

ottl/context_inferrer (*new)

This utility's goal is to infer the context from the provided statements. To do so, it extract all the statements's path, comparing the path.Context values, and choosing the highest one according to the provided context names list order.

Topics to consider/discuss:

  • The default inferrer implementation - by default - does not skip unknown contexts, it only take them from the paths, and prioritise it based on the oder list, which does not necessarily need to contains all found contexts. In summary, if the statement is invalid, it might return a value that's not a valid context for the parser/parser collection. For example, if the statement only contains a path like foo.bar , and the order list is [span, spanevent], it would return foo. Either way, that shouldn't be an issue considering the statement is invalid and an error will be raised.

statement rewrite utility (*new)

A new function was added to the Parser[K] (AppendStatementPathsContext), which main goal is to take a statement without path's contexts and append the main context value to it. Path's which context value matches any parser WithPathContextNames are not modified.

Under the hood, it relies on the new grammar path's field Pos lexer.Position to insert the missing context names.
For now, this utility is being used by the parser collection ParseStatementsWithContext function, which purpose is to parse "old" style statements, which path's does not contain the context prefix.

transformprocessor

It was refactored to use the new parser utilities and to support mixed configuration styles, example:

trace_statements:
  - set(span.status.code, 1) where span.attributes["http.path"] == "/health"
  - set(span.status.code, 1) where span.attributes["http.path"] == "/health"
  - context: spanevent
     statements:
       - set(attributes["domain"], "test.com")
  • When the flat configuration mode is used, it requires statements's paths to have a valid context prefix, and relies on the context inferrer to choose the right parser.
  • For the existing structured configuration mode, it relies on the parser collection & rewrite utility to transform and parse the statements under the configured context: value.

Link to tracking Issue: #29017

Testing:

  • Just a few unit tests for validation purposes , it's still lacking several test cases

Documentation:

  • No documentation changes were made at this point.

…lector-contrib into ottl_change_statements_reflect_context_draft

# Conflicts:
#	pkg/ottl/grammar.go
#	pkg/ottl/parser_test.go
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 25, 2024
evan-bradley pushed a commit that referenced this pull request Oct 7, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

This PR is part of
#29017,
and adds a grammar utility to allow extracting `ottl.path` from parsed
statements and expressions. The new functions will power the context
inferrer utility (see
[draft](#35050)),
which implementation will basically gather all declared statement's
`Path.Context`, and choose the highest priority one.

For the statements rewriter utility purpose, it changes the `grammar.go`
file including a new field `Pos lexer.Position` into the `ottl.path`
struct. This value is automatically set by the `participle` lexer and
holds the path's offsets, being useful for identifying their positions
in the raw statement, without using regexes an being coupled to the
grammar's definition.

**Additional changes**:

This proposal uses the visitor pattern to gather all path's from the
parsed statements. Considering the grammar's custom error mechanism
(`checkForCustomError`) also works with a similar pattern, I've added
the visitor implementation as part of the grammar objects, and
refactored all `checkForCustomError` functions to be part of a validator
visitor. Differently of the current implementation, it joins and
displays all errors at once instead of one by one, IMO, it's specially
useful for statements with more than one error, for example: `set(name,
int(2), float(1))`.

*&ast;* We can change it back do be error-by-error if necessary.
*&ast;* If modifying the custom validator is not desired, I can roll
those changes back keeping only the necessary code to extract the path's
+ `Pos` field.

**Link to tracking Issue:**
#29017

**Testing:** Unit tests were added

**Documentation:** No changes
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Oct 7, 2024
…-telemetry#35174)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

This PR is part of
open-telemetry#29017,
and adds a grammar utility to allow extracting `ottl.path` from parsed
statements and expressions. The new functions will power the context
inferrer utility (see
[draft](open-telemetry#35050)),
which implementation will basically gather all declared statement's
`Path.Context`, and choose the highest priority one.

For the statements rewriter utility purpose, it changes the `grammar.go`
file including a new field `Pos lexer.Position` into the `ottl.path`
struct. This value is automatically set by the `participle` lexer and
holds the path's offsets, being useful for identifying their positions
in the raw statement, without using regexes an being coupled to the
grammar's definition.

**Additional changes**:

This proposal uses the visitor pattern to gather all path's from the
parsed statements. Considering the grammar's custom error mechanism
(`checkForCustomError`) also works with a similar pattern, I've added
the visitor implementation as part of the grammar objects, and
refactored all `checkForCustomError` functions to be part of a validator
visitor. Differently of the current implementation, it joins and
displays all errors at once instead of one by one, IMO, it's specially
useful for statements with more than one error, for example: `set(name,
int(2), float(1))`.

*&ast;* We can change it back do be error-by-error if necessary.
*&ast;* If modifying the custom validator is not desired, I can roll
those changes back keeping only the necessary code to extract the path's
+ `Pos` field.

**Link to tracking Issue:**
open-telemetry#29017

**Testing:** Unit tests were added

**Documentation:** No changes
…lector-contrib into ottl_change_statements_reflect_context_draft

# Conflicts:
#	pkg/ottl/paths.go
#	pkg/ottl/paths_test.go
ghost pushed a commit to sematext/opentelemetry-collector-contrib that referenced this pull request Oct 9, 2024
…-telemetry#35174)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

This PR is part of
open-telemetry#29017,
and adds a grammar utility to allow extracting `ottl.path` from parsed
statements and expressions. The new functions will power the context
inferrer utility (see
[draft](open-telemetry#35050)),
which implementation will basically gather all declared statement's
`Path.Context`, and choose the highest priority one.

For the statements rewriter utility purpose, it changes the `grammar.go`
file including a new field `Pos lexer.Position` into the `ottl.path`
struct. This value is automatically set by the `participle` lexer and
holds the path's offsets, being useful for identifying their positions
in the raw statement, without using regexes an being coupled to the
grammar's definition.

**Additional changes**:

This proposal uses the visitor pattern to gather all path's from the
parsed statements. Considering the grammar's custom error mechanism
(`checkForCustomError`) also works with a similar pattern, I've added
the visitor implementation as part of the grammar objects, and
refactored all `checkForCustomError` functions to be part of a validator
visitor. Differently of the current implementation, it joins and
displays all errors at once instead of one by one, IMO, it's specially
useful for statements with more than one error, for example: `set(name,
int(2), float(1))`.

*&ast;* We can change it back do be error-by-error if necessary.
*&ast;* If modifying the custom validator is not desired, I can roll
those changes back keeping only the necessary code to extract the path's
+ `Pos` field.

**Link to tracking Issue:**
open-telemetry#29017

**Testing:** Unit tests were added

**Documentation:** No changes
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 23, 2024
TylerHelmuth pushed a commit that referenced this pull request Oct 31, 2024
… paths context (#35716)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR is part of
#29017,
and adds the `ottl.Parser[K].AppendStatementPathsContext` function,
allowing components to rewrite statements appending missing `ottl.path`
context names.

For examples, the following context-less statement:

```
set(value, 1) where name == attributes["foo.name"]
```

Would be rewritten using the `span` context as:

```

set(span.value, 1) where span.name == span.attributes["foo.name"]
```

**Why do we need to rewrite statements?**

This utility will be used during the transition from structured OTTL
statements to flat statements.
Components such as the `transformprocessor` will leverage it to support
both configuration styles, without forcing
users to adapt/rewrite their existing config files. 

Once the component turns on the `ottl.Parser[K]` path's context
validation, new configuration style usages will be validated, requiring
all paths to have a context prefix, and old configuration styles will
automatically rewrite the statements using this function.

For more details, please have a look at the complete
[draft](#35050)
implementation.

<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

#29017

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Describe the documentation added.-->
#### Documentation
No changes

<!--Please delete paragraphs that you did not use before submitting.-->
…lector-contrib into ottl_change_statements_reflect_context_draft

# Conflicts:
#	pkg/ottl/context_inferrer.go
#	pkg/ottl/context_inferrer_test.go
@edmocosta edmocosta force-pushed the ottl_change_statements_reflect_context_draft branch from 93dd1c2 to 3c37b13 Compare November 4, 2024 18:22
ArthurSens pushed a commit to ArthurSens/opentelemetry-collector-contrib that referenced this pull request Nov 4, 2024
… paths context (open-telemetry#35716)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR is part of
open-telemetry#29017,
and adds the `ottl.Parser[K].AppendStatementPathsContext` function,
allowing components to rewrite statements appending missing `ottl.path`
context names.

For examples, the following context-less statement:

```
set(value, 1) where name == attributes["foo.name"]
```

Would be rewritten using the `span` context as:

```

set(span.value, 1) where span.name == span.attributes["foo.name"]
```

**Why do we need to rewrite statements?**

This utility will be used during the transition from structured OTTL
statements to flat statements.
Components such as the `transformprocessor` will leverage it to support
both configuration styles, without forcing
users to adapt/rewrite their existing config files. 

Once the component turns on the `ottl.Parser[K]` path's context
validation, new configuration style usages will be validated, requiring
all paths to have a context prefix, and old configuration styles will
automatically rewrite the statements using this function.

For more details, please have a look at the complete
[draft](open-telemetry#35050)
implementation.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

open-telemetry#29017

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Describe the documentation added.-->
#### Documentation
No changes

<!--Please delete paragraphs that you did not use before submitting.-->
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 19, 2024
@evan-bradley evan-bradley added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Nov 19, 2024
@evan-bradley
Copy link
Contributor

Marking this as never-stale since it will help inform us on the approach for #29017 as we implement the actual PRs. This issue is high-priority, so we won't forget about this PR.

sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
… paths context (open-telemetry#35716)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

This PR is part of
open-telemetry#29017,
and adds the `ottl.Parser[K].AppendStatementPathsContext` function,
allowing components to rewrite statements appending missing `ottl.path`
context names.

For examples, the following context-less statement:

```
set(value, 1) where name == attributes["foo.name"]
```

Would be rewritten using the `span` context as:

```

set(span.value, 1) where span.name == span.attributes["foo.name"]
```

**Why do we need to rewrite statements?**

This utility will be used during the transition from structured OTTL
statements to flat statements.
Components such as the `transformprocessor` will leverage it to support
both configuration styles, without forcing
users to adapt/rewrite their existing config files. 

Once the component turns on the `ottl.Parser[K]` path's context
validation, new configuration style usages will be validated, requiring
all paths to have a context prefix, and old configuration styles will
automatically rewrite the statements using this function.

For more details, please have a look at the complete
[draft](open-telemetry#35050)
implementation.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue

open-telemetry#29017

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Unit tests

<!--Describe the documentation added.-->
#### Documentation
No changes

<!--Please delete paragraphs that you did not use before submitting.-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
never stale Issues marked with this label will be never staled and automatically removed pkg/ottl processor/transform Transform processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants