feat(promql): type-driven histogram_quantile discrimination (#79) - #94
Merged
Merged
Conversation
`histogram_quantile(φ, m)`'s classic-bucket (exact interpolation) vs sketch-able (generic Quantile) choice was a purely structural heuristic (`by (le)` / `_bucket` name / `le=` matcher), with false-positive (`_bucket`-named non-histogram) and false-negative (suffix-less classic histogram) failure modes. Drive it from the argument's declared sample type when available. - `histogram` module: `HistogramKind` (ClassicBucket / Native / RawSamples; only ClassicBucket is non-sketch-able) and a `HistogramCatalog` (metric → kind) the client supplies. Installed for the duration of a lowering call via an RAII thread-local guard — the discrimination is consulted in exactly one place (`walk_histogram`), so an ambient catalog avoids threading a parameter through the whole free-function `walk` recursion. - `histogram_arg_is_sketchable`: a declared kind for any metric referenced in the argument decides it; otherwise fall back to the structural `is_classic_bucket_arg` heuristic (unchanged for undeclared metrics). - New entry point `lower_promql_with_histograms(query, accuracy, catalog)`; `lower_promql` stays heuristic-only. The heuristic remains as the deliberate fallback (not every metric carries metadata); the metadata simply overrides it where the true sample type is known. Tests: `histogram_metadata.rs` pins the heuristic baseline, the false-negative fix (declared ClassicBucket → HistogramQuantile), the false-positive fix (declared Raw/Native → Quantile), heuristic fallback for undeclared metrics, and that the ambient catalog does not leak across calls. Unit tests cover `is_sketchable`, catalog lookup, and guard install/restore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #79.
Problem
histogram_quantile(φ, m)picks between exact classic-bucket interpolation (HistogramQuantile, not sketch-able) and the generic sketch-ableQuantileusing a purely structural heuristic (by (le)grouping /_bucketmetric name /le=matcher). The true signal is the argument's sample type, which structure only proxies — so the heuristic has real failure modes:…_bucket(or a native histogram so named) → interpolation;_bucketand queried withoutle→ the sketch path, which is wrong (you can't sketch pre-aggregated buckets).Fix: drive it from declared sample type
histogrammodule:HistogramKind(ClassicBucket/Native/RawSamples— onlyClassicBucketis non-sketch-able) and aHistogramCatalog(metric → kind) the client supplies.histogram_arg_is_sketchable: a declared kind for any metric referenced in the argument decides it; otherwise fall back to the existing structural heuristic. So metadata overrides structure where the sample type is known, and undeclared metrics behave exactly as before.lower_promql_with_histograms(query, accuracy, catalog).lower_promqlstays heuristic-only.This directly serves the design the two lowerings exist for: a client holding raw samples can declare them
RawSamplesand get the sketch-ableQuantileeven when the user wrotehistogram_quantile.On the plumbing
The discrimination is consulted in exactly one place (
walk_histogram), reached from deep in the recursive, free-functionwalkchain. Rather than thread a catalog parameter through ~20 functions / 36 call sites, the catalog is installed as an RAII-guarded thread-local for the duration of one lowering call (lowering is synchronous, one query at a time). A test asserts it doesn't leak across calls.Scope
The heuristic is kept as the deliberate fallback — not every metric will carry metadata, so removing it would regress undeclared queries.
#79also floated per-series tagging and an L4-deferred variant; per-metric catalog is the right granularity for the lowering-time decision and is what the client can actually declare. The mechanism is now in place for a production catalog to feed.Tests
histogram_metadata.rs:ClassicBucketon a suffix-less metric →HistogramQuantile;RawSamples/Nativeon a_bucket-named metric →Quantile;Unit tests cover
is_sketchable, catalog lookup, and guard install/restore. Full workspace suite green; clippy clean.