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] Determine approach to looping #29289

Open
evan-bradley opened this issue Nov 15, 2023 · 9 comments
Open

[pkg/ottl] Determine approach to looping #29289

evan-bradley opened this issue Nov 15, 2023 · 9 comments
Labels
discussion needed Community discussion needed enhancement New feature or request never stale Issues marked with this label will be never staled and automatically removed pkg/ottl

Comments

@evan-bradley
Copy link
Contributor

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

Looping over maps or lists is required to perform a common operation on each item. Currently we handle modifying these structures by having Editors that handle performing some operation while looping over the structure.

However, having editors abstract over looping sometimes presents difficulties when the operations need to be customizable. This requires putting a lot of parameters on the Editor to allow the user to change the operation they're looking to do. This situation can be currently seen in #27820, but has come up a number of times.

Describe the solution you'd like

Determine how we want to handle loops in OTTL. I see a few approaches here:

  1. Handle all loops inside Editors as they currently are and document that this is the process for handling loops. Editors that need a lot of customization will likely have to deal with an extensive set of parameters for cases where they need to handle broad use-cases.
  2. Provide for-each loops that allow looping over maps and lists. This would look similar to for key, val in attributes or for i, val in attributes["list"]. I think this could be a good option between flexibility and complexity if done carefully.
  3. Provide a way to define functions in OTTL and allow Editors to take these functions. This could be used to simplify some aspects of approach 1.
  4. Provide a standard suite of functional programming-style functions (filter, map, reduce, etc.) to allow handling these types. I think this would likely get too complicated too quickly.

Describe alternatives you've considered

Restrict the use-cases the OTTL standard function suite supports and require users to define their own functions in more cases. I'd prefer to instead make it easier for OTTL to support a wide range of cases

Additional context

No response

@evan-bradley evan-bradley added enhancement New feature or request needs triage New item requiring triage pkg/ottl discussion needed Community discussion needed and removed needs triage New item requiring triage labels Nov 15, 2023
@TylerHelmuth
Copy link
Member

I prefer option 2. I feels pretty natural to write "for each item in this path, perform this statement". Something like

for item in attributes, replace_pattern(item, "password=(.*)", "password=****") where IsString(item)

feels pretty natural from a syntax perspective. I don't know yet how we'd pass item as a path later in the statement. Maybe item is a keyword that OTTL can know not to pass to the Context's PathParser and we'd have some fancy getter that handles grabbing items from attributes and passing them on?

@TylerHelmuth
Copy link
Member

@evan-bradley I am curious if you've got a vision for how this solves #27820

@evan-bradley
Copy link
Contributor Author

@evan-bradley I am curious if you've got a vision for how this solves #27820

It allows us to take the solution you outlined here #27820 (comment) and extend it to replace_all_patterns. If we have a way to allow users to do more complex operations with map and list structures, we can put better guardrails around what's included in the Editors.

@TylerHelmuth
Copy link
Member

@evan-bradley I don't see yet how to apply the looping functionality to an editor that handles the looping itself. My current mental model for this feature is to loop over the items and reference individual items in the statement. With that model I don't see how we'd apply the loops to an editor like replace_all_patterns which already has the looping built in.

In my head the issue in #27820 is that we have no good way to pass only a specific capture group to the option Function parameter. It isn't clear to me yet how a looping construct in the language solves that problem.

@evan-bradley
Copy link
Contributor Author

Sorry, I could have worded that better.

What I mean to say is that you outlined how to accomplish #27820 on a single string using what already exists in OTTL. This means that if we can't offer this functionality cleanly from replace_pattern, users still have a way of doing this.

However, users are unable perform this operation across all items in a map since there's no way to loop, which would motivate us to make changes to the function parameters in replace_all_patterns to accommodate this. If we instead use loops to solve this, we can apply your solution to this case as well.

I don't know that this solves #27820 so much as it would allow us to more carefully consider whether we think the tradeoff of function parameter complexity vs. statement complexity is worth it. The reason I brought it up is because it showcases how putting a lot of functionality inside Editors may have drawbacks. There are other issues that could also demonstrate this point.

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 Jan 15, 2024
@evan-bradley evan-bradley added never stale Issues marked with this label will be never staled and automatically removed and removed Stale labels Jan 16, 2024
@jsuereth
Copy link
Contributor

hey folks -

I spent a few hours prototyping an OTTL compiler + type system. I'd like to suggest avoiding imperative iteration and use something closer to list-comprehensions (python, typescript, e.g.).

Specifically, I think you could have something like:

set(attributes, from attributes a where a.key in ["my", "list", "of", "keys"] yield a)

You have a lot of options for syntax, e.g.

expr := for {id} in {expr} (where {expr})? yield {expr} | ...your other expressions...

I'm happy to help contribute this. I have a larger more formal proposal in addition.

@TylerHelmuth
Copy link
Member

@jsuereth this is a cool option.

I believe we determined that this issue isn't required for #28892 so I haven't been thinking about it much. We are open to solutions, and if you're interested in contributing to OTTL solving any of the issues in #28892 would be extremely helpful.

@TylerHelmuth
Copy link
Member

This feature will be useful for #31133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Community discussion needed enhancement New feature or request never stale Issues marked with this label will be never staled and automatically removed pkg/ottl
Projects
None yet
Development

No branches or pull requests

3 participants