[Do Not Merge] POC optional hook #11409
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| receivers: | ||
| otlp: | ||
| protocols: | ||
| http: | ||
| endpoint: localhost:4317 | ||
| cors: | ||
| allowed_origins: | ||
| - http://test.com | ||
| allowed_headers: | ||
| - Example-Header | ||
| max_age: 7200 | ||
|
|
||
| exporters: | ||
| otlphttp: | ||
| endpoint: localhost:4318 | ||
| max_idle_conns: 567 | ||
|
|
||
| service: | ||
| pipelines: | ||
| traces: | ||
| receivers: [otlp] | ||
| exporters: [otlphttp] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler | |
| WeaklyTypedInput: false, | ||
| MatchName: caseSensitiveMatchName, | ||
| DecodeHook: mapstructure.ComposeDecodeHookFunc( | ||
| optionalHookFunc(), | ||
| useExpandValue(), | ||
| expandNilStructPointersHookFunc(), | ||
| mapstructure.StringToSliceHookFunc(","), | ||
|
|
@@ -431,6 +432,40 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { | |
| } | ||
| } | ||
|
|
||
| // optionalHookFunc applies logic when the to.Type is optional.Optional. When decoding primitive types, | ||
| // we use reflect.Value's FieldByName API (https://pkg.go.dev/reflect#Value.FieldByName) and Set (https://pkg.go.dev/reflect#Value.Set) APIs | ||
| // in order to set the Value and HasValue. Set cannot be used for struct field types, as the set value must be assignable to the target type, | ||
| // so for structs we use optionals custom unmarshaller in order to cast the map to the struct. | ||
| // | ||
| // This logic relies on the fact that the hook only gets called when the value is explicitely set in the config, | ||
| // so if we are in this hook, we know we can set HasValue to true. | ||
| func optionalHookFunc() mapstructure.DecodeHookFuncValue { | ||
| return func(from reflect.Value, to reflect.Value) (any, error) { | ||
| // As the type is generic, we check for the prefix. | ||
| if strings.HasPrefix(to.Type().String(), "optional.Optional") { | ||
| // the optional.Optional field is a struct | ||
| if _, ok := from.Interface().(map[string]any); ok { | ||
| unmarshaler, ok := to.Addr().Interface().(Unmarshaler) | ||
| if !ok { | ||
| return from.Interface(), nil | ||
| } | ||
| c := NewFromStringMap(from.Interface().(map[string]any)) | ||
| if err := unmarshaler.Unmarshal(c); err != nil { | ||
| return nil, err | ||
| } | ||
| return to.Interface(), nil | ||
| } | ||
|
|
||
| // the optional.Optional field is a primitive type | ||
|
There was a problem hiding this comment. I'm just spitballing here, but would it be possible to instead have an interface specifically for unmarshalling primitive values? For example, maybe someone would want to do the following: type EsotericIntEncoding int
func (e *EsotericIntEncoding) UnmarshalPrimitive(val any) error {
x, ok := val.(int)
if !ok {
return errors.New("not an int")
}
*e = EsotericIntEncoding(x)
return nil
}We could then check if There was a problem hiding this comment. This would also allow the There was a problem hiding this comment.
I'm not sure to understand how this would work:
I've made changes that unexport the Optional structs fields, by using a custom unmarshaller. There was a problem hiding this comment.
Yeah, I think an
Yes, I think it could help with other primitive types (e.g. special encodings for strings, ints, etc.) in addition to helping here with if _, ok = from.Interface().(map[string]any); !ok {
unmarshaler, ok := toPtr.(PrimitiveUnmarshaler)
if !ok {
return from.Interface(), nil
}
if err := unmarshaler.UnmarshalPrimitive(from.Interface()); err != nil {
return nil, err
}
return unmarshaler, nil
}I tested this locally and it appears to work. There was a problem hiding this comment. I see, thanks for the explanation! I've removed the optional hook and moved all logic to There was a problem hiding this comment. I can clean up the PR and add tests if we choose to go with this approach. Do we need other reviewers ? perhaps @mx-psi or @yurishkuro ? There was a problem hiding this comment. I would like input from both of them before continuing. We should also check whether @yurishkuro would like to continue work on #10260, since that was opened before this. If you have time, would you be willing to add/update some basic tests, or report on the output of manually running the Collector with these changes, to show that this is viable? There was a problem hiding this comment. I will mention as an aside that I looked at The downside here is that There was a problem hiding this comment.
I added a let me know if this is enough, or if I should still add tests ? There was a problem hiding this comment.
Happy to close this in favor of Yuri's PR. The only additions/substraction I made here:
|
||
| to.FieldByName("Value").Set(from) | ||
| to.FieldByName("HasValue").SetBool(true) | ||
| return to.Interface(), nil | ||
| } | ||
|
|
||
| return from.Interface(), nil | ||
| } | ||
| } | ||
|
|
||
| // Provides a mechanism for individual structs to define their own unmarshal logic, | ||
| // by implementing the Unmarshaler interface, unless skipTopLevelUnmarshaler is | ||
| // true and the struct matches the top level object being unmarshaled. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package optional | ||
|
|
||
| import "go.opentelemetry.io/collector/confmap" | ||
|
|
||
| type Optional[T any] struct { | ||
| Value T | ||
| HasValue bool | ||
| } | ||
|
|
||
| func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { | ||
| o.HasValue = true | ||
| if err := conf.Unmarshal(&o.Value); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.