opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

[PoC] Show how configoptional would work

Open mx-psi opened this issue 6 months ago • 13 comments
trafficstars

Description

Adds configoptional. Tests it on confighttp and otlpreceiver modules. Thanks @jade-guiton-dd for the feedback and help with designing this, and thanks to @yurishkuro for the initial version (see #10260)

Sanity check

Sanity check on the drawbacks we set out to fix on the RFC:

It may not be immediately obvious to a new user that a pointer type indicates a field is optional.

:heavy_check_mark: Yes, this is now explicit

The distinction between values that are conventionally pointers (e.g. gRPC configs) and optional values is lost.

:heavy_check_mark: Yes, we can differentiate those now

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.

:heavy_check_mark: Yes, you can see the OTLP receiver unmarshal method.

The potential for null pointer exceptions is created.

:x: I don't think there is much of an advantage here

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.

:question: The only way I exposed is a Get() and GetOrInsertDefault() method, this is because it is inconvenient to call methods with pointer receivers if we don't. We could have a Value() method, that would prevent mutability but I haven't found a use case for this.

Cons of doing this

  1. You have a to put a lot of Gets to access optional fields. That's more annoying than Go automatically de-referencing things for you.
  2. GetOrInsertDefault is a long name and I don't like it.
  3. You can still call the configoptional.NewFactory locally inside a function and shoot yourself in the foot (or at least make your life inconvenient).

FAQ

Why not Default(t T) instead of Default(f Factory[T])?

To prevent people from passing values with pointers and accidentally sharing those pointers.

Why not Default(f func() T) instead of Default(f Factory[T])?

So that the function pointer is the same across instances and assert.Equal is happy. For example, so you can do:

f := otlpreceiver.NewFactory()

assert.Equal(t, f.CreateDefaultConfig(), f.CreateDefaultConfig())`

(or even with different factories!)

Okay, but why not Default(f *func() T) instead of Default(f Factory[T])?

This is invalid:


func foo() {}

var bar = &foo  // COMPILE TIME ERROR

This is valid


func foo() {}

func getPointer(f func()) *func() { return &f }

var bar = getPointer(foo)

Why store *DefaultFunc[T] instead of DefaultFunc[T]?

So that you can compare optional values easily. Per reflect.DeepEqual's doc,

Func values are deeply equal if both are nil; otherwise they are not deeply equal.

so we need to store the pointer instead to compare by address.

Why do you have both defaultFn and hasValue?

So that var none Optional[T] is equivalent to none := None[T].

Why expose the factory via GetOrInsertDefault()?

On internal/e2e you can see cases where you want to get the default outside of the component module. This allows you to get it without exposing extra API.

Why Get() *T instead of Value() T?

Apart from avoiding allocations, so that you can easily call methods with pointer receivers.

I don't like the names

Let's bikeshed after we decide if this is a good idea in the first place.

Link to tracking issue

Informs #12981

mx-psi avatar May 12 '25 15:05 mx-psi