Validate the controller config.

[ogaca-dd]

Description

This PR adds an option to validate the controller config.

EDIT: Always validate the config.

Alternative implementation:

Here is an alternative implementation:

• Move collector.Config to its own package (to avoid dependency cycle)
• Embeds the collector.Config into the controllerConfig

// Controller.Config
type Config struct {
  collector.Config
  // remaining fields 
}

• Avoid copying fields from collector.Config to controller.Config. Replace this code by

controlerCfg := &controller.Config{
		         Config: cfg,
			ExecutableReporter:     controllerOption.executableReporter,
			OnShutdown:             controllerOption.onShutdown,
		}

• Move validation checks from controller.Config to collector.Config.

[felixge]

nit: maybe change the func to take an enabled bool argument. This often makes it easier for callers that need to construct the config dynamically. Without the bool arg, they have to dynamically assembly a list of []option values (can they even do that if the type is unexported?) and do some branches. With the bool arg it's as simple as WithConfigValidation(*validationFlag == true) (example).

[ogaca-dd]

I assumed that users would either always want configuration validation or never — but not switch dynamically.
As you pointed out, since it’s not possible to have a list of []option (which is quite limiting), I introduced a validation bool to cover the use case you mentioned.n

[dmathieu]

Why do this as an option? I would run validation all the time.
If we were stable, this would be a breaking change and a different question. But we're far from that.

[ogaca-dd]

@dmathieu

Why do this as an option? I would run validation all the time.

Currently, validation only occurs if WithConfigValidation(true) is explicitly set. By default, no validation is performed.

In practice, validation is triggered in main.go within this repository, as well as in our repository. This is why I believe providing it as a configurable feature adds value.

If we were stable, this would be a breaking change and a different question. But we're far from that.

Could you expand a bit on this point?

[dmathieu]

What I mean is that if we move validation check into this controller, I don't see the need to increase the package's complexity by adding an option. I would just run the validation.

[florianl]

In practice, validation is triggered in main.go within this repository, as well as in our repository. This is why I believe providing it as a configurable feature adds value.

As the idea is to remove main.go and integrate with OTel collector, I think, the focus should be how this change would fit best in the OTel collector concept.

[ogaca-dd]

What I mean is that if we move validation check into this controller, I don't see the need to increase the package's complexity by adding an option. I would just run the validation.

That makes sense, if adding a minor potential breaking change is acceptable to you.

@florianl: Do you have any objections to always validating the config?

[dmathieu]

This is still under heavy development, and we don't even have releases yet. So there's no notion of breaking changes 😉

[dmathieu]

Sorry for the back and forth.
But this condition doesn't seem valid. It only checks if there is no error and we expect one.
What if there is an error and we don't expect any?

Suggested change

	// Handle nil errors
	if err != nil || tt.wantError != nil {
	require.ErrorAs(t, err, &tt.wantError)
	}
	if tt.wantError == nil {
	require.NoError(t, err)
	} else {
	require.ErrorAs(t, err, &tt.wantError)
	}

[ogaca-dd]

What if there is an error and we don't expect any?

if there is an error err != nil is true and so require.ErrorAs(t, err, &tt.wantError) is run.

[rogercoll]

I don't think this function is intended to be called by component authors, as it is being called for each component during collector setup: https://github.com/open-telemetry/opentelemetry-collector/blob/v0.137.0/otelcol/config.go#L49

Indeed, the Validate call was removed from all collector contrib components during open-telemetry/opentelemetry-collector-contrib#33498

Wdyt of adding the component.Config interface type assertion for the receiver's Config structure and test the validation with component.ValidateConfig instead? (example https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/35199/files)

[ogaca-dd]

@rogercoll : This is an interesting idea.
Note that collector.Config doesn't have a Validate method but controller.Config does (controller.Config is internal).

I can think of two solutions to add a Validate method to controller.Config:

1. Duplicating the checks in controller.Config
2. Embeds collector.Config inside controller.Config (This solution is described in Alternative implementation in the description of this PR).

I am open to implement any of these solutions (Or another one), but as there were quite a lot of back and forth, I would like every reviewers to be agreed before starting the implementation. Any preferences between 1. or 2. or another implementation ?

cc @dmathieu

[dmathieu]

I think following what the core collector components are doing is always better.

[ogaca-dd]

@dmathieu Any preference between solution 1. or 2. ?

[dmathieu]

I would go with option 2 to avoid copying things even more.
But I'm also starting to think we should do a broader rethinking of how the config is handled within the repository. This is quite messy.

[ogaca-dd]

As none of the code in this PR will be keep, close this PR in favor of #859