-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: add basic validation in otlphttpexporter #4860
fix: add basic validation in otlphttpexporter #4860
Conversation
|
@@ -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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The settings for otlphttp exporters are validated here:
opentelemetry-collector/exporter/otlphttpexporter/factory.go
Lines 63 to 76 in 8399098
func composeSignalURL(oCfg *Config, signalOverrideURL string, signalName string) (string, error) { | |
switch { | |
case signalOverrideURL != "": | |
_, err := url.Parse(signalOverrideURL) | |
if err != nil { | |
return "", fmt.Errorf("%s_endpoint must be a valid URL", signalName) | |
} | |
return signalOverrideURL, nil | |
case oCfg.Endpoint == "": | |
return "", fmt.Errorf("either endpoint or %s_endpoint must be specified", signalName) | |
default: | |
return oCfg.Endpoint + "/v1/" + signalName, nil | |
} | |
} |
Is the idea to remove that validation as part of this change?
Codecov Report
@@ Coverage Diff @@
## main #4860 +/- ##
=======================================
Coverage 90.60% 90.60%
=======================================
Files 180 180
Lines 10622 10625 +3
=======================================
+ Hits 9624 9627 +3
Misses 779 779
Partials 219 219
Continue to review full report at Codecov.
|
@codeboten you cannot remove that, since during validation (at least for the moment) we don't have access to the information about what type of component the factory will produce. The benefit of having the validation sooner than "start" is because we can implement things like "--dry-run" to check for config (or most of the config, as much as we can). |
@bogdandrutu / @codeboten Is this good to merge? |
Description: <Describe what has changed.
Adding basic validation to check if at least one endpoint is specified in otlphttpexporter configuration
Link to tracking Issue:
Closes #4709
Testing: < Describe what testing was performed and which tests were added.>
Tested with no endpoints specified and at least one endpoint specified.