[chore][docs/coding-guidelines] Document approach to optional fields

[mx-psi]

Description

Documents the approach discussed on 2025-02-10 stability meeting.

Link to tracking issue

I argue that this, together with #12323, fixes #9478.

We may still want to add an optional wrapper in the future, this is covered by #10266

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.96%. Comparing base (f658d9f) to head (258c11c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #12352   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files         467      467           
  Lines       25459    25459           
=======================================
  Hits        23414    23414           
  Misses       1639     1639           
  Partials      406      406           

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

[yurishkuro]

I think #10260 was a much cleaner solution, which was nearly complete but blocked on #11755, which was stalled for unclear reason.

[evan-bradley]

Suggested change

	When representing optional fields in the configuration, we distinguish two cases:
	1. For scalars (e.g. `int`, `string`, `bool`, `time.Duration`...), the zero value of the base type
	is used to represent the absence of the field. The default value for this field may be different
	from the zero value, so, when using the Go API you MUST follow the guidance on the "Default
	Configuration" section below.
	When representing optional fields in the configuration for which the zero value indicates the absence of the field, we distinguish two cases:
	1. For scalars (e.g. `int`, `string`, `bool`, `time.Duration`...), the zero value of the base type
	should be used to represent the absence of the field instead of a pointer. The default value for this field may be different
	from the zero value, so, when using the Go API you MUST follow the guidance on the "Default
	Configuration" section below.

I think we should limit this to only applying to the case where the zero value is not a valid value for the field and can indicate absence of the field. For cases where this doesn't apply, I think we still need to evaluate whether to use pointers or an optional type.

[evan-bradley]

I'm wondering if we should suggest just using non-pointer values for now so we can be consistent across structs.

Our stable config modules don't use pointers for any struct fields, so this would allow us to be consistent. Once we've arrived at a decision, we can later indicate that pointer values (or optional types) should be used for types where zero values don't indicate absence of the field.

[evan-bradley]

@yurishkuro Sorry, #11755 stalling is on me. I'd like to confirm that we can confidently say it's not a breaking change for proceeding. I will try to get back to it soon and see if we can progress it further, thank you for your patience.

The goal of this PR is to unblock the decision of whether to adopt optional types from stabilizing a few config modules so we can consider it separately from our 1.0 efforts. I would still like them, but this allows us to be more deliberate.

[yurishkuro]

@evan-bradley tldr is it's using Optional[T] with runtime defaulting capability. It's not a breaking change from user perspective, but it makes it obvious in the code if the value is set or just zero. Having conventions for interpreting zero values always runs into issues like naming for new fields (enabled vs disabled), but more importantly it doesn't do anything for complex types like these:

backends:
  grps:
  http:

[evan-bradley]

I agree about the benefits of the Optional[T] type.

What this solves is cases like #12323 where we are working with values where zero doesn't indicate a valid value for the field. In particular with that PR, the underlying Go standard library we are configuring considers a value of 0 to disable the option.

I think the case you're calling out is more complex and would benefit from an optional type.

[mx-psi]

@yurishkuro Do you think confighttp and configgrpc would benefit from this alternative approach? To be clear, as Evan said, this does not close the door to adding an optional type in the future, I just want to solve #9478

[yurishkuro]

@mx-psi definitely any complex defaults would benefit, because right now they require overriding Unmarshal with some type-unsafe checks for field names. Optional[T] with defaulting makes it all cleaner and strongly typed.

For the primitive fields I don't have a strong opinion. On one hand, if we focused on actually landing Optional[T] then it would take care of the primitive fields problem as well. On the other hand, I don't really see why #9478 even needs solving - so we use pointers for now, why is it a problem other than "it's unusual"? Switching from pointers to zero values just creates another convention issue that was hotly discussed in the spec (e.g. open-telemetry/opentelemetry-specification#4351)

[mx-psi]

so we use pointers for now, why is it a problem other than "it's unusual"?

Because it would be an API breaking change to change it in the future and we want to avoid doing a v2 just because of that.

[yurishkuro]

Because it would be an API breaking change to change it in the future and we want to avoid doing a v2 just because of that.

I see. Changing to Optional[T] would also be a breaking change then, but arguably in a better direction. If we don't try to support custom defaulting logic in the Optional I think it's relatively straightforward to support it for just basic set/unset purpose (in the worst case it can be done with a custom mapstructure hook that will run on the parent struct of Optional fields, detect them, and do what's needed).