-
Notifications
You must be signed in to change notification settings - Fork 33
Add query cost estimation and limits proposal #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,133 @@ | ||||||
| ## Query cost estimation and limits | ||||||
|
|
||||||
| * **Owners:** | ||||||
| * Julien Pivotto [@roidelapluie](https://github.com/roidelapluie) | ||||||
|
|
||||||
| * **Implementation Status:** Not Implemented. | ||||||
|
|
||||||
| * **Related Issues and PRs:** | ||||||
| * `<GH Issues/PRs>` | ||||||
|
|
||||||
| * **Other docs or links:** | ||||||
|
|
||||||
| > TL;DR: A single expensive query can hurt a whole Prometheus. We have knobs to cap it (`--query.max-samples`, `--query.timeout`), but no way to tell a user *before* they run a query how expensive it is, and no per-query, reloadable ceilings. This proposal adds a cheap cost *estimate* (series touched, samples scanned) exposed through `/api/v1/query_cost`, reloadable cost *limits* enforced during execution, and an estimated-vs-actual `cost` object on the query response. All behind a `query-cost` feature flag. | ||||||
|
|
||||||
| ## Why | ||||||
|
|
||||||
| Prometheus already protects itself from runaway queries, but the tools are blunt: | ||||||
|
|
||||||
| * `--query.max-samples` caps peak samples in memory, not the total scanned. | ||||||
| * `--query.timeout` and `--query.max-concurrency` are process-wide flags, not reloadable and not per-query. | ||||||
| * Nothing tells a user, an autocomplete UI, or an alerting rule author how heavy a query is *before* it runs. | ||||||
|
|
||||||
| Operators want ceilings they can tune without a restart. Users and tools (Grafana, dashboards, recording rules) want a cheap way to gauge cost up front so they can refuse or rewrite a query before it lands on the server. | ||||||
|
|
||||||
| ### Pitfalls of the current solution | ||||||
|
|
||||||
| * The existing limits are set at startup. Changing them means a restart. | ||||||
| * They are global. A single tenant or dashboard cannot be given a tighter budget. | ||||||
| * There is no pre-execution estimate. The only way to learn a query's cost today is to run it, which is exactly what we want to avoid for the expensive ones. | ||||||
| * `--query.max-samples` measures peak in-memory samples, which does not map cleanly to "how much index and how many samples did this touch". | ||||||
|
|
||||||
| ## Goals | ||||||
|
|
||||||
| * Give a cheap, index-based cost *estimate* (series touched, samples scanned) without executing the query. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we clear up what is executing the query here? Is this referring to expanding posting or decoding chunk? Maybe only me, we can keep this as it is if this is considered clear. |
||||||
| * Expose the estimate through a new API so clients can gauge cost before running a query. | ||||||
| * Add reloadable cost limits (`query_max_series`, `query_max_samples_scanned`, `query_max_duration`) enforced during execution. | ||||||
| * Let a client *lower* those ceilings per query, never raise them. | ||||||
| * Surface estimated-vs-actual cost on the normal query response, so the estimate can be validated against reality. | ||||||
| * Keep it all opt-in behind a feature flag until the model is proven. | ||||||
|
|
||||||
| ### Audience | ||||||
|
|
||||||
| Operators running shared Prometheus servers, and UI/tooling authors (Grafana, recording rules) that build queries on a user's behalf. | ||||||
|
|
||||||
| ## Non-Goals | ||||||
|
|
||||||
| * Not replacing `--query.max-samples`, `--query.timeout`, or `--query.max-concurrency`. | ||||||
| * Not a billing or chargeback system. The numbers are upper bounds, not exact accounting. | ||||||
| * Not a slow-query log. | ||||||
| * Not per-tenant configuration, as Prometheus is not multi-tenant. Limits are global, with per-query lowering only. | ||||||
| * Not exact cost prediction. The estimate is intentionally cheap and approximate. | ||||||
|
|
||||||
| ## How | ||||||
|
|
||||||
| Three pieces, all gated by `--enable-feature=query-cost`. | ||||||
|
|
||||||
| **1. Estimation (`promql.EstimateCost`).** Parse the query, walk it for every vector and matrix selector, compute the effective time window each selector reads (mirroring the engine's `getTimeRangesForSelector`/`populateSeries`), and ask storage for the series count per selector via a single querier over the union window. `SeriesTouched` is the sum across selectors — an upper bound, because a series shared between selectors is counted once per selector. `SamplesScanned` models the engine's incremental per-step reads (full range window at step 0, then only the samples that advance past the previous cutoff), scaled by a measured average per-point cost so native-histogram points are sized by bucket rather than counted as one float unit. The estimate is index-only apart from decoding at most `histogramSampleLimit` (50) points per selector to size histograms. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on how the sample count estimation would work? Would this require decoding each chunk, or would it use the sample count stored on each chunk to avoid decoding chunks?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are 2 options. In Prometheus we can take the scrape interval for estimationm, Looking at the chunks header would be almost as I/O expensive as running the query. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would this apply to chunks written with remote write? (what scrape interval would be used?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes if we go that way. I am exploring sampling with a limited number of chunks as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm understanding correctly, the goal is to roughly estimate the value of the "samples read" statistic introduced in prometheus/prometheus#18081 (rather than the "total samples" statistic that existed before that PR). Is that correct? If so, it might be worth mentioning that here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Any reason on picking 50? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nits. Should we use canonical term of instant vector and range vector selector instead of internal struct naming vector and matrix selector?
Suggested change
|
||||||
|
|
||||||
| **2. API.** Two new endpoints estimate cost without executing: | ||||||
|
|
||||||
| ``` | ||||||
| GET|POST /api/v1/query_cost | ||||||
| GET|POST /api/v1/query_range_cost | ||||||
| ``` | ||||||
|
|
||||||
| They take the same parameters as `/api/v1/query` and `/api/v1/query_range` and return: | ||||||
|
|
||||||
| ```json | ||||||
| { | ||||||
| "estimate": { | ||||||
| "seriesTouched": 42, | ||||||
| "samplesScanned": 5040 | ||||||
| } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| The instant and range endpoints also gain a `cost` parameter. When set, the response `data` carries an estimated-vs-actual comparison: | ||||||
|
|
||||||
| ```json | ||||||
| "cost": { | ||||||
| "estimated": { "seriesTouched": 42, "samplesScanned": 5040 }, | ||||||
| "actual": { "seriesTouched": 40, "samplesScanned": 4980, "peakSamples": 320 } | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| Note: `cost=1` adds a second index lookup on top of executing the query. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can have cost=2 later that e.g. read chunks metadata ? |
||||||
|
|
||||||
| **3. Limits.** Three reloadable knobs under `global:`: | ||||||
|
|
||||||
| ```yaml | ||||||
| global: | ||||||
| query_max_series: 0 # 0 = no limit | ||||||
| query_max_samples_scanned: 0 | ||||||
| query_max_duration: 0s | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this different to the existing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not, more a normalization of it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the existing flag / parameter be deprecated? Otherwise we have two ways to do the same thing. |
||||||
| ``` | ||||||
|
|
||||||
| These are enforced *during* execution against the query's actual running cost, not against the estimate: a query is rejected as soon as it loads too many series or scans too many samples, and `query_max_duration` surfaces as a query timeout. A client may lower any ceiling for a single request via `max_series`, `max_samples_scanned`, `max_query_duration`; these can only tighten, never loosen, the operator-set value. The estimate is never used to reject a query — enforcement is always on the real cost. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a request tries to override these limits to higher values than what is set on the server, is the request rejected? Or are the requested limits ignored?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "A client may lower any ceiling". The request is not rejected but you will be capped at the server's limit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My 2c is that requests which ask for a higher value should be rejected - this makes it very clear that the requested limit wasn't applied.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if it might actually make sense to enforce the limit based on the estimation, rather than allowing the query to run up until a limit is hit. Feels wasteful to let a query that will likely be limited, run and fetch data.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say it will depend on how we can accurately estimate the limit upfront. |
||||||
|
|
||||||
| ### Testing and verification | ||||||
|
|
||||||
| * Unit tests for limit enforcement (reject paths) in `promql`. | ||||||
| * Estimation-accuracy tests against known fixtures, plus the `cost` object which lets us compare estimated and actual on every executed query. | ||||||
| * API tests for the new endpoints and the `cost` parameter. | ||||||
| * OpenAPI golden files updated for the new paths and schemas. | ||||||
|
|
||||||
| ### Migration | ||||||
|
|
||||||
| Purely additive and behind a feature flag. Default config (all limits `0`) changes no behaviour. Nothing to migrate. | ||||||
|
|
||||||
| ### Known unknowns | ||||||
|
|
||||||
| * **Estimate accuracy.** `SeriesTouched` over-counts shared series and series with no in-window samples; `SamplesScanned` assumes samples land exactly at the scrape interval and that sampled series are representative. Is an upper bound the right contract, or do we want something tighter? | ||||||
| * **Scrape interval.** The estimator uses the global scrape interval; per-target intervals are not modelled. | ||||||
| * **Subqueries.** Only one level of nesting is modelled exactly. | ||||||
| * **Lookback delta.** The storage-only estimator uses the package default, not the engine's configured value. | ||||||
| * **Agent mode.** Estimation is unavailable (no queryable index). | ||||||
| * **Config surface.** Should limits live under `global:`, or a dedicated `query:` section? | ||||||
|
|
||||||
| ## Alternatives | ||||||
|
|
||||||
| 1. **Estimate from postings cardinality directly, bypassing `storage.Querier`.** Cheaper, but ties the estimator to the TSDB index and breaks for any other `storage.Queryable` (remote read, federation). Using the portable `Select` path keeps it storage-agnostic. | ||||||
| 2. **Reject queries based on the estimate.** Rejected: the estimate is an upper bound and can be wrong in both directions. Rejecting on an estimate would refuse queries that would actually run fine. Enforcement is on real cost; the estimate is advisory only. | ||||||
| 3. **Reuse `--query.max-samples` and friends.** They are start-time flags measuring peak in-memory samples, not reloadable and not per-query. Extending them to be reloadable and per-query would overload their meaning; new, clearly-scoped knobs are cleaner. | ||||||
| 4. **Do nothing / client-side estimation.** Clients cannot cheaply see the server's index cardinality, so any client-side guess is worse than a server estimate. | ||||||
|
|
||||||
| ## Action Plan | ||||||
|
|
||||||
| * [ ] `promql.EstimateCost` and the sample-unit cost model | ||||||
| * [ ] `/api/v1/query_cost` and `/api/v1/query_range_cost` endpoints | ||||||
| * [ ] `cost` parameter on instant/range queries (estimated vs actual) | ||||||
| * [ ] Reloadable `query_max_series` / `query_max_samples_scanned` / `query_max_duration` under `global:` | ||||||
| * [ ] Per-query lowering via `max_series` / `max_samples_scanned` / `max_query_duration` | ||||||
| * [ ] `query-cost` feature flag, docs, OpenAPI spec, UI surfacing | ||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits. Mentioned in the non-goal below, but isn't we know prometheus doesn't support multiple tenant?