From bb5b4281e68b6684acd4b4865f0362a8ab16e8b3 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Sat, 24 Feb 2024 17:32:55 -0800 Subject: [PATCH 1/4] [confmap] confmap honors `Unmarshal` methods on config embedded structs. --- .../support_embedded_structs_confmap.yaml | 25 ++++++++ cmd/mdatagen/metricdata.go | 4 +- confmap/confmap.go | 36 +++++++++++ confmap/confmap_test.go | 59 ++++++++++++++++++- 4 files changed, 120 insertions(+), 4 deletions(-) create mode 100755 .chloggen/support_embedded_structs_confmap.yaml diff --git a/.chloggen/support_embedded_structs_confmap.yaml b/.chloggen/support_embedded_structs_confmap.yaml new file mode 100755 index 00000000000..7d010e9524d --- /dev/null +++ b/.chloggen/support_embedded_structs_confmap.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: confmap honors `Unmarshal` methods on config embedded structs. + +# One or more tracking issues or pull requests related to the change +issues: [6671] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] diff --git a/cmd/mdatagen/metricdata.go b/cmd/mdatagen/metricdata.go index f4515136aae..5bf77985cc9 100644 --- a/cmd/mdatagen/metricdata.go +++ b/cmd/mdatagen/metricdata.go @@ -125,7 +125,7 @@ func (d *gauge) Unmarshal(parser *confmap.Conf) error { if err := d.MetricValueType.Unmarshal(parser); err != nil { return err } - return parser.Unmarshal(d) + return parser.Unmarshal(d, confmap.WithIgnoreUnused()) } func (d gauge) Type() string { @@ -155,7 +155,7 @@ func (d *sum) Unmarshal(parser *confmap.Conf) error { if err := d.MetricValueType.Unmarshal(parser); err != nil { return err } - return parser.Unmarshal(d) + return parser.Unmarshal(d, confmap.WithIgnoreUnused()) } // TODO: Currently, this func will not be called because of https://github.com/open-telemetry/opentelemetry-collector/issues/6671. Uncomment function and diff --git a/confmap/confmap.go b/confmap/confmap.go index 9664109f9ab..c43932f2c85 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -157,6 +157,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), + embeddedStructsHookFunc(result), zeroSliceHookFunc(), ), } @@ -261,6 +262,41 @@ func mapKeyStringToMapKeyTextUnmarshalerHookFunc() mapstructure.DecodeHookFuncTy } } +func embeddedStructsHookFunc(_ any) mapstructure.DecodeHookFuncValue { + return func(from reflect.Value, to reflect.Value) (any, error) { + if to.Type().Kind() != reflect.Struct { + return from.Interface(), nil + } + + finalFrom := from.Interface() + + for i := 0; i < to.Type().NumField(); i++ { + if to.Type().Field(i).IsExported() && to.Type().Field(i).Anonymous { + f := to.Field(i) + if unmarshaler, ok := f.Addr().Interface().(Unmarshaler); ok { + fromMap, ok := finalFrom.(map[string]any) + if !ok { + return from.Interface(), nil + } + if err := unmarshaler.Unmarshal(NewFromStringMap(fromMap)); err != nil { + return nil, err + } + conf := New() + if err := conf.Marshal(unmarshaler); err != nil { + return nil, err + } + resultMap := conf.ToStringMap() + for k, v := range resultMap { + fromMap[k] = v + } + finalFrom = fromMap + } + } + } + return finalFrom, nil + } +} + // Provides a mechanism for individual structs to define their own unmarshal logic, // by implementing the Unmarshaler interface. func unmarshalerHookFunc(result any) mapstructure.DecodeHookFuncValue { diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 6680a1bc517..ea402bad6ec 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -309,8 +309,41 @@ func newConfFromFile(t testing.TB, fileName string) map[string]any { } type testConfig struct { - Next *nextConfig `mapstructure:"next"` - Another string `mapstructure:"another"` + Next *nextConfig `mapstructure:"next"` + Another string `mapstructure:"another"` + EmbeddedConfig `mapstructure:",squash"` + EmbeddedConfig2 `mapstructure:",squash"` +} + +type testConfigWithoutUnmarshaler struct { + Next *nextConfig `mapstructure:"next"` + Another string `mapstructure:"another"` + EmbeddedConfig `mapstructure:",squash"` + EmbeddedConfig2 `mapstructure:",squash"` +} + +type EmbeddedConfig struct { + Some string `mapstructure:"some"` +} + +func (ec *EmbeddedConfig) Unmarshal(component *Conf) error { + if err := component.Unmarshal(ec, WithIgnoreUnused()); err != nil { + return err + } + ec.Some += " is also called" + return nil +} + +type EmbeddedConfig2 struct { + Some2 string `mapstructure:"some_2"` +} + +func (ec *EmbeddedConfig2) Unmarshal(component *Conf) error { + if err := component.Unmarshal(ec, WithIgnoreUnused()); err != nil { + return err + } + ec.Some2 += " also called2" + return nil } func (tc *testConfig) Unmarshal(component *Conf) error { @@ -340,12 +373,34 @@ func TestUnmarshaler(t *testing.T) { "string": "make sure this", }, "another": "make sure this", + "some": "make sure this", + "some_2": "this better be", }) tc := &testConfig{} assert.NoError(t, cfgMap.Unmarshal(tc)) assert.Equal(t, "make sure this", tc.Another) assert.Equal(t, "make sure this is called", tc.Next.String) + assert.Equal(t, "make sure this is also called", tc.EmbeddedConfig.Some) + assert.Equal(t, "this better be also called2", tc.EmbeddedConfig2.Some2) +} + +func TestEmbeddedUnmarshaler(t *testing.T) { + cfgMap := NewFromStringMap(map[string]any{ + "next": map[string]any{ + "string": "make sure this", + }, + "another": "make sure this", + "some": "make sure this", + "some_2": "this better be", + }) + + tc := &testConfigWithoutUnmarshaler{} + assert.NoError(t, cfgMap.Unmarshal(tc)) + assert.Equal(t, "make sure this", tc.Another) + assert.Equal(t, "make sure this is called", tc.Next.String) + assert.Equal(t, "make sure this is also called", tc.EmbeddedConfig.Some) + assert.Equal(t, "this better be also called2", tc.EmbeddedConfig2.Some2) } func TestUnmarshalerKeepAlreadyInitialized(t *testing.T) { From 4e8a0931334fae8ccd076a8852bb3bcd5842ba42 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Fri, 1 Mar 2024 17:55:37 -0800 Subject: [PATCH 2/4] code review --- confmap/confmap.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index d8aa67306cd..1678385140a 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -157,7 +157,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), - unmarshalerEmbeddedStructsHookFunc(result), + unmarshalerEmbeddedStructsHookFunc(), zeroSliceHookFunc(), ), } @@ -262,7 +262,9 @@ func mapKeyStringToMapKeyTextUnmarshalerHookFunc() mapstructure.DecodeHookFuncTy } } -func unmarshalerEmbeddedStructsHookFunc(_ any) mapstructure.DecodeHookFuncValue { +// unmarshalerEmbeddedStructsHookFunc provides a mechanism for embedded structs to define their own unmarshal logic, +// by implementing the Unmarshaler interface. +func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { return func(from reflect.Value, to reflect.Value) (any, error) { if to.Type().Kind() != reflect.Struct { return from.Interface(), nil From 9ce3bb89bb055f530b69d8deb373a6937e7e26d0 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Thu, 7 Mar 2024 11:15:36 -0800 Subject: [PATCH 3/4] adding comments as per code review --- confmap/confmap.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/confmap/confmap.go b/confmap/confmap.go index 1678385140a..be8515a8faf 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -157,6 +157,8 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), + // after the main unmarshaler hook is called, + // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), zeroSliceHookFunc(), ), @@ -274,11 +276,15 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } for i := 0; i < to.Type().NumField(); i++ { + // embedded structs passed in via `squash` cannot be pointers. We just check if they are structs: if to.Type().Field(i).IsExported() && to.Type().Field(i).Anonymous { if unmarshaler, ok := to.Field(i).Addr().Interface().(Unmarshaler); ok { if err := unmarshaler.Unmarshal(NewFromStringMap(fromAsMap)); err != nil { return nil, err } + // the struct we receive from this unmarshaling only contains fields related to the embedded struct. + // we merge this partially unmarshaled struct with the rest of the result. + // note we already unmarshaled the main struct earlier, and therefore merge with it. conf := New() if err := conf.Marshal(unmarshaler); err != nil { return nil, err From 5096427d70b443a585f2e74a8c83a43746ea1892 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Mon, 11 Mar 2024 23:39:08 -0700 Subject: [PATCH 4/4] add skipped test --- component/config_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/component/config_test.go b/component/config_test.go index fcddc343d20..2359dc09419 100644 --- a/component/config_test.go +++ b/component/config_test.go @@ -11,6 +11,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" ) var _ fmt.Stringer = (Type)("") @@ -417,3 +419,28 @@ func TestNewType(t *testing.T) { }) } } + +type configWithEmbeddedStruct struct { + String string `mapstructure:"string"` + Num int `mapstructure:"num"` + embeddedUnmarshallingConfig +} + +type embeddedUnmarshallingConfig struct { +} + +func (euc *embeddedUnmarshallingConfig) Unmarshal(_ *confmap.Conf) error { + return nil // do nothing. +} +func TestStructWithEmbeddedUnmarshaling(t *testing.T) { + t.Skip("Skipping, to be fixed with https://github.com/open-telemetry/opentelemetry-collector/issues/7102") + cfgMap := confmap.NewFromStringMap(map[string]any{ + "string": "foo", + "num": 123, + }) + tc := &configWithEmbeddedStruct{} + err := UnmarshalConfig(cfgMap, tc) + require.NoError(t, err) + assert.Equal(t, "foo", tc.String) + assert.Equal(t, 123, tc.Num) +}