From 0718d0a9614e5d9b6b2823a3ac7f76e7b9ae6730 Mon Sep 17 00:00:00 2001 From: Dan Jaglowski Date: Wed, 14 Dec 2022 14:51:09 -0500 Subject: [PATCH] Consolidate otelcol configunmarshaler into generic implementation --- .chloggen/generic-configunmarshaler.yaml | 16 ++ otelcol/internal/configunmarshaler/configs.go | 40 ++-- .../configunmarshaler/configs_test.go | 187 ++++++++++-------- otelcol/unmarshaler.go | 31 ++- 4 files changed, 158 insertions(+), 116 deletions(-) create mode 100755 .chloggen/generic-configunmarshaler.yaml diff --git a/.chloggen/generic-configunmarshaler.yaml b/.chloggen/generic-configunmarshaler.yaml new file mode 100755 index 00000000000..8fcdf00741d --- /dev/null +++ b/.chloggen/generic-configunmarshaler.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: configunmarshaler + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Consolidate package into generic implementation + +# One or more tracking issues or pull requests related to the change +issues: [6801] + +# (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: diff --git a/otelcol/internal/configunmarshaler/configs.go b/otelcol/internal/configunmarshaler/configs.go index 52eb78a10e8..75266ade8e9 100644 --- a/otelcol/internal/configunmarshaler/configs.go +++ b/otelcol/internal/configunmarshaler/configs.go @@ -22,49 +22,49 @@ import ( "go.opentelemetry.io/collector/confmap" ) -type Configs struct { +type Configs[F component.Factory] struct { cfgs map[component.ID]component.Config - factories map[component.Type]component.Factory + factories map[component.Type]F } -func NewConfigs(factories map[component.Type]component.Factory) *Configs { - return &Configs{factories: factories} +func NewConfigs[F component.Factory](factories map[component.Type]F) *Configs[F] { + return &Configs[F]{factories: factories} } -func (p *Configs) Unmarshal(conf *confmap.Conf) error { - rawProcs := make(map[component.ID]map[string]interface{}) - if err := conf.Unmarshal(&rawProcs, confmap.WithErrorUnused()); err != nil { +func (c *Configs[F]) Unmarshal(conf *confmap.Conf) error { + rawCfgs := make(map[component.ID]map[string]interface{}) + if err := conf.Unmarshal(&rawCfgs, confmap.WithErrorUnused()); err != nil { return err } // Prepare resulting map. - p.cfgs = make(map[component.ID]component.Config) - // Iterate over processors and create a config for each. - for id, value := range rawProcs { - // Find processor factory based on "type" that we read from config source. - factory := p.factories[id.Type()] - if factory == nil { - return errorUnknownType(id, reflect.ValueOf(p.factories).MapKeys()) + c.cfgs = make(map[component.ID]component.Config) + // Iterate over raw configs and create a config for each. + for id, value := range rawCfgs { + // Find factory based on component kind and type that we read from config source. + factory, ok := c.factories[id.Type()] + if !ok { + return errorUnknownType(id, reflect.ValueOf(c.factories).MapKeys()) } - // Create the default config for this processor. - processorCfg := factory.CreateDefaultConfig() + // Create the default config for this component. + cfg := factory.CreateDefaultConfig() // Now that the default config struct is created we can Unmarshal into it, // and it will apply user-defined config on top of the default. - if err := component.UnmarshalConfig(confmap.NewFromStringMap(value), processorCfg); err != nil { + if err := component.UnmarshalConfig(confmap.NewFromStringMap(value), cfg); err != nil { return errorUnmarshalError(id, err) } - p.cfgs[id] = processorCfg + c.cfgs[id] = cfg } return nil } -func (p *Configs) Configs() map[component.ID]component.Config { - return p.cfgs +func (c *Configs[F]) Configs() map[component.ID]component.Config { + return c.cfgs } func errorUnknownType(id component.ID, factories []reflect.Value) error { diff --git a/otelcol/internal/configunmarshaler/configs_test.go b/otelcol/internal/configunmarshaler/configs_test.go index f769302f8c5..fbd14fa1018 100644 --- a/otelcol/internal/configunmarshaler/configs_test.go +++ b/otelcol/internal/configunmarshaler/configs_test.go @@ -23,94 +23,125 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/exporter/exportertest" + "go.opentelemetry.io/collector/extension/extensiontest" "go.opentelemetry.io/collector/processor/processortest" + "go.opentelemetry.io/collector/receiver/receivertest" ) -func TestProcessorsUnmarshal(t *testing.T) { - factories := map[component.Type]component.Factory{ - "nop": processortest.NewNopFactory(), - } - - procs := NewConfigs(factories) - conf := confmap.NewFromStringMap(map[string]interface{}{ - "nop": nil, - "nop/myprocessor": nil, - }) - require.NoError(t, procs.Unmarshal(conf)) - - cfgWithName := factories["nop"].CreateDefaultConfig() - assert.Equal(t, map[component.ID]component.Config{ - component.NewID("nop"): factories["nop"].CreateDefaultConfig(), - component.NewIDWithName("nop", "myprocessor"): cfgWithName, - }, procs.cfgs) -} - -func TestProcessorsUnmarshalError(t *testing.T) { - var testCases = []struct { - name string - conf *confmap.Conf - // string that the error must contain - expectedError string - }{ - { - name: "invalid-type", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nop": nil, - "/custom": nil, - }), - expectedError: "the part before / should not be empty", +var testKinds = []struct { + kind string + factories map[component.Type]component.Factory +}{ + { + kind: "receiver", + factories: map[component.Type]component.Factory{ + "nop": receivertest.NewNopFactory(), }, - { - name: "invalid-name-after-slash", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nop": nil, - "nop/": nil, - }), - expectedError: "the part after / should not be empty", + }, + { + kind: "processor", + factories: map[component.Type]component.Factory{ + "nop": processortest.NewNopFactory(), }, - { - name: "unknown-type", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nosuchprocessor": nil, - }), - expectedError: "unknown type: \"nosuchprocessor\"", + }, + { + kind: "exporter", + factories: map[component.Type]component.Factory{ + "nop": exportertest.NewNopFactory(), }, - { - name: "duplicate", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nop /exp ": nil, - " nop/ exp": nil, - }), - expectedError: "duplicate name", + }, + { + kind: "extension", + factories: map[component.Type]component.Factory{ + "nop": extensiontest.NewNopFactory(), }, - { - name: "invalid-section", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nop": map[string]interface{}{ - "unknown_section": "processor", - }, - }), - expectedError: "error reading configuration for \"nop\"", - }, - { - name: "invalid-processor-sub-config", - conf: confmap.NewFromStringMap(map[string]interface{}{ - "nop": "tests", - }), - expectedError: "'[nop]' expected a map, got 'string'", - }, - } + }, +} - factories := map[component.Type]component.Factory{ - "nop": exportertest.NewNopFactory(), +func TestUnmarshal(t *testing.T) { + for _, tk := range testKinds { + t.Run(tk.kind, func(t *testing.T) { + cfgs := NewConfigs(tk.factories) + conf := confmap.NewFromStringMap(map[string]interface{}{ + "nop": nil, + "nop/my" + tk.kind: nil, + }) + require.NoError(t, cfgs.Unmarshal(conf)) + + assert.Equal(t, map[component.ID]component.Config{ + component.NewID("nop"): tk.factories["nop"].CreateDefaultConfig(), + component.NewIDWithName("nop", "my"+tk.kind): tk.factories["nop"].CreateDefaultConfig(), + }, cfgs.Configs()) + }) } +} + +func TestUnmarshalError(t *testing.T) { + for _, tk := range testKinds { + t.Run(tk.kind, func(t *testing.T) { + var testCases = []struct { + name string + conf *confmap.Conf + // string that the error must contain + expectedError string + }{ + { + name: "invalid-type", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nop": nil, + "/custom": nil, + }), + expectedError: "the part before / should not be empty", + }, + { + name: "invalid-name-after-slash", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nop": nil, + "nop/": nil, + }), + expectedError: "the part after / should not be empty", + }, + { + name: "unknown-type", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nosuch" + tk.kind: nil, + }), + expectedError: "unknown type: \"nosuch" + tk.kind + "\"", + }, + { + name: "duplicate", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nop /my" + tk.kind + " ": nil, + " nop/ my" + tk.kind: nil, + }), + expectedError: "duplicate name", + }, + { + name: "invalid-section", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nop": map[string]interface{}{ + "unknown_section": tk.kind, + }, + }), + expectedError: "error reading configuration for \"nop\"", + }, + { + name: "invalid-sub-config", + conf: confmap.NewFromStringMap(map[string]interface{}{ + "nop": "tests", + }), + expectedError: "'[nop]' expected a map, got 'string'", + }, + } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - procs := NewConfigs(factories) - err := procs.Unmarshal(tt.conf) - require.Error(t, err) - assert.Contains(t, err.Error(), tt.expectedError) + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + cfgs := NewConfigs(tk.factories) + err := cfgs.Unmarshal(tt.conf) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedError) + }) + } }) } } diff --git a/otelcol/unmarshaler.go b/otelcol/unmarshaler.go index f469848ff51..482b38557fe 100644 --- a/otelcol/unmarshaler.go +++ b/otelcol/unmarshaler.go @@ -17,20 +17,23 @@ package otelcol // import "go.opentelemetry.io/collector/otelcol" import ( "go.uber.org/zap/zapcore" - "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configtelemetry" "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/exporter" + "go.opentelemetry.io/collector/extension" "go.opentelemetry.io/collector/otelcol/internal/configunmarshaler" + "go.opentelemetry.io/collector/processor" + "go.opentelemetry.io/collector/receiver" "go.opentelemetry.io/collector/service" "go.opentelemetry.io/collector/service/telemetry" ) type configSettings struct { - Receivers *configunmarshaler.Configs `mapstructure:"receivers"` - Processors *configunmarshaler.Configs `mapstructure:"processors"` - Exporters *configunmarshaler.Configs `mapstructure:"exporters"` - Extensions *configunmarshaler.Configs `mapstructure:"extensions"` - Service service.ConfigService `mapstructure:"service"` + Receivers *configunmarshaler.Configs[receiver.Factory] `mapstructure:"receivers"` + Processors *configunmarshaler.Configs[processor.Factory] `mapstructure:"processors"` + Exporters *configunmarshaler.Configs[exporter.Factory] `mapstructure:"exporters"` + Extensions *configunmarshaler.Configs[extension.Factory] `mapstructure:"extensions"` + Service service.ConfigService `mapstructure:"service"` } // unmarshal the configSettings from a confmap.Conf. @@ -38,10 +41,10 @@ type configSettings struct { func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) { // Unmarshal top level sections and validate. cfg := &configSettings{ - Receivers: configunmarshaler.NewConfigs(toFactoryMap(factories.Receivers)), - Processors: configunmarshaler.NewConfigs(toFactoryMap(factories.Processors)), - Exporters: configunmarshaler.NewConfigs(toFactoryMap(factories.Exporters)), - Extensions: configunmarshaler.NewConfigs(toFactoryMap(factories.Extensions)), + Receivers: configunmarshaler.NewConfigs(factories.Receivers), + Processors: configunmarshaler.NewConfigs(factories.Processors), + Exporters: configunmarshaler.NewConfigs(factories.Exporters), + Extensions: configunmarshaler.NewConfigs(factories.Extensions), // TODO: Add a component.ServiceFactory to allow this to be defined by the Service. Service: service.ConfigService{ Telemetry: telemetry.Config{ @@ -69,11 +72,3 @@ func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) { return cfg, v.Unmarshal(&cfg, confmap.WithErrorUnused()) } - -func toFactoryMap[F component.Factory](factories map[component.Type]F) map[component.Type]component.Factory { - ret := make(map[component.Type]component.Factory, len(factories)) - for k := range factories { - ret[k] = factories[k] - } - return ret -}