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.

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

Conversation

anupamdalmia10
Copy link
Contributor

@anupamdalmia10 anupamdalmia10 commented Feb 14, 2022

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.

@anupamdalmia10 anupamdalmia10 requested a review from a team as a code owner February 14, 2022 11:39
@project-bot project-bot bot added this to In progress in Collector Feb 14, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 14, 2022

CLA Signed

The committers are authorized under a signed CLA.

@@ -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.

Copy link
Contributor

@codeboten codeboten left a 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:

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
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #4860 (1c561bf) into main (26e62ce) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
exporter/otlphttpexporter/config.go 100.00% <100.00%> (ø)

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 26e62ce...1c561bf. Read the comment docs.

@bogdandrutu
Copy link
Member

@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).

@anupamdalmia10
Copy link
Contributor Author

@bogdandrutu / @codeboten Is this good to merge?

Collector automation moved this from In progress to Reviewer approved Feb 16, 2022
@codeboten codeboten merged commit ec3384d into open-telemetry:main Feb 16, 2022
@jpkrohling jpkrohling added this to the v0.45.0 milestone Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

No validations in otlphttpexporter
4 participants