WIP do not merge - optional config fields

[yurishkuro]

Description

Experiment with support optional config structs

Link to tracking issue

#10266

Testing

Documentation

[github-actions]

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

[github-actions]

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

[evan-bradley]

@yurishkuro Sorry for the really long delay in getting to this. I still think this approach is the best way to handle optional config fields.

If there were tests in the confmap package to test this with config unmarshaling, I think it would be ready to come out of draft.

[yurishkuro]

@evan-bradley tests wouldn't be a problem, but I ran into a roadblock that I described in #10266 (comment)

[yurishkuro]

WithDefault[T]() was a good idea from @mx-psi, but it didn't work either. When an item is defined in YAML but without any value, i.e. item: with no content, then mapstructure treats the value as nil, and does not perform any processing. As a result, the custom Unmarshal() is still not being called.

See test https://github.com/open-telemetry/opentelemetry-collector/pull/10260/files#diff-24c5961a7871021bd9801008aa5f1fad72efd43f8783a00c7d09dc52c4c64e0dR81

I opened a ticket upstream go-viper/mapstructure#37

[github-actions]

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

[yurishkuro]

keep

[github-actions]

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

[mx-psi]

I'll have time to look into this next week, apologies for the delay

[yurishkuro]

Pending upstream fix go-viper/mapstructure#42

[yurishkuro]

Pending another upstream fix go-viper/mapstructure#45

[codecov]

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.79%. Comparing base (d17559b) to head (7157a80).
Report is 263 commits behind head on main.

Files with missing lines	Patch %	Lines
confmap/optional/optional.go	77.77%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10260      +/-   ##
==========================================
- Coverage   91.80%   91.79%   -0.02%     
==========================================
  Files         424      425       +1     
  Lines       20094    20114      +20     
==========================================
+ Hits        18447    18463      +16     
- Misses       1268     1271       +3     
- Partials      379      380       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yurishkuro]

@mahadzaryab1 fyi this is the main PR I was working on, which needed changes to mapstructure lib. The fun continues, now with contrib packages failing.

[mackjmr]

@yurishkuro I tested this PR using type optional.Optional[int] for MaxIdleConns in confighttp but get the following error:

Error: failed to get config: cannot unmarshal the configuration: decoding failed due to the following error(s):
error decoding 'exporters': error reading configuration for "otlphttp": decoding failed due to the following error(s):
'max_idle_conns' expected a map, got 'int'

It seems like Optionals Unmarshal is never called due to the following logic in the Hookfunc.

I updated ClientConfig as so:

// ClientConfig defines settings for creating an HTTP client.
type ClientConfig struct {
...
	// MaxIdleConns is used to set a limit to the maximum idle HTTP connections the client can keep open.
	// By default, it is set to 100.
	MaxIdleConns optional.Optional[int] `mapstructure:"max_idle_conns"`
...
}

and updated otlphttpexporters createDefaultConfig (which I am testing with) as so:

func createDefaultConfig() component.Config {
	return &Config{
                 ...
		ClientConfig: confighttp.ClientConfig{
			Endpoint: "",
                         ...
			MaxIdleConns: optional.WithDefault(
				func() int {
					return 555
				},
			),
		},
	}
}

Is there something I am doing wrong here ?

[github-actions]

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

[github-actions]

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

[evan-bradley]

@yurishkuro Just checking in, any updates on this? I have some time to dig into it this week, let me know if there's anything you'd like me to investigate.

[github-actions]

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

[mahadzaryab1]

I've got a PR for the upstream patch open at go-viper/mapstructure#52. To test this patch out, I believe we'll need to re-write the existing hooks in confmap to return reflect.Value instead of the actual values to be able to decode nil values.