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] Add support for optional parameters in functions #20879

Closed
evan-bradley opened this issue Apr 12, 2023 · 10 comments
Closed

[pkg/ottl] Add support for optional parameters in functions #20879

evan-bradley opened this issue Apr 12, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium

Comments

@evan-bradley
Copy link
Contributor

evan-bradley commented Apr 12, 2023

Component(s)

pkg/ottl

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

Currently there is no support for optional parameters in OTTL functions. Optional parameters have been proposed as a solution to allow functions to take on more functionality while keeping function calls cleaner in the general case, for example with ConvertCase: #18084.

Describe the solution you'd like

See prior discussion here: #18822 (comment).

The approach introduced in #14712 opens the possibility to take advantage of the additional factory structure to annotate arguments as optional. One approach for this could be to annotate the fields on the Arguments struct as being optional. I think this would need two components to allow both the parser and factory to handle optional parameters:

  1. An ottloptional struct tag on the field representing the optional parameter. This would signal to the parser that an argument doesn't necessarily need to be passed in an invocation.
  2. An ottl.OptionalParameter[T any] type for optional parameters that don't have a sensible default value. Go's use of zero-values makes it difficult to tell whether an argument has been set or not, so we would need some way of easily specifying this. We could also do this with pointers, but I think a proper type would be a cleaner solution.

An example of what this could look like with a type where we want to check whether a parameter has been passed:

type ConvertCaseArguments[K any] struct {
	Target ottl.StringGetter[K] `ottlarg:"0"`
	ToCase string               `ottlarg:"1"`
	ToDelimited ottl.OptionalParameter[string] `ottlarg:"1",ottloptional`
}

Checking whether ToDelimited was passed in an invocation could then be done with a ToDelimited.IsEmpty() or something similar.

For parameters with default values, here's a contrived example:

type LimitArguments[K any] struct {
	Target       ottl.PMapGetter[K] `ottlarg:"0"`
	PriorityKeys []string           `ottlarg:"1"`
	Limit        int64              `ottlarg:"2",ottloptional`
}

func (f LimitFactory[K]) CreateDefaultArguments() ottl.Arguments {
	return &LimitArguments[K]{ Limit: 10 }
}

Describe alternatives you've considered

We could also add support for keyword/named arguments, e.g. delete_key(target="attributes", key="test") which would allow argument reordering and would make having multiple or mutually exclusive optional parameters much easier. However, this would be a larger change and provides an open door for "do everything" functions.

Additional context

No response

@evan-bradley
Copy link
Contributor Author

cc codeowners @bogdandrutu @TylerHelmuth @kentquirk

@TylerHelmuth
Copy link
Member

Hype!

@bogdandrutu
Copy link
Member

I am confused about the "ottloptional". We need to ensure that only one optional argument is defined correct? Otherwise from foo("a", "b") (and if there are 2 optional params { first "optional", second, third "optional" }) you will not be able to know which of them are set (first or third).

Do we need ottlarg? I think it complicates, we just use the order defined in the struct.

An alternative to this optional thing we can consider some other options:

  • Use python style named arguments
  • Use order to always say that last arguments are optional. For example if you have 5 arguments and first 2 are "required" then you can call with first 2 or first 3 or first 4 or all 5.

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Apr 13, 2023

Do we need ottlarg? I think it complicates, we just use the order defined in the struct.

While not require, we discussed in the SIG meeting and it was agreed that a struct tag removes any ambiguity caused by relying on only the struct's field order. I personally like the tag a lot more.

@TylerHelmuth
Copy link
Member

@evan-bradley In your ConvertCaseArguments example should the optional arg have ottlarg:2 ?

I agree that optional args should always be after required args.

@evan-bradley
Copy link
Contributor Author

I am confused about the "ottloptional". We need to ensure that only one optional argument is defined correct?

Use order to always say that last arguments are optional. For example if you have 5 arguments and first 2 are "required" then you can call with first 2 or first 3 or first 4 or all 5.

My goal with ottloptional was to clearly annotate the arguments with which ones are optional. We would need to do a runtime check to ensure that required arguments aren't defined after optional arguments, but it should be straightfoward outside that. I agree with the behavior in your example.

Do we need ottlarg? I think it complicates, we just use the order defined in the struct.

While not require, #14712 (comment) and it was agreed that a struct tag removes any ambiguity caused by relying on only the struct's field order.

I made it a requirement after the SIG discussion for the sake of consistency, and struct fields without the tag will cause an error during parsing. Personally I'm okay with relying on the struct order, but I understand that it may not be immediately clear that field ordering is guaranteed in Go.

Use python style named arguments

I would support this, I think it removes a lot of ambiguity.

@evan-bradley In your ConvertCaseArguments example should the optional arg have ottlarg:2 ?

Thanks for catching this, I've edited the code to correct that.

@evan-bradley
Copy link
Contributor Author

I have a working prototype of this that uses named arguments in the style of Python's keyword arguments. I'm aiming to have a draft PR ready for design discussion by sometime next week.

@evan-bradley evan-bradley self-assigned this Jul 17, 2023
@rnishtala-sumo
Copy link
Contributor

@evan-bradley would you be able to share the working prototype as a draft PR?

@evan-bradley
Copy link
Contributor Author

@rnishtala-sumo sure thing, here it is: #24456.

evan-bradley added a commit that referenced this issue Sep 20, 2023
**Description:**

Provides one possible path for allowing functions to define optional
parameters in OTTL.

This uses Python-style keyword arguments, so function calls will look
like `ConvertCase(attributes["source"], delimiter = ".")`. Proposed
rules for arguments after this change:
1. camelCase/snake_case versions of the struct field names for the
parameter can be used in function calls to specify an argument.
2. All named arguments must come after unnamed arguments.
3. Unnamed arguments must be in function signature order, named
arguments can be in any order.
4. Arguments can be passed in unnamed or named.

**Link to tracking Issue:**


#20879

Co-authored-by: Evan Bradley <[email protected]>
@evan-bradley
Copy link
Contributor Author

Resolved via #24456

jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
**Description:**

Provides one possible path for allowing functions to define optional
parameters in OTTL.

This uses Python-style keyword arguments, so function calls will look
like `ConvertCase(attributes["source"], delimiter = ".")`. Proposed
rules for arguments after this change:
1. camelCase/snake_case versions of the struct field names for the
parameter can be used in function calls to specify an argument.
2. All named arguments must come after unnamed arguments.
3. Unnamed arguments must be in function signature order, named
arguments can be in any order.
4. Arguments can be passed in unnamed or named.

**Link to tracking Issue:**


open-telemetry#20879

Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

4 participants