opentelemetry-collector
opentelemetry-collector copied to clipboard
[PoC] Show how configoptional would work
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
- 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. GetOrInsertDefaultis a long name and I don't like it.- You can still call the
configoptional.NewFactorylocally 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