13 Commits (0bc38e5775464fa2327e2b2cd548bcbe2e46b075)

Author	SHA1	Message	Date
Karsten Jeschkies	6948c4a179	Turn frontend Tripperware into a Middleware. (#10688) ... **What this PR does / why we need it**: Currently, a request to Loki's frontend API goes through these conversions: ``` http.Request ↓ limitedRoundTripper queryrangebase.Request ↓ queryrangebase.Middlware … ↓ queryrangebase.Request ↓ limitedRoundTripper http.Request ↓ grpcRoundTripperAdapter httpgrpc ↓ grpcRoundTripperAdapter http.Response ↓ limitedRoundTripper queryrangebase.Response ↓ limitedRoundtripper http.Response ``` Since `httgrpc` and `queryrangebase.Request` are Protobufs there's no good reason to encode and decode them to HTTP responses/requests. Furthermore, the encoding to HTTP makes it harder for us to encode query plans. Thus the conversions are changed to the following: ``` http.Request ↓ queryrangebase.Request ↓ queryrangebase.Middlware … ↓ queryrangebase.Request ↓ httpgrpc ↓ queryrangebase.Response ↓ http.Response ``` In order to achieve this the `http.RoundTripper` is pushed to the outside. Only the serialization layer from `http.Request` to `queryrangebase.Request` and `http.Response` to `queryrangebase.Response` will be an `http.RoundTripper`. Everything else is either a `queryrangebase.Handler` or `queryrangebase.Middleware`. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e3ec)	3 years ago
Charles Korn	bde65667f7	Upgrade dskit to use packages migrated from weaveworks/common (#10164) ... **What this PR does / why we need it**: This PR upgrades dskit and replaces use of packages from weaveworks/common with their migrated equivalents in dskit. See https://github.com/grafana/dskit/pull/342 for more details. Note that Loki still uses some packages from weaveworks/common that I haven't migrated (`aws` and `test`) - I'll migrate these separately. If this PR needs to be rebuilt, I used `rewrite.sh` ([source](https://gist.github.com/charleskorn/48efe62a09d6d70f3de30327003df5c5#file-rewrite-sh)) to generate most of these changes. **Which issue(s) this PR fixes**: (none) **Special notes for your reviewer**: (none) **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [n/a] Documentation added - [n/a] Tests updated - [n/a] `CHANGELOG.md` updated - [n/a] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [n/a] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [n/a] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e3ec) Co-authored-by: Kaviraj Kanagaraj <kavirajkanagaraj@gmail.com>	3 years ago
Trevor Whitney	4a56445686	Upgrade `golangci-lint` and fix linting errors (#9601) ... **What this PR does / why we need it**: Upgrade `golangci-lint` and fixes all the errors. The upgrade includes some stricter linting.	3 years ago
Karsten Jeschkies	ff36227d87	Ignore broken query limits header. (#9103)	3 years ago
Karsten Jeschkies	de2b0c6e9d	Rename per-request query limits. (#9011) ... **What this PR does / why we need it**: `maxQueryTime` is more in line with the other per-request limits we have. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`	3 years ago
Ed Welch	edc6b0bff7	Loki: Add a limit for the [range] value on range queries (#8343) ... Signed-off-by: Edward Welch <edward.welch@grafana.com> **What this PR does / why we need it**: Loki does not currently split queries by time to a value smaller than what's in the [range] of a range query. Example ``` sum(rate({job="foo"}[2d])) ``` Imagine now this query being executed over a longer window of a few days with a step of something like 30m. Every step evaluation would query the last [2d] of data. There are use cases where this is desired, specifically if you force the step to match the value in the range, however what is more common is someone accidentally uses `[$__range]` in here instead of `[$__interval]` within Grafana and then sets the query time selector to a large value like 7 days. This PR adds a limit which will fail queries that set the [range] value higher than the configured limit. It's disabled by default. In the future it may be possible for Loki to perform splits within the [range] and remove the need for this limit, but until then this can be an important safeguard in clusters with a lot of data. **Which issue(s) this PR fixes**: Fixes #8746 **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` --------- Signed-off-by: Edward Welch <edward.welch@grafana.com> Co-authored-by: Karsten Jeschkies <karsten.jeschkies@grafana.com> Co-authored-by: Vladyslav Diachenko <82767850+vlad-diachenko@users.noreply.github.com>	3 years ago
Salva Corts	28a7733ede	Rename config for enforcing a minimum number of label matchers (#8940) ... **What this PR does / why we need it**: Followup PR for https://github.com/grafana/loki/pull/8918 renaming config. See https://github.com/grafana/loki/pull/8918/files#r1151820792. **Which issue(s) this PR fixes**: Fixes https://github.com/grafana/loki-private/issues/699 **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` --------- Co-authored-by: Dylan Guedes <djmgguedes@gmail.com>	3 years ago
Salva Corts	d421feafe6	Log when returning query-time limit (#8938) ... **What this PR does / why we need it**: At https://github.com/grafana/loki/pull/8727 we introduced various limits that can now be configured at query time. We always compare the value of the limit configured at query time with the value set on the overrides for the tenant or the default if not configured (aka original); applying the most restrictive one. If the most restrictive is the original value or the limit is not configured at query-time, we print the following debug message: 9e1846c47a/pkg/util/querylimits/limiter.go (L43) It will be printed many times: every time the query is not configured at query-time, as well as every time the original value is more restrictive. Moreover, this log message lacks some useful information such as the original value, the query-time value, the tenant ID and the limit name. This PR fixes this by removing the debug message above and printing a new debug message only if we return the query-time limit. This new log also prints the tenant ID, the limit name, as well as the original and the query-time limit values. **Which issue(s) this PR fixes**: Fixes https://github.com/grafana/loki/issues/8932 **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`	3 years ago
Salva Corts	45775c82f7	Implement `RequiredNumberLabels` query limit (#8918) ... **What this PR does / why we need it**: As pointed out in https://github.com/grafana/loki/pull/8851, some queries can impose a great workload on a cluster by selecting too many streams. Similarly to the `RequiredLabels` limit introduced at https://github.com/grafana/loki/pull/8851, here we add a new limit `RequiredNumberLabels` to require queries to specify at least N label. For example, if the limit is set to 2, then the query should contain at least 2 label matchers. This limit can be configured per tenant and at query time.  **Which issue(s) this PR fixes**: Fixes https://github.com/grafana/loki-private/issues/699 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` --------- Co-authored-by: Dylan Guedes <djmgguedes@gmail.com>	3 years ago
Salva Corts	d24fe3e68b	Max bytes read limit (#8670) ... **What this PR does / why we need it**: This PR implements two new per-tenant limits that are enforced on log and metric queries (both range and instant) when TSDB is used: - `max_query_bytes_read`: Refuse queries that would read more than the configured bytes here. Overall limit regardless of splitting/sharding. The goal is to refuse queries that would take too long. The default value of 0 disables this limit. - `max_querier_bytes_read`: Refuse queries in which any of their subqueries after splitting and sharding would read more than the configured bytes here. The goal is to avoid a querier from running a query that would load too much data in memory and can potentially get OOMed. The default value of 0 disables this limit. These new limits can be configured per tenant and per query (see https://github.com/grafana/loki/pull/8727). The bytes a query would read are estimated through TSDB's index stats. Even though they are not exact, they are good enough to have a rough estimation of whether a query is too big to run or not. For more details on this refer to this discussion in the PR: https://github.com/grafana/loki/pull/8670#discussion_r1124858508. Both limits are implemented in the frontend. Even though we considered implementing `max_querier_bytes_read` in the querier, this way, the limits for pre and post splitting/sharding queries are enforced close to each other on the same component. Moreover, this way we can reduce the number of index stats requests issued to the index gateways by reusing the stats gathered while sharding the query. With regard to how index stats requests are issued: - We parallelize index stats requests by splitting them into queries that span up to 24h since our indices are sharded by 24h periods. On top of that, this prevents a single index gateway from processing a single huge request like `{app=~".+"} for 30d`. - If sharding is enabled and the query is shardable, for `max_querier_bytes_read`, we re-use the stats requests issued by the sharding ware. Specifically, we look at the [bytesPerShard][1] to enforce this limit. Note that once we merge this PR and enable these limits, the load of index stats requests will increase substantially and we may discover bottlenecks in our index gateways and TSDB. After speaking with @owen-d, we think it should be fine as, if needed, we can scale up our index gateways and support caching index stats requests. Here's a demo of this working: <img width="1647" alt="image" src="https://user-images.githubusercontent.com/8354290/226918478-d4b6c2fd-de4d-478a-9c8b-e38fe148fa95.png"> <img width="1647" alt="image" src="https://user-images.githubusercontent.com/8354290/226918798-a71b1db8-ea68-4d00-933b-e5eb1524d240.png"> **Which issue(s) this PR fixes**: This PR addresses https://github.com/grafana/loki-private/issues/674. **Special notes for your reviewer**: - @jeschkies has reviewed the changes related to query-time limits. - I've done some refactoring in this PR: - Extracted logic to get stats for a set of matches into a new function [getStatsForMatchers][2]. - Extracted the _Handler_ interface implementation for [queryrangebase.roundTripper][3] into a new type [queryrangebase.roundTripperHandler][4]. This is used to create the handler that skips the rest of configured middlewares when sending an index stat quests ([example][5]). **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` [1]: ff847305af/pkg/querier/queryrange/shard_resolver.go (L179-L186) [2]: ff847305af/pkg/querier/queryrange/shard_resolver.go (L72) [3]: 3d2fff3a2d/pkg/querier/queryrange/queryrangebase/roundtrip.go (L124) [4]: 3d2fff3a2d/pkg/querier/queryrange/queryrangebase/roundtrip.go (L163) [5]: f422e0a52b/pkg/querier/queryrange/roundtrip.go (L521)	3 years ago
Karsten Jeschkies	94725e7908	Define `RequiredLabels` query limit. (#8851) ... **What this PR does / why we need it**: Some end-users can impose great workload on a cluster by selecting too many streams in their queries. We should be able to limit them. Therefore we introduce a new limit `RequiredLabelMatchers` which list label names that must be included in the stream selectors. The implementation follows the same approach as for max query limit. **Which issue(s) this PR fixes**: Fixes #8745 **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`	3 years ago
Karsten Jeschkies	8eac851962	Remove print statements introduced by #8727. (#8775)	3 years ago
Callum Styan	5a85f6647e	Add initial implementation of per-query limits (#8727) ... **What this PR does / why we need it**: Sometimes we want to limit the impact of a single query by imposing limits that are stricter than the current tenant limit. E.g. the maximum query length could be seven days but based on the query or an admins decision a query should just have a maximum length of one day. This is where per-request limits come into play. They are passed via the `X-Loki-Query-Limit` header and extracted into the requests context. It is the responsibility of the operator or admin that the header is valid. **Which issue(s) this PR fixes**: Fixes #8762 **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [x] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md` --------- Signed-off-by: Callum Styan <callumstyan@gmail.com> Co-authored-by: Karsten Jeschkies <karsten.jeschkies@grafana.com>	3 years ago