Keda: Use http.Client instead of http.Get

Created on 10 Sep 2020  路  4Comments  路  Source: kedacore/keda

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.

bug

Most helpful comment

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.

All 4 comments

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

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.

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.

Created https://github.com/kedacore/charts/issues/109 to surface this in our Helm chart as well.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

tomkerkhove picture tomkerkhove  路  4Comments

genadyk picture genadyk  路  3Comments

turbaszek picture turbaszek  路  4Comments

cwhfa picture cwhfa  路  4Comments

TAnas0 picture TAnas0  路  5Comments