From 946fd50d5317534473a2679764a0aacecad0f828 Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sun, 6 Sep 2026 20:51:24 +0100 Subject: [PATCH] Seek log/metric-bucket queries by key range instead of scanning Log reads (logs_since, the unsorted Logs query path) now seek the IngestedLog range directly from since_sequence/before_sequence instead of scanning from IndexKey::Start and filtering every prior entry out in a closure. Metric-comparison filters and bucket_values now seek to a single metric's Bucket key range instead of scanning every declared metric's buckets. evaluate_promql sits on top of bucket_values, so its per-poll dashboard evaluation is narrowed by the same fix without further changes; caching prior evaluations across polls is left for a follow-up if profiling ever shows it's needed (per ticket 081's own scope, not built speculatively). Resolves tickets 077, 078, 081. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TeyhRfMYjMditek6geDRoQ --- src/engine/snapshot/metrics.rs | 12 +++- src/engine/snapshot/mod.rs | 46 +++++++++--- src/engine/snapshot/query.rs | 15 ++-- src/engine/tests.rs | 65 +++++++++++++++++ src/engine/timestamp.rs | 5 ++ ...l-index-instead-of-seeking-by-key-range.md | 72 ------------------- ...ilter-scans-all-buckets-for-every-query.md | 56 --------------- ...build-full-series-from-scratch-per-poll.md | 52 -------------- 8 files changed, 128 insertions(+), 195 deletions(-) delete mode 100644 tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md delete mode 100644 tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md delete mode 100644 tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md diff --git a/src/engine/snapshot/metrics.rs b/src/engine/snapshot/metrics.rs index 9fa1bb7..b288eda 100644 --- a/src/engine/snapshot/metrics.rs +++ b/src/engine/snapshot/metrics.rs @@ -298,8 +298,18 @@ impl EngineSnapshot { labels_filter: Option<&'a BTreeMap>, ) -> impl Iterator, &'a (u64, Timestamp, MetricPrimitive))> + 'a { + // IndexKey::Bucket sorts by (metric_name, labels), so bound the range to just this + // metric's buckets instead of ranging MetricBucketSplit..End over every declared + // metric. '\u{10FFFF}' is the maximum Unicode scalar value, so `metric_name` followed + // by it sorts after every real key for this metric (any labels) and before any key + // for a lexicographically greater metric name. + let lower = IndexKey::Bucket { metric_name: metric_name.to_string(), labels: BTreeMap::new() }; + let upper = IndexKey::Bucket { + metric_name: format!("{metric_name}\u{10FFFF}"), + labels: BTreeMap::new(), + }; self.index - .range(IndexKey::MetricBucketSplit..IndexKey::End) + .range(lower..upper) .filter_map(move |(key, value)| match (key, value) { (IndexKey::Bucket { metric_name: name, labels }, DataValue::BucketValue { values }) if name == metric_name && labels_filter.is_none_or(|filter| filter == labels) => diff --git a/src/engine/snapshot/mod.rs b/src/engine/snapshot/mod.rs index 9b1cb83..c33d8c9 100644 --- a/src/engine/snapshot/mod.rs +++ b/src/engine/snapshot/mod.rs @@ -157,13 +157,41 @@ impl EngineSnapshot { } pub(crate) fn logs_range(&self) -> impl Iterator { - self.index.range(IndexKey::Start..IndexKey::LogParamSplit).filter_map(|(key, value)| { - match (key, value) { - (IndexKey::IngestedLog { sequence, timestamp }, DataValue::LogLiteral { text }) => { - Some((*sequence, *timestamp, text.as_str())) - } - _ => None, + self.logs_in_sequence_range(..) + } + + /// Seeks directly to `sequence_range` within the `IngestedLog` key range instead of + /// scanning from `IndexKey::Start` and filtering after the fact. `IngestedLog` sorts by + /// `(sequence, timestamp)`, and `sequence` is assigned monotonically at ingest + /// (`handle.rs`), so a bound on `sequence` alone is an exact seek — `Timestamp::MIN` is + /// used as the synthetic sub-key since it sorts below any real timestamp for that sequence. + pub(crate) fn logs_in_sequence_range( + &self, + sequence_range: impl std::ops::RangeBounds, + ) -> impl Iterator { + let lower = match sequence_range.start_bound() { + std::ops::Bound::Included(&sequence) => { + IndexKey::IngestedLog { sequence, timestamp: Timestamp::MIN } } + std::ops::Bound::Excluded(&sequence) => { + IndexKey::IngestedLog { sequence: sequence.saturating_add(1), timestamp: Timestamp::MIN } + } + std::ops::Bound::Unbounded => IndexKey::Start, + }; + let upper = match sequence_range.end_bound() { + std::ops::Bound::Included(&sequence) => { + IndexKey::IngestedLog { sequence: sequence.saturating_add(1), timestamp: Timestamp::MIN } + } + std::ops::Bound::Excluded(&sequence) => { + IndexKey::IngestedLog { sequence, timestamp: Timestamp::MIN } + } + std::ops::Bound::Unbounded => IndexKey::LogParamSplit, + }; + self.index.range(lower..upper).filter_map(|(key, value)| match (key, value) { + (IndexKey::IngestedLog { sequence, timestamp }, DataValue::LogLiteral { text }) => { + Some((*sequence, *timestamp, text.as_str())) + } + _ => None, }) } @@ -173,10 +201,8 @@ impl EngineSnapshot { since_ms: i64, now_ms: i64, ) -> impl Iterator { - self.logs_range().filter(move |(sequence, timestamp, _)| { - *sequence < before_sequence - && timestamp.as_millis() > since_ms - && timestamp.as_millis() <= now_ms + self.logs_in_sequence_range(..before_sequence).filter(move |(_, timestamp, _)| { + timestamp.as_millis() > since_ms && timestamp.as_millis() <= now_ms }) } } diff --git a/src/engine/snapshot/query.rs b/src/engine/snapshot/query.rs index 948569d..bfa76c2 100644 --- a/src/engine/snapshot/query.rs +++ b/src/engine/snapshot/query.rs @@ -56,11 +56,14 @@ pub(super) fn handle_query_inner( } let limit = params.limit.unwrap_or(500).min(10_000); let compiled_params = snapshot.compiled_param_declarations(); + let since_bound = match params.since_sequence { + Some(since) => std::ops::Bound::Excluded(since), + None => std::ops::Bound::Unbounded, + }; let filtered = snapshot - .logs_range() - .filter(|(sequence, timestamp, _)| { - params.since_sequence.is_none_or(|since| *sequence > since) - && params.start_ms.is_none_or(|start| timestamp.as_millis() >= start) + .logs_in_sequence_range((since_bound, std::ops::Bound::Unbounded)) + .filter(|(_, timestamp, _)| { + params.start_ms.is_none_or(|start| timestamp.as_millis() >= start) && params.end_ms.is_none_or(|end| timestamp.as_millis() <= end) }) .filter(|(sequence, timestamp, text)| { @@ -80,6 +83,10 @@ pub(super) fn handle_query_inner( }) .collect(), Some(sort_by) => { + // Not range-bounded by since_sequence/start_ms: SortHelperKey::WithParam + // sorts by the param's value first (that's the point — it's what gives + // sort-by-param order), so sequence/time bounds can't narrow a BTreeMap + // range seek here the way they do for the unsorted path above. static EMPTY_HELPER: std::sync::LazyLock> = std::sync::LazyLock::new(BTreeMap::new); let helper_index = diff --git a/src/engine/tests.rs b/src/engine/tests.rs index 956f26c..61a5103 100644 --- a/src/engine/tests.rs +++ b/src/engine/tests.rs @@ -381,6 +381,34 @@ use std::collections::{BTreeMap, HashMap, HashSet}; assert_eq!(index.approx_bytes_total, expected_bytes); } + #[test] + fn logs_in_sequence_range_seeks_instead_of_scanning_from_start() { + let mut index = EngineSnapshot::default(); + for sequence in 0..1000u64 { + index.index.insert( + IndexKey::IngestedLog { sequence, timestamp: Timestamp::from_millis(sequence as i64) }, + DataValue::LogLiteral { text: format!("line {sequence}") }, + ); + } + + // An excluded lower bound (as `since_sequence` polling uses) must start exactly past + // the bound, not include it or anything before it. + let since_990: Vec<_> = + index.logs_in_sequence_range(990..).map(|(seq, _, _)| seq).collect(); + assert_eq!(since_990, vec![990, 991, 992, 993, 994, 995, 996, 997, 998, 999]); + + let after_990: Vec<_> = index + .logs_in_sequence_range((std::ops::Bound::Excluded(990u64), std::ops::Bound::Unbounded)) + .map(|(seq, _, _)| seq) + .collect(); + assert_eq!(after_990, vec![991, 992, 993, 994, 995, 996, 997, 998, 999]); + + // An exclusive upper bound (as `logs_since`'s `before_sequence` uses) must stop + // strictly before it. + let before_5: Vec<_> = index.logs_in_sequence_range(..5u64).map(|(seq, _, _)| seq).collect(); + assert_eq!(before_5, vec![0, 1, 2, 3, 4]); + } + #[test] fn evict_excess_logs_is_noop_when_unlimited_or_under_cap() { let mut index = EngineSnapshot::default(); @@ -1381,6 +1409,43 @@ use std::collections::{BTreeMap, HashMap, HashSet}; assert_eq!(*value, MetricPrimitive::Number(Number::Int(5))); } + #[test] + fn bucket_values_seeks_by_metric_name_and_ignores_lexicographic_neighbors() { + let mut index = EngineSnapshot::default(); + declare_test_param( + &mut index, + "value", + r"value=(?P\d+)", + "value", + MetricPrimitiveKind::Number, + ); + // "aaa" and "aab" are adjacent lexicographically, so this exercises the exact + // boundary the metric-name seek relies on: each metric's own message_match_regex + // gates it to only its own line, so if the seek pulled in the wrong metric's bucket + // range, the value asserted below would be wrong. + index + .declare_metric(metric_request_with_value("aaa", r"^aaa value=\d+$", vec![], Some("value"))) + .unwrap(); + index + .declare_metric(metric_request_with_value("aab", r"^aab value=\d+$", vec![], Some("value"))) + .unwrap(); + + ingest_log(&mut index, 0, "aaa value=1"); + ingest_log(&mut index, 1, "aab value=2"); + + let aaa_values: Vec<_> = index + .bucket_values("aaa", None) + .map(|(_, (_, _, value))| value.clone()) + .collect(); + assert_eq!(aaa_values, vec![MetricPrimitive::Number(Number::Int(1))]); + + let aab_values: Vec<_> = index + .bucket_values("aab", None) + .map(|(_, (_, _, value))| value.clone()) + .collect(); + assert_eq!(aab_values, vec![MetricPrimitive::Number(Number::Int(2))]); + } + fn declare_durations_metric(index: &mut EngineSnapshot) { let pattern = r"request path=(?P\S+) duration=(?P\d+)"; declare_test_param(index, "path", pattern, "path", MetricPrimitiveKind::String); diff --git a/src/engine/timestamp.rs b/src/engine/timestamp.rs index 3adfce4..37a62ae 100644 --- a/src/engine/timestamp.rs +++ b/src/engine/timestamp.rs @@ -5,6 +5,11 @@ use std::time::{SystemTime, UNIX_EPOCH}; pub struct Timestamp(i64); impl Timestamp { + /// The smallest representable timestamp — used as a synthetic sub-key when seeking + /// `IndexKey::IngestedLog { sequence, timestamp }` by `sequence` alone, since it sorts + /// below any real timestamp recorded for that sequence. + pub const MIN: Timestamp = Timestamp(i64::MIN); + pub fn now() -> Self { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md b/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md deleted file mode 100644 index 799217b..0000000 --- a/tickets/077-log-queries-scan-full-index-instead-of-seeking-by-key-range.md +++ /dev/null @@ -1,72 +0,0 @@ -# 077 — Log queries scan the full index instead of seeking by key range - -## Summary - -Every log read — including polling reads that only want logs newer -than `since_sequence` — walks the *entire* retained log range and -filters with a closure, rather than seeking the `BTreeMap` directly to -the relevant key range. This is the read-path optimization the -`left-right`/sort-helper-index work never got around to: iterators -should narrow the underlying key range as far as the caller's bounds -allow, and accept range bounds as a parameter instead of scanning from -`Start`/`MetricBucketSplit` and filtering after the fact. - -## Where - -- `EngineSnapshot::logs_range` (`src/engine/snapshot/mod.rs:159-168`) - always ranges `IndexKey::Start..IndexKey::LogParamSplit` — i.e. every - ingested log ever retained — with no way for a caller to pass a - narrower bound. -- `EngineSnapshot::logs_since` (`src/engine/snapshot/mod.rs:170-181`) - calls `logs_range()` and then filters by `sequence < before_sequence` - and a timestamp window in a closure — it has all the information - needed to seek instead (`before_sequence` bounds `sequence` - directly), but throws that away and scans linearly regardless of how - small the actual window is. -- The unsorted log-query path in `handle_query_inner` - (`src/engine/snapshot/query.rs:59-65`) does the same thing: - `snapshot.logs_range().filter(|(sequence, timestamp, _)| { since ... - start_ms ... end_ms ... })`. On every poll (frontend passes - `since_sequence` — see `webapp/src/lib/api.ts:263,343`), this walks - every previously-seen log line again just to throw it away in the - filter, before even getting to the new ones. -- `IndexKey::IngestedLog { sequence, timestamp }` - (`src/engine/index_key.rs:10-13`) derives `Ord` field-order-first on - `sequence`, so a lower bound of - `IndexKey::IngestedLog { sequence: since + 1, timestamp: Timestamp::MIN }` - (or equivalent) is a valid, correct seek — sequence is assigned - monotonically at ingest (`src/engine/handle.rs:118-119`), so this - isn't a heuristic, it's exact. - -## Scope - -- Change `logs_range` to accept a key range (e.g. - `RangeBounds` or a `(Bound, Bound)` over - sequence) instead of hardcoding `Start..LogParamSplit`, so callers - can seek. -- Rework `logs_since` and the `EngineQuery::Logs` handler in - `query.rs:59-65` to build a lower bound from `since_sequence` (and, - where possible, from `start_ms`/note below) and pass it through, - instead of filtering post-hoc from the very start of the index. -- Note: because `IngestedLog` keys sort by `(sequence, timestamp)` and - not `(timestamp, sequence)`, a `start_ms`/`end_ms`-only query (no - `since_sequence`) can't be turned into an exact key-range seek from - this index alone even though ingestion is roughly time-ordered by - sequence — call this out explicitly rather than seeking incorrectly; - it may be acceptable to leave that case as a scan, or to binary-search - for the first sequence at/after `start_ms` since timestamps are - non-decreasing with sequence in practice. -- The sorted branch (`query.rs:82-128`) iterates `sort_helper_index` - directly rather than `logs_range`, so it's less affected, but still - does a per-candidate `index.get()` (`query.rs:106-107`) with no early - bound — check whether the helper index itself can be range-bounded - once `since_sequence`/`start_ms` are known. -- Add/extend a test asserting that a `since_sequence` poll on a large - log store does not touch (or does not compare against) log entries - at or before `since_sequence` — a range-based seek should make this - observably true, not just faster. - -## Depends on - -None. Related to ticket 060/061 (sort helper indexes) — same -underlying principle (avoid full scans by indexing/seeking). diff --git a/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md b/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md deleted file mode 100644 index a96a8a0..0000000 --- a/tickets/078-metric-comparison-filter-scans-all-buckets-for-every-query.md +++ /dev/null @@ -1,56 +0,0 @@ -# 078 — Metric-comparison filters scan every metric's buckets on every query - -## Summary - -Same underlying issue as ticket 077 (iterators should narrow the key -range as far as possible, and take a range as a parameter), applied to -metrics: a metric-comparison filter (e.g. `latency_ms[value]<200` in -the filter bar) rebuilds its matching-sequence set from scratch on -every single query by scanning *all* buckets for *all* metrics, then -discarding everything that doesn't match the requested metric name — -instead of seeking directly to that metric's key range. - -## Where - -- `EngineSnapshot::bucket_values` - (`src/engine/snapshot/metrics.rs:295-312`) ranges - `IndexKey::MetricBucketSplit..IndexKey::End` — every bucket for every - declared metric — and filters by `name == metric_name` inside the - `filter_map` closure. `IndexKey::Bucket { metric_name, labels }` - (`src/engine/index_key.rs:23-26`) derives `Ord` on `metric_name` - first, so this is seekable: a lower bound of - `IndexKey::Bucket { metric_name: metric_name.into(), labels: - BTreeMap::new() }` and an upper bound one metric name past it would - narrow the range to just that metric's buckets. -- The compare-filter compiler, - `compile_metric_comparison` (`src/engine/snapshot/filter.rs:83-104`), - calls `self.bucket_values(metric, None)` and builds a fresh - `HashSet` of matching sequences on every call — this runs once - per filter compile, and filters are compiled fresh per query - (`compile_filter_query`, called from - `src/engine/snapshot/query.rs:46`), so a dashboard or search bar - polling with a metric-comparison filter re-scans all bucket values - every poll interval. Unlike the log sort-helper indexes - (`src/engine/snapshot/sort.rs`), there is no incremental cache here - keyed by `(metric, param, op, value)`. - -## Scope - -- Narrow `bucket_values` to accept/derive a key range for the - requested `metric_name` instead of ranging - `MetricBucketSplit..End` and filtering by name in the closure. This - alone removes the "every other metric's buckets" part of the scan - regardless of caching. -- Separately (bigger win, more invasive — may warrant its own - follow-up ticket): consider whether metric-comparison results can be - incrementally maintained similarly to `sort_helper_indexes` - (`src/engine/snapshot/sort.rs`) rather than recomputed per query, if - profiling shows compare-filtered dashboard polling is a hot path. - Keep this ticket scoped to the range-narrowing fix; note the caching - idea here so it isn't lost, but split it out if it turns out to need - design work (cache invalidation on new bucket values, per unique - filter expression). - -## Depends on - -None. Same principle as ticket 077. diff --git a/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md b/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md deleted file mode 100644 index 5820c06..0000000 --- a/tickets/081-promql-dashboard-queries-rebuild-full-series-from-scratch-per-poll.md +++ /dev/null @@ -1,52 +0,0 @@ -# 081 — PromQL dashboard queries rebuild full label→points maps from scratch every poll - -## Summary - -A dashboard panel backed by a PromQL-style query re-evaluates its -entire series (all labels, all points) from scratch on every polling -interval, with no reuse of the previous evaluation's work and no -incremental narrowing by the time range the panel actually needs — -same "narrow the range, don't rescan everything" principle as the -other read-path tickets, applied to the dashboard/metrics query path -this time. - -## Where - -- `evaluate_promql` and its helpers - (`src/engine/snapshot/query.rs:412-548`) build label→points maps for - the query's underlying metric(s) fresh on every call — there is no - reuse across repeated calls with the same query string and an - overlapping time window, which is exactly the pattern dashboard - polling produces (`webapp/src/lib/polling.ts` re-requests the same - panel query on an interval). -- This sits on top of `bucket_values` (ticket 078), so once that's - narrowed to a metric's own key range, `evaluate_promql` still - recomputes the full aggregation/label-grouping logic over whatever - `bucket_values` returns, every single call, even for the portion of - the time range it already computed on the previous poll. - -## Scope - -- Start by checking whether panels pass a time range/step down to - `evaluate_promql` at all — if they do, apply the same key-range - narrowing as ticket 077/078 first (bound the underlying - `bucket_values` range by the panel's requested window) before - considering caching, since that alone may remove most of the - redundant work. -- If polling the *same* query with an advancing time window is common - (sliding dashboard windows), investigate caching prior evaluation - results keyed by `(query string, label set)` and only computing the - delta for the newly-advanced portion of the window — this is more - involved (needs a cache invalidation story tied to new bucket - ingestion, similar to `sort_helper_indexes`' - `purge_stale_sort_helper_indexes`, - `src/engine/snapshot/sort.rs`) — do not build this speculatively - without first confirming via profiling that repeated PromQL - evaluation (not `bucket_values` scanning, addressed by ticket 078) - is the actual bottleneck for dashboards with multiple panels. - -## Depends on - -Should land after ticket 078 (metric bucket range narrowing), since -that changes what `evaluate_promql` is scanning underneath and may -reduce the size of this problem before any caching work is designed.