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

Use http.Client instead of http.Get #1133

Closed
aman-bansal opened this issue Sep 10, 2020 · 7 comments
Closed

Use http.Client instead of http.Get #1133

aman-bansal opened this issue Sep 10, 2020 · 7 comments
Labels
bug Something isn't working Hacktoberfest help wanted Looking for support from community stale All issues that are marked as stale due to inactivity

Comments

@aman-bansal
Copy link
Contributor

At many places, HTTP requests have been implemented using default HTTP client like r, err := http.Get(s.metadata.url). As per HTTP package, default client is defined like this var DefaultClient = &Client{}. This has huge drawbacks as it doesn't allow the application to define timeout behaviors (among other things). The default value of timeout is 0 which means no timeout. In production environments, this could prove costly. The more intuitive way would be to create your own client like

client := &http.Client{
	Timeout: x * time.Second, // x seconds timeout
}

and http.NewRequest() to create new requests.

@aman-bansal aman-bansal added the bug Something isn't working label Sep 10, 2020
@turbaszek
Copy link
Contributor

@aman-bansal do you think we should allow users to specify such parameters on a case by case basis or should we introduce good-enough default?

@aman-bansal
Copy link
Contributor Author

There are two ways to look at this:

  1. In cases of client-controlled logic like metricsAPIScaler, it makes more sense to define timeouts on a case by case basis based on their application logic, scale, and use cases.
  2. For other scalers provided by Identity Providers like AWS, AZURE, it makes more sense to define a good enough default.

What I think is we should define one parameter in metadata for each scaler (where timeouts make sense) with good enough default value.

@arschles
Copy link
Contributor

arschles commented Oct 13, 2020

I agree with both of those points @aman-bansal, but I would add that we should allow users to set a global timeout should they desire.

I would like to work on this. I'll send a draft PR as a start.

arschles added a commit to arschles/keda that referenced this issue Oct 13, 2020
arschles added a commit to arschles/keda that referenced this issue Oct 21, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Oct 26, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>

reading timeout from env var, and storing HTTP client in ScalerConfig

Signed-off-by: Aaron Schlesinger <[email protected]>

fixing undeclared name client

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Oct 26, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>

reading timeout from env var, and storing HTTP client in ScalerConfig

Signed-off-by: Aaron Schlesinger <[email protected]>

fixing undeclared name client

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Dec 1, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>

reading timeout from env var, and storing HTTP client in ScalerConfig

Signed-off-by: Aaron Schlesinger <[email protected]>

fixing undeclared name client

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Dec 1, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>

reading timeout from env var, and storing HTTP client in ScalerConfig

Signed-off-by: Aaron Schlesinger <[email protected]>

fixing undeclared name client

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Dec 1, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>

reading timeout from env var, and storing HTTP client in ScalerConfig

Signed-off-by: Aaron Schlesinger <[email protected]>

fixing undeclared name client

Signed-off-by: Aaron Schlesinger <[email protected]>
arschles added a commit to arschles/keda that referenced this issue Dec 4, 2020
fixes kedacore#1133

Signed-off-by: Aaron Schlesinger <[email protected]>
@tomkerkhove
Copy link
Member

Created kedacore/charts#109 to surface this in our Helm chart as well.

@arschles
Copy link
Contributor

@aman-bansal after #1758, I don't see anywhere else that uses raw http.Get or http.Client{} values without timeouts. Do you see anywhere I'm missing?

@tomkerkhove tomkerkhove added help wanted Looking for support from community Hacktoberfest labels Sep 28, 2021
@stale
Copy link

stale bot commented Nov 27, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Nov 27, 2021
@zroubalik
Copy link
Member

This has been done, we can close this issue.

SpiritZhou pushed a commit to SpiritZhou/keda that referenced this issue Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Hacktoberfest help wanted Looking for support from community stale All issues that are marked as stale due to inactivity
Projects
None yet
Development

No branches or pull requests

5 participants