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.

Sign up for GitHub

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

Jump to bottom

fix: add basic validation in otlphttpexporter #4860

Merged
merged 5 commits into from
Feb 16, 2022
Merged

fix: add basic validation in otlphttpexporter #4860

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19df779
fix: add basic validation in otlphttpexporter
anupamdalmia10 Feb 14, 2022
b4c1a03
chore: add to changelog
anupamdalmia10 Feb 14, 2022
e7565e6
Merge branch 'main' into add-basic-validations-to-otlphttpexporter
Feb 14, 2022
13a44ad
fix: split testfile into good and bad config
anupamdalmia10 Feb 15, 2022
1c561bf
Merge branch 'main' into add-basic-validations-to-otlphttpexporter
bogdandrutu Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

- Reject invalid queue size exporterhelper (#4799)
- Transform configmapprovider.Retrieved interface to a struct (#4789)
- Add validation to check at least one endpoint is specified in otlphttpexporter's configuration (#4860)

## 🚩 Deprecations 🚩

Expand Down
5 changes: 5 additions & 0 deletions exporter/otlphttpexporter/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package otlphttpexporter // import "go.opentelemetry.io/collector/exporter/otlphttpexporter"

import (
"fmt"

"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/confighttp"
"go.opentelemetry.io/collector/exporter/exporterhelper"
Expand All @@ -41,5 +43,8 @@ var _ config.Exporter = (*Config)(nil)

// Validate checks if the exporter configuration is valid
func (cfg *Config) Validate() error {
if cfg.Endpoint == "" && cfg.TracesEndpoint == "" && cfg.MetricsEndpoint == "" && cfg.LogsEndpoint == "" {
return fmt.Errorf("at least one endpoint must be specified")
}
return nil
}
2 changes: 1 addition & 1 deletion exporter/otlphttpexporter/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestLoadConfig(t *testing.T) {
factories.Exporters[typeStr] = factory
cfg, err := servicetest.LoadConfigAndValidate(filepath.Join("testdata", "config.yaml"), factories)

require.NoError(t, err)
require.Error(t, err)
require.NotNil(t, cfg)
Copy link
Member

Choose a reason for hiding this comment

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

Strange to check for error and not nil.

Should we have also a successful test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that does look inappropriate.
To incorporate test case for the changes, shall we split up the config.yaml in testdata with one file containing blank otlphttpexporter configuration and the other containing the config as in current config.yaml's otlphttp/2?

Copy link
Member

Choose a reason for hiding this comment

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

I think is ok to have 2 files one ok and one with an error.

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 have split up the config into good and bad config files and updated the test code accordingly.


e0 := cfg.Exporters[config.NewComponentID(typeStr)]
Expand Down