sidecar: allow custom http con pool size fix #1953 #1955
Conversation
blockloop
force-pushed
the
master
branch
6 times, most recently
from
fb70390
to
c31e774
Compare
CHANGELOG.md
Outdated
| @@ -13,6 +13,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
|
|
|||
| ### Fixed | |||
|
|
|||
| - [#1955](https://github.com/thanos-io/thanos/pull/1955) Sidecar: enable command line flag to set http connection pool size `--prometheus.connection-pool-size` | |||
There was a problem hiding this comment.
It's rather Added I think. Also please update #Unreleased section instead as v0.10.0 is cut today.
Also:
| - [#1955](https://github.com/thanos-io/thanos/pull/1955) Sidecar: enable command line flag to set http connection pool size `--prometheus.connection-pool-size` | |
| - [#1955](https://github.com/thanos-io/thanos/pull/1955) Sidecar: Add command line flag to set http connection pool size `--prometheus.connection-pool-size` |
cmd/thanos/sidecar.go
Outdated
| @@ -45,6 +45,9 @@ func registerSidecar(m map[string]setupFunc, app *kingpin.Application) { | |||
| promReadyTimeout := cmd.Flag("prometheus.ready_timeout", "Maximum time to wait for the Prometheus instance to start up"). | |||
| Default("10m").Duration() | |||
|
|
|||
| connectionPoolSize := cmd.Flag("prometheus.connection-pool-size", "sets the MaxIdleConnsPerHost and MaxIdleConns when connecting to prometheus"). | |||
There was a problem hiding this comment.
I would split it then to 2 separate flags. I think there are many use cases where those two should have different values allowing more than one spammy client.
pkg/store/prometheus.go
Outdated
| ) (*PrometheusStore, error) { | ||
| if logger == nil { | ||
| logger = log.NewNopLogger() | ||
| } | ||
| if client == nil { | ||
| t := http.DefaultTransport.(*http.Transport) |
There was a problem hiding this comment.
This is a common bug and let's avoid this. This will actually change the value for all clients used by this binary that derives from http.DefaultTransport
I would add some helper method in some Thanos package like exthttp that will literally create NewTransport:
func NewTransport() *http.Transport {
return &Transport{
Proxy: ProxyFromEnvironment,
DialContext: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
DualStack: true,
}).DialContext,
ForceAttemptHTTP2: true,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
}
}.. ideally let's also pass *http.Transport in constructor of PrometheusStore for extensibility (:
|
Hey @bwplotka I'll make these changes and push another PR. This was from the master branch of my fork |
|
Sure, hope you can address my comments there (: |
|
Yep will do. I already separated the two flags in the PR I made to receive. I was going to do that here also. I'll send up another PR shortly. |
|
reopened as #1969 |
closed and reopened as #1969