query: Add store.response-timeout

[povilasv]

Changes

Improves handling of timed out data/
Adds --store.response-timeout in order to have a timeout on a single stream operation.

Some times GRPC streams get stuck and don't send any data for long periods of time
E.G. Simple GRPC Store API, which just sleeps.

Right now the only option to time them out in GRPC is to set context.WithTimeout()
The issue is that it is a global timeout,
if we set it to 10s, we won't ever get results from Thanos Store (backed by object store)
if we set it to 120s, we will always be waiting 120s for the HTTP queries to finish, which is :/
As we will be waiting for the slowest one to finish

This fix adds a timeout per single data receive operation

Ref: #928

Thanos Query will log if timeout was reached:

thanos-query-66f5864584-kh6wp thanos-query level=warn ts=2019-03-19T11:12:41.659624416Z caller=proxy.go:373 err="failed to receive any data in 500ms from Addr: prometheus-0.thanos-sidecar.sys-mon:10901 Labels: [{cloud_provider aws {} [] 0} {kubernetes_cluster dev-aws {} [] 0} {monitor sys-mon-prometheus {} [] 0} {replica sys-mon-prometheus-0 {} [] 0} {uw_environment dev {} [] 0}] Mint: 1552824000000 Maxt: 9223372036854775807:: context deadline exceeded" msg="returning partial response"
thanos-query-66f5864584-kh6wp thanos-query level=warn ts=2019-03-19T11:13:50.291321886Z caller=proxy.go:373 err="failed to receive any data in 500ms from Addr: thanos-store.telecom:10901 Labels: [] Mint: 1535673600000 Maxt: 1552989600000:: context deadline exceeded" msg="returning partial response"
thanos-query-66f5864584-kh6wp thanos-query level=warn ts=2019-03-19T11:15:37.430210112Z caller=proxy.go:373 err="failed to receive any data in 500ms from Addr: thanos-store.customer-platform:10901 Labels: [] Mint: 1549807838621 Maxt: 1552989600000:: context deadline exceeded" msg="returning partial response"

Verification

Tested locally on our cluster, both turned on and turned off (default).

Unit tests.

[bwplotka]

Generally I am fine with that (added suggestions), but curious what @devnev and @mjd95 think about this. Can we have your opinions?

[mjd95]

👍 looks like a sensible mitigation. Would be great to look in to the underlying store issue as follow-up.

[bwplotka]

LGTM just:

• flag name suggestion
• CI test suggestion

[povilasv]

@bwplotka I've introduced THANOS_ENABLE_STORE_READ_TIMEOUT_TESTS ENV to run timeout tests. Maybe you could introduce somekind of a circle config which would run this once a day with that environment enabled? I would like to know if somebody introduces any regressions on this timeout.

https://github.com/improbable-eng/thanos/pull/928/files#diff-e7ba964cd2afa54767a2adc1bac73e67R438

[bwplotka]

All good and I would be happy to merge if not the confusion around gRPC metric we add here. See my comment.

[bwplotka]

Also I am happy to add this periodic test in CircleCi for this env, can you add Github issue on me for that?

[povilasv]

Also I am happy to add this periodic test in CircleCi for this env, can you add Github issue on me for that?

@bwplotka #960

[bwplotka]

Still before merging we need to remove the metrics from here I guess?

[povilasv]

Still before merging we need to remove the metrics from here I guess?

Yup, also made a PR for metrics grpc-ecosystem/go-grpc-prometheus#71

[povilasv]

@bwplotka Do you want me to squash commits before merge? :)

I've squashed it

[bwplotka]

Nice, just one minor nit, not a blocker though. LGTM, thanks!