Replace function interfaces with a factory

[bogdandrutu]

Describe the issue you're reporting

Right now we pass functions as interfaces, and then go over arguments using reflection, identifying some internal, etc. I think we should use a "factory" pattern similar with our components:

type Arguments interface{}

type Factory[K any] interface {
  CreateEmptyArguments() Arguments

  CreateFunction(ctx context.Context, set component.TelemetrySettings, args Arguments) (ExprFunc[K], error)
}

We will do the same "reflection" unmarshalling but for members of the FunctionArguments.

Here is how for example DeleteKey:

type DeleteKeyArguments {
  Target ottl.Getter[K]
  Key string
}

type DeleteKeyFactory[K any] struct {}

func (f *DeleteKeyFactory[K])  CreateEmptyArguments() Arguments {
  return &DeleteKeyArguments{}
}

func (f *DeleteKeyFactory[K]) CreateFunction(ctx context.Context, set component.TelemetrySettings, args Arguments) (ExprFunc[K], error) {
  oargs := args.(*DeleteKeyArguments)
  return func(ctx K) interface{} {
		val := oargs.Target.Get(ctx)
		if val == nil {
			return nil
		}

		if attrs, ok := val.(pcommon.Map); ok {
			attrs.Remove(oargs.Key)
		}
		return nil
	}, nil 
}

This solution does not fix everything, but brings more structure to us, and allows in the future if needed to allow Factories to implement optional interfaces that will allow us to extend the functionality.

[TylerHelmuth]

@bogdandrutu since ottl is now specific to the collector, I like the idea of each function getting some standard arguments. Instead of separating content.Context and component.TelemetrySettings into their own params, how do you feel about nesting them in a single struct?

type ottlSettings struct {
  ctx context.Context
  set component.TelemetrySettings
}

type Arguments interface{}

type Factory[K any] interface {
  CreateFunction(ottlSet ottlSettings, args Arguments) (ExprFunc[K], error)
}

The advantage to this would be that if we want to add some other default params we can add it to the struct without needing to change the definition of CreateFunction.

I'd also argue that we should start without CreateEmptyArguments as the user should always be specifying all the params of a function. I don't think we should fill in any blanks with default values like we do for a components configuration. If a user doesn't provide a param I'd rather parsing fail.

NVM, I understand now why you added CreateEmptyArguments.

[TylerHelmuth]

I played around with this some today and I think the biggest concern I have is the amount of "boilerplate" code that each function would have to define. If we are gonna introduce the need for these 3 items in each function

type DeleteKeyArguments {
  Target ottl.Getter[K]
  Key string
}

type DeleteKeyFactory[K any] struct {}

func (f *DeleteKeyFactory[K])  CreateEmptyArguments() Arguments {
  return &DeleteKeyArguments{}
}

Then I want to make sure its for a good reason. @bogdandrutu do you have some functionality in mind that we'd want to extend in the future where we'd need the interface?

As for ensuring each function has the default params, I think we could handle that with the existing solution be updating newFunctionCall to always add the default params at the beginning. Any function that doesn't implement this wouldn't be callable by the reflection. Downside is it is not enforced by the compiler.

[TylerHelmuth]

@bogdandrutu what did you have in mind for how we'd build out the Arguments interface for each function with reflection? The existing solution relies on the function definition to define the order of the parameter and performs a "1 to 1" match on the incoming order of the parsed values. How can we map the incoming params to the correct struct field in Arguments? Is the idea that the order of the fields in the struct definition matters?

[evan-bradley]

This solution does not fix everything, but brings more structure to us, and allows in the future if needed to allow Factories to implement optional interfaces that will allow us to extend the functionality.

@bogdandrutu Could you go into more detail about this? What would be left unfixed, and what additional interfaces might be useful to implement?

I'm having trouble coming up with a use case where we wouldn't prefer to instead make the factory function return types more extensible, maybe along the lines of what is described in #14713. The CreateFunction method seems like it would be sufficient for most situations where the OTTL would be used. I do like the idea of implicitly supplying every function with runtime data supplied by the component, which I think we could do in a fairly straightforward way if we turn all OTTL function factories into methods on a struct. I could see it looking like this:

type OTTLComponentData[K any] struct {
	ctx context.Context
	settings component.TelemetrySettings
}

func (ocd *OTTLComponentData[K]) DeleteKey(target ottl.Getter[K], key string) (ottl.ExprFunc[K], error) {
	return func(ctx K) any {
		val := target.Get(ctx)
		if val == nil {
			return nil
		}

		if attrs, ok := val.(pcommon.Map); ok {
			ocd.settings.Logger.Debug("Removing key")
			attrs.Remove(key)
		}
		return nil
	}, nil 
}

// SamplingComponentData provides a sampler for a hypothetical sampling processor using the OTTL.
type SamplingComponentData[K any] struct {
	OTTLComponentData

	// sampler samples telemetry data as an example of stateful processing using the OTTL.
	sampler Sampler
}

func (scd *SamplingComponentData[K]) Sample(target ottl.Getter[K]) (ExprFunc, error) {
	return func(ctx K) any {
		return scd.sampler.ShouldSample(target.Get(ctx))
	}, nil
}

func Functions[K any](ocd *OTTLComponentData[K], scd *SamplingComponentData[K]) map[string]any {
	return map[string]any{
		"delete_key":           ocd.DeleteKey[K],
		"sample":               scd.Sample[K],
	}
}

This would have a few benefits over how the OTTL currently works:

1. Provides a consistent way to get data from the component into OTTL functions.
2. Allows for components to use custom data structures for their own functions.
3. Minimal boilerplate code per function.

The primary downside I see is that components would need to do a little more setup to use the OTTL since they would need to handle instantiating the structs.

[TylerHelmuth]

@evan-bradley I like how your solution handles the providing access to the shared context and keeps the function signature simple. Requiring the component to instantiate the structs is a downside, but maybe we could provide a helper method for ottlfunc functions. We also pass the same values to the Parser, so it would be nice to not have to pass that information twice.

I don't think it solves the issue that @bogdandrutu has raised, though, which is removing the need for map[string]any and replacing it with map[string]Factory so that function generation can be extended in the future.

[github-actions]

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:

• pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[TylerHelmuth]

@bogdandrutu I reviewed this issue again and here are my thoughts:

I like the idea of extending function creation to be more structured via a factory so that the package can scale better. I like the idea of the functions being members of structs so that they can get access to the struct's fields, such as TelemetrySettings.

I don't like the idea of using an Arguments struct and CreateEmptyArguments function as the source of the function parameters for 2 reasons.

1. I don't like relying on the order of the struct's fields to map to the order of the function's parameters in the grammar. To me this feels unintuitive as there is nothing in Go that is enforcing this ordered behavior.
2. I don't like that CreateEmptyArguments has to return a pointer to the function's argument struct in order for the reflection to work. Go allows CreateEmptyArguments to return the struct directly, which wouldn't work in the reflection, but it wouldn't be know until runtime.

In both situations I am worried about function creators getting tripped up by requirements that aren't enforced by Golang.

To try to reconcile my dislikes I came up with an approach that lets us continue to use a function during reflection but also use a factory:

type FunctionFactory[K any] interface {
	GetFunctionSignature() reflect.Type

	CreateFunction(args []reflect.Value) (ExprFunc[K], error)
}

func (p *Parser[K]) newFunctionCall(inv invocation) (Expr[K], error) {
	f, ok := p.functions[inv.Function]
	if !ok {
		return Expr[K]{}, fmt.Errorf("undefined function %v", inv.Function)
	}

	args, err := p.buildArgs(inv, f.GetFunctionSignature())
	if err != nil {
		return Expr[K]{}, fmt.Errorf("error while parsing arguments for call to '%v': %w", inv.Function, err)
	}

	exprFunc, err := f.CreateFunction(args)
	if err != nil {
		return Expr[K]{}, err
	}

	return Expr[K]{exprFunc: exprFunc}, err
}

type functionWithIntFactory[K any] struct{}

func (f functionWithIntFactory[K]) GetFunctionSignature() reflect.Type {
	return reflect.TypeOf(f.functionWithInt)
}

func (f functionWithIntFactory[K]) CreateFunction(args []reflect.Value) (ExprFunc[K], error) {
	return convert[K](reflect.ValueOf(f.functionWithInt).Call(args))
}

func (f functionWithIntFactory[K]) functionWithInt(int64) (ExprFunc[K], error) {
	return func(context.Context, K) (interface{}, error) {
		return "anything", nil
	}, nil
}

# Helper function to handle the response of reflective calls to the function.  
# Open to better names
func convert[K any](returnVals []reflect.Value) (ExprFunc[K], error) {
	if returnVals[1].IsNil() {
		return returnVals[0].Interface().(ExprFunc[K]), nil
	}
	return returnVals[0].Interface().(ExprFunc[K]), returnVals[1].Interface().(error)
}

This attempt moves some of the logic that was hidden in newFunctionCall and makes it the responsibility of the Factory. We get a factory that can be expanded in the future, a struct for the functions to live on, and individual functions for ottl to use during reflection. As a bonus, I like how the actual function's logic is no longer inside a function called CreateFunction.

[TylerHelmuth]

I've implemented both solutions as separate commits in https://github.com/TylerHelmuth/opentelemetry-collector-contrib/tree/ottl-function-factories

[TylerHelmuth]

Playing around with my solution I realized we could assert the args to the type the functions require instead of using reflection to make the call

func (f functionWithIntFactory[K]) CreateFunction(args []reflect.Value) (ExprFunc[K], error) {
	return f.functionWithInt(args[0].Int())
}

func (f functionWithIntFactory[K]) functionWithInt(int64) (ExprFunc[K], error) {
	return func(context.Context, K) (interface{}, error) {
		return "anything", nil
	}, nil
}

When used in the context of OTTL's ParseStatement this would be safe as the parser handles ensuring that the types are correct and there is a perfect amount, but if used anywhere else it would be pretty unsafe.

[evan-bradley]

@TylerHelmuth I took some time to look at this, and I have a few thoughts about each solution presented here. I also took a look through your prototype implementations, thanks for writing those.

Putting the functions on a struct

My proposal to put the functions we have now on common (or individual) structs was intended as a simpler solution that forgoes some boilerplate at the cost of flexibility, in case we wanted something simpler. After some time to think about it, I now think that the added flexibility of using interfaces here is likely worth the additional boilerplate and we should probably go for a Factory interface instead of just making OTTL functions into methods on a struct.

However, I still think it would be beneficial to have some more concrete direction on what problems we see this solving. I think the main benefit having all function factories implement Factory interface is that additional functionality can later be provided through optional interfaces for the function factories to implement. However, it's not immediately clear to me what sorts of optional functionality we might want to provide. What are your thoughts on this?

Using a factory interface with an arguments struct

I have a few concerns about this approach, but I think we should be able to mostly address a few of the issues you brought up:

I don't like relying on the order of the struct's fields to map to the order of the function's parameters in the grammar. To me this feels unintuitive as there is nothing in Go that is enforcing this ordered behavior.

Struct field ordering is essential to struct type identity according to the Go specification, so I think it is safe to rely on. From the spec: "Two struct types are identical if they have the same sequence of fields [...]"

However, I agree that putting the arguments into a struct doesn't feel intuitive, and it isn't immediately obvious that Go considers the ordering significant. The one redeeming factor here is that if the functions are all written like this, the pattern becomes more apparent. However it's still not obvious without first seeing that.

I don't like that CreateEmptyArguments has to return a pointer to the function's argument struct in order for the reflection to work. Go allows CreateEmptyArguments to return the struct directly, which wouldn't work in the reflection, but it wouldn't be know until runtime.

This should be avoidable by using reflection to determine whether the underlying type for Arguments is a struct or a pointer. I think doing a check in the newFunctionCall method and providing a helper for OTTL function authors to cast Arguments to the arguments struct for their function should help resolve this.

Using a factory interface with methods on a struct

Compared to using an arguments struct, I like how your solution allows keeping the function signature and the functionality in a standard function definition. I think this is more conventional and should be more obvious for OTTL function authors compared to having the arguments in a struct. I also agree that having the definition outside a CreateFunction method is a plus.

One concern I have is that compared to using an arguments struct, this approach seems to require more boilerplate on whole. I tried to play around with this a little bit and wasn't able to reduce it any further. Additionally, how do you see providing context.Context and TelemetrySettings to each function factory fitting into this approach? My first guess is that we would need to either wrap the function definition or use embedded structs to accomplish that.

Playing around with my solution I realized we could assert the args to the type the functions require instead of using reflection to make the call

I do like that this cuts down on some of the reflection calls and necessary translation. However, it's unfortunate that this would require duplicating the arguments list, so I'm not sure whether it is worth the trade-off.

[TylerHelmuth]

@evan-bradley thanks for the feedback.

This should be avoidable by using reflection to determine whether the underlying type for Arguments is a struct or a pointer.

In my first (and technically only) attempt at implementing the reflection using the argument struct I wasn't able to get Go to see the response of CreateEmptyArguments() as anything but an Argument unless I returned a pointer. With a pointer I could see the underlying type, put as a value I could only find an Argument. I'm not great with Go's reflection, though, so I'd love some help if I did something wrong.

One concern I have is that compared to using an arguments struct, this approach seems to require more boilerplate on whole

Ya. I'd say applying any sort of structure/api to the function generation is gonna add boilerplate since the current solution is so bare-bones :/. Feels bad to add more complication to the process, but I think its main benefit will be handling common parameters.

If we're looking for justification for why to do this I keep going back to context.Context and TelemetrySettings. Today the reflection can handling injecting TelemetrySettings, but not context.Context. The solution for injecting TelemetrySetting ends up being pretty complicated because the OTTL grammar doesn't know about that function parameter. Whatever solution we choose here will allow us to supply context.Context and TelemetrySettings to the factory (and therefore to the function indirectly), allowing us to remove all the injection logic from newFunctionCall.

For my attempt specifically I didn't add it (I think it would be added in a followup PR to keep the PRs smaller), but I think it could be done via CreateFunction, adding the values to the struct before creating the function, or separated out into a specific function that adds them to the struct.

I lean towards a separate function as it would prompt function creators to add the set of OTTL fields to their struct

type OTTLFunctionFields struct {
	ctx context.Context
	settings component.TelemetrySettings
}

Of course this is all even more boilerplate code 😭

[evan-bradley]

I've implemented and tested a solution to use the Arguments struct on this branch: https://github.com/evan-bradley/opentelemetry-collector-contrib/commits/ottl-function-factory-interface

I've split the work into two commits, one with the relevant changes and another to get everything to compile so I could test it. In my testing, I was able to process traces whether CreateEmptyArguments returns a struct or a pointer to a struct directly. Getting a settable value if the struct is passed directly is tricky, but it does seem to work.

[TylerHelmuth]

@evan-bradley nice!

Does castArgsToPtrType become required when asserting the Argument type?

[evan-bradley]

Does castArgsToPtrType become required when asserting the Argument type?

Thankfully no, that's just a convenience function so the user doesn't have to worry about whether the value returned from CreateEmptyArguments is a pointer or not.

[TylerHelmuth]

@evan-bradley how do you want to proceed? I'm still not in love with the idea of struct field order being important. Wanna discuss during a SIG meeting?

[evan-bradley]

@evan-bradley how do you want to proceed? I'm still not in love with the idea of struct field order being important.

I'm still undecided. I don't like the fact that we're defining arguments through a non-obvious and unconventional mechanism either, but I think using an arguments struct does have some advantages:

1. The arguments are easily accessible from any method on the factory struct, which is possible but more challenging if they are defined in a function signature. This would let a factory use the invocation arguments in methods that fulfill an optional interface. Of course, it's hard to say how useful this would be without some optional interfaces in mind.
2. It is a little bit simpler overall since we don't have to worry about reflection in the factory definition.

I think there are also a few factors that make it more palatable:

1. End-users shouldn't be affected since they can consult the function documentation and won't need to look at the code.
2. Between covering defining a function in the OTTL documentation and seeing it in already defined functions, it should be somewhat obvious to function authors that struct field order determines parameter order.
3. Automated tests can help function authors verify that the intended order for the function invocation is reflected in their arguments struct.

Wanna discuss during a SIG meeting?

Sounds good to me. I've added it to the agenda for this week's meeting.

[TylerHelmuth]

You make a compelling argument. Lets hear more opinions at the meeting and then hopefully start implementing something.

[evan-bradley]

To summarize the discussion so far:

We're looking to turn our current OTTL function factories into factory structs that implement a Factory interface. Our primary goals for this change are:

1. Allow extending factories in the future by having them implement optional interfaces.
2. Provide a way to pass common parameters to OTTL functions. Right now these are context.Context and component.TelemetrySettings.

From my perspective, the following questions are still open:

1. Should we provide a mechanism for a component to pass state to factories? If so, how does this look?
2. Are there any potential optional interfaces we can think of right now for factories to implement?
3. Will we want to add additional common parameters to OTTL functions in addition to context.Context and component.TelemetrySettings in the future?

We have two proposals for how to implement this interface that differ on how to define the function signature of the DSL function.

1. Use an Arguments struct that lists function parameters in field order. An outline of this can be seen in the opening comment.
2. Continue to define the function signature using Go functions. An outline of this can be seen in this comment.

There are merits to both approaches, but I've tried to outline some of the differences I see here.

[evan-bradley]

To summarize the discussion from today's SIG meeting, we will try an approach using an Arguments struct and use struct tags to define argument ordering.

[github-actions]

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:

• pkg/ottl: @TylerHelmuth @kentquirk @bogdandrutu @evan-bradley

See Adding Labels via Comments if you do not have permissions to add labels yourself.