[confighttp] Review usage of 'pointer to type'

[mx-psi]

There are a few fields in confighttp structs that use e.g. a pointer to int instead of an int. This is an unusual pattern in configuration from components, so I think we should review why we use it and whether it is justified.

To fix this issue we need to:

• Define when we can use 'pointer to type' in configuration vs just type and relying on the zero value
• Review the fields on confighttp based on this.

On configgrpc we have a similar pattern for the configtls configuration.

[bogdandrutu]

There are a few fields in confighttp structs that use e.g. a pointer to int instead of an int. This is an unusual pattern in configuration from components, so I think we should review why we use it and whether it is justified.

Some background: Lots of these were added after the initial release of the confighttp, and authors/reviewers decide to use pointers to signal the presence of the field.

I do agree that this is not ideal, but would like to have a concrete option/design on how we would support adding new members to the config when we need to know the presence. I believe we can do some tricks with Unmarshal, but would like others to think about :)

[evan-bradley]

Maybe not fully idiomatic Go, but OTTL makes signaling field presence a little easier using an Optional type, which are available out-of-the-box in other languages. We could potentially use a type like this to help make it more obvious when a field is optional, since pointers may not immediately make that intention clear.

we need to: Define when we can use 'pointer to type' in configuration vs just type and relying on the zero value

I think for config structs the reasons are:

1. We need to know the presence of a field.
2. We need to modify a struct field in-place without making the config struct a pointer type.
3. A third party library uses pointers and we want a field that directly references the library type.

The field docs in confighttp indicate that these fields are optional because there's a default, so I think we can keep these.

I believe we can do some tricks with Unmarshal

It's possible we could do this, but it would be nice if we could make the definition of whether a field is optional exist on the type itself and let the libraries handle the unmarshaling. If we do that we would need to indicate presence, either through a sentinel value, another field on the struct, or a type that supports indicating value presence. I think pointers are a fine way to do this, but would personally prefer a type that is explicitly intended for that purpose, e.g. an Optional type. I'd prefer to reserve pointer usage for cases (2) and (3) above to make it clear they indicate mutability, or to just discourage their use in general if we don't want the config structs to be mutable.

[bogdandrutu]

This reminds me of something. We should add a NewDefaultFoo for all configs, so that we can easily add new fields and set the right default value.

So we can say that in order to guarantee "backwards compatible behavior" user should always initialize the config with default and overwrite anything as needed instead of creating a struct with "initial value" and leave others unset.

[mx-psi]

+1 to using something like Optional, we can probably use encoding.TextUnmarshaler to make it work nicely with the configuration without resorting to custom unmarshaling.

@bogdandrutu can you open a separate issue for #9478 (comment) ? (edit: Done, see #9508)

[atoulme]

I recommend we go at this in multiple PRs:

• add the defaults to the pointer types in the NewDefaultClientConfig function, so these defaults are always set. This changes none of the logic.
• Make everyone start to use NewDefaultClientConfig instead of creating their own struct to reduce the blast radius
• Break the API by moving away from pointer types.

[mackjmr]

I've looked into this, and here are the options I considered:

Using optional type: I've attempted to make the pointer fields in confighttp an Optional type based on this POC: #10260. This does not work as our current custom unmarshalling hook (meant to call confmap.Unmarshaler) is meant for types convertible to maps as can be seen here.

We may be able to get this working, if we explicitely ignore the Optional type in our existing hooks, and create a hook specifically meant for our optional type. However, I'm not sure if this added complexity is a better solution than pointers.

Using a sentinel value: We could use a sentinel value. That said, I don't think this is a cleaner solution than using pointers.

Pointers is a common strategy in Go in order to indicate presence of a field, so I'm unsure how beneficial it would be to move away from this if it adds more complexity.

[mx-psi]

Thanks @mackjmr! Based on this and the discussions we have had offline, I also agree that using a pointer seems like the best option. I think to continue the discussion, we can open a PR on the Coding guidelines to settle that this is the way to represent optional stuff.

Then the remaining step is to review the existing fields and ensure that them being optional is the right choice (IMO after skimming again through the list it probably is).

[evan-bradley]

We discussed this issue during the 2024-10-09 Collector stability working group meeting and determined that the benefits of adding Optional type need to meaningfully outweigh the complexity it adds in order to offer it in place of using pointers for optional fields.

I think Optional fields offer the following advantages over using pointers, based on the usage I see in config structs in core and contrib:

1. A field with type Optional[T], where T is not a pointer to a type, will not be mutable. Pointer fields will stay mutable as the config struct is passed around, which is generally undesirable.
2. Pointers lead to the possibility of null pointer exceptions. While this isn't frequent and can be tested for, it is a possibility.
3. Optional[int] is clearer than *int at communicating that a field is optional without any requiring any additional context.

An Optional type would have the following disadvantages:

1. It's a custom type that also isn't necessarily idiomatic in Go. Pointers at least, by their nature of having a zero value of nil that doesn't exist in the possible values for the actual type, confer some sense of being optional when inside a struct.
2. If we don't have other uses for something like an UnmarshalPrimitive interface like is currently being prototyped in [Do Not Merge] POC optional hook #11409, it would require a little additional unmarshaling logic inside confmap.
3. Minor, but we would need to adapt existing config structs. I don't believe any stable modules use pointers to types to indicate optional fields.