[Do Not Merge] POC optional hook

[mackjmr]

Description

This PR is a POC to potentially resolve #9478. It creates an optional.Optional type which has a Value field and a HasValue field.

• If there is no default, the struct initialisation will have HasValue to false.
• If there is a default, this will be set in the Value field as well as HasValue:true at initialization.
• In both cases, if a value is set by the user, then the hook will be triggered and we will set (or override if there was a default) the value in the hook.

Link to tracking issue

Fixes #

Testing

Documentation

[evan-bradley]

Thanks for continuing to dig into this.

[evan-bradley]

I'm just spitballing here, but would it be possible to instead have an interface specifically for unmarshalling primitive values? For example, maybe someone would want to do the following:

type EsotericIntEncoding int

func (e *EsotericIntEncoding) UnmarshalPrimitive(val any) error {
	x, ok := val.(int)
	if !ok {
		return errors.New("not an int")
	}
	*e = EsotericIntEncoding(x)
	return nil
}

We could then check if from is not a map and if to implements the PrimitiveUnmarshaler interface and unmarshal things like Optional[int] this way. The interface would also let component authors unmarshal custom types that alias primitive values like EsotericIntEncoding.

[evan-bradley]

This would also allow the Optional type to handle this value itself and to avoid needing to make its fields public so they can be accessed by reflection.

[mackjmr]

I'm just spitballing here, but would it be possible to instead have an interface specifically for unmarshalling primitive values

I'm not sure to understand how this would work:

• Seems like the receiver would need to be Optional[T] if we want to set the hasValue field ?
• With this approach, would we need to have a separate UnmarshalPrimitive func for each type we want to support ?

This would also allow the Optional type to handle this value itself and to avoid needing to make its fields public so they can be accessed by reflection.

I've made changes that unexport the Optional structs fields, by using a custom unmarshaller.

[evan-bradley]

Seems like the receiver would need to be Optional[T] if we want to set the hasValue field ?

Yeah, I think an *Optional[T] receiver is the way to go. The way you currently have it looks good to me.

With this approach, would we need to have a separate UnmarshalPrimitive func for each type we want to support ?

Yes, I think it could help with other primitive types (e.g. special encodings for strings, ints, etc.) in addition to helping here with Optional[T] where T is a primitive type. It also means we don't need a special hook for only our type if we put logic like the following in unmarshalerHookFunk:

if _, ok = from.Interface().(map[string]any); !ok {
	unmarshaler, ok := toPtr.(PrimitiveUnmarshaler)
	if !ok {
		return from.Interface(), nil
	}
	if err := unmarshaler.UnmarshalPrimitive(from.Interface()); err != nil {
		return nil, err
	}
	return unmarshaler, nil
}

I tested this locally and it appears to work.

[mackjmr]

I see, thanks for the explanation! I've removed the optional hook and moved all logic to unmarshalerHookFunk in last commit

[mackjmr]

I can clean up the PR and add tests if we choose to go with this approach. Do we need other reviewers ? perhaps @mx-psi or @yurishkuro ?

[evan-bradley]

I would like input from both of them before continuing. We should also check whether @yurishkuro would like to continue work on #10260, since that was opened before this.

If you have time, would you be willing to add/update some basic tests, or report on the output of manually running the Collector with these changes, to show that this is viable?

[evan-bradley]

I will mention as an aside that I looked at encoding.TextUnmarshaler as an alternative that wouldn't require us to provide a new interface, but ultimately we want to use the string -> primitive type unmarshaling provided by mapstructure, so in the end I think this would add more complexity than it removes. Implementing our own interface allows us to accept partially-unmarshaled values and do any final transformations on them.

The downside here is that encoding.TextUnmarshaler and our PrimitiveUnmarshaler interfaces would have some overlap, and users would need to know which one to use. I also don't have any immediate use-cases for PrimitiveUnmarshaler in mind outside this optional type.

[mackjmr]

If you have time, would you be willing to add/update some basic tests, or report on the output of manually running the Collector with these changes, to show that this is viable?

I added a collector.yaml and print statements in this PR which demonstrates the results, see output:

2024-10-16T14:48:51.880+0200	info	service/service.go:135	Setting up own telemetry...
2024-10-16T14:48:51.880+0200	info	telemetry/metrics.go:70	Serving metrics	{"address": "localhost:8888", "metrics level": "Normal"}
2024-10-16T14:48:51.881+0200	info	service/service.go:207	Starting otelcorecol...	{"Version": "0.111.0-dev", "NumCPU": 10}
2024-10-16T14:48:51.881+0200	info	extensions/extensions.go:39	Starting extensions...
MaxIdleConns: {value:567 hasValue:true}
MaxIdleConnsPerHost: {value:0 hasValue:false}
AUTH: {value:{Authentication:{AuthenticatorID:{typeVal:{name:} nameVal:}} RequestParameters:[]} hasValue:false}
CORS: {value:{AllowedOrigins:[http://test.com] AllowedHeaders:[Example-Header] MaxAge:7200} hasValue:true}
2024-10-16T14:48:51.881+0200	info	otlpreceiver/otlp.go:169	Starting HTTP server	{"kind": "receiver", "name": "otlp", "data_type": "traces", "endpoint": "localhost:4317"}
2024-10-16T14:48:51.881+0200	info	service/service.go:230	Everything is ready. Begin running and processing data.

let me know if this is enough, or if I should still add tests ?

[mackjmr]

We should also check whether @yurishkuro would like to continue work on #10260, since that was opened before this.

Happy to close this in favor of Yuri's PR. The only additions/substraction I made here:

• Remove defaultFn field in Optional struct, I don't think this is required
• Add another unmarshaller. 10260 errors for optional types that contain a primitive value, as they cannot be cast to a map. (see this comment: WIP do not merge - optional config fields #10260 (comment))

[yurishkuro]

How is the approach here different from #10260 ?

[evan-bradley]

How is the approach here different from #10260 ?

I don't think it meaningfully differs, the goal was to answer this question concerning what is required to make unmarshaling work.

I'm personally satisfied that an PrimitiveUnmarshaler or similar interface that handles aliases to, or wrappers around, primitive types is a simple-enough solution so long as it covers all our cases. What do you think?

[mx-psi]

Agreed that encoding.TextUnmarshaler is not workable here because of the string -> primitive thing.

I also don't see other use cases for PrimitiveUnmarshaler. I wonder if we can make this interface private in some way if that's the case, so that we can remove support for this if we remove the Optional type. If that's possible to do, then I feel like doing this is okay: we could in the future remove support for Optional without a major version bump on confmap

[evan-bradley]

I wonder if we can make this interface private in some way if that's the case, so that we can remove support for this if we remove the Optional type.

Since interfaces are implemented implicitly in Go, we can just have the Optional type implement the interface and it should still work. We just won't be able to refer to the interface for things like asserting the implementation inside optional package.

Wouldn't it still be a breaking change (albeit on undocumented behavior) if we were to later remove support for the interface in confmap? I don't see anything in our versioning guidelines that gives me a clear idea on what our guarantees are here.

[mackjmr]

Since interfaces are implemented implicitly in Go, we can just have the Optional type implement the interface and it should still work. We just won't be able to refer to the interface for things like asserting the implementation inside optional package.

pushed a commit that shows this.

[yurishkuro]

Remove defaultFn field in Optional struct, I don't think this is required

It depends on what your goal is. My goal was not to simply label a field as optional (a pointer technically achieves the same), but to provide the ability to supply a default value iff the user did not provide any. That ability specifically, or rather lack of it, is what forces different configs to implement custom marshaling logic.

I will see this weekend if this custom marshaler interface would solve the issue I had in my PR without requiring mapstructure change.

[mackjmr]

Remove defaultFn field in Optional struct, I don't think this is required

It depends on what your goal is. My goal was not to simply label a field as optional (a pointer technically achieves the same), but to provide the ability to supply a default value iff the user did not provide any. That ability specifically, or rather lack of it, is what forces different configs to implement custom marshaling logic.

The logic in this PR provides support for adding a default (see here). If you want a default, add an optional.Optional with the default value and hasValue to true in the Config in createDefaultConfig. If there is no user input, it will stay as is. If there is a user input, the unmarshalling will override the default value with the user provided one.

[yurishkuro]

The logic in this PR provides support for adding a default

is that default unconditional? The specific use case I wanted to solve is if a user provides an empty http: entry for e.g. http server in the receiver. If the user does not provide this empty section, the default should not apply either.

[mackjmr]

is that default unconditional? The specific use case I wanted to solve is if a user provides an empty http: entry for e.g. http server in the receiver. If the user does not provide this empty section, the default should not apply either.

It's conditional on the provided config section triggering a call to Unmarshal, which in turns calls CreateDefaultConfig which sets the defaults.

I think that for the use case you mentioned, the call to Unmarshal will still be triggered so the default will still be set.

IMO if a call to CreateDefaultConfig is triggered, then its ok to set the defaults ?

[yurishkuro]

Can you demonstrate it by removing

opentelemetry-collector/receiver/otlpreceiver/config.go

Line 72 in 388bae7

if !conf.IsSet(protoHTTP) {

?

[mackjmr]

Can you demonstrate it by removing

Okay, I understand what you mean now. Indeed we won't be able to remove this, as setting the Default in createDefaultConfig directly will make it so there is always a value, even though a section wasn't set explicitely. I think the defaultfn from your PR solves this, so that within createDefaultConfig hasValue is still false, and the defaultfn is only applied when relevant sections are in the config ?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.