add `validatable` interface to all the component config #2898

mxiamxia · 2021-04-07T05:37:41Z

Description:
Extend previous PR - #2856. Add validatable interface to config.processor | extension | exporter. And add the config validation in config.Validate()

Next
Implement the empty Validate() func in component Config. For at least the core component we want to release stable.

Link to tracking Issue:
#2541

codecov · 2021-04-07T05:44:52Z

Codecov Report

Merging #2898 (b8d0446) into main (c5ddd09) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2898      +/-   ##
==========================================
+ Coverage   91.74%   91.76%   +0.01%     
==========================================
  Files         286      306      +20     
  Lines       15086    15116      +30     
==========================================
+ Hits        13841    13871      +30     
  Misses        851      851              
  Partials      394      394

Impacted Files	Coverage Δ
config/config.go	`100.00% <100.00%> (ø)`
config/exporter.go	`100.00% <100.00%> (ø)`
config/extension.go	`100.00% <100.00%> (ø)`
config/processor.go	`100.00% <100.00%> (ø)`
exporter/fileexporter/config.go	`100.00% <100.00%> (ø)`
exporter/jaegerexporter/config.go	`100.00% <100.00%> (ø)`
exporter/kafkaexporter/config.go	`100.00% <100.00%> (ø)`
exporter/loggingexporter/config.go	`100.00% <100.00%> (ø)`
exporter/opencensusexporter/config.go	`100.00% <100.00%> (ø)`
exporter/otlpexporter/config.go	`100.00% <100.00%> (ø)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5ddd09...b8d0446. Read the comment docs.

mx-psi

I think there are opportunities for moving existing code into the validation functions to test that the interface makes sense and tidy up the code. I checked just the exporters and found some examples that we can move and left them as inline comments. Not sure if we want to do all of this on this PR or not but I wanted to point it out so that we at least track it somehow.

mx-psi · 2021-04-07T17:06:04Z

exporter/jaegerexporter/config.go

+var _ config.Exporter = (*Config)(nil)
+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {


We could move

opentelemetry-collector/exporter/jaegerexporter/factory.go

Lines 60 to 66 in 4a6a25e

if expCfg.Endpoint == "" {

// TODO: Improve error message, see #215

err := fmt.Errorf(

"%q config requires a non-empty \"endpoint\"",

expCfg.Name())

return nil, err

}

into the validation function.

mx-psi · 2021-04-07T17:10:13Z

exporter/loggingexporter/config.go

+func (cfg *Config) Validate() error {
+	return nil
+}


Maybe checking if the logging level is valid here? We do something similar on the factory function:

opentelemetry-collector/exporter/loggingexporter/factory.go

Lines 89 to 92 in 4a6a25e

err := (&level).UnmarshalText([]byte(cfg.LogLevel))

if err != nil {

return nil, err

}

mx-psi · 2021-04-07T17:11:05Z

exporter/opencensusexporter/config.go

+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {
+	return nil


Move these here

opentelemetry-collector/exporter/opencensusexporter/opencensus.go

Lines 61 to 68 in c5ddd09

if cfg.Endpoint == "" {

return nil, errors.New("OpenCensus exporter cfg requires an Endpoint")

}

if cfg.NumWorkers <= 0 {

return nil, errors.New("OpenCensus exporter cfg requires at least one worker")

}

mx-psi · 2021-04-07T17:12:12Z

exporter/otlpexporter/config.go

+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {
+	return nil


Check for endpoint here

opentelemetry-collector/exporter/otlpexporter/otlp.go

Lines 50 to 52 in c5ddd09

if oCfg.Endpoint == "" {

return nil, errors.New("OTLP exporter config requires an Endpoint")

}

mx-psi · 2021-04-07T17:12:56Z

exporter/otlphttpexporter/config.go

+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {
+	return nil


Check endpoint here

opentelemetry-collector/exporter/otlphttpexporter/otlp.go

Lines 61 to 66 in c5ddd09

if oCfg.Endpoint != "" {

_, err := url.Parse(oCfg.Endpoint)

if err != nil {

return nil, errors.New("endpoint must be a valid URL")

}

}

mx-psi · 2021-04-07T17:13:44Z

exporter/prometheusexporter/config.go

+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {
+	return nil


Check endpoint here

opentelemetry-collector/exporter/prometheusexporter/prometheus.go

Lines 47 to 50 in c5ddd09

addr := strings.TrimSpace(config.Endpoint)

if strings.TrimSpace(config.Endpoint) == "" {

return nil, errBlankPrometheusAddress

}

mx-psi · 2021-04-07T17:15:22Z

exporter/zipkinexporter/config.go

+
+// Validate checks if the exporter configuration is valid
+func (cfg *Config) Validate() error {
+	return nil


Check endpoint here

opentelemetry-collector/exporter/zipkinexporter/factory.go

Lines 72 to 75 in c5ddd09

if zc.Endpoint == "" {

// TODO https://github.com/open-telemetry/opentelemetry-collector/issues/215

return nil, errors.New("exporter config requires a non-empty 'endpoint'")

}

mxiamxia · 2021-04-08T00:11:29Z

I think there are opportunities for moving existing code into the validation functions to test that the interface makes sense and tidy up the code. I checked just the exporters and found some examples that we can move and left them as inline comments. Not sure if we want to do all of this on this PR or not but I wanted to point it out so that we at least track it somehow.

Thanks for commenting. I will create a separate PR to move those common sense config validations into its Validate() func.

…fig.Validate from core can get compiled (#3020) As we're adding `validatable` interface to `config.processor | extension | exporter` in core package. `Contrib` package is failed to be built due to the following errors. This PR is to fix the ambiguous Config.Validate errors. ``` exporter/awsemfexporter/factory.go:40:9: cannot use &Config literal (type *Config) as type config.Exporter in return argument: *Config does not implement config.Exporter (wrong type for Validate method) have Validate() want Validate() error extension/observer/k8sobserver/factory.go:64:15: Config.Validate is ambiguous ``` **Link to tracking Issue:** open-telemetry/opentelemetry-collector#2898

…fig.Validate from core can get compiled (open-telemetry#3020) As we're adding `validatable` interface to `config.processor | extension | exporter` in core package. `Contrib` package is failed to be built due to the following errors. This PR is to fix the ambiguous Config.Validate errors. ``` exporter/awsemfexporter/factory.go:40:9: cannot use &Config literal (type *Config) as type config.Exporter in return argument: *Config does not implement config.Exporter (wrong type for Validate method) have Validate() want Validate() error extension/observer/k8sobserver/factory.go:64:15: Config.Validate is ambiguous ``` **Link to tracking Issue:** open-telemetry/opentelemetry-collector#2898

mxiamxia requested a review from a team April 7, 2021 05:37

mxiamxia force-pushed the comp-validate branch 4 times, most recently from b30c848 to 4a6a25e Compare April 7, 2021 06:19

mx-psi reviewed Apr 7, 2021

View reviewed changes

mxiamxia mentioned this pull request Apr 8, 2021

Fix ambiguous Config.Validate errors open-telemetry/opentelemetry-collector-contrib#3020

Merged

mx-psi approved these changes Apr 8, 2021

View reviewed changes

mxiamxia force-pushed the comp-validate branch from 4a6a25e to 7bb786d Compare April 8, 2021 19:51

Add validatable interface to all the component config

b8d0446

mxiamxia force-pushed the comp-validate branch from 7bb786d to b8d0446 Compare April 9, 2021 01:46

bogdandrutu approved these changes Apr 12, 2021

View reviewed changes

bogdandrutu merged commit 1ca65c8 into open-telemetry:main Apr 12, 2021

mx-psi mentioned this pull request Apr 27, 2021

[exporter/datadog]: add ignore_resources configuration option open-telemetry/opentelemetry-collector-contrib#3245

Merged

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Bump github.com/opencontainers/runc to v1.1.5 (open-telemetry#2898)

63e1db5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `validatable` interface to all the component config #2898

add `validatable` interface to all the component config #2898

mxiamxia commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading

mx-psi left a comment

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mx-psi Apr 7, 2021

mxiamxia commented Apr 8, 2021

	if expCfg.Endpoint == "" {
	// TODO: Improve error message, see #215
	err := fmt.Errorf(
	"%q config requires a non-empty \"endpoint\"",
	expCfg.Name())
	return nil, err
	}

	err := (&level).UnmarshalText([]byte(cfg.LogLevel))
	if err != nil {
	return nil, err
	}

	if cfg.Endpoint == "" {
	return nil, errors.New("OpenCensus exporter cfg requires an Endpoint")
	}

	if cfg.NumWorkers <= 0 {
	return nil, errors.New("OpenCensus exporter cfg requires at least one worker")
	}

	if oCfg.Endpoint == "" {
	return nil, errors.New("OTLP exporter config requires an Endpoint")
	}

	if oCfg.Endpoint != "" {
	_, err := url.Parse(oCfg.Endpoint)
	if err != nil {
	return nil, errors.New("endpoint must be a valid URL")
	}
	}

	addr := strings.TrimSpace(config.Endpoint)
	if strings.TrimSpace(config.Endpoint) == "" {
	return nil, errBlankPrometheusAddress
	}

	if zc.Endpoint == "" {
	// TODO https://github.com/open-telemetry/opentelemetry-collector/issues/215
	return nil, errors.New("exporter config requires a non-empty 'endpoint'")
	}

add validatable interface to all the component config #2898

add validatable interface to all the component config #2898

Conversation

mxiamxia commented Apr 7, 2021 • edited Loading

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mx-psi Apr 7, 2021

Choose a reason for hiding this comment

mxiamxia commented Apr 8, 2021

add `validatable` interface to all the component config #2898

add `validatable` interface to all the component config #2898

mxiamxia commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading