Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create configload.Parser struct to abstract away from Viper #2728

Merged
merged 17 commits into from
Mar 25, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions component/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
"context"

"github.com/spf13/viper"

"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand Down Expand Up @@ -67,18 +67,25 @@ type Factory interface {
// ConfigUnmarshaler interface is an optional interface that if implemented by a Factory,
// the configuration loading system will use to unmarshal the config.
type ConfigUnmarshaler interface {
// Unmarshal is a function that un-marshals a viper data into a config struct in a custom way.
// componentViperSection *viper.Viper
// The config for this specific component. May be nil or empty if no config available.
// Unmarshal is a function that loads data into a config struct in a custom way.
// componentSection *configload.Loader
// The loader for this specific component. May be nil or empty if no config available.
// intoCfg interface{}
// An empty interface wrapping a pointer to the config struct to unmarshal into.
Unmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error
Unmarshal(componentSection *configload.Loader, intoCfg interface{}) error
}

// CustomUnmarshaler is a function that un-marshals a viper data into a config struct
// CustomUnmarshaler is a function that loads data into a config struct
// in a custom way.
// componentViperSection *viper.Viper
// The config for this specific component. May be nil or empty if no config available.
// componentSection *configload.Loader
// The loader for this specific component. May be nil or empty if no config available.
// intoCfg interface{}
// An empty interface wrapping a pointer to the config struct to unmarshal into.
type CustomUnmarshaler func(componentViperSection *viper.Viper, intoCfg interface{}) error
type CustomUnmarshaler func(componentSection *configload.Loader, intoCfg interface{}) error

// ToCustomUnmarshaler creates a custom unmarshaler from a Viper unmarshaler.
func ToCustomUnmarshaler(viperUnmarshaler func(*viper.Viper, interface{}) error) CustomUnmarshaler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split for the moment this PR into 2:

  1. All changes except changes in component.go (we keep the old interface for the moment), but we add the Parser and move helpers for viper.
  2. Changes in the component.go (reason is maybe we just don't do these changes anymore and jump to add the new unmarshaler directly on the config, and deal that's it).

return func(componentSection *configload.Loader, intoCfg interface{}) error {
return viperUnmarshaler(componentSection.Viper(), intoCfg)
}
}
30 changes: 10 additions & 20 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/spf13/viper"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand All @@ -44,11 +45,6 @@ const (
errUnmarshalTopLevelStructureError
)

const (
// ViperDelimiter is used as the default key delimiter in the default viper instance
ViperDelimiter = "::"
)

type configError struct {
msg string // human readable error message.
code configErrorCode // internal error code.
Expand Down Expand Up @@ -98,12 +94,6 @@ type pipelineSettings struct {
// typeAndNameSeparator is the separator that is used between type and name in type/name composite keys.
const typeAndNameSeparator = "/"

// Creates a new Viper instance with a different key-delimitor "::" instead of the
// default ".". This way configs can have keys that contain ".".
func NewViper() *viper.Viper {
return viper.NewWithOptions(viper.KeyDelimiter(ViperDelimiter))
}

// Load loads a Config from Viper.
// After loading the config, need to check if it is valid by calling `ValidateConfig`.
func Load(
Expand Down Expand Up @@ -262,7 +252,7 @@ func loadExtensions(exts map[string]interface{}, factories map[configmodels.Type
// 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.
unm := unmarshaler(factory)
if err := unm(componentConfig, extensionCfg); err != nil {
if err := unm(configload.FromViper(componentConfig), extensionCfg); err != nil {
return nil, errorUnmarshalError(extensionsKeyName, fullName, err)
}

Expand Down Expand Up @@ -298,7 +288,7 @@ func LoadReceiver(componentConfig *viper.Viper, typeStr configmodels.Type, fullN
// 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.
unm := unmarshaler(factory)
if err := unm(componentConfig, receiverCfg); err != nil {
if err := unm(configload.FromViper(componentConfig), receiverCfg); err != nil {
return nil, errorUnmarshalError(receiversKeyName, fullName, err)
}

Expand Down Expand Up @@ -371,7 +361,7 @@ func loadExporters(exps map[string]interface{}, factories map[configmodels.Type]
// 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.
unm := unmarshaler(factory)
if err := unm(componentConfig, exporterCfg); err != nil {
if err := unm(configload.FromViper(componentConfig), exporterCfg); err != nil {
return nil, errorUnmarshalError(exportersKeyName, fullName, err)
}

Expand Down Expand Up @@ -414,7 +404,7 @@ func loadProcessors(procs map[string]interface{}, factories map[configmodels.Typ
// 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.
unm := unmarshaler(factory)
if err := unm(componentConfig, processorCfg); err != nil {
if err := unm(configload.FromViper(componentConfig), processorCfg); err != nil {
return nil, errorUnmarshalError(processorsKeyName, fullName, err)
}

Expand Down Expand Up @@ -556,8 +546,8 @@ func unmarshaler(factory component.Factory) component.CustomUnmarshaler {
return defaultUnmarshaler
}

func defaultUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{}) error {
return componentViperSection.UnmarshalExact(intoCfg)
func defaultUnmarshaler(componentSection *configload.Loader, intoCfg interface{}) error {
return componentSection.UnmarshalExact(intoCfg)
}

// Copied from the Viper but changed to use the same delimiter
Expand All @@ -566,11 +556,11 @@ func defaultUnmarshaler(componentViperSection *viper.Viper, intoCfg interface{})
func ViperSubExact(v *viper.Viper, key string) (*viper.Viper, error) {
data := v.Get(key)
if data == nil {
return NewViper(), nil
return configload.NewViper(), nil
}

if reflect.TypeOf(data).Kind() == reflect.Map {
subv := NewViper()
subv := configload.NewViper()
// Cannot return error because the subv is empty.
_ = subv.MergeConfigMap(cast.ToStringMap(data))
return subv, nil
Expand All @@ -582,7 +572,7 @@ func ViperSubExact(v *viper.Viper, key string) (*viper.Viper, error) {
}

func viperFromStringMap(data map[string]interface{}) *viper.Viper {
v := NewViper()
v := configload.NewViper()
// Cannot return error because the subv is empty.
_ = v.MergeConfigMap(cast.ToStringMap(data))
return v
Expand Down
3 changes: 2 additions & 1 deletion config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/stretchr/testify/require"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/internal/testcomponents"
Expand Down Expand Up @@ -495,7 +496,7 @@ func TestLoadEmptyAllSections(t *testing.T) {

func loadConfigFile(t *testing.T, fileName string, factories component.Factories) (*configmodels.Config, error) {
// Read yaml config from file
v := NewViper()
v := configload.NewViper()
v.SetConfigFile(fileName)
require.NoErrorf(t, v.ReadInConfig(), "unable to read the file %v", fileName)

Expand Down
64 changes: 64 additions & 0 deletions config/configload/configload.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// package configload implements the configuration Loader struct.
package configload
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

import (
"github.com/spf13/cast"
"github.com/spf13/viper"
)

const (
// ViperDelimiter is used as the default key delimiter in the default viper instance
ViperDelimiter = "::"
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
)

// Creates a new Viper instance with a different key-delimitor "::" instead of the
// default ".". This way configs can have keys that contain ".".
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
func NewViper() *viper.Viper {
return viper.NewWithOptions(viper.KeyDelimiter(ViperDelimiter))
}

// NewLoader creates a new Loader instanceg
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
func NewLoader() *Loader {
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
return &Loader{
v: NewViper(),
}
}

// FromViper creates a Loader from a Viper instance
mx-psi marked this conversation as resolved.
Show resolved Hide resolved
func FromViper(v *viper.Viper) *Loader {
return &Loader{v: v}
}

// Loader loads configuration
type Loader struct {
v *viper.Viper
}

// Viper returns the underlying Viper instance
func (l *Loader) Viper() *viper.Viper {
return l.v
}

// UnmarshalExact unmarshals the config into a struct, erroring if a field is nonexistent
func (l *Loader) UnmarshalExact(intoCfg interface{}) error {
return l.v.UnmarshalExact(intoCfg)
}

// ToStringMap creates a map[string]interface{} from a ConfigLoader
func (l *Loader) ToStringMap() map[string]interface{} {
return cast.ToStringMap(l.v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this will return the expected result, we need to test this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some tests, looks like it doesn't so I brought back the implementation I had initially.

}
3 changes: 2 additions & 1 deletion config/configtest/configtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand All @@ -30,7 +31,7 @@ import (
// Example usage for testing can be found in configtest_test.go
func NewViperFromYamlFile(t *testing.T, fileName string) *viper.Viper {
// Read yaml config from file
v := config.NewViper()
v := configload.NewViper()
v.SetConfigFile(fileName)
require.NoErrorf(t, v.ReadInConfig(), "unable to read the file %v", fileName)

Expand Down
7 changes: 3 additions & 4 deletions exporter/exporterhelper/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ package exporterhelper
import (
"context"

"github.com/spf13/viper"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configerror"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand Down Expand Up @@ -148,6 +147,6 @@ type factoryWithUnmarshaler struct {
}

// Unmarshal un-marshals the config using the provided custom unmarshaler.
func (f *factoryWithUnmarshaler) Unmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error {
return f.customUnmarshaler(componentViperSection, intoCfg)
func (f *factoryWithUnmarshaler) Unmarshal(componentSection *configload.Loader, intoCfg interface{}) error {
return f.customUnmarshaler(componentSection, intoCfg)
}
4 changes: 2 additions & 2 deletions exporter/exporterhelper/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ import (
"errors"
"testing"

"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"go.uber.org/zap"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configerror"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/consumer/pdata"
)
Expand Down Expand Up @@ -107,6 +107,6 @@ func createLogsExporter(context.Context, component.ExporterCreateParams, configm
return nopLogsExporter, nil
}

func customUnmarshaler(*viper.Viper, interface{}) error {
func customUnmarshaler(*configload.Loader, interface{}) error {
return errors.New("my error")
}
7 changes: 3 additions & 4 deletions extension/extensionhelper/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ package extensionhelper
import (
"context"

"github.com/spf13/viper"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand Down Expand Up @@ -94,6 +93,6 @@ type factoryWithUnmarshaler struct {
}

// Unmarshal un-marshals the config using the provided custom unmarshaler.
func (f *factoryWithUnmarshaler) Unmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error {
return f.customUnmarshaler(componentViperSection, intoCfg)
func (f *factoryWithUnmarshaler) Unmarshal(componentSection *configload.Loader, intoCfg interface{}) error {
return f.customUnmarshaler(componentSection, intoCfg)
}
4 changes: 2 additions & 2 deletions extension/extensionhelper/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
"errors"
"testing"

"github.com/spf13/viper"
"github.com/stretchr/testify/assert"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
)

Expand Down Expand Up @@ -74,7 +74,7 @@ func createExtension(context.Context, component.ExtensionCreateParams, configmod
return nopExtensionInstance, nil
}

func customUnmarshaler(*viper.Viper, interface{}) error {
func customUnmarshaler(*configload.Loader, interface{}) error {
return errors.New("my error")
}

Expand Down
7 changes: 3 additions & 4 deletions internal/testcomponents/example_exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ package testcomponents
import (
"context"

"github.com/spf13/viper"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/consumer/pdata"
"go.opentelemetry.io/collector/exporter/exporterhelper"
Expand Down Expand Up @@ -59,8 +58,8 @@ func createExporterDefaultConfig() configmodels.Exporter {
}
}

func customUnmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error {
return componentViperSection.UnmarshalExact(intoCfg)
func customUnmarshal(componentSection *configload.Loader, intoCfg interface{}) error {
return componentSection.UnmarshalExact(intoCfg)
}

func createTracesExporter(
Expand Down
7 changes: 3 additions & 4 deletions processor/processorhelper/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ package processorhelper
import (
"context"

"github.com/spf13/viper"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configerror"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/consumer"
)
Expand Down Expand Up @@ -144,6 +143,6 @@ type factoryWithUnmarshaler struct {
}

// Unmarshal un-marshals the config using the provided custom unmarshaler.
func (f *factoryWithUnmarshaler) Unmarshal(componentViperSection *viper.Viper, intoCfg interface{}) error {
return f.customUnmarshaler(componentViperSection, intoCfg)
func (f *factoryWithUnmarshaler) Unmarshal(componentSection *configload.Loader, intoCfg interface{}) error {
return f.customUnmarshaler(componentSection, intoCfg)
}
4 changes: 2 additions & 2 deletions processor/processorhelper/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
"errors"
"testing"

"github.com/spf13/viper"
"github.com/stretchr/testify/assert"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config/configload"
"go.opentelemetry.io/collector/config/configmodels"
"go.opentelemetry.io/collector/consumer"
)
Expand Down Expand Up @@ -91,6 +91,6 @@ func createLogsProcessor(context.Context, component.ProcessorCreateParams, confi
return nil, nil
}

func customUnmarshaler(*viper.Viper, interface{}) error {
func customUnmarshaler(*configload.Loader, interface{}) error {
return errors.New("my error")
}
Loading