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] Basic math capabilities #15711

Merged
merged 16 commits into from
Nov 10, 2022

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Oct 28, 2022

Description:
Adds basic math operations to the grammar. The grammar now supports +, -, *, and / for int64 and float64, as well as invocations and paths that return ints and floats. More types (strings, bytes, etc), could be supported in the future.

Since paths and invocations are supported, calculation of the expressions has to be done during data processing instead of startup. This means that Expressions are always captured as Getters, and in order for a function to support an Expression it must use a Getter. In the future I'd like to improve this by creating Typed Getters, but I believe that should be saved for a future PR.

To support both Expressions and literal values in the value struct I needed to reorganize the grammar so that it can differentiate between an Invocation/Path/Float/Int that is the start of an expression and an Invocation/Path/Float/Int that is "standalone". This is achieved by using the grammar library's negative lookahead feature. In order for negative lookahead to work correctly in the value's OR clause, it needed to appear only once. As a result, all grammar types that are supported by Expression were moved into a sub struct. This caused a lot of changes in the PR, but I do believe is necessary. See alecthomas/participle#279 for some details on how I landed at this conclusion.

Link to tracking Issue:
Closes #15675

Testing:
Added new unit tests

Documentation:
Update ottl grammar doc

@TylerHelmuth TylerHelmuth force-pushed the ottl-add-math-to-grammar branch from f1b755b to ac72c3d Compare November 3, 2022 15:53
@TylerHelmuth TylerHelmuth marked this pull request as ready for review November 3, 2022 19:11
@TylerHelmuth TylerHelmuth requested a review from a team November 3, 2022 19:11
@TylerHelmuth TylerHelmuth force-pushed the ottl-add-math-to-grammar branch from f31e5b2 to de58231 Compare November 3, 2022 19:43
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, I agree with your conclusion on the grammar changes based on what I've read. I will take a more in-depth look at the grammar on Monday.

One question I have is around the operations we want to support: do we expect to only have the four in this PR, or will we want more in the future (exponentiation, bitwise operations, etc.)? As it is right now, setting precedence for additional operations would cause the parsed expression to have a pretty deep structure.

pkg/ottl/README.md Outdated Show resolved Hide resolved
pkg/ottl/README.md Outdated Show resolved Hide resolved
pkg/ottl/README.md Outdated Show resolved Hide resolved
pkg/ottl/math_test.go Outdated Show resolved Hide resolved
pkg/ottl/math.go Outdated Show resolved Hide resolved
pkg/ottl/math.go Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Nov 4, 2022

will we want more in the future

@evan-bradley good question. I think we could expect to add those once there is a use case for them. Unfortunately the more operations we add the more precedence we have to handle and the deeper the expression structure will go. I don't think there is a good way to handle this as the Term/Factor/Value concept is really the best way to do precedence.

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me. After taking a second look, I agree with your conclusions about the grammar.

pkg/ottl/README.md Outdated Show resolved Hide resolved
@TylerHelmuth TylerHelmuth requested a review from kovrus November 9, 2022 17:24
pkg/ottl/README.md Outdated Show resolved Hide resolved
.chloggen/ottl-add-math-to-grammar.yaml Outdated Show resolved Hide resolved
pkg/ottl/README.md Outdated Show resolved Hide resolved
Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I have raised a couple of issues, but I do think they could be fixed here or saved for another day. In general this looks great, so in the interests of keeping things moving I'd be fine with merging this.

pkg/ottl/README.md Show resolved Hide resolved
pkg/ottl/functions.go Outdated Show resolved Hide resolved
pkg/ottl/lexer_test.go Outdated Show resolved Hide resolved
pkg/ottl/math.go Outdated Show resolved Hide resolved
{
name: "multiply large floats",
input: "1.79769313486231570814527423731704356798070e+308 * 1.79769313486231570814527423731704356798070e+308",
expected: math.Inf(0),
Copy link
Member

Choose a reason for hiding this comment

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

ok, this opens up a small can of worms. What happens if inf and NaN come through the pipeline? Do we support them as constants in ottl? Should we?

I can definitely imagine somebody wanting something like drop() where attributes["math_field"] == NaN

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya this was an outcome of testing I wasn't really expecting. I think +Inf isn't really a concern, but NaN will be useful once we can get in those situations. I think with the operations we've defined we will always avoid NaN tho.

pkg/ottl/parser.go Outdated Show resolved Hide resolved
@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Nov 10, 2022
@djaglowski djaglowski merged commit ee5bc50 into open-telemetry:main Nov 10, 2022
@TylerHelmuth TylerHelmuth deleted the ottl-add-math-to-grammar branch November 10, 2022 20:14
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/ottl ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Add math operators to the grammar
5 participants