Adapt queryTimeout to be a per-tenant configuration #6835

DylanGuedes · 2022-08-03T17:35:15Z

What this PR does / why we need it:

Adds context timeout to metadata calls based on the new per-tenant
query timeout
Add span logs to calls missing it

The motivation to change this configuration to be per-tenant instead of global is that it makes sense to alleviate the timeout for particular tenants or make it more strict for others. Especially useful for scenarios where the Loki client doesn't handle context cancellations correctly; in such scenarios, since Loki would still process expensive queries that were canceled, this allows one to have small timeouts for most tenants, which will help mitigate unnecessary work without having a timeout that is too short for important tenants.

Which issue(s) this PR fixes:
N/A

- Adds context timeout to metadata calls based on the new per-tenant query timeout - Add span logs to calls missing it

grafanabot · 2022-08-03T17:39:51Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-2%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

- Use per-tenant query timeout everywhere instead of the old engine timeout configuration

grafanabot · 2022-08-03T23:43:16Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-2%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

DylanGuedes · 2022-08-04T10:03:24Z

Still missing updating the docs and testing a few things, but I'm moving it to ready in case someone is willing to review it/help me finding problems with my approach here 😄

pkg/logcli/client/file.go

kavirajk · 2022-08-08T07:43:19Z

pkg/logql/engine.go

@@ -226,7 +219,14 @@ func (q *query) Exec(ctx context.Context) (logqlmodel.Result, error) {
 }

 func (q *query) Eval(ctx context.Context) (promql_parser.Value, error) {
-	ctx, cancel := context.WithTimeout(ctx, q.timeout)
+	queryTimeout := time.Minute * 2


I think better we move this to package level const? (e.g: DefaultQueryTimeout). And Q: what's the rationale behind 2 minutes as default? (Previously query engine was using 5 * minute as default timeout. Is it to standardize it? then we may need to add it in upgrade guide as well I think)

hmm where did you find that the engine had 5 minutes default timeout?

kavirajk · 2022-08-08T07:45:45Z

pkg/querier/http.go

@@ -376,14 +408,23 @@ func (q *QuerierAPI) TailHandler(w http.ResponseWriter, r *http.Request) {
 // SeriesHandler returns the list of time series that match a certain label set.
 // See https://prometheus.io/docs/prometheus/latest/querying/api/#finding-series-by-label-matchers
 func (q *QuerierAPI) SeriesHandler(w http.ResponseWriter, r *http.Request) {
+	log, ctx := spanlogger.New(r.Context(), "query.Series")


Seeing it is being repeated for almost all the handler, should we make this as middleware?

hmm I think it shouldn't, as some handlers won't want this (ex: the tail handler)

I definitely agree about the middleware; we can apply the middleware selectively - which is already done in pkg/loki/modules.go:

queryHandlers := map[string]http.Handler{ "/loki/api/v1/query_range": httpMiddleware.Wrap(http.HandlerFunc(t.querierAPI.RangeQueryHandler)), "/loki/api/v1/query": httpMiddleware.Wrap(http.HandlerFunc(t.querierAPI.InstantQueryHandler)), "/loki/api/v1/label": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/labels": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/label/{name}/values": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/series": http.HandlerFunc(t.querierAPI.SeriesHandler), "/loki/api/v1/index/stats": http.HandlerFunc(t.querierAPI.IndexStatsHandler),

…timeout

grafanabot · 2022-08-15T18:04:24Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-1.9%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-08-15T18:39:04Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-1.9%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-08-15T19:05:05Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-1.9%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

pkg/logql/engine.go

pkg/querier/http.go

dannykopping · 2022-08-24T06:46:16Z

pkg/querier/http.go

@@ -376,14 +408,23 @@ func (q *QuerierAPI) TailHandler(w http.ResponseWriter, r *http.Request) {
 // SeriesHandler returns the list of time series that match a certain label set.
 // See https://prometheus.io/docs/prometheus/latest/querying/api/#finding-series-by-label-matchers
 func (q *QuerierAPI) SeriesHandler(w http.ResponseWriter, r *http.Request) {
+	log, ctx := spanlogger.New(r.Context(), "query.Series")


I definitely agree about the middleware; we can apply the middleware selectively - which is already done in pkg/loki/modules.go:

queryHandlers := map[string]http.Handler{ "/loki/api/v1/query_range": httpMiddleware.Wrap(http.HandlerFunc(t.querierAPI.RangeQueryHandler)), "/loki/api/v1/query": httpMiddleware.Wrap(http.HandlerFunc(t.querierAPI.InstantQueryHandler)), "/loki/api/v1/label": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/labels": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/label/{name}/values": http.HandlerFunc(t.querierAPI.LabelHandler), "/loki/api/v1/series": http.HandlerFunc(t.querierAPI.SeriesHandler), "/loki/api/v1/index/stats": http.HandlerFunc(t.querierAPI.IndexStatsHandler),

pkg/querier/querier.go

grafanabot · 2022-08-24T14:58:53Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-0.2%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Use timeout variable instead of hardcoded value. Co-authored-by: Danny Kopping <dannykopping@gmail.com>

grafanabot · 2022-08-24T16:08:21Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-0.2%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

- Interrupt requests when tenantID couldn' be fetch.

grafanabot · 2022-08-24T19:30:17Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-0.4%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

dannykopping

LGTM!

docs/sources/configuration/_index.md

pkg/querier/querier.go

Improve deprecation notice. Co-authored-by: Danny Kopping <dannykopping@gmail.com>

grafanabot · 2022-08-24T21:05:20Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
-            querier	-0.4%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

What this PR does / why we need it: On PR #6835 I mistakenly removed the querier:query_timeout YAML configuration. This PR returns it and uses it if it is still configured while logging a warning message to alert users of the upcoming changes.

What this PR does / why we need it: * Add a new per-tenant query timeout * Adds new middleware to query calls. This new middleware will timeout requests based on the new per-tenant query timeout * Add span logs to query calls missing it * Deprecate `engine.timeout` configuration The motivation to change this configuration to be per-tenant instead of global is that it makes sense to alleviate the timeout for particular tenants or make it more strict for others. Especially useful for scenarios where the Loki client doesn't handle context cancellations correctly; in such scenarios, since Loki would still process expensive queries that were canceled, this allows one to have small timeouts for most tenants, which will help mitigate unnecessary work without having a timeout that is too short for important tenants.

What this PR does / why we need it: On PR grafana#6835 I mistakenly removed the querier:query_timeout YAML configuration. This PR returns it and uses it if it is still configured while logging a warning message to alert users of the upcoming changes.

Change queryTimeout to be a per-tenant configuration.

7c395b5

- Adds context timeout to metadata calls based on the new per-tenant query timeout - Add span logs to calls missing it

pull-request-size bot added the size/L label Aug 3, 2022

Get rid of engine timeout configuration.

80c4f15

- Use per-tenant query timeout everywhere instead of the old engine timeout configuration

DylanGuedes force-pushed the per-tenant-query-timeout branch from 8da2d32 to 80c4f15 Compare August 3, 2022 22:04

If ctx doesn't has org id, use default value.

b7c01ca

DylanGuedes marked this pull request as ready for review August 4, 2022 10:03

DylanGuedes requested a review from a team as a code owner August 4, 2022 10:03

kavirajk reviewed Aug 8, 2022

View reviewed changes

Merge branch 'main' of github.com:grafana/loki into per-tenant-query-…

f10a910

…timeout

Change logcli client to have 5minute timeout.

92a31ec

Remove unused code.

f3f46ab

dannykopping reviewed Aug 24, 2022

View reviewed changes

DylanGuedes added 3 commits August 24, 2022 10:39

Implement new middleware and uses it on query calls.

9376341

Fix lint.

59219f7

Other lint fix.

d891f09

Apply suggestions from code review

b4561b9

Use timeout variable instead of hardcoded value. Co-authored-by: Danny Kopping <dannykopping@gmail.com>

DylanGuedes added 2 commits August 24, 2022 15:30

Return flags as deprecated and interrupt requests.

5f9d8d3

- Interrupt requests when tenantID couldn' be fetch.

Add deprecation notices.

2e4d9a5

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Aug 24, 2022

dannykopping approved these changes Aug 24, 2022

View reviewed changes

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

pkg/querier/querier.go Show resolved Hide resolved

Update docs/sources/configuration/_index.md

6e10425

Improve deprecation notice. Co-authored-by: Danny Kopping <dannykopping@gmail.com>

DylanGuedes merged commit 15f8f42 into grafana:main Aug 24, 2022

dannykopping mentioned this pull request Aug 25, 2022

Return Querier/QueryTimeout YAML configuration #6972

Merged

jphx mentioned this pull request Jan 11, 2023

Update Loki configuration doc querier.query_timeout -> limits_config.query_timeout #8090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt queryTimeout to be a per-tenant configuration #6835

Adapt queryTimeout to be a per-tenant configuration #6835

DylanGuedes commented Aug 3, 2022 •

edited

Loading

grafanabot commented Aug 3, 2022

grafanabot commented Aug 3, 2022

DylanGuedes commented Aug 4, 2022

kavirajk Aug 8, 2022

DylanGuedes Aug 15, 2022

kavirajk Aug 8, 2022

DylanGuedes Aug 15, 2022

dannykopping Aug 24, 2022

grafanabot commented Aug 15, 2022

grafanabot commented Aug 15, 2022

grafanabot commented Aug 15, 2022

dannykopping Aug 24, 2022

grafanabot commented Aug 24, 2022

grafanabot commented Aug 24, 2022

grafanabot commented Aug 24, 2022

dannykopping left a comment

grafanabot commented Aug 24, 2022

Adapt queryTimeout to be a per-tenant configuration #6835

Adapt queryTimeout to be a per-tenant configuration #6835

Conversation

DylanGuedes commented Aug 3, 2022 • edited Loading

grafanabot commented Aug 3, 2022

grafanabot commented Aug 3, 2022

DylanGuedes commented Aug 4, 2022

kavirajk Aug 8, 2022

Choose a reason for hiding this comment

DylanGuedes Aug 15, 2022

Choose a reason for hiding this comment

kavirajk Aug 8, 2022

Choose a reason for hiding this comment

DylanGuedes Aug 15, 2022

Choose a reason for hiding this comment

dannykopping Aug 24, 2022

Choose a reason for hiding this comment

grafanabot commented Aug 15, 2022

grafanabot commented Aug 15, 2022

grafanabot commented Aug 15, 2022

dannykopping Aug 24, 2022

Choose a reason for hiding this comment

grafanabot commented Aug 24, 2022

grafanabot commented Aug 24, 2022

grafanabot commented Aug 24, 2022

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Aug 24, 2022

DylanGuedes commented Aug 3, 2022 •

edited

Loading