Skip to content

Commit

Permalink
[otelcol] Obtain the Collector's effective configuration from `otelco…
Browse files Browse the repository at this point in the history
…l.Config` (#10139)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

The `otelcol.ConfmapProvider` interface accurately reports the config
provided to the Collector by the user, but fails to effectively report
the Collector's effective configuration. In particular, it misses:
* Default values for fields in Config structs.
* Transformations done to Config structs by their `Unmarshal` or
`Validate` methods.
* Custom marshaling of types after we know the type of the config. This
is most obvious with `configopaque.String`, where we want these values
to always be redacted when sent out of the Collector.

I think we should attempt to get the Collector's effective configuration
from `otelcol.Config` instead of using the map compiled by the
confmap.Resolver. This also allows us to get rid of
`otelcol.ConfmapProvider`.

I have manually tested this using an OpAMP Supervisor setup, but wasn't
able to add unit tests due to the way `otelcol.Collector` is written.

---------

Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
  • Loading branch information
evan-bradley and mx-psi authored Jul 12, 2024
1 parent 6227646 commit 3087c53
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 30 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: 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) 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)
}

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) {
conf, err := cm.mapResolver.Resolve(ctx)

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

0 comments on commit 3087c53

Please sign in to comment.