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] Add parser utility to rewrite statements appending missing paths context #35716

Conversation

edmocosta
Copy link
Contributor

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 implementation.

Link to tracking issue

#29017

Testing

Unit tests

Documentation

No changes

@github-actions github-actions bot requested a review from kentquirk October 9, 2024 12:22
@edmocosta edmocosta changed the title [pkg/ottl] Add ottl.Parser utility to rewrite statements appending missing paths context [pkg/ottl] Add parser utility to rewrite statements appending missing paths context Oct 9, 2024
pkg/ottl/paths.go Outdated Show resolved Hide resolved
pkg/ottl/paths.go Outdated Show resolved Hide resolved
pkg/ottl/paths.go Outdated Show resolved Hide resolved
@edmocosta
Copy link
Contributor Author

I've moved the AppendStatementPathsContext into the parser.go file as @TylerHelmuth suggested.
Do you folks have any thoughts on this #35716 (comment)? Thank you!

pkg/ottl/parser.go Outdated Show resolved Hide resolved
pkg/ottl/parser.go Outdated Show resolved Hide resolved
@edmocosta edmocosta force-pushed the ottl-add-statement-context-append-utility branch from 1be9d9c to 8225cec Compare October 31, 2024 12:14
@edmocosta
Copy link
Contributor Author

edmocosta commented Oct 31, 2024

@TylerHelmuth @evan-bradley, I've addressed Evan's suggestions at 8225cec
In addition to that, I've include two safe guards, one for invalid offsets so it won't panic if somehow invalid values are passed on, and another to ensure the offsets are sorted, otherwise the function wouldn't work.

It should not receive invalid offsets, but considering those values are coming from the grammar lib, I think it's ok to handle them and provide an error message instead of panicking.

Currently, the offsets values are sorted, but that assumes that the grammar visitor will never change the order it collects the paths, which IMO, is not a commitment we should make.

Please let me know if you prefer to not add those conditions. Thanks!

@evan-bradley evan-bradley added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 31, 2024
@evan-bradley
Copy link
Contributor

It's probably not strictly necessary to add checks since this is an internal function, but the checks are easy and I think it's good to be cautious. Thanks for thinking ahead!

@TylerHelmuth TylerHelmuth merged commit 909b1d3 into open-telemetry:main Oct 31, 2024
167 of 168 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 31, 2024
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.-->
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
pkg/ottl Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants