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] expose a parser explicitly for parsing conditions #13545

Closed
Tracked by #28892
TylerHelmuth opened this issue Aug 23, 2022 · 7 comments · Fixed by #29598
Closed
Tracked by #28892

[pkg/ottl] expose a parser explicitly for parsing conditions #13545

TylerHelmuth opened this issue Aug 23, 2022 · 7 comments · Fixed by #29598
Assignees
Labels
never stale Issues marked with this label will be never staled and automatically removed pkg/ottl priority:p1 High

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Aug 23, 2022

Is your feature request related to a problem? Please describe.
The OTTL package provides a function for parsing Queries, but that is all. There is no solution for parsing only a OTTL condition. This limits other components to requiring functions where they may only need a condition.

Describe the solution you'd like
Refactor the tql package to provide NewParsers to parse Queries and Conditions

Describe alternatives you've considered
Add a new Noop function that does nothing so that queries can be parsed and then the related condition can be used.

Additional context

Related to #13158. Also related to general idea that OTTL can be used as generic filter for any component.

@TylerHelmuth
Copy link
Member Author

/cc @kentquirk @bogdandrutu

@TylerHelmuth TylerHelmuth self-assigned this Aug 24, 2022
@TylerHelmuth
Copy link
Member Author

#13544 introduces a parser refactor that will be necessary for the refactor this issue requires.

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Nov 10, 2022
@TylerHelmuth TylerHelmuth added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Nov 10, 2022
@djaglowski
Copy link
Member

+1 for this idea.

I think condition matching would be extremely useful in and of itself. Executing a function when a condition is met is a secondary use case in my opinion.

@TylerHelmuth
Copy link
Member Author

@kovrus you had expressed interest with this in the past. Is it still on your radar?

@TylerHelmuth TylerHelmuth changed the title [pkg/telemetryquerylanguage] expose a parser explicitly for parsing conditions [pkg/ottl] expose a parser explicitly for parsing conditions Feb 14, 2023
@kovrus
Copy link
Member

kovrus commented Feb 15, 2023

@TylerHelmuth yes, I think we can reopen #14783 and I can follow your suggestion

I was instead anticipating that components that rely on OTTL would use either different config
sections, or in the case of the routing processor, a type field that would identify an expression 
as Transformation or Condition.

when integration this change into the routing processor.

@TylerHelmuth TylerHelmuth removed their assignment May 24, 2023
@TylerHelmuth TylerHelmuth added the help wanted Extra attention is needed label Oct 6, 2023
TylerHelmuth added a commit that referenced this issue Oct 10, 2023
**Description:** 
We noticed that if you pass in a condition that fails parsing that the
error message includes `drop() where ...` and it was jarring to see
`drop` as the function when using filterottl to generate a condition
that was not related to dropping data. While we wait for the ability to
parse conditions in isolation
(#13545),
this change makes `filterottl` a little friendlier.

**Testing:** <Describe what testing was performed and which tests were
added.>
Updated unit tests
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
…ry#27471)

**Description:** 
We noticed that if you pass in a condition that fails parsing that the
error message includes `drop() where ...` and it was jarring to see
`drop` as the function when using filterottl to generate a condition
that was not related to dropping data. While we wait for the ability to
parse conditions in isolation
(open-telemetry#13545),
this change makes `filterottl` a little friendlier.

**Testing:** <Describe what testing was performed and which tests were
added.>
Updated unit tests
@TylerHelmuth TylerHelmuth self-assigned this Nov 15, 2023
@TylerHelmuth TylerHelmuth removed the help wanted Extra attention is needed label Nov 15, 2023
@TylerHelmuth
Copy link
Member Author

With the progress made in internal/filter and more components looking to use OTTL only for conditions I've come back to this issue.

Last year we had a lot of discussion around whether or not OTTL should combine the parsing of statements (ottl strings that contain editors and possible a where clause) and conditions (ottl strings that are only the boolean expression of a where clause). A lot of this discussion came from the routingprocessor's desire to use OTTL to make decisions and transform data.

Since then OTTL and the collector have continued to grow. The collector now supports connects and the routing connector, which is deprecating the routingprocessor, does not have the need to transform telemetry since it allows moving data to a second pipeline. OTTL introduced the Statements struct to supplement a slice of Statemnentand how it interacts with ErrorMode. We've also added internal/filteottl to allow components to use OTTL to make decisions.

After talking with @evan-bradley we feel keeping statements and condition separate will allow each OTTL feature to grow independently, which is important since both features a distinct. By adding distinct parsing functions to the Parser we force components to make a conscious decision about how they want to use OTTL, but still allowing users to use both features should they need.

TylerHelmuth added a commit that referenced this issue Nov 17, 2023
**Description:** 
This PR adds a new public API to support parsing and using conditions. 

This implementation opted to add ParseConditions directly to the
existing Parser instead of creating a second Parser struct. Since any
Parser needs the Context, Functions, PathExpressionParser, EnumParser,
and telemetry settings the 2 structs would be incredibly similar.

See these structs implemented in components here:
#29294

**Link to tracking Issue:** <Issue number if applicable>
Works towards
#13545

**Testing:** <Describe what testing was performed and which tests were
added.>
Added tests

**Documentation:** <Describe the documentation added.>
Added godoc strings
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
**Description:** 
This PR adds a new public API to support parsing and using conditions. 

This implementation opted to add ParseConditions directly to the
existing Parser instead of creating a second Parser struct. Since any
Parser needs the Context, Functions, PathExpressionParser, EnumParser,
and telemetry settings the 2 structs would be incredibly similar.

See these structs implemented in components here:
open-telemetry#29294

**Link to tracking Issue:** <Issue number if applicable>
Works towards
open-telemetry#13545

**Testing:** <Describe what testing was performed and which tests were
added.>
Added tests

**Documentation:** <Describe the documentation added.>
Added godoc strings
TylerHelmuth added a commit that referenced this issue Nov 30, 2023
**Description:** 

Adds a new `ConditionSequence` struct to help handle processing a list
of Conditions. The primary reason to use a `ConditionSequence` is to let
the struct handle errors. Since that is its defining purpose, I opted to
make `ErrorMode` a required argument in the "constructor" instead of an
Option.

I also update `internal/filterottl` to use this struct instead of
`Statements`. This is a non-breaking change.

If we like this pattern, I will do a breaking change to replace
`Statements` with a similar `StatementSequence` struct in a future PR .

See these structs implemented in components here:
#29294

**Link to tracking Issue:** <Issue number if applicable>

Related to
#13545

**Testing:**

Added new tests

**Documentation:** <Describe the documentation added.>
Added godoc comments

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
evan-bradley pushed a commit that referenced this issue Dec 1, 2023
**Description:** 
This PR brings the `Statements` API up-to-date with the
`ConditionSequence` API.

**Link to tracking Issue:** <Issue number if applicable>
Closes
#13545

**Testing:** 
Unit tests

**Documentation:** <Describe the documentation added.>
Filled in godoc comments.
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 priority:p1 High
Projects
None yet
3 participants