-
Notifications
You must be signed in to change notification settings - Fork 2.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
[exporter/signalfx] Update code to use HTTPClientSettings #6803
[exporter/signalfx] Update code to use HTTPClientSettings #6803
Conversation
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 worry that this is some unrelated changes going in and would make it harder to debug tests in future.
b96dfff
to
16a7ca0
Compare
16a7ca0
to
41214f0
Compare
72bf05c
to
49e4fb8
Compare
49e4fb8
to
1db0c5e
Compare
{ | ||
name: "negative_duration", | ||
config: &Config{ | ||
ExporterSettings: config.NewExporterSettings(config.NewComponentID(typeStr)), | ||
AccessToken: "testToken", | ||
Realm: "lab", | ||
TimeoutSettings: exporterhelper.TimeoutSettings{Timeout: -2 * time.Second}, | ||
ExporterSettings: config.NewExporterSettings(config.NewComponentID(typeStr)), | ||
AccessToken: "testToken", | ||
Realm: "lab", | ||
HTTPClientSettings: confighttp.HTTPClientSettings{Timeout: -2 * time.Second}, | ||
}, | ||
errorMessage: "failed to process \"signalfx\" config: cannot have a negative \"timeout\"", |
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.
Can you also include a test for zero duration?
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.
This unit test is for testing failure, but as per current implementation for zero value we will not get an error and therefore we think we should not add it in this test. As the scope of this PR was to refactor the code to use HTTPConfigSettings, we haven’t changed the main implementation logic of this.
@dmitryax Please share your thoughts.
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.
A zero value is an interesting case since if directly setting the raw http client mean that the server must immediately return a result (which is physically possible) so it should be considered 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.
Please address comment from @MovieStoreGuy, otherwise LGTM
1db0c5e
to
e3582d3
Compare
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@manang-splunk , are you still working on this PR? |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Signed-off-by: Bogdan Drutu <[email protected]> Signed-off-by: Bogdan Drutu <[email protected]> Signed-off-by: Bogdan Drutu <[email protected]>
Description: Updated the signalfx exporter(metrics and logs) to use HTTPClientSettings to create HttpClient.
Testing: Updated the test cases as per new changes