From 19fa719270f0d42b85f6af806509c11ea844cf82 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 29 Jul 2025 08:44:04 -0400 Subject: [PATCH 01/25] Allow configoptional to wrap scalar values --- config/configoptional/optional.go | 20 ++++++++++++++++++++ confmap/confmap.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index b6eabafe9a3a..aee6771ed4e0 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -135,6 +135,7 @@ func (o *Optional[T]) Get() *T { } var _ confmap.Unmarshaler = (*Optional[any])(nil) +var _ confmap.ScalarUnmarshaler = (*Optional[any])(nil) // Unmarshal the configuration into the Optional value. // @@ -165,6 +166,25 @@ func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { return nil } +func (o *Optional[T]) UnmarshalScalar(val any) (any, error) { + if o.flavor == noneFlavor && val == nil { + // If the Optional is None and the configuration is nil, we do nothing. + // This replicates the behavior of unmarshaling into a field with a nil pointer. + return nil, nil + } + + if val != nil { + v, ok := val.(T) + if !ok { + return nil, fmt.Errorf("val is not type %T", v) + } + o.value = v + o.flavor = someFlavor + } + + return o.value, nil +} + var _ confmap.Marshaler = (*Optional[any])(nil) // Marshal the Optional value into the configuration. diff --git a/confmap/confmap.go b/confmap/confmap.go index 4009cd786530..0aa743fae089 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -287,6 +287,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result, skipTopLevelUnmarshaler), + scalarunmarshalerHookFunc(result), // after the main unmarshaler hook is called, // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), @@ -563,6 +564,26 @@ func unmarshalerHookFunc(result any, skipTopLevelUnmarshaler bool) mapstructure. }) } +// 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. +func scalarunmarshalerHookFunc(result any) mapstructure.DecodeHookFuncValue { + return safeWrapDecodeHookFunc(func(from reflect.Value, to reflect.Value) (any, error) { + if !to.CanAddr() { + return from.Interface(), nil + } + + toPtr := to.Addr().Interface() + + unmarshaler, ok := toPtr.(ScalarUnmarshaler) + if !ok { + return from.Interface(), nil + } + + return unmarshaler.UnmarshalScalar(from) + }) +} + // marshalerHookFunc returns a DecodeHookFuncValue that checks structs that aren't // the original to see if they implement the Marshaler interface. func marshalerHookFunc(orig any) mapstructure.DecodeHookFuncValue { @@ -608,6 +629,15 @@ type Unmarshaler interface { Unmarshal(component *Conf) error } +// ScalarUnmarshaler is an interface which may be implemented by wrapper types +// to customize their behavior when the type under the wrapper is a scalar value. +type ScalarUnmarshaler interface { + // Unmarshal a Conf into the struct in a custom way. + // The Conf for this specific component may be nil or empty if no config available. + // This method should only be called by decoding hooks when calling Conf.Unmarshal. + UnmarshalScalar(val any) (any, error) +} + // Marshaler defines an optional interface for custom configuration marshaling. // A configuration struct can implement this interface to override the default // marshaling. From 3ef4aa6327e1bebc44408c703d38bfc5b3f8b84a Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 29 Jul 2025 08:45:07 -0400 Subject: [PATCH 02/25] Revert "[chore] [exporterhelper] Revert moving StorageID to configpoptional (#13376)" This reverts commit 1ebb9b20cfa54b65ed2de8b2d9643d488c7e9c87. --- exporter/exporterhelper/internal/base_exporter.go | 2 +- exporter/exporterhelper/internal/base_exporter_test.go | 4 ++-- exporter/exporterhelper/internal/queue/persistent_queue.go | 2 +- .../exporterhelper/internal/queue/persistent_queue_test.go | 7 +++---- exporter/exporterhelper/internal/queue/queue.go | 5 +++-- exporter/exporterhelper/internal/queuebatch/config.go | 7 +++---- exporter/exporterhelper/internal/queuebatch/config_test.go | 4 ++-- .../exporterhelper/internal/queuebatch/queue_batch_test.go | 6 +++--- exporter/exporterhelper/logs_test.go | 3 ++- exporter/exporterhelper/metrics_test.go | 3 ++- exporter/exporterhelper/traces_test.go | 3 ++- exporter/exporterhelper/xexporterhelper/go.mod | 2 +- exporter/exporterhelper/xexporterhelper/profiles_test.go | 3 ++- 13 files changed, 27 insertions(+), 24 deletions(-) diff --git a/exporter/exporterhelper/internal/base_exporter.go b/exporter/exporterhelper/internal/base_exporter.go index 7631699a2926..1ee43265286e 100644 --- a/exporter/exporterhelper/internal/base_exporter.go +++ b/exporter/exporterhelper/internal/base_exporter.go @@ -213,7 +213,7 @@ func WithQueueBatch(cfg queuebatch.Config, set QueueBatchSettings[request.Reques o.ExportFailureMessage += " Try enabling sending_queue to survive temporary failures." return nil } - if cfg.StorageID != nil && set.Encoding == nil { + if cfg.StorageID.HasValue() && set.Encoding == nil { return errors.New("`QueueBatchSettings.Encoding` must not be nil when persistent queue is enabled") } o.queueBatchSettings = set diff --git a/exporter/exporterhelper/internal/base_exporter_test.go b/exporter/exporterhelper/internal/base_exporter_test.go index f43216c990ec..a2eb7ef20516 100644 --- a/exporter/exporterhelper/internal/base_exporter_test.go +++ b/exporter/exporterhelper/internal/base_exporter_test.go @@ -15,6 +15,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/config/configretry" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/requesttest" @@ -52,8 +53,7 @@ func TestQueueOptionsWithRequestExporter(t *testing.T) { require.Error(t, err) qCfg := NewDefaultQueueConfig() - storageID := component.NewID(component.MustNewType("test")) - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(component.MustNewID("test")) _, err = NewBaseExporter(exportertest.NewNopSettings(exportertest.NopType), pipeline.SignalMetrics, noopExport, WithQueueBatchSettings(newFakeQueueBatch()), WithRetry(configretry.NewDefaultBackOffConfig()), diff --git a/exporter/exporterhelper/internal/queue/persistent_queue.go b/exporter/exporterhelper/internal/queue/persistent_queue.go index 23c43115367a..cd58f6098882 100644 --- a/exporter/exporterhelper/internal/queue/persistent_queue.go +++ b/exporter/exporterhelper/internal/queue/persistent_queue.go @@ -102,7 +102,7 @@ func newPersistentQueue[T any](set Settings[T]) readableQueue[T] { activeSizer: set.activeSizer(), itemsSizer: set.ItemsSizer, bytesSizer: set.BytesSizer, - storageID: *set.StorageID, + storageID: *set.StorageID.Get(), id: set.ID, signal: set.Signal, blockOnOverflow: set.BlockOnOverflow, diff --git a/exporter/exporterhelper/internal/queue/persistent_queue_test.go b/exporter/exporterhelper/internal/queue/persistent_queue_test.go index 72bb81c53bdf..25e1ee1c54d6 100644 --- a/exporter/exporterhelper/internal/queue/persistent_queue_test.go +++ b/exporter/exporterhelper/internal/queue/persistent_queue_test.go @@ -20,6 +20,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/experr" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/hosttest" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" @@ -231,8 +232,7 @@ func newSettings(sizerType request.SizerType, capacity int64) Settings[int64] { func newSettingsWithStorage(sizerType request.SizerType, capacity int64) Settings[int64] { set := newSettings(sizerType, capacity) - storageID := component.ID{} - set.StorageID = &storageID + set.StorageID = configoptional.Some(component.ID{}) return set } @@ -513,8 +513,7 @@ func TestInvalidStorageExtensionType(t *testing.T) { } func TestPersistentQueue_StopAfterBadStart(t *testing.T) { - storageID := component.ID{} - pq := newPersistentQueue[int64](Settings[int64]{StorageID: &storageID}) + pq := newPersistentQueue[int64](Settings[int64]{StorageID: configoptional.Some(component.ID{})}) // verify that stopping a un-start/started w/error queue does not panic assert.NoError(t, pq.Shutdown(context.Background())) } diff --git a/exporter/exporterhelper/internal/queue/queue.go b/exporter/exporterhelper/internal/queue/queue.go index 9583be9fb25b..f286a95dd081 100644 --- a/exporter/exporterhelper/internal/queue/queue.go +++ b/exporter/exporterhelper/internal/queue/queue.go @@ -8,6 +8,7 @@ import ( "errors" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" "go.opentelemetry.io/collector/pipeline" ) @@ -63,7 +64,7 @@ type Settings[T any] struct { WaitForResult bool BlockOnOverflow bool Signal pipeline.Signal - StorageID *component.ID + StorageID configoptional.Optional[component.ID] Encoding Encoding[T] ID component.ID Telemetry component.TelemetrySettings @@ -96,7 +97,7 @@ func NewQueue[T request.Request](set Settings[T], next ConsumeFunc[T]) (Queue[T] func newBaseQueue[T any](set Settings[T]) (readableQueue[T], error) { // Configure memory queue or persistent based on the config. - if set.StorageID == nil { + if !set.StorageID.HasValue() { return newMemoryQueue[T](set), nil } if set.ItemsSizer == nil { diff --git a/exporter/exporterhelper/internal/queuebatch/config.go b/exporter/exporterhelper/internal/queuebatch/config.go index 5f031c1652a2..7caa75e08605 100644 --- a/exporter/exporterhelper/internal/queuebatch/config.go +++ b/exporter/exporterhelper/internal/queuebatch/config.go @@ -33,10 +33,9 @@ type Config struct { // If true, the component will wait for space; otherwise, operations will immediately return a retryable error. BlockOnOverflow bool `mapstructure:"block_on_overflow"` - // StorageID if not empty, enables the persistent storage and uses the component specified + // StorageID, if not empty, enables the persistent storage and uses the component specified // as a storage extension for the persistent queue. - // TODO: This will be changed to Optional when available. - StorageID *component.ID `mapstructure:"storage"` + StorageID configoptional.Optional[component.ID] `mapstructure:"storage"` // NumConsumers is the maximum number of concurrent consumers from the queue. // This applies across all different optional configurations from above (e.g. wait_for_result, blockOnOverflow, persistent, etc.). @@ -75,7 +74,7 @@ func (cfg *Config) Validate() error { } // Only support request sizer for persistent queue at this moment. - if cfg.StorageID != nil && cfg.WaitForResult { + if cfg.StorageID.HasValue() && cfg.WaitForResult { return errors.New("`wait_for_result` is not supported with a persistent queue configured with `storage`") } diff --git a/exporter/exporterhelper/internal/queuebatch/config_test.go b/exporter/exporterhelper/internal/queuebatch/config_test.go index c3da0a1d7cb9..62065cd7ed1f 100644 --- a/exporter/exporterhelper/internal/queuebatch/config_test.go +++ b/exporter/exporterhelper/internal/queuebatch/config_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" ) @@ -29,10 +30,9 @@ func TestConfig_Validate(t *testing.T) { cfg.QueueSize = 0 require.EqualError(t, cfg.Validate(), "`queue_size` must be positive") - storageID := component.MustNewID("test") cfg = newTestConfig() cfg.WaitForResult = true - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(component.MustNewID("test")) require.EqualError(t, cfg.Validate(), "`wait_for_result` is not supported with a persistent queue configured with `storage`") cfg = newTestConfig() diff --git a/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go b/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go index fb962d6cf400..8bef8d251eb9 100644 --- a/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go +++ b/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go @@ -151,7 +151,7 @@ func TestQueueBatchDifferentSizers(t *testing.T) { func TestQueueBatchPersistenceEnabled(t *testing.T) { cfg := newTestConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) qb, err := NewQueueBatch(newFakeRequestSettings(), cfg, sendertest.NewNopSenderFunc[request.Request]()) require.NoError(t, err) @@ -168,7 +168,7 @@ func TestQueueBatchPersistenceEnabledStorageError(t *testing.T) { storageError := errors.New("could not get storage client") cfg := newTestConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) qb, err := NewQueueBatch(newFakeRequestSettings(), cfg, sendertest.NewNopSenderFunc[request.Request]()) require.NoError(t, err) @@ -184,7 +184,7 @@ func TestQueueBatchPersistentEnabled_NoDataLossOnShutdown(t *testing.T) { cfg := newTestConfig() cfg.NumConsumers = 1 storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) mockReq := &requesttest.FakeRequest{Items: 2} qSet := newFakeRequestSettings() diff --git a/exporter/exporterhelper/logs_test.go b/exporter/exporterhelper/logs_test.go index 867b66649162..eb89505c8984 100644 --- a/exporter/exporterhelper/logs_test.go +++ b/exporter/exporterhelper/logs_test.go @@ -22,6 +22,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/consumertest" @@ -173,7 +174,7 @@ func TestLogs_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/metrics_test.go b/exporter/exporterhelper/metrics_test.go index 81e98ad84b1e..9c62ea51f906 100644 --- a/exporter/exporterhelper/metrics_test.go +++ b/exporter/exporterhelper/metrics_test.go @@ -22,6 +22,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/consumertest" @@ -173,7 +174,7 @@ func TestMetrics_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_metrics", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/traces_test.go b/exporter/exporterhelper/traces_test.go index 59e622dc9aeb..84ea4721af16 100644 --- a/exporter/exporterhelper/traces_test.go +++ b/exporter/exporterhelper/traces_test.go @@ -22,6 +22,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/consumertest" @@ -171,7 +172,7 @@ func TestTraces_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/xexporterhelper/go.mod b/exporter/exporterhelper/xexporterhelper/go.mod index ad4b2ac29b8a..940a735057aa 100644 --- a/exporter/exporterhelper/xexporterhelper/go.mod +++ b/exporter/exporterhelper/xexporterhelper/go.mod @@ -6,6 +6,7 @@ require ( github.com/stretchr/testify v1.10.0 go.opentelemetry.io/collector/component v1.37.0 go.opentelemetry.io/collector/component/componenttest v0.131.0 + go.opentelemetry.io/collector/config/configoptional v0.131.0 go.opentelemetry.io/collector/consumer v1.37.0 go.opentelemetry.io/collector/consumer/consumererror v0.131.0 go.opentelemetry.io/collector/consumer/consumererror/xconsumererror v0.131.0 @@ -45,7 +46,6 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/auto/sdk v1.1.0 // indirect go.opentelemetry.io/collector/client v1.37.0 // indirect - go.opentelemetry.io/collector/config/configoptional v0.131.0 // indirect go.opentelemetry.io/collector/config/configretry v1.37.0 // indirect go.opentelemetry.io/collector/confmap v1.37.0 // indirect go.opentelemetry.io/collector/extension v1.37.0 // indirect diff --git a/exporter/exporterhelper/xexporterhelper/profiles_test.go b/exporter/exporterhelper/xexporterhelper/profiles_test.go index 687346703373..44bd2d0806dd 100644 --- a/exporter/exporterhelper/xexporterhelper/profiles_test.go +++ b/exporter/exporterhelper/xexporterhelper/profiles_test.go @@ -20,6 +20,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/consumer/consumererror" "go.opentelemetry.io/collector/consumer/consumererror/xconsumererror" @@ -171,7 +172,7 @@ func TestProfiles_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := exporterhelper.NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ From b916d0c94b5528000ca0e087d5189b4255e5c473 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 29 Jul 2025 16:59:14 -0400 Subject: [PATCH 03/25] confmap changes --- config/configoptional/optional.go | 30 ++++++- confmap/confmap.go | 113 ++++++++++---------------- confmap/internal/marshaloption.go | 29 +++++++ confmap/internal/unmarshaloption.go | 30 +++++++ confmap/xconfmap/go.mod | 2 +- confmap/xconfmap/scalarmarshaler.go | 43 ++++++++++ confmap/xconfmap/scalarunmarshaler.go | 81 ++++++++++++++++++ 7 files changed, 251 insertions(+), 77 deletions(-) create mode 100644 confmap/internal/marshaloption.go create mode 100644 confmap/internal/unmarshaloption.go create mode 100644 confmap/xconfmap/scalarmarshaler.go create mode 100644 confmap/xconfmap/scalarunmarshaler.go diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index aee6771ed4e0..d25df9ee6331 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -4,12 +4,14 @@ package configoptional // import "go.opentelemetry.io/collector/config/configoptional" import ( + "encoding" "errors" "fmt" "reflect" "strings" "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/xconfmap" ) type flavor int @@ -135,7 +137,7 @@ func (o *Optional[T]) Get() *T { } var _ confmap.Unmarshaler = (*Optional[any])(nil) -var _ confmap.ScalarUnmarshaler = (*Optional[any])(nil) +var _ xconfmap.ScalarUnmarshaler = (*Optional[any])(nil) // Unmarshal the configuration into the Optional value. // @@ -158,7 +160,7 @@ func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { return nil } - if err := conf.Unmarshal(&o.value); err != nil { + if err := conf.Unmarshal(&o.value, xconfmap.WithScalarUnmarshaler()); err != nil { return err } @@ -176,7 +178,7 @@ func (o *Optional[T]) UnmarshalScalar(val any) (any, error) { if val != nil { v, ok := val.(T) if !ok { - return nil, fmt.Errorf("val is not type %T", v) + return nil, fmt.Errorf("val is %T, not %T", val, v) } o.value = v o.flavor = someFlavor @@ -185,7 +187,12 @@ func (o *Optional[T]) UnmarshalScalar(val any) (any, error) { return o.value, nil } +func (o *Optional[T]) ScalarType() any { + return o.value +} + var _ confmap.Marshaler = (*Optional[any])(nil) +var _ xconfmap.ScalarMarshaler = (*Optional[any])(nil) // Marshal the Optional value into the configuration. // If the Optional is None or Default, it does not marshal anything. @@ -203,9 +210,24 @@ func (o Optional[T]) Marshal(conf *confmap.Conf) error { return conf.Marshal(map[string]any(nil)) } - if err := conf.Marshal(o.value); err != nil { + if err := conf.Marshal(o.value, xconfmap.WithScalarMarshaler()); err != nil { return fmt.Errorf("configoptional: failed to marshal Optional value: %w", err) } return nil } + +// MarshalScalar implements xconfmap.ScalarMarshaler. +func (o Optional[T]) MarshalScalar() (string, error) { + if tm, ok := any(o.value).(encoding.TextMarshaler); ok { + str, err := tm.MarshalText() + + if err != nil { + return "", err + } + + return string(str), nil + } + + return "", nil +} diff --git a/confmap/confmap.go b/confmap/confmap.go index 0aa743fae089..cc902b8bea13 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -18,6 +18,7 @@ import ( "github.com/knadh/koanf/providers/confmap" "github.com/knadh/koanf/v2" + "go.opentelemetry.io/collector/confmap/internal" encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" "go.opentelemetry.io/collector/confmap/internal/third_party/composehook" ) @@ -71,47 +72,39 @@ func (l *Conf) AllKeys() []string { } type UnmarshalOption interface { - apply(*unmarshalOption) -} - -type unmarshalOption struct { - ignoreUnused bool + internal.UnmarshalOption } // WithIgnoreUnused sets an option to ignore errors if existing // keys in the original Conf were unused in the decoding process // (extra keys). func WithIgnoreUnused() UnmarshalOption { - return unmarshalOptionFunc(func(uo *unmarshalOption) { - uo.ignoreUnused = true + return internal.UnmarshalOptionFunc(func(uo *internal.UnmarshalOptions) { + uo.IgnoreUnused = true }) } -type unmarshalOptionFunc func(*unmarshalOption) - -func (fn unmarshalOptionFunc) apply(set *unmarshalOption) { - fn(set) -} - // Unmarshal unmarshalls the config into a struct using the given options. // Tags on the fields of the structure must be properly set. func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { - set := unmarshalOption{} + set := internal.UnmarshalOptions{} for _, opt := range opts { - opt.apply(&set) + internal.ApplyUnmarshalOption(opt, &set) } - return decodeConfig(l, result, !set.ignoreUnused, l.skipTopLevelUnmarshaler) + return decodeConfig(l, result, set, l.skipTopLevelUnmarshaler) } -type marshalOption struct{} - type MarshalOption interface { - apply(*marshalOption) + internal.MarshalOption } // Marshal encodes the config and merges it into the Conf. -func (l *Conf) Marshal(rawVal any, _ ...MarshalOption) error { - enc := encoder.New(encoderConfig(rawVal)) +func (l *Conf) Marshal(rawVal any, opts ...MarshalOption) error { + set := internal.MarshalOptions{} + for _, opt := range opts { + internal.ApplyMarshalOption(opt, &set) + } + enc := encoder.New(encoderConfig(rawVal, set)) data, err := enc.Encode(rawVal) if err != nil { return err @@ -271,28 +264,30 @@ func (l *Conf) ToStringMap() map[string]any { // uniqueness of component IDs (see mapKeyStringToMapKeyTextUnmarshalerHookFunc). // Decodes time.Duration from strings. Allows custom unmarshaling for structs implementing // encoding.TextUnmarshaler. Allows custom unmarshaling for structs implementing confmap.Unmarshaler. -func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler bool) error { +func decodeConfig(m *Conf, result any, settings internal.UnmarshalOptions, skipTopLevelUnmarshaler bool) error { + hooks := []mapstructure.DecodeHookFunc{ + useExpandValue(), + expandNilStructPointersHookFunc(), + mapstructure.StringToSliceHookFunc(","), + mapKeyStringToMapKeyTextUnmarshalerHookFunc(), + mapstructure.StringToTimeDurationHookFunc(), + mapstructure.TextUnmarshallerHookFunc(), + unmarshalerHookFunc(result, skipTopLevelUnmarshaler), + // after the main unmarshaler hook is called, + // we unmarshal the embedded structs if present to merge with the result: + unmarshalerEmbeddedStructsHookFunc(), + zeroSliceAndMapHookFunc(), + } + hooks = append(hooks, settings.AdditionalDecodeHookFuncs...) + dc := &mapstructure.DecoderConfig{ - ErrorUnused: errorUnused, + ErrorUnused: !settings.IgnoreUnused, Result: result, TagName: MapstructureTag, WeaklyTypedInput: false, MatchName: caseSensitiveMatchName, DecodeNil: true, - DecodeHook: composehook.ComposeDecodeHookFunc( - useExpandValue(), - expandNilStructPointersHookFunc(), - mapstructure.StringToSliceHookFunc(","), - mapKeyStringToMapKeyTextUnmarshalerHookFunc(), - mapstructure.StringToTimeDurationHookFunc(), - mapstructure.TextUnmarshallerHookFunc(), - unmarshalerHookFunc(result, skipTopLevelUnmarshaler), - scalarunmarshalerHookFunc(result), - // after the main unmarshaler hook is called, - // we unmarshal the embedded structs if present to merge with the result: - unmarshalerEmbeddedStructsHookFunc(), - zeroSliceAndMapHookFunc(), - ), + DecodeHook: composehook.ComposeDecodeHookFunc(hooks...), } decoder, err := mapstructure.NewDecoder(dc) if err != nil { @@ -310,13 +305,16 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler // encoderConfig returns a default encoder.EncoderConfig that includes // an EncodeHook that handles both TextMarshaller and Marshaler // interfaces. -func encoderConfig(rawVal any) *encoder.EncoderConfig { +func encoderConfig(rawVal any, set internal.MarshalOptions) *encoder.EncoderConfig { + hooks := []mapstructure.DecodeHookFunc{ + encoder.YamlMarshalerHookFunc(), + encoder.TextMarshalerHookFunc(), + } + hooks = append(hooks, set.AdditionalEncodeHookFuncs...) + hooks = append(hooks, marshalerHookFunc(rawVal)) + return &encoder.EncoderConfig{ - EncodeHook: mapstructure.ComposeDecodeHookFunc( - encoder.YamlMarshalerHookFunc(), - encoder.TextMarshalerHookFunc(), - marshalerHookFunc(rawVal), - ), + EncodeHook: mapstructure.ComposeDecodeHookFunc(hooks...), } } @@ -564,26 +562,6 @@ func unmarshalerHookFunc(result any, skipTopLevelUnmarshaler bool) mapstructure. }) } -// 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. -func scalarunmarshalerHookFunc(result any) mapstructure.DecodeHookFuncValue { - return safeWrapDecodeHookFunc(func(from reflect.Value, to reflect.Value) (any, error) { - if !to.CanAddr() { - return from.Interface(), nil - } - - toPtr := to.Addr().Interface() - - unmarshaler, ok := toPtr.(ScalarUnmarshaler) - if !ok { - return from.Interface(), nil - } - - return unmarshaler.UnmarshalScalar(from) - }) -} - // marshalerHookFunc returns a DecodeHookFuncValue that checks structs that aren't // the original to see if they implement the Marshaler interface. func marshalerHookFunc(orig any) mapstructure.DecodeHookFuncValue { @@ -629,15 +607,6 @@ type Unmarshaler interface { Unmarshal(component *Conf) error } -// ScalarUnmarshaler is an interface which may be implemented by wrapper types -// to customize their behavior when the type under the wrapper is a scalar value. -type ScalarUnmarshaler interface { - // Unmarshal a Conf into the struct in a custom way. - // The Conf for this specific component may be nil or empty if no config available. - // This method should only be called by decoding hooks when calling Conf.Unmarshal. - UnmarshalScalar(val any) (any, error) -} - // Marshaler defines an optional interface for custom configuration marshaling. // A configuration struct can implement this interface to override the default // marshaling. diff --git a/confmap/internal/marshaloption.go b/confmap/internal/marshaloption.go new file mode 100644 index 000000000000..0eb0a296070e --- /dev/null +++ b/confmap/internal/marshaloption.go @@ -0,0 +1,29 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package internal // import "go.opentelemetry.io/collector/confmap/internal" + +import "github.com/go-viper/mapstructure/v2" + +type MarshalOption interface { + apply(*MarshalOptions) +} + +// MarshalOptions is used by (*Conf).Marshal to toggle unmarshaling settings. +// It is in the `internal` package so experimental options can be added in xconfmap. +type MarshalOptions struct { + AdditionalEncodeHookFuncs []mapstructure.DecodeHookFunc +} + +type MarshalOptionFunc func(*MarshalOptions) + +func (fn MarshalOptionFunc) apply(set *MarshalOptions) { + fn(set) +} + +// Apply Option simply calls (MarshalOption).apply. This function allows us +// to keep the `apply“ function private and therefore keep `confmap.MarshalOption` +// without any exported methods. +func ApplyMarshalOption(mo MarshalOption, set *MarshalOptions) { + mo.apply(set) +} diff --git a/confmap/internal/unmarshaloption.go b/confmap/internal/unmarshaloption.go new file mode 100644 index 000000000000..19f5109b2033 --- /dev/null +++ b/confmap/internal/unmarshaloption.go @@ -0,0 +1,30 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package internal // import "go.opentelemetry.io/collector/confmap/internal" + +import "github.com/go-viper/mapstructure/v2" + +type UnmarshalOption interface { + apply(*UnmarshalOptions) +} + +// UnmarshalOptions is used by (*Conf).Unmarshal to toggle unmarshaling settings. +// It is in the `internal` package so experimental options can be added in xconfmap. +type UnmarshalOptions struct { + IgnoreUnused bool + AdditionalDecodeHookFuncs []mapstructure.DecodeHookFunc +} + +type UnmarshalOptionFunc func(*UnmarshalOptions) + +func (fn UnmarshalOptionFunc) apply(set *UnmarshalOptions) { + fn(set) +} + +// Apply Option simply calls (UnmarshalOption).apply. This function allows us +// to keep the `apply“ function private and therefore keep `confmap.UnmarshalOption` +// without any exported methods. +func ApplyUnmarshalOption(uo UnmarshalOption, set *UnmarshalOptions) { + uo.apply(set) +} diff --git a/confmap/xconfmap/go.mod b/confmap/xconfmap/go.mod index 0e7df7d624fb..9a6abdf38637 100644 --- a/confmap/xconfmap/go.mod +++ b/confmap/xconfmap/go.mod @@ -3,13 +3,13 @@ module go.opentelemetry.io/collector/confmap/xconfmap go 1.23.0 require ( + github.com/go-viper/mapstructure/v2 v2.4.0 github.com/stretchr/testify v1.10.0 go.opentelemetry.io/collector/confmap v1.37.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect - github.com/go-viper/mapstructure/v2 v2.4.0 // indirect github.com/gobwas/glob v0.2.3 // indirect github.com/hashicorp/go-version v1.7.0 // indirect github.com/knadh/koanf/maps v0.1.2 // indirect diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go new file mode 100644 index 000000000000..d39a5738b71c --- /dev/null +++ b/confmap/xconfmap/scalarmarshaler.go @@ -0,0 +1,43 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "reflect" + + "github.com/go-viper/mapstructure/v2" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/internal" +) + +func WithScalarMarshaler() confmap.MarshalOption { + return internal.MarshalOptionFunc(func(mo *internal.MarshalOptions) { + mo.AdditionalEncodeHookFuncs = append(mo.AdditionalEncodeHookFuncs, scalarmarshalerHookFunc()) + }) +} + +// ScalarUnmarshaler is an interface which may be implemented by wrapper types +// to customize their behavior when the type under the wrapper is a scalar value. +type ScalarMarshaler interface { + // Unmarshal a Conf into the struct in a custom way. + // The Conf for this specific component may be nil or empty if no config available. + // This method should only be called by decoding hooks when calling Conf.Unmarshal. + MarshalScalar() (string, error) +} + +// 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. +func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { + return safeWrapDecodeHookFunc(func(from reflect.Value, _ reflect.Value) (any, error) { + marshaler, ok := from.Interface().(ScalarMarshaler) + if !ok { + return from.Interface(), nil + } + + return marshaler.MarshalScalar() + + }) +} diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go new file mode 100644 index 000000000000..1f0246f928aa --- /dev/null +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -0,0 +1,81 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "encoding" + "reflect" + + "github.com/go-viper/mapstructure/v2" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/internal" +) + +func WithScalarUnmarshaler() confmap.UnmarshalOption { + return internal.UnmarshalOptionFunc(func(uo *internal.UnmarshalOptions) { + uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc()) + }) +} + +// ScalarUnmarshaler is an interface which may be implemented by wrapper types +// to customize their behavior when the type under the wrapper is a scalar value. +type ScalarUnmarshaler interface { + // Unmarshal a Conf into the struct in a custom way. + // The Conf for this specific component may be nil or empty if no config available. + // This method should only be called by decoding hooks when calling Conf.Unmarshal. + UnmarshalScalar(val any) (any, error) + + ScalarType() any +} + +// 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. +func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { + return safeWrapDecodeHookFunc(func(from reflect.Value, to reflect.Value) (any, error) { + if !to.CanAddr() { + return from.Interface(), nil + } + + toPtr := to.Addr().Interface() + + unmarshaler, ok := toPtr.(ScalarUnmarshaler) + if !ok { + return from.Interface(), nil + } + + v := from.Interface() + tp := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) + t := tp.Interface() + if tu, ok := t.(encoding.TextUnmarshaler); ok { + // Should we error out here? + if str, ok := from.Interface().(string); ok { + if err := tu.UnmarshalText([]byte(str)); err != nil { + return nil, err + } + v = tp.Elem().Interface() + } + } + + return unmarshaler.UnmarshalScalar(v) + }) +} + +// safeWrapDecodeHookFunc wraps a DecodeHookFuncValue to ensure fromVal is a valid `reflect.Value` +// object and therefore it is safe to call `reflect.Value` methods on fromVal. +// +// Use this only if the hook does not need to be called on untyped nil values. +// Typed nil values are safe to call and will be passed to the hook. +// See https://github.com/golang/go/issues/51649 +func safeWrapDecodeHookFunc( + f mapstructure.DecodeHookFuncValue, +) mapstructure.DecodeHookFuncValue { + return func(fromVal reflect.Value, toVal reflect.Value) (any, error) { + if !fromVal.IsValid() { + return nil, nil + } + return f(fromVal, toVal) + } +} From 60348d7bb0b296dd7e50d2901d27c63c488ed646 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 29 Jul 2025 16:59:21 -0400 Subject: [PATCH 04/25] exporterhelper changes --- .../internal/queuebatch/config.go | 21 +++++----- .../internal/queuebatch/config_test.go | 42 ++++++++++++++++++- .../internal/queuebatch/queue_batch.go | 2 +- .../internal/queuebatch/testdata/config.yaml | 4 ++ 4 files changed, 57 insertions(+), 12 deletions(-) create mode 100644 exporter/exporterhelper/internal/queuebatch/testdata/config.yaml diff --git a/exporter/exporterhelper/internal/queuebatch/config.go b/exporter/exporterhelper/internal/queuebatch/config.go index 7caa75e08605..205c690efa17 100644 --- a/exporter/exporterhelper/internal/queuebatch/config.go +++ b/exporter/exporterhelper/internal/queuebatch/config.go @@ -10,45 +10,46 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/xconfmap" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" ) // Config defines configuration for queueing and batching incoming requests. type Config struct { // Enabled indicates whether to not enqueue and batch before exporting. - Enabled bool `mapstructure:"enabled"` + Enabled bool `mapstructure:"enabled,omitempty"` // WaitForResult determines if incoming requests are blocked until the request is processed or not. // Currently, this option is not available when persistent queue is configured using the storage configuration. - WaitForResult bool `mapstructure:"wait_for_result"` + WaitForResult bool `mapstructure:"wait_for_result,omitempty"` // Sizer determines the type of size measurement used by this component. // It accepts "requests", "items", or "bytes". - Sizer request.SizerType `mapstructure:"sizer"` + Sizer request.SizerType `mapstructure:"sizer,omitempty"` // QueueSize represents the maximum data size allowed for concurrent storage and processing. - QueueSize int64 `mapstructure:"queue_size"` + QueueSize int `mapstructure:"queue_size,omitempty"` // BlockOnOverflow determines the behavior when the component's TotalSize limit is reached. // If true, the component will wait for space; otherwise, operations will immediately return a retryable error. - BlockOnOverflow bool `mapstructure:"block_on_overflow"` + BlockOnOverflow bool `mapstructure:"block_on_overflow,omitempty"` // StorageID, if not empty, enables the persistent storage and uses the component specified // as a storage extension for the persistent queue. - StorageID configoptional.Optional[component.ID] `mapstructure:"storage"` + StorageID configoptional.Optional[component.ID] `mapstructure:"storage,omitempty"` // NumConsumers is the maximum number of concurrent consumers from the queue. // This applies across all different optional configurations from above (e.g. wait_for_result, blockOnOverflow, persistent, etc.). // TODO: This will also control the maximum number of shards, when supported: // https://github.com/open-telemetry/opentelemetry-collector/issues/12473. - NumConsumers int `mapstructure:"num_consumers"` + NumConsumers int `mapstructure:"num_consumers,omitempty"` // BatchConfig it configures how the requests are consumed from the queue and batch together during consumption. - Batch configoptional.Optional[BatchConfig] `mapstructure:"batch"` + Batch configoptional.Optional[BatchConfig] `mapstructure:"batch,omitempty"` } func (cfg *Config) Unmarshal(conf *confmap.Conf) error { - if err := conf.Unmarshal(cfg); err != nil { + if err := conf.Unmarshal(cfg, xconfmap.WithScalarUnmarshaler()); err != nil { return err } @@ -80,7 +81,7 @@ func (cfg *Config) Validate() error { if cfg.Batch.HasValue() && cfg.Batch.Get().Sizer == cfg.Sizer { // Avoid situations where the queue is not able to hold any data. - if cfg.Batch.Get().MinSize > cfg.QueueSize { + if cfg.Batch.Get().MinSize > int64(cfg.QueueSize) { return errors.New("`min_size` must be less than or equal to `queue_size`") } } diff --git a/exporter/exporterhelper/internal/queuebatch/config_test.go b/exporter/exporterhelper/internal/queuebatch/config_test.go index 62065cd7ed1f..5d277433f473 100644 --- a/exporter/exporterhelper/internal/queuebatch/config_test.go +++ b/exporter/exporterhelper/internal/queuebatch/config_test.go @@ -4,6 +4,7 @@ package queuebatch import ( + "path/filepath" "testing" "time" @@ -12,6 +13,9 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configoptional" + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" + "go.opentelemetry.io/collector/confmap/xconfmap" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" ) @@ -36,7 +40,7 @@ func TestConfig_Validate(t *testing.T) { require.EqualError(t, cfg.Validate(), "`wait_for_result` is not supported with a persistent queue configured with `storage`") cfg = newTestConfig() - cfg.QueueSize = cfg.Batch.Get().MinSize - 1 + cfg.QueueSize = int(cfg.Batch.Get().MinSize - 1) require.EqualError(t, cfg.Validate(), "`min_size` must be less than or equal to `queue_size`") cfg = newTestConfig() @@ -74,6 +78,42 @@ func TestBatchConfig_Validate(t *testing.T) { require.EqualError(t, cfg.Validate(), "`max_size` must be greater or equal to `min_size`") } +func TestLoadConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + + disk := component.MustNewIDWithName("disk", "") + + wantCfg := &Config{ + QueueSize: 1000, + Enabled: true, + NumConsumers: 10, + StorageID: configoptional.Some(disk), + } + defaultConfig := &Config{} + + require.NoError(t, cm.Unmarshal(defaultConfig, xconfmap.WithScalarUnmarshaler())) + assert.Equal(t, wantCfg, defaultConfig) +} + +func TestMarshalConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + + disk := component.MustNewIDWithName("disk", "") + + conf := confmap.New() + cfg := &Config{ + QueueSize: 1000, + Enabled: true, + NumConsumers: 10, + StorageID: configoptional.Some(disk), + } + + require.NoError(t, conf.Marshal(cfg, xconfmap.WithScalarMarshaler())) + assert.Equal(t, cm.ToStringMap(), conf.ToStringMap()) +} + func newTestBatchConfig() BatchConfig { return BatchConfig{ FlushTimeout: 200 * time.Millisecond, diff --git a/exporter/exporterhelper/internal/queuebatch/queue_batch.go b/exporter/exporterhelper/internal/queuebatch/queue_batch.go index 715a2a0841ae..da98fbc4a380 100644 --- a/exporter/exporterhelper/internal/queuebatch/queue_batch.go +++ b/exporter/exporterhelper/internal/queuebatch/queue_batch.go @@ -56,7 +56,7 @@ func NewQueueBatch( SizerType: cfg.Sizer, ItemsSizer: set.ItemsSizer, BytesSizer: set.BytesSizer, - Capacity: cfg.QueueSize, + Capacity: int64(cfg.QueueSize), NumConsumers: cfg.NumConsumers, WaitForResult: cfg.WaitForResult, BlockOnOverflow: cfg.BlockOnOverflow, diff --git a/exporter/exporterhelper/internal/queuebatch/testdata/config.yaml b/exporter/exporterhelper/internal/queuebatch/testdata/config.yaml new file mode 100644 index 000000000000..ebec5826c61d --- /dev/null +++ b/exporter/exporterhelper/internal/queuebatch/testdata/config.yaml @@ -0,0 +1,4 @@ +queue_size: 1000 +enabled: true +num_consumers: 10 +storage: disk \ No newline at end of file From 3a7c71992f447a7a1fd85aaba64008a889ebacfd Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 29 Jul 2025 17:12:22 -0400 Subject: [PATCH 05/25] Fix up options --- confmap/confmap.go | 7 ++++++- confmap/internal/marshaloption.go | 2 +- confmap/xconfmap/scalarmarshaler.go | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index cc902b8bea13..8671db6851e2 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -310,7 +310,12 @@ func encoderConfig(rawVal any, set internal.MarshalOptions) *encoder.EncoderConf encoder.YamlMarshalerHookFunc(), encoder.TextMarshalerHookFunc(), } - hooks = append(hooks, set.AdditionalEncodeHookFuncs...) + + if set.ScalarMarshalingEncodeHookFunc != nil { + hooks = append(hooks, set.ScalarMarshalingEncodeHookFunc) + } + + // This hook must come after the scalar marshaling hook, if present. hooks = append(hooks, marshalerHookFunc(rawVal)) return &encoder.EncoderConfig{ diff --git a/confmap/internal/marshaloption.go b/confmap/internal/marshaloption.go index 0eb0a296070e..c29c7e20fb34 100644 --- a/confmap/internal/marshaloption.go +++ b/confmap/internal/marshaloption.go @@ -12,7 +12,7 @@ type MarshalOption interface { // MarshalOptions is used by (*Conf).Marshal to toggle unmarshaling settings. // It is in the `internal` package so experimental options can be added in xconfmap. type MarshalOptions struct { - AdditionalEncodeHookFuncs []mapstructure.DecodeHookFunc + ScalarMarshalingEncodeHookFunc mapstructure.DecodeHookFunc } type MarshalOptionFunc func(*MarshalOptions) diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index d39a5738b71c..804773d5b526 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -14,7 +14,7 @@ import ( func WithScalarMarshaler() confmap.MarshalOption { return internal.MarshalOptionFunc(func(mo *internal.MarshalOptions) { - mo.AdditionalEncodeHookFuncs = append(mo.AdditionalEncodeHookFuncs, scalarmarshalerHookFunc()) + mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc() }) } From 6a2a600489561c8deaed6e0c9ddd5d75579085de Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 30 Jul 2025 14:19:06 -0400 Subject: [PATCH 06/25] Make ScalarMarshaler interface more opinionated --- config/configoptional/optional.go | 17 +++++++---------- confmap/xconfmap/scalarmarshaler.go | 21 +++++++++++++++++++-- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index d25df9ee6331..862c587e7df2 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -4,7 +4,6 @@ package configoptional // import "go.opentelemetry.io/collector/config/configoptional" import ( - "encoding" "errors" "fmt" "reflect" @@ -218,16 +217,14 @@ func (o Optional[T]) Marshal(conf *confmap.Conf) error { } // MarshalScalar implements xconfmap.ScalarMarshaler. -func (o Optional[T]) MarshalScalar() (string, error) { - if tm, ok := any(o.value).(encoding.TextMarshaler); ok { - str, err := tm.MarshalText() - - if err != nil { - return "", err - } - - return string(str), nil +func (o Optional[T]) MarshalScalar(in *string) (string, error) { + if in != nil { + return *in, nil } return "", nil } + +func (o Optional[T]) GetScalarValue() any { + return o.value +} diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index 804773d5b526..db58a43a37c9 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -4,6 +4,7 @@ package xconfmap import ( + "encoding" "reflect" "github.com/go-viper/mapstructure/v2" @@ -24,7 +25,9 @@ type ScalarMarshaler interface { // Unmarshal a Conf into the struct in a custom way. // The Conf for this specific component may be nil or empty if no config available. // This method should only be called by decoding hooks when calling Conf.Unmarshal. - MarshalScalar() (string, error) + MarshalScalar(*string) (string, error) + + GetScalarValue() any } // Provides a mechanism for individual structs to define their own unmarshal logic, @@ -37,7 +40,21 @@ func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } - return marshaler.MarshalScalar() + var s *string + v := marshaler.GetScalarValue() + + if tm, ok := v.(encoding.TextMarshaler); ok { + b, err := tm.MarshalText() + + if err != nil { + return nil, err + } + + bs := string(b) + s = &bs + } + + return marshaler.MarshalScalar(s) }) } From 6571d6b01b65d3b023216f37782ae99f7938d29a Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 30 Jul 2025 21:58:19 -0400 Subject: [PATCH 07/25] Adjust interfaces --- config/configoptional/optional.go | 12 ++++++------ confmap/xconfmap/scalarmarshaler.go | 2 +- confmap/xconfmap/scalarunmarshaler.go | 18 +++++++++++++----- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index 862c587e7df2..ac4027fc799c 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -167,23 +167,23 @@ func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { return nil } -func (o *Optional[T]) UnmarshalScalar(val any) (any, error) { +func (o *Optional[T]) UnmarshalScalar(val any) error { if o.flavor == noneFlavor && val == nil { // If the Optional is None and the configuration is nil, we do nothing. // This replicates the behavior of unmarshaling into a field with a nil pointer. - return nil, nil + return nil } if val != nil { v, ok := val.(T) if !ok { - return nil, fmt.Errorf("val is %T, not %T", val, v) + return fmt.Errorf("val is %T, not %T", val, v) } o.value = v o.flavor = someFlavor } - return o.value, nil + return nil } func (o *Optional[T]) ScalarType() any { @@ -217,12 +217,12 @@ func (o Optional[T]) Marshal(conf *confmap.Conf) error { } // MarshalScalar implements xconfmap.ScalarMarshaler. -func (o Optional[T]) MarshalScalar(in *string) (string, error) { +func (o Optional[T]) MarshalScalar(in *string) (any, error) { if in != nil { return *in, nil } - return "", nil + return o.value, nil } func (o Optional[T]) GetScalarValue() any { diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index db58a43a37c9..9b7580a821ef 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -25,7 +25,7 @@ type ScalarMarshaler interface { // Unmarshal a Conf into the struct in a custom way. // The Conf for this specific component may be nil or empty if no config available. // This method should only be called by decoding hooks when calling Conf.Unmarshal. - MarshalScalar(*string) (string, error) + MarshalScalar(*string) (any, error) GetScalarValue() any } diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 1f0246f928aa..0158f6e8fb9c 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -25,7 +25,7 @@ type ScalarUnmarshaler interface { // Unmarshal a Conf into the struct in a custom way. // The Conf for this specific component may be nil or empty if no config available. // This method should only be called by decoding hooks when calling Conf.Unmarshal. - UnmarshalScalar(val any) (any, error) + UnmarshalScalar(val any) error ScalarType() any } @@ -46,10 +46,9 @@ func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } - v := from.Interface() + var v any tp := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) - t := tp.Interface() - if tu, ok := t.(encoding.TextUnmarshaler); ok { + if tu, ok := tp.Interface().(encoding.TextUnmarshaler); ok { // Should we error out here? if str, ok := from.Interface().(string); ok { if err := tu.UnmarshalText([]byte(str)); err != nil { @@ -57,9 +56,18 @@ func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { } v = tp.Elem().Interface() } + } else if from.CanConvert(tp.Elem().Type()) { + from.Convert(tp.Elem().Type()) + v = from.Interface() + } else { + v = tp.Elem().Interface() } - return unmarshaler.UnmarshalScalar(v) + if err := unmarshaler.UnmarshalScalar(v); err != nil { + return nil, err + } + + return unmarshaler, nil }) } From f1eae1aef6dae946965185ec329a0cf9a9b84c7f Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 30 Jul 2025 22:04:26 -0400 Subject: [PATCH 08/25] Questionable update to unmarshaling --- confmap/xconfmap/scalarunmarshaler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 0158f6e8fb9c..9bfa0971e159 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -59,8 +59,10 @@ func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { } else if from.CanConvert(tp.Elem().Type()) { from.Convert(tp.Elem().Type()) v = from.Interface() - } else { + } else if _, ok := from.Interface().(map[string]any); ok { v = tp.Elem().Interface() + } else { + v = from.Interface() } if err := unmarshaler.UnmarshalScalar(v); err != nil { From a9b8788a2404e50fca5cf4f2016bbfe5a3730f0f Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 30 Jul 2025 22:04:34 -0400 Subject: [PATCH 09/25] Add unit tests --- confmap/xconfmap/scalarmarshaler_test.go | 132 +++++++++++++++++++++ confmap/xconfmap/scalarunmarshaler_test.go | 33 ++++++ confmap/xconfmap/testdata/config.yaml | 10 ++ 3 files changed, 175 insertions(+) create mode 100644 confmap/xconfmap/scalarmarshaler_test.go create mode 100644 confmap/xconfmap/scalarunmarshaler_test.go create mode 100644 confmap/xconfmap/testdata/config.yaml diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go new file mode 100644 index 000000000000..b159c5b60e2e --- /dev/null +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -0,0 +1,132 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "bytes" + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" +) + +func (tms textMarshalerStruct) MarshalText() ([]byte, error) { + return tms.data, nil +} + +func (tms *textMarshalerStruct) UnmarshalText(data []byte) error { + tms.data = data + return nil +} + +type textMarshalerStruct struct { + id int + data []byte +} + +type nonTextMarshalerStruct struct { + id int + data []byte +} + +type textMarshalerAlias string + +func (tma textMarshalerAlias) MarshalText() ([]byte, error) { + return bytes.NewBufferString(string(tma)).Bytes(), nil +} + +func (tma *textMarshalerAlias) UnmarshalText(data []byte) error { + *tma = textMarshalerAlias(data) + return nil +} + +type nonTextMarshalerAlias string + +type NonImplWrapperType[T any] struct { + inner T `mapstructure:"-"` +} + +var _ ScalarMarshaler = wrapperType[any]{} +var _ ScalarUnmarshaler = (*wrapperType[any])(nil) + +type wrapperType[T any] struct { + inner T `mapstructure:"-"` +} + +func (wt wrapperType[T]) MarshalScalar(in *string) (any, error) { + if in != nil { + return *in, nil + } + + return wt.inner, nil +} + +func (wt wrapperType[T]) GetScalarValue() any { + return wt.inner +} + +func (wt *wrapperType[T]) UnmarshalScalar(val any) error { + v, ok := val.(T) + + if !ok { + return fmt.Errorf("val is %T, not %T", val, v) + } + + wt.inner = v + return nil +} + +func (wt *wrapperType[T]) ScalarType() any { + return wt.inner +} + +type testConfig struct { + // Handled by confmap, treated as string + Tma textMarshalerAlias `mapstructure:"tma"` + Ntma nonTextMarshalerAlias `mapstructure:"ntma"` + Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` + Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` + Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_tms"` + Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_ntms"` + Implint wrapperType[int] `mapstructure:"impl_int"` + Implstr wrapperType[string] `mapstructure:"impl_str"` + Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_tms"` + Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_ntms"` +} + +func (cfg *testConfig) Unmarshal(conf *confmap.Conf) error { + if err := conf.Unmarshal(cfg, WithScalarUnmarshaler()); err != nil { + return err + } + + return nil +} + +func TestMarshalConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + wantCfg := &testConfig{} + require.NoError(t, cm.Unmarshal(wantCfg, WithScalarUnmarshaler())) + require.NoError(t, cm.Marshal(wantCfg, WithScalarMarshaler())) + + conf := confmap.New() + cfg := &testConfig{ + Tma: textMarshalerAlias("test"), + Ntma: nonTextMarshalerAlias("test"), + Nonimplint: NonImplWrapperType[int]{inner: 1}, + Nonimplstr: NonImplWrapperType[string]{inner: "test"}, + Nonimpltms: NonImplWrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{47}}}, + Nonimplntms: NonImplWrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{48}}}, + Implint: wrapperType[int]{inner: 1}, + Implstr: wrapperType[string]{inner: "test"}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, + Implntms: wrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{80}}}, + } + + require.NoError(t, conf.Marshal(cfg, WithScalarMarshaler())) + require.EqualValues(t, cm.ToStringMap(), conf.ToStringMap()) +} diff --git a/confmap/xconfmap/scalarunmarshaler_test.go b/confmap/xconfmap/scalarunmarshaler_test.go new file mode 100644 index 000000000000..9380c84c9063 --- /dev/null +++ b/confmap/xconfmap/scalarunmarshaler_test.go @@ -0,0 +1,33 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" +) + +func TestUnmarshalConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + wantCfg := &testConfig{} + require.NoError(t, cm.Unmarshal(wantCfg, WithScalarUnmarshaler())) + require.NoError(t, cm.Marshal(wantCfg, WithScalarMarshaler())) + + conf := confmap.New() + cfg := &testConfig{ + Tma: textMarshalerAlias("test"), + Ntma: nonTextMarshalerAlias("test"), + Implint: wrapperType[int]{inner: 1}, + Implstr: wrapperType[string]{inner: "test"}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, + } + + require.NoError(t, conf.Unmarshal(cfg)) + require.Equal(t, wantCfg, cfg) +} diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml new file mode 100644 index 000000000000..068dc973dae0 --- /dev/null +++ b/confmap/xconfmap/testdata/config.yaml @@ -0,0 +1,10 @@ +impl_int: 1 +impl_ntms: {} +impl_str: test +impl_tms: P +non_impl_int: {} +non_impl_ntms: {} +non_impl_str: {} +non_impl_tms: {} +ntma: test +tma: test \ No newline at end of file From 579d40d92d9846197ddc29090521c667e6cf60e2 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 31 Jul 2025 16:02:09 -0400 Subject: [PATCH 10/25] Tweak interfaces --- config/configoptional/optional.go | 11 +---------- confmap/xconfmap/scalarmarshaler.go | 23 +++++++++++------------ confmap/xconfmap/scalarunmarshaler.go | 6 +++--- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index ac4027fc799c..111f1cb0a125 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -216,15 +216,6 @@ func (o Optional[T]) Marshal(conf *confmap.Conf) error { return nil } -// MarshalScalar implements xconfmap.ScalarMarshaler. -func (o Optional[T]) MarshalScalar(in *string) (any, error) { - if in != nil { - return *in, nil - } - +func (o Optional[T]) GetScalarValue() (any, error) { return o.value, nil } - -func (o Optional[T]) GetScalarValue() any { - return o.value -} diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index 9b7580a821ef..bbf090a203b7 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -22,12 +22,10 @@ func WithScalarMarshaler() confmap.MarshalOption { // ScalarUnmarshaler is an interface which may be implemented by wrapper types // to customize their behavior when the type under the wrapper is a scalar value. type ScalarMarshaler interface { - // Unmarshal a Conf into the struct in a custom way. - // The Conf for this specific component may be nil or empty if no config available. - // This method should only be called by decoding hooks when calling Conf.Unmarshal. - MarshalScalar(*string) (any, error) - - GetScalarValue() any + // GetScalarValue gets the scalar value and marshals it. + // The struct implementing the interface is free to + // do any pre-processing to the value as part of marshaling. + GetScalarValue() (any, error) } // Provides a mechanism for individual structs to define their own unmarshal logic, @@ -40,8 +38,11 @@ func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } - var s *string - v := marshaler.GetScalarValue() + v, err := marshaler.GetScalarValue() + + if err != nil { + return nil, err + } if tm, ok := v.(encoding.TextMarshaler); ok { b, err := tm.MarshalText() @@ -50,11 +51,9 @@ func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return nil, err } - bs := string(b) - s = &bs + return string(b), nil } - return marshaler.MarshalScalar(s) - + return v, nil }) } diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 9bfa0971e159..6b1e29aced5c 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -22,11 +22,11 @@ func WithScalarUnmarshaler() confmap.UnmarshalOption { // ScalarUnmarshaler is an interface which may be implemented by wrapper types // to customize their behavior when the type under the wrapper is a scalar value. type ScalarUnmarshaler interface { - // Unmarshal a Conf into the struct in a custom way. - // The Conf for this specific component may be nil or empty if no config available. - // This method should only be called by decoding hooks when calling Conf.Unmarshal. + //UnmarshalScalar unmarshals a scalar into a value in a custom way. UnmarshalScalar(val any) error + // ScalarType returns a value that can be used to get the type + // of the scalar using reflection. ScalarType() any } From 3749c2fe8a4c1b071d54a8e22f4403e0e747899c Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Thu, 31 Jul 2025 16:02:21 -0400 Subject: [PATCH 11/25] Preapre for possible additional decoding --- confmap/confmap.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index 8671db6851e2..0aaccec62e74 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -91,7 +91,7 @@ func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { for _, opt := range opts { internal.ApplyUnmarshalOption(opt, &set) } - return decodeConfig(l, result, set, l.skipTopLevelUnmarshaler) + return decode(l.toStringMapWithExpand(), result, set, l.skipTopLevelUnmarshaler) } type MarshalOption interface { @@ -257,14 +257,16 @@ func (l *Conf) ToStringMap() map[string]any { return sanitize(l.toStringMapWithExpand()).(map[string]any) } -// decodeConfig decodes the contents of the Conf into the result argument, using a +// decode decodes the contents of the Conf into the result argument, using a // mapstructure decoder with the following notable behaviors. Ensures that maps whose // values are nil pointer structs resolved to the zero value of the target struct (see // expandNilStructPointers). Converts string to []string by splitting on ','. Ensures // uniqueness of component IDs (see mapKeyStringToMapKeyTextUnmarshalerHookFunc). // Decodes time.Duration from strings. Allows custom unmarshaling for structs implementing // encoding.TextUnmarshaler. Allows custom unmarshaling for structs implementing confmap.Unmarshaler. -func decodeConfig(m *Conf, result any, settings internal.UnmarshalOptions, skipTopLevelUnmarshaler bool) error { +// +// TODO Move this to `internal`. +func decode(input any, result any, settings internal.UnmarshalOptions, skipTopLevelUnmarshaler bool) error { hooks := []mapstructure.DecodeHookFunc{ useExpandValue(), expandNilStructPointersHookFunc(), @@ -293,7 +295,7 @@ func decodeConfig(m *Conf, result any, settings internal.UnmarshalOptions, skipT if err != nil { return err } - if err = decoder.Decode(m.toStringMapWithExpand()); err != nil { + if err = decoder.Decode(input); err != nil { if strings.HasPrefix(err.Error(), "error decoding ''") { return errors.Unwrap(err) } From a85c4cbbf4f7d61253e20bb25582cb5e9be8f7a5 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 4 Aug 2025 10:48:34 -0400 Subject: [PATCH 12/25] Use confmap facilities for marshaling and unmarshaling --- confmap/xconfmap/scalarmarshaler.go | 12 ------ confmap/xconfmap/scalarmarshaler_test.go | 45 +++++++++++++++------- confmap/xconfmap/scalarunmarshaler.go | 35 +++++++---------- confmap/xconfmap/scalarunmarshaler_test.go | 25 ++++++------ confmap/xconfmap/testdata/config.yaml | 3 +- 5 files changed, 59 insertions(+), 61 deletions(-) diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index bbf090a203b7..05f68ee5c5c8 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -4,7 +4,6 @@ package xconfmap import ( - "encoding" "reflect" "github.com/go-viper/mapstructure/v2" @@ -39,21 +38,10 @@ func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { } v, err := marshaler.GetScalarValue() - if err != nil { return nil, err } - if tm, ok := v.(encoding.TextMarshaler); ok { - b, err := tm.MarshalText() - - if err != nil { - return nil, err - } - - return string(b), nil - } - return v, nil }) } diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go index b159c5b60e2e..4a64a05646c8 100644 --- a/confmap/xconfmap/scalarmarshaler_test.go +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -50,6 +50,7 @@ type NonImplWrapperType[T any] struct { inner T `mapstructure:"-"` } +var _ confmap.Unmarshaler = (*wrapperType[any])(nil) var _ ScalarMarshaler = wrapperType[any]{} var _ ScalarUnmarshaler = (*wrapperType[any])(nil) @@ -57,6 +58,22 @@ type wrapperType[T any] struct { inner T `mapstructure:"-"` } +func (wt *wrapperType[T]) Unmarshal(conf *confmap.Conf) error { + if err := conf.Unmarshal(&wt.inner, WithScalarUnmarshaler()); err != nil { + return err + } + + return nil +} + +func (wt wrapperType[T]) Marshal(conf *confmap.Conf) error { + if err := conf.Marshal(wt.inner, WithScalarMarshaler()); err != nil { + return fmt.Errorf("failed to marshal wrapperType value: %w", err) + } + + return nil +} + func (wt wrapperType[T]) MarshalScalar(in *string) (any, error) { if in != nil { return *in, nil @@ -65,15 +82,15 @@ func (wt wrapperType[T]) MarshalScalar(in *string) (any, error) { return wt.inner, nil } -func (wt wrapperType[T]) GetScalarValue() any { - return wt.inner +func (wt wrapperType[T]) GetScalarValue() (any, error) { + return wt.inner, nil } func (wt *wrapperType[T]) UnmarshalScalar(val any) error { v, ok := val.(T) if !ok { - return fmt.Errorf("val is %T, not %T", val, v) + return fmt.Errorf("could not unmarshal scalar: val is %T, not %T", val, v) } wt.inner = v @@ -86,16 +103,17 @@ func (wt *wrapperType[T]) ScalarType() any { type testConfig struct { // Handled by confmap, treated as string - Tma textMarshalerAlias `mapstructure:"tma"` - Ntma nonTextMarshalerAlias `mapstructure:"ntma"` - Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` - Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` - Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_tms"` - Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_ntms"` - Implint wrapperType[int] `mapstructure:"impl_int"` - Implstr wrapperType[string] `mapstructure:"impl_str"` - Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_tms"` - Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_ntms"` + Tma textMarshalerAlias `mapstructure:"tma"` + Ntma nonTextMarshalerAlias `mapstructure:"ntma"` + Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` + Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` + Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_tms"` + Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_ntms"` + Implint wrapperType[int] `mapstructure:"impl_int"` + Implstr wrapperType[string] `mapstructure:"impl_str"` + Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_tms"` + Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_ntms"` + Recursive wrapperType[wrapperType[textMarshalerStruct]] `mapstructure:"recursive"` } func (cfg *testConfig) Unmarshal(conf *confmap.Conf) error { @@ -125,6 +143,7 @@ func TestMarshalConfig(t *testing.T) { Implstr: wrapperType[string]{inner: "test"}, Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, Implntms: wrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{80}}}, + Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 2, data: []byte{80}}}}, } require.NoError(t, conf.Marshal(cfg, WithScalarMarshaler())) diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 6b1e29aced5c..7777fa47492d 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -4,7 +4,6 @@ package xconfmap import ( - "encoding" "reflect" "github.com/go-viper/mapstructure/v2" @@ -15,7 +14,7 @@ import ( func WithScalarUnmarshaler() confmap.UnmarshalOption { return internal.UnmarshalOptionFunc(func(uo *internal.UnmarshalOptions) { - uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc()) + uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(*uo)) }) } @@ -33,12 +32,19 @@ type ScalarUnmarshaler interface { // 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. -func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { +func scalarunmarshalerHookFunc(opts internal.UnmarshalOptions) mapstructure.DecodeHookFuncValue { return safeWrapDecodeHookFunc(func(from reflect.Value, to reflect.Value) (any, error) { + opts.AdditionalDecodeHookFuncs = append(opts.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(opts)) + if !to.CanAddr() { return from.Interface(), nil } + if from.Kind() == reflect.Struct || + from.Kind() == reflect.Pointer && from.Elem().Kind() == reflect.Struct { + return from.Interface(), nil + } + toPtr := to.Addr().Interface() unmarshaler, ok := toPtr.(ScalarUnmarshaler) @@ -46,26 +52,13 @@ func scalarunmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return from.Interface(), nil } - var v any - tp := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) - if tu, ok := tp.Interface().(encoding.TextUnmarshaler); ok { - // Should we error out here? - if str, ok := from.Interface().(string); ok { - if err := tu.UnmarshalText([]byte(str)); err != nil { - return nil, err - } - v = tp.Elem().Interface() - } - } else if from.CanConvert(tp.Elem().Type()) { - from.Convert(tp.Elem().Type()) - v = from.Interface() - } else if _, ok := from.Interface().(map[string]any); ok { - v = tp.Elem().Interface() - } else { - v = from.Interface() + resultVal := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) + + if err := internal.Decode(from.Interface(), resultVal.Interface(), opts, false); err != nil { + return nil, err } - if err := unmarshaler.UnmarshalScalar(v); err != nil { + if err := unmarshaler.UnmarshalScalar(resultVal.Elem().Interface()); err != nil { return nil, err } diff --git a/confmap/xconfmap/scalarunmarshaler_test.go b/confmap/xconfmap/scalarunmarshaler_test.go index 9380c84c9063..152d324dfb3a 100644 --- a/confmap/xconfmap/scalarunmarshaler_test.go +++ b/confmap/xconfmap/scalarunmarshaler_test.go @@ -8,26 +8,23 @@ import ( "testing" "github.com/stretchr/testify/require" - "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" ) func TestUnmarshalConfig(t *testing.T) { + wantCfg := &testConfig{ + Tma: textMarshalerAlias("test"), + Ntma: nonTextMarshalerAlias("test"), + Implint: wrapperType[int]{inner: 1}, + Implstr: wrapperType[string]{inner: "test"}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, + Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}}, + } + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) require.NoError(t, err) - wantCfg := &testConfig{} - require.NoError(t, cm.Unmarshal(wantCfg, WithScalarUnmarshaler())) - require.NoError(t, cm.Marshal(wantCfg, WithScalarMarshaler())) - - conf := confmap.New() - cfg := &testConfig{ - Tma: textMarshalerAlias("test"), - Ntma: nonTextMarshalerAlias("test"), - Implint: wrapperType[int]{inner: 1}, - Implstr: wrapperType[string]{inner: "test"}, - Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, - } + cfg := &testConfig{} + require.NoError(t, cm.Unmarshal(cfg, WithScalarUnmarshaler())) - require.NoError(t, conf.Unmarshal(cfg)) require.Equal(t, wantCfg, cfg) } diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml index 068dc973dae0..b02eaf27d5f3 100644 --- a/confmap/xconfmap/testdata/config.yaml +++ b/confmap/xconfmap/testdata/config.yaml @@ -7,4 +7,5 @@ non_impl_ntms: {} non_impl_str: {} non_impl_tms: {} ntma: test -tma: test \ No newline at end of file +tma: test +recursive: P \ No newline at end of file From ff828f7cf2c5a63aa18be7f003f6d84ddbe65dd7 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 14:52:42 -0400 Subject: [PATCH 13/25] Fix merge --- confmap/internal/decoder.go | 29 ++++++++++--------- confmap/internal/encoder.go | 20 +++++++++---- confmap/internal/unmarshaloption.go | 10 ------- .../internal/queuebatch/config_test.go | 7 ++--- .../internal/queuebatch/queue_batch.go | 2 +- 5 files changed, 33 insertions(+), 35 deletions(-) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 17f172a2ec34..489d0ab8dd32 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -40,6 +40,20 @@ func WithIgnoreUnused() UnmarshalOption { // Decodes time.Duration from strings. Allows custom unmarshaling for structs implementing // encoding.TextUnmarshaler. Allows custom unmarshaling for structs implementing confmap.Unmarshaler. func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshaler bool) error { + hooks := []mapstructure.DecodeHookFunc{ + useExpandValue(), + expandNilStructPointersHookFunc(), + mapstructure.StringToSliceHookFunc(","), + mapKeyStringToMapKeyTextUnmarshalerHookFunc(), + mapstructure.StringToTimeDurationHookFunc(), + mapstructure.TextUnmarshallerHookFunc(), + unmarshalerHookFunc(result, skipTopLevelUnmarshaler), + // after the main unmarshaler hook is called, + // we unmarshal the embedded structs if present to merge with the result: + unmarshalerEmbeddedStructsHookFunc(), + zeroSliceAndMapHookFunc(), + } + hooks = append(hooks, settings.AdditionalDecodeHookFuncs...) dc := &mapstructure.DecoderConfig{ ErrorUnused: !settings.IgnoreUnused, Result: result, @@ -47,20 +61,9 @@ func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshale WeaklyTypedInput: false, MatchName: caseSensitiveMatchName, DecodeNil: true, - DecodeHook: composehook.ComposeDecodeHookFunc( - useExpandValue(), - expandNilStructPointersHookFunc(), - mapstructure.StringToSliceHookFunc(","), - mapKeyStringToMapKeyTextUnmarshalerHookFunc(), - mapstructure.StringToTimeDurationHookFunc(), - mapstructure.TextUnmarshallerHookFunc(), - unmarshalerHookFunc(result, skipTopLevelUnmarshaler), - // after the main unmarshaler hook is called, - // we unmarshal the embedded structs if present to merge with the result: - unmarshalerEmbeddedStructsHookFunc(), - zeroSliceAndMapHookFunc(), - ), + DecodeHook: composehook.ComposeDecodeHookFunc(hooks), } + decoder, err := mapstructure.NewDecoder(dc) if err != nil { return err diff --git a/confmap/internal/encoder.go b/confmap/internal/encoder.go index 6df9675b41a5..08a2f86b3014 100644 --- a/confmap/internal/encoder.go +++ b/confmap/internal/encoder.go @@ -14,13 +14,21 @@ import ( // EncoderConfig returns a default encoder.EncoderConfig that includes // an EncodeHook that handles both TextMarshaler and Marshaler // interfaces. -func EncoderConfig(rawVal any, _ MarshalOptions) *encoder.EncoderConfig { +func EncoderConfig(rawVal any, set MarshalOptions) *encoder.EncoderConfig { + hooks := []mapstructure.DecodeHookFunc{ + encoder.YamlMarshalerHookFunc(), + encoder.TextMarshalerHookFunc(), + } + + if set.ScalarMarshalingEncodeHookFunc != nil { + hooks = append(hooks, set.ScalarMarshalingEncodeHookFunc) + } + + // This hook must come after the scalar marshaling hook, if present. + hooks = append(hooks, marshalerHookFunc(rawVal)) + return &encoder.EncoderConfig{ - EncodeHook: mapstructure.ComposeDecodeHookFunc( - encoder.YamlMarshalerHookFunc(), - encoder.TextMarshalerHookFunc(), - marshalerHookFunc(rawVal), - ), + EncodeHook: mapstructure.ComposeDecodeHookFunc(hooks...), } } diff --git a/confmap/internal/unmarshaloption.go b/confmap/internal/unmarshaloption.go index 1daed4d915e4..19f5109b2033 100644 --- a/confmap/internal/unmarshaloption.go +++ b/confmap/internal/unmarshaloption.go @@ -3,11 +3,8 @@ package internal // import "go.opentelemetry.io/collector/confmap/internal" -<<<<<<< HEAD import "github.com/go-viper/mapstructure/v2" -======= ->>>>>>> upstream/main type UnmarshalOption interface { apply(*UnmarshalOptions) } @@ -15,12 +12,8 @@ type UnmarshalOption interface { // UnmarshalOptions is used by (*Conf).Unmarshal to toggle unmarshaling settings. // It is in the `internal` package so experimental options can be added in xconfmap. type UnmarshalOptions struct { -<<<<<<< HEAD IgnoreUnused bool AdditionalDecodeHookFuncs []mapstructure.DecodeHookFunc -======= - IgnoreUnused bool ->>>>>>> upstream/main } type UnmarshalOptionFunc func(*UnmarshalOptions) @@ -28,7 +21,6 @@ type UnmarshalOptionFunc func(*UnmarshalOptions) func (fn UnmarshalOptionFunc) apply(set *UnmarshalOptions) { fn(set) } -<<<<<<< HEAD // Apply Option simply calls (UnmarshalOption).apply. This function allows us // to keep the `apply“ function private and therefore keep `confmap.UnmarshalOption` @@ -36,5 +28,3 @@ func (fn UnmarshalOptionFunc) apply(set *UnmarshalOptions) { func ApplyUnmarshalOption(uo UnmarshalOption, set *UnmarshalOptions) { uo.apply(set) } -======= ->>>>>>> upstream/main diff --git a/exporter/exporterhelper/internal/queuebatch/config_test.go b/exporter/exporterhelper/internal/queuebatch/config_test.go index 2a33c0072cc3..83aef849e355 100644 --- a/exporter/exporterhelper/internal/queuebatch/config_test.go +++ b/exporter/exporterhelper/internal/queuebatch/config_test.go @@ -13,10 +13,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configoptional" -<<<<<<< HEAD "go.opentelemetry.io/collector/confmap" -======= ->>>>>>> upstream/main "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/xconfmap" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" @@ -39,11 +36,11 @@ func TestConfig_Validate(t *testing.T) { cfg = newTestConfig() cfg.WaitForResult = true - cfg.StorageID = configoptional.Some(StorageID) + cfg.StorageID = configoptional.Some(component.MustNewID("test")) require.EqualError(t, xconfmap.Validate(cfg), "`wait_for_result` is not supported with a persistent queue configured with `storage`") cfg = newTestConfig() - cfg.QueueSize = cfg.Batch.Get().MinSize - 1 + cfg.QueueSize = int(cfg.Batch.Get().MinSize) - 1 require.EqualError(t, xconfmap.Validate(cfg), "`min_size` must be less than or equal to `queue_size`") cfg = newTestConfig() diff --git a/exporter/exporterhelper/internal/queuebatch/queue_batch.go b/exporter/exporterhelper/internal/queuebatch/queue_batch.go index b9e67f9c6fe0..0e6ef211b655 100644 --- a/exporter/exporterhelper/internal/queuebatch/queue_batch.go +++ b/exporter/exporterhelper/internal/queuebatch/queue_batch.go @@ -64,7 +64,7 @@ func NewQueueBatch( SizerType: cfg.Sizer, ItemsSizer: set.ItemsSizer, BytesSizer: set.BytesSizer, - Capacity: cfg.QueueSize, + Capacity: int64(cfg.QueueSize), NumConsumers: cfg.NumConsumers, WaitForResult: cfg.WaitForResult, BlockOnOverflow: cfg.BlockOnOverflow, From ba9487c22adf9232c764fd95a59e512a1176cadd Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:21:09 -0400 Subject: [PATCH 14/25] Other fixes --- confmap/internal/conf.go | 5 +---- confmap/internal/decoder.go | 2 +- confmap/internal/encoder.go | 9 +++++++++ confmap/xconfmap/scalarmarshaler.go | 6 +++--- confmap/xconfmap/scalarunmarshaler.go | 4 ++-- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/confmap/internal/conf.go b/confmap/internal/conf.go index 1760cdd425cb..8ac0beefc39e 100644 --- a/confmap/internal/conf.go +++ b/confmap/internal/conf.go @@ -11,8 +11,6 @@ import ( "github.com/knadh/koanf/maps" "github.com/knadh/koanf/providers/confmap" "github.com/knadh/koanf/v2" - - encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" ) const ( @@ -67,8 +65,7 @@ func (l *Conf) Marshal(rawVal any, opts ...MarshalOption) error { for _, opt := range opts { opt.apply(&set) } - enc := encoder.New(EncoderConfig(rawVal, set)) - data, err := enc.Encode(rawVal) + data, err := Encode(rawVal, set) if err != nil { return err } diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 489d0ab8dd32..d1e6c046a98d 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -61,7 +61,7 @@ func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshale WeaklyTypedInput: false, MatchName: caseSensitiveMatchName, DecodeNil: true, - DecodeHook: composehook.ComposeDecodeHookFunc(hooks), + DecodeHook: composehook.ComposeDecodeHookFunc(hooks...), } decoder, err := mapstructure.NewDecoder(dc) diff --git a/confmap/internal/encoder.go b/confmap/internal/encoder.go index 08a2f86b3014..8e71d19fc534 100644 --- a/confmap/internal/encoder.go +++ b/confmap/internal/encoder.go @@ -11,6 +11,15 @@ import ( encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" ) +func Encode(rawVal any, set MarshalOptions) (any, error) { + enc := encoder.New(EncoderConfig(rawVal, set)) + data, err := enc.Encode(rawVal) + if err != nil { + return nil, err + } + return data, nil +} + // EncoderConfig returns a default encoder.EncoderConfig that includes // an EncodeHook that handles both TextMarshaler and Marshaler // interfaces. diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index 05f68ee5c5c8..ee32c05f70f7 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -14,7 +14,7 @@ import ( func WithScalarMarshaler() confmap.MarshalOption { return internal.MarshalOptionFunc(func(mo *internal.MarshalOptions) { - mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc() + mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc(*mo) }) } @@ -30,7 +30,7 @@ type ScalarMarshaler interface { // 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. -func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { +func scalarmarshalerHookFunc(mo internal.MarshalOptions) mapstructure.DecodeHookFuncValue { return safeWrapDecodeHookFunc(func(from reflect.Value, _ reflect.Value) (any, error) { marshaler, ok := from.Interface().(ScalarMarshaler) if !ok { @@ -42,6 +42,6 @@ func scalarmarshalerHookFunc() mapstructure.DecodeHookFuncValue { return nil, err } - return v, nil + return internal.Encode(v, mo) }) } diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 7777fa47492d..bcde44ed027e 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -33,7 +33,7 @@ type ScalarUnmarshaler interface { // by implementing the Unmarshaler interface, unless skipTopLevelUnmarshaler is // true and the struct matches the top level object being unmarshaled. func scalarunmarshalerHookFunc(opts internal.UnmarshalOptions) mapstructure.DecodeHookFuncValue { - return safeWrapDecodeHookFunc(func(from reflect.Value, to reflect.Value) (any, error) { + return safeWrapDecodeHookFunc(func(from, to reflect.Value) (any, error) { opts.AdditionalDecodeHookFuncs = append(opts.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(opts)) if !to.CanAddr() { @@ -75,7 +75,7 @@ func scalarunmarshalerHookFunc(opts internal.UnmarshalOptions) mapstructure.Deco func safeWrapDecodeHookFunc( f mapstructure.DecodeHookFuncValue, ) mapstructure.DecodeHookFuncValue { - return func(fromVal reflect.Value, toVal reflect.Value) (any, error) { + return func(fromVal, toVal reflect.Value) (any, error) { if !fromVal.IsValid() { return nil, nil } From 4bebc321ac3aef1dcdf112feb58722fc90a1459d Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:24:47 -0400 Subject: [PATCH 15/25] Fix spelling --- confmap/xconfmap/scalarmarshaler_test.go | 6 +++--- confmap/xconfmap/testdata/config.yaml | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go index 4a64a05646c8..8b36563dca00 100644 --- a/confmap/xconfmap/scalarmarshaler_test.go +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -107,12 +107,12 @@ type testConfig struct { Ntma nonTextMarshalerAlias `mapstructure:"ntma"` Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` - Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_tms"` - Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_ntms"` + Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_text_marshaler_struct"` + Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_non_text_marshaler_struct"` Implint wrapperType[int] `mapstructure:"impl_int"` Implstr wrapperType[string] `mapstructure:"impl_str"` Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_tms"` - Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_ntms"` + Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_non_text_marshaler_struct"` Recursive wrapperType[wrapperType[textMarshalerStruct]] `mapstructure:"recursive"` } diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml index b02eaf27d5f3..89585619718e 100644 --- a/confmap/xconfmap/testdata/config.yaml +++ b/confmap/xconfmap/testdata/config.yaml @@ -1,11 +1,11 @@ impl_int: 1 -impl_ntms: {} +impl_non_text_marshaler_struct: {} impl_str: test impl_tms: P non_impl_int: {} -non_impl_ntms: {} +non_impl_non_text_marshaler_struct: {} non_impl_str: {} -non_impl_tms: {} +non_impl_text_marshaler_struct: {} ntma: test tma: test recursive: P \ No newline at end of file From cd3b1e0299366628b96c5f6f482cf1571a7bd113 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:37:11 -0400 Subject: [PATCH 16/25] Add changelog --- .chloggen/configoptional-scalars-2.yaml | 25 +++++++++++++++++++++++++ .chloggen/configoptional-scalars.yaml | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 .chloggen/configoptional-scalars-2.yaml create mode 100644 .chloggen/configoptional-scalars.yaml diff --git a/.chloggen/configoptional-scalars-2.yaml b/.chloggen/configoptional-scalars-2.yaml new file mode 100644 index 000000000000..80e01bd9f7be --- /dev/null +++ b/.chloggen/configoptional-scalars-2.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: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: pkg/config/configoptional + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add support for marshaling and unmarshaling scalar values + +# One or more tracking issues or pull requests related to the change +issues: [13421] + +# (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: This allows using fields like `Optional[int]` + +# 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: [api] diff --git a/.chloggen/configoptional-scalars.yaml b/.chloggen/configoptional-scalars.yaml new file mode 100644 index 000000000000..15e471f4a826 --- /dev/null +++ b/.chloggen/configoptional-scalars.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: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: pkg/confmap/xconfmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add `WithScalarMarshaler` and `WithScalarUnmarshaler` options to support handling scalar values in confmap + +# One or more tracking issues or pull requests related to the change +issues: [13421] + +# (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: [api] From 3f0fbe4904ec6521e1acd5977d722f72bf9f610e Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:39:50 -0400 Subject: [PATCH 17/25] Fix more checks --- config/configoptional/optional.go | 12 ++++++++---- confmap/xconfmap/scalarmarshaler.go | 2 +- confmap/xconfmap/scalarunmarshaler.go | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index 03d39ec7f896..aff5da070242 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -163,8 +163,10 @@ func (o *Optional[T]) GetOrInsertDefault() *T { return o.Get() } -var _ confmap.Unmarshaler = (*Optional[any])(nil) -var _ xconfmap.ScalarUnmarshaler = (*Optional[any])(nil) +var ( + _ confmap.Unmarshaler = (*Optional[any])(nil) + _ xconfmap.ScalarUnmarshaler = (*Optional[any])(nil) +) // Unmarshal the configuration into the Optional value. // @@ -218,8 +220,10 @@ func (o *Optional[T]) ScalarType() any { return o.value } -var _ confmap.Marshaler = (*Optional[any])(nil) -var _ xconfmap.ScalarMarshaler = (*Optional[any])(nil) +var ( + _ confmap.Marshaler = (*Optional[any])(nil) + _ xconfmap.ScalarMarshaler = (*Optional[any])(nil) +) // Marshal the Optional value into the configuration. // If the Optional is None or Default, it does not marshal anything. diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index ee32c05f70f7..3b0ef5ac6e5c 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -1,7 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -package xconfmap +package xconfmap // import "go.opentelemetry.io/collector/confmap/xconfmap" import ( "reflect" diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index bcde44ed027e..5095d3a5a5ea 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -1,7 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -package xconfmap +package xconfmap // import "go.opentelemetry.io/collector/confmap/xconfmap" import ( "reflect" From f77968453efe0f52b7d327a9ca501fa2659c6be5 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:42:10 -0400 Subject: [PATCH 18/25] More spelling --- confmap/xconfmap/scalarmarshaler_test.go | 6 +++--- confmap/xconfmap/testdata/config.yaml | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go index 8b36563dca00..059e084a7a4d 100644 --- a/confmap/xconfmap/scalarmarshaler_test.go +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -103,15 +103,15 @@ func (wt *wrapperType[T]) ScalarType() any { type testConfig struct { // Handled by confmap, treated as string - Tma textMarshalerAlias `mapstructure:"tma"` - Ntma nonTextMarshalerAlias `mapstructure:"ntma"` + Tma textMarshalerAlias `mapstructure:"text_marshaler_alias"` + Ntma nonTextMarshalerAlias `mapstructure:"non_text_marshaler_alias"` Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_text_marshaler_struct"` Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_non_text_marshaler_struct"` Implint wrapperType[int] `mapstructure:"impl_int"` Implstr wrapperType[string] `mapstructure:"impl_str"` - Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_tms"` + Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_text_marshaler_struct"` Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_non_text_marshaler_struct"` Recursive wrapperType[wrapperType[textMarshalerStruct]] `mapstructure:"recursive"` } diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml index 89585619718e..8989fa889d94 100644 --- a/confmap/xconfmap/testdata/config.yaml +++ b/confmap/xconfmap/testdata/config.yaml @@ -1,11 +1,11 @@ impl_int: 1 impl_non_text_marshaler_struct: {} impl_str: test -impl_tms: P +impl_text_marshaler_struct: P non_impl_int: {} non_impl_non_text_marshaler_struct: {} non_impl_str: {} non_impl_text_marshaler_struct: {} -ntma: test -tma: test +non_text_marshaler_alias: test +text_marshaler_alias: test recursive: P \ No newline at end of file From 435b7a9df5213dfd6b4605e9eb01fc51a61a2d10 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 15:49:07 -0400 Subject: [PATCH 19/25] Fix changelog components --- .chloggen/configoptional-scalars-2.yaml | 2 +- .chloggen/configoptional-scalars.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.chloggen/configoptional-scalars-2.yaml b/.chloggen/configoptional-scalars-2.yaml index 80e01bd9f7be..66f9770f3959 100644 --- a/.chloggen/configoptional-scalars-2.yaml +++ b/.chloggen/configoptional-scalars-2.yaml @@ -4,7 +4,7 @@ change_type: enhancement # The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) -component: pkg/config/configoptional +component: all # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). note: Add support for marshaling and unmarshaling scalar values diff --git a/.chloggen/configoptional-scalars.yaml b/.chloggen/configoptional-scalars.yaml index 15e471f4a826..4de7d609d861 100644 --- a/.chloggen/configoptional-scalars.yaml +++ b/.chloggen/configoptional-scalars.yaml @@ -4,7 +4,7 @@ change_type: enhancement # The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) -component: pkg/confmap/xconfmap +component: pkg/confmap # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). note: Add `WithScalarMarshaler` and `WithScalarUnmarshaler` options to support handling scalar values in confmap From b9f88072d20c60aaf6e530a66c29c688daa1f631 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 16:01:04 -0400 Subject: [PATCH 20/25] Fix lint --- confmap/internal/marshaloption.go | 2 +- confmap/internal/unmarshaloption.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/confmap/internal/marshaloption.go b/confmap/internal/marshaloption.go index c29c7e20fb34..482426c3c4dd 100644 --- a/confmap/internal/marshaloption.go +++ b/confmap/internal/marshaloption.go @@ -21,7 +21,7 @@ func (fn MarshalOptionFunc) apply(set *MarshalOptions) { fn(set) } -// Apply Option simply calls (MarshalOption).apply. This function allows us +// ApplyMarshalOption simply calls (MarshalOption).apply. This function allows us // to keep the `apply“ function private and therefore keep `confmap.MarshalOption` // without any exported methods. func ApplyMarshalOption(mo MarshalOption, set *MarshalOptions) { diff --git a/confmap/internal/unmarshaloption.go b/confmap/internal/unmarshaloption.go index 19f5109b2033..0240bf628393 100644 --- a/confmap/internal/unmarshaloption.go +++ b/confmap/internal/unmarshaloption.go @@ -22,7 +22,7 @@ func (fn UnmarshalOptionFunc) apply(set *UnmarshalOptions) { fn(set) } -// Apply Option simply calls (UnmarshalOption).apply. This function allows us +// ApplyUnmarshalOption simply calls (UnmarshalOption).apply. This function allows us // to keep the `apply“ function private and therefore keep `confmap.UnmarshalOption` // without any exported methods. func ApplyUnmarshalOption(uo UnmarshalOption, set *UnmarshalOptions) { From 53ba6283c8b6d39a2b03333f46598228e2621a82 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 16:05:44 -0400 Subject: [PATCH 21/25] Fix lint --- confmap/xconfmap/scalarmarshaler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go index 059e084a7a4d..b4b1d84734b1 100644 --- a/confmap/xconfmap/scalarmarshaler_test.go +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" ) From 7f0f48b4a28cc07bda1fe32b8e365a053f7e8544 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Tue, 14 Oct 2025 16:20:49 -0400 Subject: [PATCH 22/25] Fix lint --- confmap/xconfmap/scalarunmarshaler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/confmap/xconfmap/scalarunmarshaler_test.go b/confmap/xconfmap/scalarunmarshaler_test.go index 152d324dfb3a..f2d8dbbdcca7 100644 --- a/confmap/xconfmap/scalarunmarshaler_test.go +++ b/confmap/xconfmap/scalarunmarshaler_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/confmap/confmaptest" ) From d279cdc95675d0294264a50de6cbe91bef2bb839 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 15 Oct 2025 11:50:46 -0400 Subject: [PATCH 23/25] UnmarshalV2 prototype --- config/configoptional/optional.go | 4 ++++ config/configoptional/optional_test.go | 20 ++++++++++---------- confmap/internal/decoder.go | 6 ++++-- confmap/xconfmap/scalarmarshaler.go | 15 ++++++++++++--- confmap/xconfmap/scalarmarshaler_test.go | 2 +- confmap/xconfmap/scalarunmarshaler.go | 20 +++++++++++--------- confmap/xconfmap/scalarunmarshaler_test.go | 2 +- confmap/xconfmap/testdata/config.yaml | 2 +- 8 files changed, 44 insertions(+), 27 deletions(-) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index aff5da070242..bacf08fad1d8 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -249,6 +249,10 @@ func (o Optional[T]) Marshal(conf *confmap.Conf) error { } func (o Optional[T]) GetScalarValue() (any, error) { + if o.flavor == noneFlavor || o.flavor == defaultFlavor { + return nil, nil + } + return o.value, nil } diff --git a/config/configoptional/optional_test.go b/config/configoptional/optional_test.go index 619576ea9be5..cfdb7269925a 100644 --- a/config/configoptional/optional_test.go +++ b/config/configoptional/optional_test.go @@ -368,7 +368,7 @@ func TestUnmarshalOptional(t *testing.T) { t.Run(test.name, func(t *testing.T) { cfg := test.defaultCfg conf := confmap.NewFromStringMap(test.config) - require.NoError(t, conf.Unmarshal(&cfg)) + require.NoError(t, conf.Unmarshal(&cfg), xconfmap.WithScalarUnmarshaler()) require.Equal(t, test.expectedSub, cfg.Sub1.HasValue()) if test.expectedSub { require.Equal(t, test.expectedFoo, cfg.Sub1.Get().Foo) @@ -383,7 +383,7 @@ func TestUnmarshalErrorEnabledField(t *testing.T) { }) // Use zero value to avoid panic on constructor. var none Optional[WithEnabled] - require.Error(t, cm.Unmarshal(&none)) + require.Error(t, cm.Unmarshal(&none), xconfmap.WithScalarUnmarshaler()) } func TestUnmarshalConfigPointer(t *testing.T) { @@ -394,7 +394,7 @@ func TestUnmarshalConfigPointer(t *testing.T) { }) var cfg Config[*Sub] - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.NoError(t, err) assert.True(t, cfg.Sub1.HasValue()) assert.Equal(t, "bar", (*cfg.Sub1.Get()).Foo) @@ -411,7 +411,7 @@ func TestUnmarshalErr(t *testing.T) { assert.False(t, cfg.Sub1.HasValue()) - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.Error(t, err) require.ErrorContains(t, err, "has invalid keys: field") assert.False(t, cfg.Sub1.HasValue()) @@ -437,7 +437,7 @@ func TestSquashedOptional(t *testing.T) { Default(myIntDefault), } - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.NoError(t, err) assert.True(t, cfg.HasValue()) @@ -518,7 +518,7 @@ func TestOptionalMarshal(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { conf := confmap.New() - require.NoError(t, conf.Marshal(test.value)) + require.NoError(t, conf.Marshal(test.value, xconfmap.WithScalarMarshaler())) assert.Equal(t, test.expected, conf.ToStringMap()) }) } @@ -548,11 +548,11 @@ func TestComparePointerMarshal(t *testing.T) { t.Run(fmt.Sprintf("%v vs %v", test.pointer, test.optional), func(t *testing.T) { wrapPointer := Wrap[*Sub]{Sub1: test.pointer} confPointer := confmap.NewFromStringMap(nil) - require.NoError(t, confPointer.Marshal(wrapPointer)) + require.NoError(t, confPointer.Marshal(wrapPointer, xconfmap.WithScalarMarshaler())) wrapOptional := Wrap[Optional[Sub]]{Sub1: test.optional} confOptional := confmap.NewFromStringMap(nil) - require.NoError(t, confOptional.Marshal(wrapOptional)) + require.NoError(t, confOptional.Marshal(wrapOptional, xconfmap.WithScalarMarshaler())) assert.Equal(t, confPointer.ToStringMap(), confOptional.ToStringMap()) }) @@ -563,11 +563,11 @@ func TestComparePointerMarshal(t *testing.T) { t.Run(fmt.Sprintf("%v vs %v (omitempty)", test.pointer, test.optional), func(t *testing.T) { wrapPointer := WrapOmitEmpty[*Sub]{Sub1: test.pointer} confPointer := confmap.NewFromStringMap(nil) - require.NoError(t, confPointer.Marshal(wrapPointer)) + require.NoError(t, confPointer.Marshal(wrapPointer, xconfmap.WithScalarMarshaler())) wrapOptional := WrapOmitEmpty[Optional[Sub]]{Sub1: test.optional} confOptional := confmap.NewFromStringMap(nil) - require.NoError(t, confOptional.Marshal(wrapOptional)) + require.NoError(t, confOptional.Marshal(wrapOptional, xconfmap.WithScalarMarshaler())) assert.Equal(t, confPointer.ToStringMap(), confOptional.ToStringMap()) }) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index d1e6c046a98d..97d795fff190 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -47,13 +47,15 @@ func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshale mapKeyStringToMapKeyTextUnmarshalerHookFunc(), mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), + } + hooks = append(hooks, settings.AdditionalDecodeHookFuncs...) + hooks = append(hooks, unmarshalerHookFunc(result, skipTopLevelUnmarshaler), // after the main unmarshaler hook is called, // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), zeroSliceAndMapHookFunc(), - } - hooks = append(hooks, settings.AdditionalDecodeHookFuncs...) + ) dc := &mapstructure.DecoderConfig{ ErrorUnused: !settings.IgnoreUnused, Result: result, diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go index 3b0ef5ac6e5c..da9c97343a67 100644 --- a/confmap/xconfmap/scalarmarshaler.go +++ b/confmap/xconfmap/scalarmarshaler.go @@ -14,7 +14,7 @@ import ( func WithScalarMarshaler() confmap.MarshalOption { return internal.MarshalOptionFunc(func(mo *internal.MarshalOptions) { - mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc(*mo) + mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc(mo) }) } @@ -30,7 +30,7 @@ type ScalarMarshaler interface { // 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. -func scalarmarshalerHookFunc(mo internal.MarshalOptions) mapstructure.DecodeHookFuncValue { +func scalarmarshalerHookFunc(mo *internal.MarshalOptions) mapstructure.DecodeHookFuncValue { return safeWrapDecodeHookFunc(func(from reflect.Value, _ reflect.Value) (any, error) { marshaler, ok := from.Interface().(ScalarMarshaler) if !ok { @@ -42,6 +42,15 @@ func scalarmarshalerHookFunc(mo internal.MarshalOptions) mapstructure.DecodeHook return nil, err } - return internal.Encode(v, mo) + res, err := internal.Encode(v, *mo) + if err != nil { + return nil, err + } + + if res == nil { + return nil, nil + } + + return res, nil }) } diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go index b4b1d84734b1..1ee8664a9c09 100644 --- a/confmap/xconfmap/scalarmarshaler_test.go +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -142,7 +142,7 @@ func TestMarshalConfig(t *testing.T) { Nonimplntms: NonImplWrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{48}}}, Implint: wrapperType[int]{inner: 1}, Implstr: wrapperType[string]{inner: "test"}, - Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{81}}}, Implntms: wrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{80}}}, Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 2, data: []byte{80}}}}, } diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index 5095d3a5a5ea..b1ef130db101 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -14,7 +14,7 @@ import ( func WithScalarUnmarshaler() confmap.UnmarshalOption { return internal.UnmarshalOptionFunc(func(uo *internal.UnmarshalOptions) { - uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(*uo)) + uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(uo)) }) } @@ -32,18 +32,16 @@ type ScalarUnmarshaler interface { // 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. -func scalarunmarshalerHookFunc(opts internal.UnmarshalOptions) mapstructure.DecodeHookFuncValue { +func scalarunmarshalerHookFunc(opts *internal.UnmarshalOptions) mapstructure.DecodeHookFuncValue { return safeWrapDecodeHookFunc(func(from, to reflect.Value) (any, error) { - opts.AdditionalDecodeHookFuncs = append(opts.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(opts)) - if !to.CanAddr() { return from.Interface(), nil } - if from.Kind() == reflect.Struct || - from.Kind() == reflect.Pointer && from.Elem().Kind() == reflect.Struct { - return from.Interface(), nil - } + // if from.Kind() == reflect.Struct || + // from.Kind() == reflect.Pointer && from.Elem().Kind() == reflect.Struct { + // return from.Interface(), nil + // } toPtr := to.Addr().Interface() @@ -52,9 +50,13 @@ func scalarunmarshalerHookFunc(opts internal.UnmarshalOptions) mapstructure.Deco return from.Interface(), nil } + if to.Addr().IsNil() { + unmarshaler = reflect.New(to.Type()).Interface().(ScalarUnmarshaler) + } + resultVal := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) - if err := internal.Decode(from.Interface(), resultVal.Interface(), opts, false); err != nil { + if err := internal.Decode(from.Interface(), resultVal.Interface(), *opts, false); err != nil { return nil, err } diff --git a/confmap/xconfmap/scalarunmarshaler_test.go b/confmap/xconfmap/scalarunmarshaler_test.go index f2d8dbbdcca7..2cf9e45a10d3 100644 --- a/confmap/xconfmap/scalarunmarshaler_test.go +++ b/confmap/xconfmap/scalarunmarshaler_test.go @@ -18,7 +18,7 @@ func TestUnmarshalConfig(t *testing.T) { Ntma: nonTextMarshalerAlias("test"), Implint: wrapperType[int]{inner: 1}, Implstr: wrapperType[string]{inner: "test"}, - Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{81}}}, Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}}, } diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml index 8989fa889d94..e04b9585db9b 100644 --- a/confmap/xconfmap/testdata/config.yaml +++ b/confmap/xconfmap/testdata/config.yaml @@ -1,7 +1,7 @@ impl_int: 1 impl_non_text_marshaler_struct: {} impl_str: test -impl_text_marshaler_struct: P +impl_text_marshaler_struct: Q non_impl_int: {} non_impl_non_text_marshaler_struct: {} non_impl_str: {} From 019a3125eaa7010bdbbe8ac506114e86bfcf6b8c Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 3 Nov 2025 15:46:04 -0500 Subject: [PATCH 24/25] Support specifying null for scalars --- confmap/xconfmap/scalarunmarshaler.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index b1ef130db101..d6ad8527628c 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -56,12 +56,20 @@ func scalarunmarshalerHookFunc(opts *internal.UnmarshalOptions) mapstructure.Dec resultVal := reflect.New(reflect.TypeOf(unmarshaler.ScalarType())) - if err := internal.Decode(from.Interface(), resultVal.Interface(), *opts, false); err != nil { - return nil, err - } - - if err := unmarshaler.UnmarshalScalar(resultVal.Elem().Interface()); err != nil { - return nil, err + // If the inner condition matches, the user specified `null` for this + // value, which was translated to a nil map by mapstructure. + if !(from.Kind() == reflect.Map && from.IsNil()) { + if err := internal.Decode(from.Interface(), resultVal.Interface(), *opts, false); err != nil { + return nil, err + } + + if err := unmarshaler.UnmarshalScalar(resultVal.Elem().Interface()); err != nil { + return nil, err + } + } else { + if err := unmarshaler.UnmarshalScalar(nil); err != nil { + return nil, err + } } return unmarshaler, nil From adb7f03adff0825b6ab7ed2128032dfed4873a53 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Mon, 3 Nov 2025 19:35:05 -0500 Subject: [PATCH 25/25] POC for typed config migrations/unions --- config/configopaque/maplist.go | 43 ++++++----- config/configopaque/maplist_test.go | 4 +- config/configoptional/optional.go | 103 ++++++++++++++++--------- config/configoptional/optional_test.go | 4 +- confmap/xconfmap/scalarunmarshaler.go | 60 ++++++++++++++ 5 files changed, 155 insertions(+), 59 deletions(-) diff --git a/config/configopaque/maplist.go b/config/configopaque/maplist.go index 71a206b633f1..435235871ec1 100644 --- a/config/configopaque/maplist.go +++ b/config/configopaque/maplist.go @@ -4,12 +4,10 @@ package configopaque // import "go.opentelemetry.io/collector/config/configopaque" import ( - "cmp" "fmt" "iter" "slices" - "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/xconfmap" ) @@ -28,27 +26,32 @@ type Pair struct { // Pairs are assumed to have distinct names. This is checked during config validation. type MapList []Pair -var _ confmap.Unmarshaler = (*MapList)(nil) +type MapFormat map[string]String -// Unmarshal is called by the Collector when unmarshalling from a map. -// When the input config is a slice, this will be skipped, -// and mapstructure's default unmarshalling logic will be used. -func (ml *MapList) Unmarshal(conf *confmap.Conf) error { - var m2 map[string]String - if err := conf.Unmarshal(&m2); err != nil { - return err +var _ xconfmap.ConfigMigrator = MapFormat{} + +func (mm MapFormat) Migrate(cfg any) (bool, error) { + ml, ok := cfg.(*MapList) + if !ok { + return false, nil + } + if ml == nil { + return false, nil + } + + for key, val := range mm { + ml.Set(key, val) } - *ml = make(MapList, 0, len(m2)) - for name, value := range m2 { - *ml = append(*ml, Pair{ - Name: name, - Value: value, - }) + + return true, nil +} + +var _ xconfmap.MigrateableConfig = (*MapList)(nil) + +func (ml MapList) Migrations() []xconfmap.ConfigMigrator { + return []xconfmap.ConfigMigrator{ + MapFormat{}, } - slices.SortFunc(*ml, func(p1, p2 Pair) int { - return cmp.Compare(p1.Name, p2.Name) - }) - return nil } var _ xconfmap.Validator = MapList(nil) diff --git a/config/configopaque/maplist_test.go b/config/configopaque/maplist_test.go index a61f6217fe62..d8113dc2c490 100644 --- a/config/configopaque/maplist_test.go +++ b/config/configopaque/maplist_test.go @@ -55,7 +55,7 @@ func TestMapListDuality(t *testing.T) { conf1, err := retrieved1.AsConf() require.NoError(t, err) var tc1 testConfig - require.NoError(t, conf1.Unmarshal(&tc1)) + require.NoError(t, conf1.Unmarshal(&tc1, xconfmap.WithScalarUnmarshaler())) assert.NoError(t, xconfmap.Validate(&tc1)) retrieved2, err := confmap.NewRetrievedFromYAML([]byte(headersMap)) @@ -63,7 +63,7 @@ func TestMapListDuality(t *testing.T) { conf2, err := retrieved2.AsConf() require.NoError(t, err) var tc2 testConfig - require.NoError(t, conf2.Unmarshal(&tc2)) + require.NoError(t, conf2.Unmarshal(&tc2, xconfmap.WithScalarUnmarshaler())) assert.NoError(t, xconfmap.Validate(&tc2)) assert.Equal(t, tc1, tc2) diff --git a/config/configoptional/optional.go b/config/configoptional/optional.go index 0db30e82b480..4dae2ffb30bd 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -36,6 +36,9 @@ type Optional[T any] struct { // flavor indicates the flavor of the Optional. // The zero value of flavor is noneFlavor. flavor flavor + + // Whether `enabled` was used + manuallySet bool } // deref a reflect.Type to its underlying type. @@ -157,7 +160,7 @@ func (o *Optional[T]) GetOrInsertDefault() *T { } empty := confmap.NewFromStringMap(map[string]any{}) - if err := empty.Unmarshal(o); err != nil { + if err := empty.Unmarshal(o, xconfmap.WithScalarUnmarshaler()); err != nil { // This should never happen, if it happens it is a bug, so this panic is not documented. panic(fmt.Errorf("failed to unmarshal empty map into %T type: %w. Please report this bug", o.value, err)) } @@ -166,7 +169,7 @@ func (o *Optional[T]) GetOrInsertDefault() *T { } var ( - _ confmap.Unmarshaler = (*Optional[any])(nil) + // _ confmap.Unmarshaler = (*Optional[any])(nil) _ xconfmap.ScalarUnmarshaler = (*Optional[any])(nil) ) @@ -196,53 +199,75 @@ var ( // // T must be derefenceable to a type with struct kind and not have an 'enabled' field. // Scalar values are not supported. -func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { - if err := assertNoEnabledField[T](); err != nil { - return err - } - - if o.flavor == noneFlavor && conf.ToStringMap() == nil { - // If the Optional is None and the configuration is nil, we do nothing. - // This replicates the behavior of unmarshaling into a field with a nil pointer. - return nil - } +// func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { +// if err := assertNoEnabledField[T](); err != nil { +// return err +// } + +// if o.flavor == noneFlavor && conf.ToStringMap() == nil { +// // If the Optional is None and the configuration is nil, we do nothing. +// // This replicates the behavior of unmarshaling into a field with a nil pointer. +// return nil +// } + +// isEnabled := true +// if addEnabledFieldFeatureGate.IsEnabled() && conf.IsSet("enabled") { +// enabled := conf.Get("enabled") +// conf.Delete("enabled") +// var ok bool +// if isEnabled, ok = enabled.(bool); !ok { +// return fmt.Errorf("unexpected type %T for 'enabled': got '%v' value expected 'true' or 'false'", enabled, enabled) +// } +// } + +// if err := conf.Unmarshal(&o.value, xconfmap.WithScalarUnmarshaler()); err != nil { +// return err +// } + +// if isEnabled { +// o.flavor = someFlavor +// } else { +// o.flavor = noneFlavor +// } + +// return nil +// } + +var _ xconfmap.ConfigMigrator = (*EnabledField[any])(nil) + +type EnabledField[T any] struct { + Enabled bool `mapstructure:"enabled"` +} - isEnabled := true - if addEnabledFieldFeatureGate.IsEnabled() && conf.IsSet("enabled") { - enabled := conf.Get("enabled") - conf.Delete("enabled") - var ok bool - if isEnabled, ok = enabled.(bool); !ok { - return fmt.Errorf("unexpected type %T for 'enabled': got '%v' value expected 'true' or 'false'", enabled, enabled) - } +func (ef EnabledField[T]) Migrate(val any) (bool, error) { + o, ok := val.(*Optional[T]) + if !ok { + return false, fmt.Errorf("expected Optional type but got %T", val) } - if err := conf.Unmarshal(&o.value, xconfmap.WithScalarUnmarshaler()); err != nil { - return err - } - - if isEnabled { + if ef.Enabled { o.flavor = someFlavor } else { o.flavor = noneFlavor } - return nil + o.manuallySet = true + + return true, nil } func (o *Optional[T]) UnmarshalScalar(val any) error { - if o.flavor == noneFlavor && val == nil { - // If the Optional is None and the configuration is nil, we do nothing. - // This replicates the behavior of unmarshaling into a field with a nil pointer. + if val == nil { return nil } - if val != nil { - v, ok := val.(T) - if !ok { - return fmt.Errorf("val is %T, not %T", val, v) - } - o.value = v + v, ok := val.(T) + if !ok { + return fmt.Errorf("val is %T, not %T", val, v) + } + o.value = v + + if !o.manuallySet { o.flavor = someFlavor } @@ -253,6 +278,14 @@ func (o *Optional[T]) ScalarType() any { return o.value } +var _ xconfmap.MigrateableConfig = (*Optional[any])(nil) + +func (o *Optional[T]) Migrations() []xconfmap.ConfigMigrator { + return []xconfmap.ConfigMigrator{ + EnabledField[T]{}, + } +} + var ( _ confmap.Marshaler = (*Optional[any])(nil) _ xconfmap.ScalarMarshaler = (*Optional[any])(nil) diff --git a/config/configoptional/optional_test.go b/config/configoptional/optional_test.go index 8937821765a9..c4f03610a34e 100644 --- a/config/configoptional/optional_test.go +++ b/config/configoptional/optional_test.go @@ -519,7 +519,7 @@ func TestAddFieldEnabledFeatureGate(t *testing.T) { t.Run(test.name, func(t *testing.T) { cfg := test.defaultCfg conf := confmap.NewFromStringMap(test.config) - require.NoError(t, conf.Unmarshal(&cfg)) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) require.Equal(t, test.expectedSub, cfg.Sub1.HasValue()) if test.expectedSub { require.Equal(t, test.expectedFoo, cfg.Sub1.Get().Foo) @@ -542,7 +542,7 @@ func TestUnmarshalErrorEnabledInvalidType(t *testing.T) { cfg := Config[Sub]{ Sub1: None[Sub](), } - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.ErrorContains(t, err, "unexpected type string for 'enabled': got 'something' value expected 'true' or 'false'") } diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go index d6ad8527628c..54b5a2344091 100644 --- a/confmap/xconfmap/scalarunmarshaler.go +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -4,7 +4,9 @@ package xconfmap // import "go.opentelemetry.io/collector/confmap/xconfmap" import ( + "fmt" "reflect" + "strings" "github.com/go-viper/mapstructure/v2" @@ -29,6 +31,14 @@ type ScalarUnmarshaler interface { ScalarType() any } +type MigrateableConfig interface { + Migrations() []ConfigMigrator +} + +type ConfigMigrator interface { + Migrate(cfg any) (bool, error) +} + // 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. @@ -45,6 +55,56 @@ func scalarunmarshalerHookFunc(opts *internal.UnmarshalOptions) mapstructure.Dec toPtr := to.Addr().Interface() + fm, fmOk := from.Interface().(map[string]interface{}) + + m, ok := toPtr.(MigrateableConfig) + if ok && !(from.Kind() == reflect.Map && from.IsNil()) && (!fmOk || len(fm) > 0) { + for _, migrator := range m.Migrations() { + mOpts := *opts + mOpts.IgnoreUnused = true + if err := internal.Decode(from.Interface(), &migrator, mOpts, false); err != nil { + // An error decoding likely means this migrator's schema + // doesn't match the input. + continue + } + + migrated, err := migrator.Migrate(toPtr) + + if err != nil { + return nil, fmt.Errorf("failed to migrate config using %T: %w", migrator, err) + } + + if migrated { + switch reflect.TypeOf(migrator).Kind() { + case reflect.Map: + // Empty out the map. + return toPtr, nil + case reflect.Struct: + mVal := reflect.TypeOf(migrator) + numFields := mVal.NumField() + + fm, ok := from.Interface().(map[string]interface{}) + if !ok { + return toPtr, nil + } + + for i := range numFields { + field := mVal.Field(i) + tag, ok := field.Tag.Lookup("mapstructure") + + if !ok { + continue + } + + name := strings.Split(tag, ",")[0] + delete(fm, name) + } + } + + } + } + } + unmarshaler, ok := toPtr.(ScalarUnmarshaler) if !ok { return from.Interface(), nil