diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index c6e59ed..de86f89 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -243,20 +243,22 @@ Everything is in a single SQLite file (`noadd.sqlite3` by default; a legacy `noa | Index | Serves | | --- | --- | | `timestamp` | the time-window filter every stats query starts with | -| `(domain, timestamp)` | top domains, unique domains | -| `(client_ip, doh_token, timestamp)` | top clients | +| `(domain, timestamp)` | the query log's domain search; top and unique domains for callers asking for domains alone | +| `(timestamp, domain, client_ip, doh_token)` | top domains and top clients together, on the Statistics page and the dashboard | | `(timestamp, blocked, cached, response_ms, query_type, has_result)` | timeline, query-type breakdown, latency histogram, outcome breakdown, the dashboard summary | | `(timestamp, upstream, response_ms) WHERE upstream IS NOT NULL` | top upstreams | -The first two composites put the grouped columns first and `timestamp` last, which is what makes them covering for a `GROUP BY … WHERE timestamp >= ?` shape — the aggregation reads the index alone instead of scanning the window and building a temp b-tree over it. Top clients went from 143 ms to 20 ms on a 447 k-row database that way. +`(domain, timestamp)` puts the grouped column first and `timestamp` last, which made it covering for a `GROUP BY … WHERE timestamp >= ?` shape — the aggregation reads the index alone instead of scanning the window and building a temp b-tree over it. The client index that sat beside it, `(client_ip, doh_token, timestamp)`, did the same for top clients and took them from 143 ms to 20 ms on a 447 k-row database. The cost of that order is that the window cannot restrict it: a 24-hour question skip-scans the whole index, one seek per distinct group. -The last one inverts that order because its queries do not group by a column at all; they filter on `timestamp` and then read a few narrow values. Carrying those values in the index avoids a row lookup into a table whose rows average ~84 bytes of strings (`domain`, `client_ip`, `upstream`, `result`) that none of those queries want: timeline 78 → 60 ms, query-type 75 → 62 ms, latency 60 → 45 ms. +Version 15 replaced the client index with `(timestamp, domain, client_ip, doh_token)`. Both lists are always asked for together, and grouped at `(domain, client_ip, doh_token)` one statement answers them; timestamp-first, it reads only the window. On a 370 k-row database the two lists went from 7 589 pages to 5 769 for a window covering the table, and from 2 389 to 747 for the dashboard's 24 hours, for 8.4 MiB more index and no measurable change in what a logger batch writes. A Statistics visit over the whole table went from 9 758 page misses to 7 937, and a dashboard tick from 4 755 to 3 113. `(domain, timestamp)` stays because the query log's domain search seeks it by prefix: without it, searching a prefix nobody has queried reads the whole table, 12 170 pages against 3. Replacing both indexes measured cheaper still on every other axis and was rejected for that alone. + +The metrics index is timestamp-first because its queries do not group by a column at all; they filter on `timestamp` and then read a few narrow values. Carrying those values in the index avoids a row lookup into a table whose rows average ~84 bytes of strings (`domain`, `client_ip`, `upstream`, `result`) that none of those queries want: timeline 78 → 60 ms, query-type 75 → 62 ms, latency 60 → 45 ms. `has_result` is a VIRTUAL generated column — `result IS NOT NULL AND result != ''` — which occupies no table space and exists so the outcome breakdown can classify a query without reading one. It is the last column of the metrics index, and the query that needs it carries an `INDEXED BY`: with `timestamp` alone also matching the range the planner picks that smaller index and pays a rowid lookup per row, which is the whole table. An index on the bare expression rather than a named column was tried first and the planner would not treat it as covering. The upstream index is partial because blocked and cached answers never reach an upstream, so more than half the rows have nothing to put in it (56% on a 370 k-row database), and its only query excludes them anyway. It is timestamp-first for the writer's sake rather than the reader's: both orders answer the dashboard's 24-hour top upstreams in about 200 pages against 1 608 through `timestamp` and a rowid lookup per row, but the logger appends at the newest end, and an upstream-first index spreads every batch across one insertion point per upstream — 65 pages written per 500-row batch against 56, where no index at all writes 53. It is 5.7 MiB on that database. -Indexes are not free here. On that same 103 MiB database `dbstat` attributes 20 MiB to `(domain, timestamp)`, 18 MiB to `(client_ip, doh_token, timestamp)`, 9 MiB to the metrics index and 7 MiB to `timestamp` — the two composites added for statistics cost about a quarter of the file. Measuring an index by the file-size delta of `CREATE INDEX` understates it whenever the database is carrying a freelist, since the new pages come out of that first; `dbstat` reports the real figure. +Indexes are not free here. On that same 103 MiB database `dbstat` attributes 20 MiB to `(domain, timestamp)`, 18 MiB to `(client_ip, doh_token, timestamp)`, 9 MiB to the metrics index and 7 MiB to `timestamp` — the two composites added for statistics cost about a quarter of the file. (Those are the figures from before version 15 replaced the client index.) Measuring an index by the file-size delta of `CREATE INDEX` understates it whenever the database is carrying a freelist, since the new pages come out of that first; `dbstat` reports the real figure. ### Measuring these queries @@ -268,7 +270,7 @@ That distinction is not hypothetical. `outcome_breakdown_since` was left uncover ### One scan per index, not one per reading -The Statistics page's five readings were four foldings of the metrics index over one window and two foldings of `(domain, timestamp)` over the same one, asked as six separate statements. Each re-walked an index another had just finished with, and the read pool round-robins them across connections holding 2 MiB of page cache each, so nothing was ever warm for the next. `window_metrics_since` groups at `(blocked, cached, has_result, query_type, response_ms)` — one statement both breakdowns and the latency histogram are derived from — and `domain_stats_since` returns the top list and the distinct count from one materialized CTE. The single-purpose functions `/api/stats/*` calls are folds over the same statements, so there is one SQL spelling per fact. Rendering a 7-day window on that 370 k-row database went from 29 274 page misses (114 MiB) to 9 674 (38 MiB). +The Statistics page's five readings were four foldings of the metrics index over one window and two foldings of `(domain, timestamp)` over the same one, asked as six separate statements. Each re-walked an index another had just finished with, and the read pool round-robins them across connections holding 2 MiB of page cache each, so nothing was ever warm for the next. `window_metrics_since` groups at `(blocked, cached, has_result, query_type, response_ms)` — one statement both breakdowns and the latency histogram are derived from — and `domain_stats_since` returns the top list and the distinct count from one materialized CTE. The single-purpose functions `/api/stats/*` calls are folds over the same statements, so there is one SQL spelling per fact. (The page's domain list has since moved to `traffic_lists_since`, which answers the client list from the same scan; `domain_stats_since` remains for callers that want domains alone, and breaks ties by name the same way so the two agree.) Rendering a 7-day window on that 370 k-row database went from 29 274 page misses (114 MiB) to 9 674 (38 MiB). The window's grain has no time bucket in it. Bucketing the shared grain was what the first version did, and it cost a scan: the outcome breakdown sums across every bucket, so the bucket only multiplied the rows the folds read, 68 846 of them against 4 658 at the 7-day range's hourly grain, while the query-type and latency folds still needed a second statement of their own. Collapsing the two into one grain fine enough for all three is 2 157 pages against 4 314. diff --git a/CLAUDE.md b/CLAUDE.md index f8f112f..7d887e2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -120,7 +120,7 @@ The query log adds the conventions for **filtering and paging**: Dashboard adds the conventions for a page that is **all readings and no controls**: -- **The numbers are in the first response.** `dashboard_page` makes the five reads `app.js` used to make on its poll — `compute_summary`, `compute_top_domains` / `_clients` / `_upstreams` in `src/admin/stats.rs`, already shared with `/api/stats/*`. A failed read renders zeroes rather than an error page: a dashboard that says nothing beats one that will not load. +- **The numbers are in the first response.** `dashboard_page` makes the five reads `app.js` used to make on its poll — `compute_summary`, `compute_top_domains_and_clients` and `compute_top_upstreams` in `src/admin/stats.rs` — the same functions the `stats` snapshot calls every tick, which is why the domain and client lists come from one scan rather than one each. A failed read renders zeroes rather than an error page: a dashboard that says nothing beats one that will not load. - **`app.js` re-draws the same markup from a pushed snapshot**, so every shape in the template has a counterpart in `DashboardPage`. The five polls are gone: the numbers arrive as `stats` events on the shared stream (see *The event stream* below), and `_apply` reads the same five response bodies the fetches returned, so every renderer is the one that read them before. Number formatting is duplicated in Rust to match (`format_num_adaptive`, `percent1`, `share_percent`, `format_qps`) — a count that changed its own notation when an update landed would read as a change in the number. - **The chart is the documented exception** to no-JS: it is drawn from a timeline series by the client. The card says so rather than sitting empty, and the client replaces that text on connect. - ⚠️ **A conditional `style` must be merged into the element's existing one.** Two `style` attributes means the second is dropped — the chart card's `animation-delay` and its `display:none` are one attribute for that reason. @@ -133,7 +133,7 @@ Statistics adds the conventions for a page whose readings sit in a **chosen wind - **The range is in the URL and the switcher is three ``s** (`/stats?range=30d`), because the range picks the *server's* window. `StatsRange::label()` is the one spelling shared by the link, the parse and every card title. An unrecognised range renders the default rather than 400ing — it is a link an operator can edit, and every window on offer is spelled out right above it. - **A date the server can only write in UTC ships as an ISO day plus its timestamp** (`data-date-ts`), and `app.js` restates it in the browser's locale — the same division as the query log's relative times, and for the same reason. - **This page is measured in page misses, not milliseconds.** Development is on an SSD and the appliance runs off an SD card, so a duration measured here says nothing about a Raspberry Pi; the pages a query fetches from the file are the same on both. `cargo nextest run --release --no-capture --run-ignored only stats_page_miss` with `BENCH_DB` pointed at a copy of a real database reports them. A wall-clock reading is the thing to distrust when the two disagree — it is what left the outcome breakdown scanning the whole table through version 11. -- **One scan per index, not one per reading.** `stats_scan_since` and `domain_stats_since` (`src/db.rs`) are the page's two scans, and every reading on it — the charts included — is folded out of one of them; `compute_range_stats` (`src/admin/stats.rs`) is what the page calls. `stats_scan_since` streams its rows and folds them in Rust rather than grouping in SQL: a grain carrying both the quarter hour and `response_ms` approaches a group per row, which would be a temp b-tree the size of the window. The single-purpose functions `/api/stats/*` uses are statements of their own — adding a seventh reading to the page means folding it out of one of those two scans, not adding a statement. +- **One scan per index, not one per reading.** `stats_scan_since` and `traffic_lists_since` (`src/db.rs`) are the page's two scans — the second answering top domains, the distinct-domain count and top clients from one grouping of `(domain, client_ip, doh_token)` — and every reading on it — the charts included — is folded out of one of them; `compute_range_stats` (`src/admin/stats.rs`) is what the page calls. `stats_scan_since` streams its rows and folds them in Rust rather than grouping in SQL: a grain carrying both the quarter hour and `response_ms` approaches a group per row, which would be a temp b-tree the size of the window. The single-purpose functions `/api/stats/*` uses are statements of their own — adding a seventh reading to the page means folding it out of one of those two scans, not adding a statement. - **A total nobody can count cheaply is maintained, not counted.** `query_logs`' row count lives in `settings` (`query_log_count`), moved by the insert batch, the prune and Clear All inside their own transactions — `SELECT COUNT(*)` walks an index end to end and the Database Health card asks on every load. A fourth write path to `query_logs` means a fourth `bump_log_count`, not a fourth reader. - Four bar lists in one template share **one askama macro** (`templates/_macros.html`); `{% call … %}` needs a matching `{% endcall %}` in askama 0.16, and `{% include %}` cannot see a loop variable at all. diff --git a/src/admin/events.rs b/src/admin/events.rs index 7f814b5..4f31d1a 100644 --- a/src/admin/events.rs +++ b/src/admin/events.rs @@ -147,11 +147,10 @@ impl Drop for StatsGuard { /// Read everything the dashboard shows, in the shapes `app.js` renders. pub async fn compute_snapshot(db: &Database, now: i64) -> Result { - let (summary, timeline, top_domains, top_clients, top_upstreams) = tokio::try_join!( + let (summary, timeline, (top_domains, top_clients), top_upstreams) = tokio::try_join!( stats::compute_summary(db, now), stats::compute_timeline(db, now, TIMELINE_HOURS), - stats::compute_top_domains(db, now, TOP_N), - stats::compute_top_clients(db, now, TOP_N), + stats::compute_top_domains_and_clients(db, now, TOP_N), stats::compute_top_upstreams(db, now, TOP_N), )?; diff --git a/src/admin/pages.rs b/src/admin/pages.rs index 9745aad..0ed2e9b 100644 --- a/src/admin/pages.rs +++ b/src/admin/pages.rs @@ -1442,12 +1442,10 @@ pub async fn dashboard_page( .unwrap_or_default(); // Ten rows each, which is what the page shows — the API's larger default is // for callers who want to do their own slicing. - let domains = crate::admin::stats::compute_top_domains(&state.db, now, 10) - .await - .unwrap_or_default(); - let clients = crate::admin::stats::compute_top_clients(&state.db, now, 10) - .await - .unwrap_or_default(); + let (domains, clients) = + crate::admin::stats::compute_top_domains_and_clients(&state.db, now, 10) + .await + .unwrap_or_default(); let upstreams = crate::admin::stats::compute_top_upstreams(&state.db, now, 10) .await .unwrap_or_default(); @@ -1667,11 +1665,10 @@ pub async fn stats_page( .and_then(stats::StatsRange::parse) .unwrap_or(stats::StatsRange::Days7); - // Three independent reads; running them together keeps the page at one - // round trip to the database rather than three in sequence. - let (range_stats, clients, health) = tokio::join!( + // Two independent reads; running them together keeps the page at one + // round trip to the database rather than two in sequence. + let (range_stats, health) = tokio::join!( stats::compute_range_stats(&state.db, now, range, STATS_TOP_N), - stats::compute_top_clients_ranged(&state.db, now, range, STATS_TOP_N), stats::compute_db_health(&state.db, now), ); @@ -1719,7 +1716,7 @@ pub async fn stats_page( ) .expect("a series of integers always serializes"); - let (query_types, outcomes, top_domains) = range_stats + let (query_types, outcomes, top_domains, top_clients) = range_stats .map(|s| { ( bar_rows(s.metrics.query_types), @@ -1731,24 +1728,26 @@ pub async fn stats_page( .map(|d| (d.domain, d.count)) .collect(), ), + // A client that came in over `DoH` is named by both, the way the + // client draws it — the IP alone would collapse every token + // behind one proxy. + bar_rows( + s.clients + .into_iter() + .map(|c| { + let label = match c.doh_token { + Some(token) if !token.is_empty() => { + format!("{} · {token}", c.client_ip) + } + _ => c.client_ip, + }; + (label, c.count) + }) + .collect(), + ), ) }) .unwrap_or_default(); - // A client that came in over `DoH` is named by both, the way the client - // draws it — the IP alone would collapse every token behind one proxy. - let top_clients = bar_rows( - clients - .unwrap_or_default() - .into_iter() - .map(|c| { - let label = match c.doh_token { - Some(token) if !token.is_empty() => format!("{} · {token}", c.client_ip), - _ => c.client_ip, - }; - (label, c.count) - }) - .collect(), - ); let health_cards = health.map(build_health_cards).unwrap_or_default(); diff --git a/src/admin/stats.rs b/src/admin/stats.rs index 06d27d5..33f06c9 100644 --- a/src/admin/stats.rs +++ b/src/admin/stats.rs @@ -153,6 +153,18 @@ pub async fn compute_top_clients( db.top_clients_since(since, limit).await } +/// The dashboard's two 24-hour lists from the one scan that answers both — +/// what it shows every tick, where asking [`compute_top_domains`] and +/// [`compute_top_clients`] separately would read two indexes. +pub async fn compute_top_domains_and_clients( + db: &Database, + now: i64, + limit: i64, +) -> Result<(Vec, Vec), DbError> { + let lists = db.traffic_lists_since(now - 86400, limit).await?; + Ok((lists.domains.top, lists.clients)) +} + pub async fn compute_top_upstreams( db: &Database, now: i64, @@ -343,15 +355,17 @@ pub async fn compute_highlights( /// a top-domain list — and every one of them re-scanned an index another had /// just walked. Three of them share /// [`crate::db::Database::stats_scan_since`] and two share -/// [`crate::db::Database::domain_stats_since`], which is two index scans +/// [`crate::db::Database::traffic_lists_since`], which is two index scans /// instead of six. /// /// The charts ride the first of those scans as `series`, rather than being -/// fetched by the browser afterwards at the cost of two more. +/// fetched by the browser afterwards at the cost of two more, and the top +/// clients ride the second rather than scanning an index of their own. pub struct RangeStats { pub metrics: crate::db::WindowMetrics, pub series: crate::db::QuarterSeries, pub domains: crate::db::DomainStats, + pub clients: Vec, } pub async fn compute_range_stats( @@ -362,14 +376,15 @@ pub async fn compute_range_stats( ) -> Result { let (window_secs, _) = range.window(); let since = now - window_secs; - let (scan, domains) = tokio::try_join!( + let (scan, lists) = tokio::try_join!( db.stats_scan_since(since, now - HEATMAP_WINDOW_SECS), - db.domain_stats_since(since, top_n), + db.traffic_lists_since(since, top_n), )?; Ok(RangeStats { metrics: scan.metrics, series: scan.series, - domains, + domains: lists.domains, + clients: lists.clients, }) } diff --git a/src/db.rs b/src/db.rs index cd91045..3f0f0e5 100644 --- a/src/db.rs +++ b/src/db.rs @@ -333,6 +333,14 @@ pub struct DomainStats { pub top: Vec, } +/// Who asked for what in a window: the busiest domains with the distinct count, +/// and the busiest clients — see [`Database::traffic_lists_since`]. +#[derive(Debug, Clone)] +pub struct TrafficLists { + pub domains: DomainStats, + pub clients: Vec, +} + #[derive(Debug, Clone, Serialize)] pub struct HeatmapCell { pub weekday: i64, // 0 = Sunday, 6 = Saturday (matches strftime('%w')) @@ -597,7 +605,7 @@ impl Database { ); CREATE INDEX IF NOT EXISTS idx_query_logs_timestamp ON query_logs(timestamp); CREATE INDEX IF NOT EXISTS idx_query_logs_domain_ts ON query_logs(domain, timestamp); - CREATE INDEX IF NOT EXISTS idx_query_logs_client_ts ON query_logs(client_ip, doh_token, timestamp); + CREATE INDEX IF NOT EXISTS idx_query_logs_ts_domain_client ON query_logs(timestamp, domain, client_ip, doh_token); -- Without has_result: a database predating version 12 has -- no such column until that migration adds it, and this -- batch runs first. Migration 12 rebuilds the index with it @@ -888,7 +896,29 @@ impl Database { )?; } - const LATEST_VERSION: i64 = 14; + if version < 15 { + // Top domains and top clients are asked together — once by the + // Statistics page, once per dashboard tick — and read two indexes + // for it: `(domain, timestamp)` and `(client_ip, doh_token, + // timestamp)`, each group-first so neither could be restricted by + // the window. On a 370 k-row database that was 7 589 pages for a + // window covering the table and 2 389 for the dashboard's 24 hours. + // One timestamp-first index carrying both answers both lists from + // one scan: 5 769 and 747. + // + // It replaces the client index, which served nothing else. The + // domain index stays: the query log's domain search seeks it by + // prefix, and without it a search for a prefix nobody queried read + // the whole table — 12 170 pages against 3. + conn.execute_batch( + "DROP INDEX IF EXISTS idx_query_logs_client_ts; + CREATE INDEX IF NOT EXISTS idx_query_logs_ts_domain_client \ + ON query_logs(timestamp, domain, client_ip, doh_token); + ANALYZE;", + )?; + } + + const LATEST_VERSION: i64 = 15; if version < LATEST_VERSION { conn.pragma_update(None, "user_version", LATEST_VERSION)?; } @@ -2136,7 +2166,7 @@ impl Database { .reader() .call(move |conn| { let mut stmt = conn.prepare_cached( - "WITH d AS ( SELECT domain, COUNT(*) AS cnt FROM query_logs WHERE timestamp >= ?1 GROUP BY domain ) SELECT (SELECT COUNT(*) FROM d), domain, cnt FROM d ORDER BY cnt DESC LIMIT ?2", + "WITH d AS ( SELECT domain, COUNT(*) AS cnt FROM query_logs WHERE timestamp >= ?1 GROUP BY domain ) SELECT (SELECT COUNT(*) FROM d), domain, cnt FROM d ORDER BY cnt DESC, domain LIMIT ?2", )?; let rows = stmt .query_map(params![since_ms, limit], |row| { @@ -2159,31 +2189,89 @@ impl Database { Ok(stats) } + /// A fold over [`Self::traffic_lists_since`], whose index is the only one + /// carrying the client columns. pub async fn top_clients_since( &self, since: i64, limit: i64, ) -> Result, DbError> { + Ok(self.traffic_lists_since(since, limit).await?.clients) + } + + /// The busiest domains, how many distinct ones there were, and the busiest + /// clients, from one scan of `idx_query_logs_ts_domain_client`. + /// + /// The Statistics page and every dashboard tick want both lists, and used + /// to read two indexes for them. Grouped at `(domain, client_ip, + /// doh_token)`, one statement carries both: the rows are one per pairing a + /// window actually held, which on a home network is a few thousand, and + /// both lists are folded out of them here. The index is timestamp-first, + /// so a short window reads a short stretch of it. + /// + /// Ties in either list break by name, the same way + /// [`Self::domain_stats_since`] breaks them, so the two spellings of top + /// domains agree row for row. + pub async fn traffic_lists_since( + &self, + since: i64, + limit: i64, + ) -> Result { let since_ms = since * 1000; - let rows = self + let limit = usize::try_from(limit).unwrap_or(0); + let lists = self .reader() .call(move |conn| { + // `INDEXED BY` because `idx_query_logs_timestamp` also matches + // the range and is smaller; taking it would be a rowid lookup + // per row. let mut stmt = conn.prepare_cached( - "SELECT client_ip, doh_token, COUNT(*) as cnt FROM query_logs WHERE timestamp >= ?1 GROUP BY client_ip, doh_token ORDER BY cnt DESC LIMIT ?2", + "SELECT domain, client_ip, doh_token, COUNT(*) \ + FROM query_logs INDEXED BY idx_query_logs_ts_domain_client \ + WHERE timestamp >= ?1 \ + GROUP BY domain, client_ip, doh_token", )?; - let rows = stmt - .query_map(params![since_ms, limit], |row| { - Ok(TopClient { - client_ip: row.get(0)?, - doh_token: row.get(1)?, - count: row.get(2)?, - }) - })? - .collect::, _>>()?; - Ok(rows) + let mut domains: HashMap = HashMap::new(); + let mut clients: HashMap<(String, Option), i64> = HashMap::new(); + let mut rows = stmt.query(params![since_ms])?; + while let Some(row) = rows.next()? { + let count: i64 = row.get(3)?; + *domains.entry(row.get(0)?).or_default() += count; + *clients.entry((row.get(1)?, row.get(2)?)).or_default() += count; + } + Ok((domains, clients)) }) .await?; - Ok(rows) + let (domains, clients) = lists; + + let unique = i64::try_from(domains.len()).unwrap_or(i64::MAX); + let mut top: Vec = domains + .into_iter() + .map(|(domain, count)| TopDomain { domain, count }) + .collect(); + top.sort_unstable_by(|a, b| b.count.cmp(&a.count).then_with(|| a.domain.cmp(&b.domain))); + top.truncate(limit); + + let mut clients: Vec = clients + .into_iter() + .map(|((client_ip, doh_token), count)| TopClient { + client_ip, + doh_token, + count, + }) + .collect(); + clients.sort_unstable_by(|a, b| { + b.count + .cmp(&a.count) + .then_with(|| a.client_ip.cmp(&b.client_ip)) + .then_with(|| a.doh_token.cmp(&b.doh_token)) + }); + clients.truncate(limit); + + Ok(TrafficLists { + domains: DomainStats { unique, top }, + clients, + }) } pub async fn top_upstreams_since( @@ -3174,15 +3262,21 @@ mod tests { } #[tokio::test] - async fn fresh_schema_has_client_index() { + async fn fresh_schema_has_the_domain_client_index_and_not_the_client_one() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("noadd.sqlite3"); let db = Database::open(path.to_str().unwrap()).await.unwrap(); let indexes = query_log_index_names(&db).await; assert!( - indexes.iter().any(|n| n == "idx_query_logs_client_ts"), - "(client_ip, doh_token, timestamp) index should exist: {indexes:?}" + indexes + .iter() + .any(|n| n == "idx_query_logs_ts_domain_client"), + "(timestamp, domain, client_ip, doh_token) index should exist: {indexes:?}" + ); + assert!( + !indexes.iter().any(|n| n == "idx_query_logs_client_ts"), + "the client index it replaced should not: {indexes:?}" ); } @@ -3466,8 +3560,11 @@ mod tests { assert!((top[0].avg_ms - 20.0).abs() < 1e-9); } + /// Version 10 added the client index and version 15 replaced it, so a + /// database from before either has to come out holding the replacement and + /// nothing of the index in between. #[tokio::test] - async fn migration_v10_adds_client_index() { + async fn a_v9_database_migrates_to_the_domain_client_index() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("v9.db"); let path_str = path.to_str().unwrap().to_string(); @@ -3501,16 +3598,22 @@ mod tests { let indexes = query_log_index_names(&db).await; assert!( - indexes.iter().any(|n| n == "idx_query_logs_client_ts"), - "client index should be created by migration: {indexes:?}" + indexes + .iter() + .any(|n| n == "idx_query_logs_ts_domain_client"), + "domain/client index should be created by migration: {indexes:?}" + ); + assert!( + !indexes.iter().any(|n| n == "idx_query_logs_client_ts"), + "the client index should be gone after migration: {indexes:?}" ); } - /// The index only pays off if the planner actually picks it — the - /// version-5 migration's comment records that a new index alone was not - /// enough there. Assert the plan, not just the index's existence. + /// `INDEXED BY` fixes which index the lists read, but not whether that + /// index answers them alone. Assert the plan is covering, which is what + /// keeps a table lookup per row out of every dashboard tick. #[tokio::test] - async fn top_clients_query_uses_the_client_index() { + async fn the_traffic_lists_are_covered_by_their_index() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("plan.db"); let db = Database::open(path.to_str().unwrap()).await.unwrap(); @@ -3538,13 +3641,15 @@ mod tests { .conn .call(|conn| { conn.execute_batch("ANALYZE;")?; + // The statement `traffic_lists_since` prepares, verbatim. let mut stmt = conn.prepare( - "EXPLAIN QUERY PLAN SELECT client_ip, doh_token, COUNT(*) c \ - FROM query_logs WHERE timestamp >= ?1 \ - GROUP BY client_ip, doh_token ORDER BY c DESC LIMIT ?2", + "EXPLAIN QUERY PLAN SELECT domain, client_ip, doh_token, COUNT(*) \ + FROM query_logs INDEXED BY idx_query_logs_ts_domain_client \ + WHERE timestamp >= ?1 \ + GROUP BY domain, client_ip, doh_token", )?; let rows = stmt - .query_map(params![0_i64, 10_i64], |row| row.get::<_, String>(3))? + .query_map(params![0_i64], |row| row.get::<_, String>(3))? .collect::, _>>()?; Ok::<_, tokio_rusqlite::Error>(rows.join(" | ")) }) @@ -3552,8 +3657,8 @@ mod tests { .unwrap(); assert!( - plan.contains("idx_query_logs_client_ts"), - "top-clients query should be served by the client index, got: {plan}" + plan.contains("COVERING INDEX idx_query_logs_ts_domain_client"), + "the traffic lists should read their index alone, got: {plan}" ); } diff --git a/tests/stats_db_test.rs b/tests/stats_db_test.rs index 949d05e..09566e6 100644 --- a/tests/stats_db_test.rs +++ b/tests/stats_db_test.rs @@ -601,6 +601,70 @@ async fn domain_stats_on_an_empty_window_reports_nothing() { assert!(stats.top.is_empty()); } +/// Both lists come out of one grouping of `(domain, client_ip, doh_token)`, so +/// the folds have to put back what the grouping split: a domain queried by +/// several clients is one domain, and a client over `DoH` is a different +/// client from the same IP over plain DNS. Counts are chosen to tie, so the +/// order ties break in is asserted too. +#[tokio::test] +async fn traffic_lists_answer_what_the_separate_lists_did() { + let db = test_db().await; + let since = 10_000; + let mut entries = Vec::new(); + for i in 0..600_i64 { + let mut e = entry(since + i, "A", false, false, Some("1.1.1.1")); + e.domain = format!("d{}.test", i % 7); + e.client_ip = format!("10.0.0.{}", i % 5); + e.doh_token = (i % 3 == 0).then(|| "phone".to_string()); + entries.push(e); + } + // Before the window: counted by nothing. + let mut early = entry(since - 1, "A", false, false, Some("1.1.1.1")); + early.domain = "d0.test".into(); + early.client_ip = "10.0.0.9".into(); + entries.push(early); + db.insert_query_logs(&entries).await.unwrap(); + + let lists = db.traffic_lists_since(since, 4).await.unwrap(); + + let domains = db.domain_stats_since(since, 4).await.unwrap(); + assert_eq!(lists.domains.unique, domains.unique); + assert_eq!(lists.domains.unique, 7); + assert_eq!(lists.domains.top, domains.top, "the two spellings disagree"); + + let mut expected: std::collections::HashMap<(String, Option), i64> = + std::collections::HashMap::new(); + for e in entries.iter().filter(|e| e.timestamp >= since * 1000) { + *expected + .entry((e.client_ip.clone(), e.doh_token.clone())) + .or_default() += 1; + } + let mut expected: Vec<_> = expected.into_iter().collect(); + expected.sort_by(|a, b| b.1.cmp(&a.1).then_with(|| a.0.cmp(&b.0))); + expected.truncate(4); + let got: Vec<_> = lists + .clients + .iter() + .map(|c| ((c.client_ip.clone(), c.doh_token.clone()), c.count)) + .collect(); + assert_eq!(got, expected); + assert!( + got.iter().all(|((ip, _), _)| ip != "10.0.0.9"), + "a query from before the window was counted" + ); + + assert_eq!(db.top_clients_since(since, 4).await.unwrap().len(), 4); +} + +#[tokio::test] +async fn traffic_lists_on_an_empty_window_report_nothing() { + let db = test_db().await; + let lists = db.traffic_lists_since(0, 5).await.unwrap(); + assert_eq!(lists.domains.unique, 0); + assert!(lists.domains.top.is_empty()); + assert!(lists.clients.is_empty()); +} + fn sorted(mut rows: Vec<(String, i64)>) -> Vec<(String, i64)> { rows.sort(); rows diff --git a/tests/stats_page_miss_bench.rs b/tests/stats_page_miss_bench.rs index 235ab14..27d78e6 100644 --- a/tests/stats_page_miss_bench.rs +++ b/tests/stats_page_miss_bench.rs @@ -18,7 +18,7 @@ use noadd::admin::stats::{ self, StatsRange, compute_db_health, compute_heatmap, compute_range_stats, - compute_stats_timeline, compute_top_clients_ranged, + compute_stats_timeline, }; use noadd::db::Database; use noadd::now_unix; @@ -64,13 +64,9 @@ async fn stats_page_miss_bench() { // Exactly what `stats_page` reads, in the order the template consumes it. let mut rows: Vec<(&str, i64)> = Vec::new(); rows.push(( - "range_stats (breakdowns+latency+charts+domains)", + "range_stats (all readings, charts, both lists)", page_misses(&db, || compute_range_stats(&db, now, range, TOP_N)).await, )); - rows.push(( - "top_clients", - page_misses(&db, || compute_top_clients_ranged(&db, now, range, TOP_N)).await, - )); rows.push(( "db_health", page_misses(&db, || compute_db_health(&db, now)).await, @@ -103,12 +99,19 @@ async fn stats_page_miss_bench() { page_misses(&db, || stats::compute_highlights(&db, now, range)).await, ), ( - " top_domains alone", + " top_domains alone (domain index)", page_misses(&db, || { stats::compute_top_domains_ranged(&db, now, range, TOP_N) }) .await, ), + ( + " both lists alone", + page_misses(&db, || { + stats::compute_top_clients_ranged(&db, now, range, TOP_N) + }) + .await, + ), ]; let mib = |pages: i64| (pages * 4096) as f64 / (1024.0 * 1024.0); diff --git a/tests/stats_page_miss_test.rs b/tests/stats_page_miss_test.rs index c15fc7a..5a346ce 100644 --- a/tests/stats_page_miss_test.rs +++ b/tests/stats_page_miss_test.rs @@ -98,6 +98,10 @@ async fn the_page_reads_less_than_its_readings_do_separately() { + page_misses(&db, || { stats::compute_top_domains_ranged(&db, now, range, 15) }) + .await + + page_misses(&db, || { + stats::compute_top_clients_ranged(&db, now, range, 15) + }) .await; assert!( @@ -242,6 +246,29 @@ async fn the_top_upstreams_never_read_the_log_table() { ); } +/// The domain and client lists read an index that starts with `timestamp`, so a +/// short window — the dashboard's 24 hours against a week of retention — reads +/// a short stretch of it. The group-first indexes they replaced could not be +/// restricted by the window at all, and read most of themselves whatever it was. +#[tokio::test] +async fn a_short_window_reads_a_short_stretch_of_the_traffic_lists() { + let db = seeded_db().await; + + let whole = page_misses(&db, || db.traffic_lists_since(0, 15)).await; + // The last tenth of the seed. + let tail = page_misses(&db, || db.traffic_lists_since(ROWS - ROWS / 10, 15)).await; + + assert!( + tail > 0, + "no pages were read at all — the measurement is not working" + ); + assert!( + tail * 4 < whole, + "a tenth of the window read {tail} pages against {whole} for all of it — \ + is the index still timestamp-first, and still named by INDEXED BY?" + ); +} + /// The heatmap reads `timestamp` and nothing else, so it belongs on the /// smallest index that carries it. `idx_query_logs_ts_metrics` also covers it /// and the planner will take it unaided, paying for four columns the query