[chore] Create RFC for Optional config types

[evan-bradley]

Description

This RFC will help us explore the use cases Optional types solve and move us toward a resolution for whether to adopt these in our config.

Related to #10266

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.09%. Comparing base (7d3e03e) to head (94e8a78).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12596      +/-   ##
==========================================
- Coverage   92.18%   92.09%   -0.09%     
==========================================
  Files         469      473       +4     
  Lines       25394    25590     +196     
==========================================
+ Hits        23409    23567     +158     
- Misses       1574     1612      +38     
  Partials      411      411              

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evan-bradley]

@yurishkuro could you take a look at this? We're taking a closer look at whether we want this type for 1.0 and want to make sure if we add it that it will cover all config use cases and won't have any major downsides. I would appreciate your input here.

[yurishkuro]

+1

[yurishkuro]

Can we give examples of external packages working with config? E.g. validation libs come to mind, but they might still work, for them Optional is just an extra nesting container.

[evan-bradley]

I'm mostly concerned about something like #10694, where we want to generate config types from a JSON schema. I'm not sure how this would treat optional types.

[yurishkuro]

ok, but how do you want to move the discussion forward? Just take a vote? Or do more cost/benefit analysis, and potentially block this on unresolved #10694?

[evan-bradley]

We'll essentially need a vote to get this merged anyway per the Collector's RFC process, so I think that's what it will come down to.

I think this is far more straightforward than #10694 anyway, so I would prefer to not block it on that. I'm just calling it out here as a risk.

[bogdandrutu]

I think we can then remove https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/confmap.go#L351

[bogdandrutu]

More than golang code, would be good to show how users in yaml will use it.

[yurishkuro]

there's no expected impact on YAML

[jmacd]

Drawback numbers 2 and 5 appear contradictory. Is there a reason why pointers are "conventionally" pointers, aside from mutability?

Suggested change

	2. The distinction between values that are conventionally pointers (e.g. gRPC
	configs) and optional values is lost.
	3. Setting a default value for a pointer field when decoding will set the field
	on the resulting config struct, and additional logic must be done to unset
	the default if the user has not specified a value.
	4. The potential for null pointer exceptions is created.
	5. Config structs are generally intended to be immutable and may be passed
	around a lot, which makes the mutability property of pointer fields
	an undesirable property.
	2. While they can be copied cheaply, pointers convey a mutable reference making
	them unsafe to share.
	3. Setting a default value for a pointer field when decoding will set the field
	on the resulting config struct, and additional logic must be done to unset
	the default if the user has not specified a value.
	4. The potential for null pointer exceptions is created.

[evan-bradley]

This may not really apply to config structs, but outside mutability, another reason to use a pointer type for a field in Go is so you don't have to pass around a large struct by value.

The specific point I was making with item 2 is that some types in popular libraries are pointers, e.g. *zap.Logger, *grpc.Server, etc. If pointers are used to indicate optional values, if you have a field that is a *zap.Logger type, it's difficult to tell if it's intentionally optional, or if that type is almost always used as a pointer type (zap.Logger as a non-pointer is rare from what I've seen). Obviously *zap.Logger isn't a perfect example because nobody is setting a logger in YAML, but hopefully my point still makes sense.