[confighttp] Review usage of 'pointer to type'

[mx-psi]

trafficstars

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 https://github.com/open-telemetry/opentelemetry-collector/issues/9478#issuecomment-1930475996 ? (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.