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] Support dynamic indexing #36719

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Dec 9, 2024

Description

Link to tracking issue

Fixes #36644

@odubajDT odubajDT force-pushed the indexing-pkg-ottl branch 5 times, most recently from 1577fcb to 87f2d33 Compare December 18, 2024 10:06
@odubajDT odubajDT force-pushed the indexing-pkg-ottl branch 2 times, most recently from d39f561 to 5e92cc6 Compare December 19, 2024 09:20
@odubajDT odubajDT marked this pull request as ready for review December 19, 2024 09:43
@odubajDT odubajDT requested a review from a team as a code owner December 19, 2024 09: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.

Thanks for taking this one on @odubajDT. I have a few questions on the implementation, but based on the tests passing, I think we're definitely on the right track.

pkg/ottl/functions.go Outdated Show resolved Hide resolved
pkg/ottl/expression.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/path.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/map.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/slice.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/value.go Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/value.go Outdated Show resolved Hide resolved
pkg/ottl/functions.go Outdated Show resolved Hide resolved
pkg/ottl/functions.go Outdated Show resolved Hide resolved
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.

This is so exciting! We've wanted this feature for a long time and I'm glad to see that it fit so nicely into our Path/Key pattern.

Can you add e2e tests that use the new ottl-context scoped syntax? I want to ensure statements like

set(log.attributes[resource.attributes["flags"]], "something33")

work.

@odubajDT odubajDT force-pushed the indexing-pkg-ottl branch 2 times, most recently from 9617f82 to 8d87af0 Compare January 8, 2025 06:50
@odubajDT
Copy link
Contributor Author

odubajDT commented Jan 8, 2025

This is so exciting! We've wanted this feature for a long time and I'm glad to see that it fit so nicely into our Path/Key pattern.

Can you add e2e tests that use the new ottl-context scoped syntax? I want to ensure statements like

set(log.attributes[resource.attributes["flags"]], "something33")

work.

Should be done, please have a look

Copy link
Contributor

@edmocosta edmocosta left a comment

Choose a reason for hiding this comment

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

I think we should add some extra type validations. Considering the ottl.Key index can now be an expression (mathExprLiteral), statements like set(severity_text, attributes[1]) and set(attributes["slice"][0.0], "bar") would be considered valid and they would panic on the FetchValueFromExpression function. I know they're invalid usages of the OTTL syntax, but I'd prefer to not panic if that happens. WDYT?

pkg/ottl/functions.go Show resolved Hide resolved
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
@odubajDT
Copy link
Contributor Author

odubajDT commented Jan 8, 2025

I think we should add some extra type validations. Considering the ottl.Key index can now be an expression (mathExprLiteral), statements like set(severity_text, attributes[1]) and set(attributes["slice"][0.0], "bar") would be considered valid and they would panic on the FetchValueFromExpression function. I know they're invalid usages of the OTTL syntax, but I'd prefer to not panic if that happens. WDYT?

You got a point here. Added some additional checks and polished the error messages to be clearer where the problem lays. Also added some e2e tests.

Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Allow indexing maps and slices with dynamic values
5 participants