[pkg/ottl]: Add a new Function Getter to the OTTL package, to allow passing Converters as literal parameters

[rnishtala-sumo]

Description: Add a new Function Getter to the OTTL package, to allow passing Converters as literal parameters

Link to tracking Issue:

Testing: Added a unit test and also tested the following configuration

replace_pattern(attributes["message"], "device=*", attributes["device_name"], SHA256)

A tentative example of how the literal function gets invoked is

type ReplacePatternArguments[K any] struct {
	Target       ottl.GetSetter[K]      `ottlarg:"0"`
	RegexPattern string                 `ottlarg:"1"`
	Replacement  ottl.StringGetter[K]   `ottlarg:"2"`
	Function     ottl.FunctionGetter[K] `ottlarg:"3"`
}

func replacePattern[K any](target ottl.GetSetter[K], regexPattern string, replacement ottl.StringGetter[K], fn ottl.Expr[K]) (ottl.ExprFunc[K], error) {
	compiledPattern, err := regexp.Compile(regexPattern)
	if err != nil {
		return nil, fmt.Errorf("the regex pattern supplied to replace_pattern is not a valid pattern: %w", err)
	}
	return func(ctx context.Context, tCtx K) (interface{}, error) {
		originalVal, err := target.Get(ctx, tCtx)
		if err != nil {
			return nil, err
		}
		replacementVal, err := fn.Eval(ctx, tCtx)
		if err != nil {
			return nil, err
		}
          .......
          updatedStr := compiledPattern.ReplaceAllString(originalValStr, replacementValHash.(string))
}

[TylerHelmuth]

@rnishtala-sumo can you add some tests to functions_test.go to show the parser can actually inject FunctionGetters correctly?

[TylerHelmuth]

@evan-bradley this issue is certainly complicated. I want to make sure we're still on track with the right idea, can you take a look.

[evan-bradley]

@evan-bradley this issue is certainly complicated. I want to make sure we're still on track with the right idea, can you take a look.

Sure thing. I have a few ideas, but I want to validate them before I endorse anything. I'll take a look hopefully in the next day or two and report back here.

[rnishtala-sumo]

@TylerHelmuth I pushed an update and described the steps here: #24356 (comment)

I also added a test for the new Caller which parses a statement to initialize a converter and then invokes it to return a result.

[evan-bradley]

Here's a draft implementation of FunctionGetter. My goal here was to do as much validation and reflection up front and only run the actual underlying OTTL function in the hot path. This is definitely just a first pass, I think some work could still be done to improve reliability and the developer experience here. @rnishtala-sumo @TylerHelmuth let me know what you think.
evan-bradley@b6edc4b

[rnishtala-sumo]

@evan-bradley The approach you're suggesting makes sense to me. I like the way that the function is being created and returned here, ready for evaluation. And then we just pass in the function and evaluate in the hot path. I like it and am definitely on-board with this approach.

[rnishtala-sumo]

@evan-bradley one question I have about this approach is, how are we resolving from a converter's canonical name to its factory function - for example below

replace_pattern(attributes["message"], "device=*", attributes["device_name"], SHA256)

we pass in the canonical name SHA256 but eventually we want to call NewSHA256Factory. An example of this is seen in the newFunctionCall definition here. I thought we could re-use newFunctionCall and that's what I did in my last commit.

[evan-bradley]

The way it's implemented is essentially a stripped down version of newFunctionCall. We do the mapping of name -> factory here, which uses the same map that newFunctionCall uses.

[rnishtala-sumo]

@evan-bradley would you like to add to this commit and raise a PR or could I prepare a new PR with this approach along with tests?

@TylerHelmuth are you in agreement with Evan's approach?

[evan-bradley]

@evan-bradley would you like to add to this commit and raise a PR or could I prepare a new PR with this approach along with tests?

Go for it. Feel free to use anything from my commit.

[TylerHelmuth]

I don't have the bandwidth to review this PR in depth today, but @evan-bradley is OTTL's function wizard, so I'm good with whatever he's suggesting.

[rnishtala-sumo]

@TylerHelmuth @evan-bradley all the tests seem to indicate that these changes should work. Please let me know if you find anything else.

[TylerHelmuth]

@rnishtala-sumo thank you for working on this and sticking with it! Definitely a complicated addition.

[evan-bradley]

Thanks for your persistence in working on this. I'd say we're really close.

[evan-bradley]

@rnishtala-sumo After reviewing your changes, I realized that the ottlarg struct tag isn't really worth the complexity. I spoke about it offline with @TylerHelmuth and we agreed that it would be okay to deprecate it. I opened #24874 to track this. It may save you some effort in implementing handling the struct tag if that issue is resolved prior to this PR getting merged. If you'd like to push ahead with this instead and work with the struct tag, that's also okay.

[rnishtala-sumo]

@evan-bradley I would like to get this in with the struct tag and let any breaking changes introduced by removing the 'ottlarg` struct tag be its own PR. While I agree that relying on the top-down ordering of fields might make struct tags redundant, I'm not sure if that should hold up this PR.

[evan-bradley]

That's good with me, let's continue as planned.