[configoptional] Allow wrapping scalar values#15175
[configoptional] Allow wrapping scalar values#15175evan-bradley wants to merge 2 commits intoopen-telemetry:mainfrom
Conversation
evan-bradley
force-pushed
the
configoptional-scalars-revived
branch
from
521f5fd to
495f28d
Compare
|
Leaving in draft while I resolve a bunch of rebase issues and prepare follow-up PRs to show this in use. |
jade-guiton-dd
left a comment
jade-guiton-dd
left a comment
There was a problem hiding this comment.
Some first reactions
| // The type returned here should be type `T` for a generic wrapper | ||
| // `Wrapper[T]` as Go's reflection utilities do not allow for retrieving the | ||
| // type of `T` directly from the wrapper type. | ||
| ScalarType() any |
There was a problem hiding this comment.
If this will only be used for reflection, maybe we can just return reflect.Type? That way we can use reflect.TypeFor[T](), which should be slightly more efficient than a dummy any value
There was a problem hiding this comment.
I thought about that, and see the benefit of having that be a return type, even if just for better clarifying the intent of the interface method. The main reason I didn't was to avoid a reflection call inside method implementations; secondarily, I was trying to reduce the complexity/boilerplate of implementations as much as possible (though a single function call isn't all that much). I don't feel strongly about this, so if you think that's not an issue or is an okay tradeoff, I'll just make the change.
| // type and then pass it to UnmarshalScalar. | ||
| if err := internal.Decode(from.Interface(), resultVal.Interface(), *opts, false); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I'm still not very happy with the idea of decoding into a single known type before passing the result into UnmarshalScalar, because:
- It's needlessly different from how
Unmarshalworks, which passes the raw input to the method and requires a recursive call toconf.Unmarshalfor simple use cases; - It doesn't work for some slightly advanced use cases, such as being able to unmarshal either a string or an integer. At the moment, these kinds of use cases are met by
Unmarshalon the surrounding struct, butOptionalis specifically trying to move away from that pattern, so I'd rather not ignore them as out of scope.
The only reason this PR works for the Optional use case despite these limitations is because of the special case above for scalar type | null which bypasses Decode. But why stop there? I think passing the any hook input directly into UnmarshalScalar would be more flexible, even if it requires slightly more code inside the handler (the scalar equivalents to the recursive conf.Unmarshal calls), and wouldn't lock us out of adding helper methods or types on top of this interface later to make common use cases easier.
As for the other potential use case of ScalarType we've discussed previously (dynamic config schema inspection): I don't think this is very relevant anymore, now that the roadmap for configuration schemas makes it clear that we eventually want to generate both the Go structs and the schema for a third source of truth, instead of generating the latter from the runtime Go types.
Like during the previous PR, I would like to reiterate that this is not a blocking comment. As long as the Optional use case is met, flexibility for other use cases is secondary.
There was a problem hiding this comment.
Thanks for all the detailed thoughts. Honestly, I'm not a huge fan of the Unmarshaler interface, and would prefer something higher-level that allows structs to customize/extend unmarshaling through interface implementation instead of being required to themselves actively participate in the unmarshaling process. That's most of the motivation behind why I have the hook do most of the heavy lifting and restrict the interface to such a particular use-case. For unmarshaling a string or integer like you're suggesting, I would first reach for the encoding.TextUnmarshaler interface, though it's possible that misses aspects of what you had in mind for that example.
As for the other potential use case of ScalarType we've discussed previously (dynamic config schema inspection)
I think this is still relevant to some degree: types that have non-trivial uses of Unmarshal, or UnmarshalScalar if we push more imperative code into implementations, are harder to generate from our configuration schemas (at a minimum, the templates become more complex). If possible, I would like to provide interfaces in our APIs that allow all current (and hopefully future) use cases to be fairly easily programmatically generated from those schemas.
At this point, I'm not as picky about the interface we end up on since these problems are hard and I'd also prefer to just get something that works for our immediate use case.
I'll try an alternative implementation that passes a curried internal.Decode function to UnmarshalScalar along with the raw value, which will work fairly close to how Unmarshal works. I think it should address your concerns while remaining fairly simple.
There was a problem hiding this comment.
See the latest commit. If we can agree on a design for unmarshaling, I'll see if I can also adjust the marshaling design.
2 participants
Description
Allow
configoptional.Optionalto wrap scalar values using new interfaces designed specifically to handle similar wrappers around scalar values.Follow up to #13524.