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

[otelcol] Obtain the Collector's effective configuration from otelcol.Config #10139

Merged
merged 7 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all 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: 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: breaking

# 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]
25 changes: 25 additions & 0 deletions .chloggen/deprecate-getconfmap.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.ConfigProvider).GetConfmap`

# 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: Call `(*confmap.Conf).Marshal(*otelcol.Config)` to get the Collector's configuration.

# 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) 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 @@

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option could be to make this only a warning log, if we're comfortable that all extensions asking for the effective configuration will get an empty map.

}

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
7 changes: 6 additions & 1 deletion 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 @@ -143,8 +145,11 @@ func (cm *configProvider) Shutdown(ctx context.Context) error {
return cm.mapResolver.Shutdown(ctx)
}

// Deprecated: [v0.105.0] Call `(*confmap.Conf).Marshal(*otelcol.Config)` to get
// the Collector's configuration instead.
func (cm *configProvider) GetConfmap(ctx context.Context) (*confmap.Conf, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep a dummy implementation of this method for one or more releases that returns an error saying that this functionality is deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this because I don't expect anyone else is using it, but thinking on it, this should go through the normal deprecation process. I've added a deprecation notice to this method and we can just remove the implementation when we remove the interface. I would feel comfortable removing both one release after deprecating them.

conf, err := cm.mapResolver.Resolve(ctx)

if err != nil {
return nil, fmt.Errorf("cannot resolve the configuration: %w", err)
}
Expand Down
Loading