Skip to content

Commit

Permalink
[otelcol] Marshal the Collector's config from otelcol.Config
Browse files Browse the repository at this point in the history
  • Loading branch information
evan-bradley committed Jul 9, 2024
1 parent 650ac0b commit 9694609
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 144 deletions.
25 changes: 25 additions & 0 deletions .chloggen/better-effective-config.yaml
Original file line number Diff line number Diff line change
@@ -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: otelcol

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Obtain the Collector's effective config from otelcol.Config

# One or more tracking issues or pull requests related to the change
issues: [10139]

# (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: "`otelcol.Collector` will now marshal `confmap.Conf` objects from `otelcol.Config` itself."

# 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]
25 changes: 25 additions & 0 deletions .chloggen/deprecate-confmapprovider.yaml
Original file line number Diff line number Diff line change
@@ -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: deprecation

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: otelcol

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Deprecate `otelcol.ConfmapProvider`

# One or more tracking issues or pull requests related to the change
issues: [10139]

# (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: "`otelcol.Collector` will now marshal `confmap.Conf` objects from `otelcol.Config` itself."

# 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]
17 changes: 6 additions & 11 deletions otelcol/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,17 +163,6 @@ func (col *Collector) Shutdown() {
func (col *Collector) setupConfigurationComponents(ctx context.Context) error {
col.setCollectorState(StateStarting)

var conf *confmap.Conf

if cp, ok := col.configProvider.(ConfmapProvider); ok {
var err error
conf, err = cp.GetConfmap(ctx)

if err != nil {
return fmt.Errorf("failed to resolve config: %w", err)
}
}

factories, err := col.set.Factories()
if err != nil {
return fmt.Errorf("failed to initialize factories: %w", err)
Expand All @@ -189,6 +178,12 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error {

col.serviceConfig = &cfg.Service

conf := confmap.New()

if err := conf.Marshal(cfg); err != nil {
return fmt.Errorf("could not marshal configuration: %w", err)

Check warning on line 184 in otelcol/collector.go

View check run for this annotation

Codecov / codecov/patch

otelcol/collector.go#L184

Added line #L184 was not covered by tests
}

col.service, err = service.New(ctx, service.Settings{
BuildInfo: col.set.BuildInfo,
CollectorConf: conf,
Expand Down
18 changes: 0 additions & 18 deletions otelcol/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -450,24 +450,6 @@ func TestCollectorDryRun(t *testing.T) {
}
}

func TestPassConfmapToServiceFailure(t *testing.T) {
set := CollectorSettings{
BuildInfo: component.NewDefaultBuildInfo(),
Factories: nopFactories,
ConfigProviderSettings: ConfigProviderSettings{
ResolverSettings: confmap.ResolverSettings{
URIs: []string{filepath.Join("testdata", "otelcol-invalid.yaml")},
ProviderFactories: []confmap.ProviderFactory{confmap.NewProviderFactory(newFailureProvider)},
},
},
}
col, err := NewCollector(set)
require.NoError(t, err)

err = col.Run(context.Background())
require.Error(t, err)
}

func startCollector(ctx context.Context, t *testing.T, col *Collector) *sync.WaitGroup {
wg := &sync.WaitGroup{}
wg.Add(1)
Expand Down
13 changes: 3 additions & 10 deletions otelcol/configprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ type ConfigProvider interface {
//
// The purpose of this interface is that otelcol.ConfigProvider structs do not
// necessarily need to use confmap.Conf as their underlying config structure.
//
// Deprecated: [v0.105.0] This interface is deprecated. otelcol.Collector will now obtain
// a confmap.Conf object from the unmarshaled config itself.
type ConfmapProvider interface {
// GetConfmap resolves the Collector's configuration and provides it as a confmap.Conf object.
//
Expand All @@ -70,7 +73,6 @@ type configProvider struct {
}

var _ ConfigProvider = (*configProvider)(nil)
var _ ConfmapProvider = (*configProvider)(nil)

// ConfigProviderSettings are the settings to configure the behavior of the ConfigProvider.
type ConfigProviderSettings struct {
Expand Down Expand Up @@ -142,12 +144,3 @@ func (cm *configProvider) Watch() <-chan error {
func (cm *configProvider) Shutdown(ctx context.Context) error {
return cm.mapResolver.Shutdown(ctx)
}

func (cm *configProvider) GetConfmap(ctx context.Context) (*confmap.Conf, error) {
conf, err := cm.mapResolver.Resolve(ctx)
if err != nil {
return nil, fmt.Errorf("cannot resolve the configuration: %w", err)
}

return conf, nil
}
105 changes: 0 additions & 105 deletions otelcol/configprovider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import (
"gopkg.in/yaml.v3"

"go.opentelemetry.io/collector/confmap"
"go.opentelemetry.io/collector/featuregate"
"go.opentelemetry.io/collector/internal/featuregates"
)

func newConfig(yamlBytes []byte, factories Factories) (*Config, error) {
Expand Down Expand Up @@ -108,106 +106,3 @@ func TestConfigProviderFile(t *testing.T) {

assert.EqualValues(t, configNop, cfg)
}

func TestGetConfmap(t *testing.T) {
uriLocation := "file:" + filepath.Join("testdata", "otelcol-nop.yaml")
fileProvider := newFakeProvider("file", func(_ context.Context, _ string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) {
return confmap.NewRetrieved(newConfFromFile(t, uriLocation[5:]))
})
set := ConfigProviderSettings{
ResolverSettings: confmap.ResolverSettings{
URIs: []string{uriLocation},
ProviderFactories: []confmap.ProviderFactory{fileProvider},
},
}

configBytes, err := os.ReadFile(filepath.Join("testdata", "otelcol-nop.yaml"))
require.NoError(t, err)

yamlMap := map[string]any{}
err = yaml.Unmarshal(configBytes, yamlMap)
require.NoError(t, err)

cp, err := NewConfigProvider(set)
require.NoError(t, err)

cmp, ok := cp.(ConfmapProvider)
require.True(t, ok)

cmap, err := cmp.GetConfmap(context.Background())
require.NoError(t, err)

assert.EqualValues(t, yamlMap, cmap.ToStringMap())
}

func TestStrictlyTypedCoda(t *testing.T) {
tests := []struct {
basename string
// isErrFromStrictTypes indicates whether the test should expect an error when the feature gate is
// disabled. If so, we check that it errs both with and without the feature gate and that the coda is never
// present.
isErrFromStrictTypes bool
}{
{basename: "weak-implicit-bool-to-string.yaml"},
{basename: "weak-implicit-int-to-string.yaml"},
{basename: "weak-implicit-bool-to-int.yaml"},
{basename: "weak-implicit-string-to-int.yaml"},
{basename: "weak-implicit-int-to-bool.yaml"},
{basename: "weak-implicit-string-to-bool.yaml"},
{basename: "weak-empty-map-to-empty-array.yaml"},
{basename: "weak-slice-of-maps-to-map.yaml"},
{basename: "weak-single-element-to-slice.yaml"},
{
basename: "otelcol-invalid-components.yaml",
isErrFromStrictTypes: true,
},
}

for _, tt := range tests {
t.Run(tt.basename, func(t *testing.T) {
filename := filepath.Join("testdata", tt.basename)
uriLocation := "file:" + filename
fileProvider := newFakeProvider("file", func(_ context.Context, _ string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) {
return confmap.NewRetrieved(newConfFromFile(t, filename))
})
cp, err := NewConfigProvider(ConfigProviderSettings{
ResolverSettings: confmap.ResolverSettings{
URIs: []string{uriLocation},
ProviderFactories: []confmap.ProviderFactory{fileProvider},
},
})
require.NoError(t, err)
factories, err := nopFactories()
require.NoError(t, err)

// Save the previous value of the feature gate and restore it after the test.
prev := featuregates.StrictlyTypedInputGate.IsEnabled()
defer func() {
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, prev))
}()

// Ensure the error does not appear with the feature gate disabled.
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, false))
_, errWeakTypes := cp.Get(context.Background(), factories)
if tt.isErrFromStrictTypes {
require.Error(t, errWeakTypes)
// Ensure coda is **NOT** present.
assert.NotContains(t, errWeakTypes.Error(), strictlyTypedMessageCoda)
} else {
require.NoError(t, errWeakTypes)
}

// Test with the feature gate enabled.
require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.StrictlyTypedInputID, true))
_, errStrictTypes := cp.Get(context.Background(), factories)
require.Error(t, errStrictTypes)
if tt.isErrFromStrictTypes {
// Ensure coda is **NOT** present.
assert.NotContains(t, errStrictTypes.Error(), strictlyTypedMessageCoda)
} else {
// Ensure coda is present.
assert.ErrorContains(t, errStrictTypes, strictlyTypedMessageCoda)
}
})
}
}

0 comments on commit 9694609

Please sign in to comment.