-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
httpprovider for Collector: load configuration from config files in HTTP servers #5876
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5876 +/- ##
==========================================
- Coverage 91.46% 91.45% -0.01%
==========================================
Files 195 196 +1
Lines 11915 11940 +25
==========================================
+ Hits 10898 10920 +22
- Misses 807 809 +2
- Partials 210 211 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Please update the PR title and commit message accordingly, even though you may upstream this, we need to use nicer commit messages
func TestEmptyURI(t *testing.T) { | ||
fp := NewTestProvider() | ||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.WriteHeader(404) |
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.
Return 200 if this is "empty"
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.
Hi Bogdan, I am still a little bit confused. I think it should return 400 instead? The case is the HTTP URI is empty, not the content of config file is empty?
…r and replace with New; Minor tweaks for Empty URI and Non-exist cases; Added one more test for shutdowned server
// Examples: | ||
// `http://localhost:3333/getConfig` - (unix, windows) | ||
func New() confmap.Provider { | ||
return &provider{client: http.Client{}} |
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.
Given this can't be controlled by the user, would it be sensible to set a default timeout value as opposed to the client's default value of 0?
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 we should do that in the context we pass to "Retrieve" from the service package. So that we have it across all the providers?
Description:
An implementation of ConfigMapProvider for HTTP (httpprovider) allows OTEL Collector the ability to load configuration for itself by fetching and reading config files stored in HTTP servers.
What I changed: added one more new provider implementation folder under confmap/provider/, also added httpprovider.New() in the initialization of a default ConfigProviderSettings.
Link to tracking Issue:
#5810
Testing:
First, I mocked how a HTTP client works while interacting with a HTTP server.
Second, I designed and passed all the unit tests for edge cases such as empty fields, invalid config files, non-exist config files and so on.
Third, I did integration tests such as:
Documentation:
Added a README.md
Sponsor:
@Aneurysm9