open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.07k stars 2.37k forks source link

[pkg/ottl] Determine approach to looping #29289

Open evan-bradley opened 11 months ago

evan-bradley commented 11 months ago

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 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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

TylerHelmuth commented 11 months ago

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 commented 11 months ago

@evan-bradley I am curious if you've got a vision for how this solves https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27820

evan-bradley commented 11 months ago

@evan-bradley I am curious if you've got a vision for how this solves https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27820

It allows us to take the solution you outlined here https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/27820#issuecomment-1767416821 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 commented 11 months ago

@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 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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 commented 11 months ago

Sorry, I could have worded that better.

What I mean to say is that you outlined how to accomplish https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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.

github-actions[bot] commented 9 months ago

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.

jsuereth commented 9 months ago

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 commented 9 months ago

@jsuereth this is a cool option.

I believe we determined that this issue isn't required for https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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 https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/28892 would be extremely helpful.

TylerHelmuth commented 8 months ago

This feature will be useful for https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/31133