Skip to content

Add query cost estimation and limits proposal - #89

Open
roidelapluie wants to merge 4 commits into
prometheus:mainfrom
roidelapluie:roidelapluie/add-query-cost-proposal
Open

Add query cost estimation and limits proposal#89
roidelapluie wants to merge 4 commits into
prometheus:mainfrom
roidelapluie:roidelapluie/add-query-cost-proposal

Conversation

@roidelapluie

Copy link
Copy Markdown
Member

No description provided.

Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/0089-query-cost.md
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Comment thread proposals/00089-query-cost.md Outdated
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
@roidelapluie

Copy link
Copy Markdown
Member Author

Hello!

I made substantial changes to the proposal and I am working on the Proof of Concept.

cc @charleskorn @lamida

Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com>
* 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether these configurations can be integrated with a RBAC system so the limit can be applied at either user or team level

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done in Mimir, but for Prometheus downstream there is nothing around RBAC.


### Known unknowns

* **Estimate accuracy.** `SeriesTouched` still over-counts shared series and series with no in-window samples; `SamplesScanned` assumes samples land exactly at the measured or scrape interval and that sampled series are representative. Partially resolved when a selector's real window fits within the 50-chunk/50-series sample budget, the measurement is now taken from that real window instead of extrapolated from a nearby proxy window, so small selectors get an exact rather than approximate density (see How, section 1). Larger selectors still extrapolate from a bounded sample. Is an upper bound the right contract for those, or do we want something tighter?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

over-counting is fine and we can check the % between estimate and actual count after accumulating the number of real queries

## 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 as the default: the estimate is an upper bound and can be wrong in both directions, so rejecting on it would refuse queries that would actually run fine. Enforcement is on real cost; the estimate is advisory only. There is a fair argument that letting a query that will almost certainly be limited run and fetch data anyway is wasteful. If the estimate proves accurate enough in practice (validated via the `cost` object's estimated-vs-actual comparison), an *opt-in* upfront rejection — reject before execution when the estimate clearly exceeds a ceiling — could be added later as a follow-up without changing the real-cost enforcement that remains the backstop.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as the difference between estimate and actual is too high (e.g. more than 20%), users will ask for a way to reject the query before it runs

* **Scrape-interval (fallback, index-only).** Assume samples land at the global scrape interval and compute window ÷ interval. Cheapest, but wrong for series scraped at a different interval and for remote-written series, which have no scrape interval at all. Used only when nothing can be sampled (see below).
* **Chunk sampling (always-on, not opt-in).** The estimator samples automatically, with no user-facing knob, whenever the storage exposes `storage.ChunkQueryable`: it reads up to a fixed `chunkSampleLimit` (50) chunks' `NumSamples` header to measure the selector's real sample interval, and decodes the first point of up to a fixed `histogramSampleLimit` (50) series to size native-histogram points by bucket count. Reading a chunk header is far cheaper than decoding its samples, so this stays much cheaper than executing the query.

Sampling prefers the *real* query window over a nearby proxy window whenever that real window is cheap enough: if a selector's actual chunk count (for density) or series count (for point cost) already fits within the 50-item budget, the estimator samples directly from `[sel.mint, sel.maxt]` and gets an exact rather than extrapolated measurement. Only when the real window has more chunks/series than the budget affords does it fall back to sampling a bounded, narrow window near the query's end and extrapolating.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's proxy window? It's not defined anywhere.

}
```

The instant and range endpoints also gain a `cost=true` boolean parameter. When set, the response `data` carries an estimated-vs-actual comparison:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't cost be an enum? Meaning: omitted, or none, or actual or compare. Or something like that? The current solution implies we're re-running the cost estimation even if it was estimated before up front.

### Known unknowns

* **Estimate accuracy.** `SeriesTouched` still over-counts shared series and series with no in-window samples; `SamplesScanned` assumes samples land exactly at the measured or scrape interval and that sampled series are representative. Partially resolved when a selector's real window fits within the 50-chunk/50-series sample budget, the measurement is now taken from that real window instead of extrapolated from a nearby proxy window, so small selectors get an exact rather than approximate density (see How, section 1). Larger selectors still extrapolate from a bounded sample. Is an upper bound the right contract for those, or do we want something tighter?
* **Scrape interval.** Mostly resolved for TSDB-backed storage: the estimator measures the real density from chunk metadata automatically whenever it's available, with no configuration needed. The caller-supplied scrape interval remains a fallback only for a plain `storage.Queryable` with no chunk metadata (e.g. some remote-read backends), or when a selector's window has nothing to sample.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You say "caller-supplied" scrape interval, but there was no mention of this in the API, only cost parameter.

Also note that if we had native metadata and we propagated it (lot o ifs) , you might get accurate scrape interval.

The per-point density can be derived two ways:

* **Scrape-interval (fallback, index-only).** Assume samples land at the global scrape interval and compute window ÷ interval. Cheapest, but wrong for series scraped at a different interval and for remote-written series, which have no scrape interval at all. Used only when nothing can be sampled (see below).
* **Chunk sampling (always-on, not opt-in).** The estimator samples automatically, with no user-facing knob, whenever the storage exposes `storage.ChunkQueryable`: it reads up to a fixed `chunkSampleLimit` (50) chunks' `NumSamples` header to measure the selector's real sample interval, and decodes the first point of up to a fixed `histogramSampleLimit` (50) series to size native-histogram points by bucket count. Reading a chunk header is far cheaper than decoding its samples, so this stays much cheaper than executing the query.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it do this reading of 50 chunks+50 samples for every series from SeriesTouched?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants