From e48c50670e23f8bef6d1df2e7268141510b38432 Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Tue, 10 Nov 2026 22:05:26 -0800 Subject: [PATCH 1/4] fix(metrics): Keep each account's metrics apart The metrics table had no account column on any backend, so two accounts that each had a table of the same name wrote to one row, and the counts and latencies /metrics reported for that name were the two tables' added together. Every metric of a request, and the TTL metrics of a table, now carry the account they were recorded for: the collector's key, the flushed bucket and the stored row have an account, and /metrics reports it as an AccountId dimension and takes an account_id filter. Metrics of the whole deployment (pool, worker, storage query, backup jobs) have none. Migration 10, account_metrics, replaces the catalog's metrics table (a collection on MongoDB) with account_metrics, whose key has the account, on every backend. A partition key cannot be changed on Cassandra and a primary key cannot be altered on SQLite, so the table is new everywhere. The old rows carry no account and are dropped. The migration is breaking and convergent. MetricsStore::query_metrics takes a MetricsFilter (account, table name, metric). extenddb_storage::metrics_conformance checks each backend's store against a migrated catalog. Part of #728. --- CONTRIBUTING.md | 47 ++-- crates/core/src/metrics/collector.rs | 164 +++++++++++-- crates/core/src/metrics/collector_query.rs | 10 + crates/core/src/metrics/collector_tests.rs | 99 +++++++- crates/core/src/metrics/drain_tests.rs | 12 +- crates/core/src/metrics/types.rs | 9 + .../pages/account_scope_reads_tests.rs | 6 +- .../server/src/console/pages/metrics_pages.rs | 6 +- .../src/console/pages/metrics_pages_tests.rs | 37 ++- crates/server/src/handler.rs | 54 +++-- crates/server/src/metrics_endpoint.rs | 89 ++++++- crates/server/src/response.rs | 26 ++- crates/server/src/workers.rs | 23 +- .../catalog/0010_account_metrics.cql | 28 +++ crates/storage-cassandra/src/catalog_store.rs | 33 ++- .../storage-cassandra/src/metadata_engine.rs | 2 +- .../storage-cassandra/src/migrations/mod.rs | 9 + crates/storage-cassandra/src/ttl_worker.rs | 5 +- .../tests/direct/keyspace_migrations.rs | 14 +- .../tests/direct/metrics_store.rs | 24 ++ .../tests/direct_integration.rs | 2 + .../0010_account_metrics.expected.json | 65 ++++++ crates/storage-cassandra/tests/migrations.rs | 4 +- .../tests/migrations_support/mod.rs | 17 +- crates/storage-mongodb/src/catalog_store.rs | 9 + .../storage-mongodb/src/management_store.rs | 51 ++-- .../src/migrations/m0010_account_metrics.rs | 91 ++++++++ crates/storage-mongodb/src/migrations/mod.rs | 12 + .../storage-mongodb/src/migrations/tests.rs | 7 + crates/storage-mongodb/src/ttl_worker.rs | 4 +- .../0010_account_metrics.expected.json | 26 +++ crates/storage-mongodb/tests/metrics_store.rs | 73 ++++++ .../storage-mongodb/tests/migrations_fresh.rs | 38 ++- .../tests/migrations_guards.rs | 1 + .../storage-mongodb/tests/migrations_locks.rs | 2 +- .../tests/migrations_released.rs | 9 +- .../tests/migrations_support/mod.rs | 24 +- .../migrations/0010_account_metrics.sql | 28 +++ crates/storage-postgres/src/catalog_store.rs | 78 +++---- crates/storage-postgres/src/migrations/mod.rs | 12 +- crates/storage-postgres/src/ttl_worker.rs | 4 +- .../storage-postgres/tests/metrics_store.rs | 81 +++++++ .../migrations/0010_account_metrics.sql | 28 +++ crates/storage-sqlite/src/catalog_store.rs | 52 +++-- crates/storage-sqlite/src/migrations/mod.rs | 9 + .../src/migrations/released_tests.rs | 56 ++++- crates/storage-sqlite/src/migrations/tests.rs | 6 +- crates/storage-sqlite/src/workers.rs | 4 +- crates/storage/src/lib.rs | 3 + crates/storage/src/management_store/mod.rs | 11 +- crates/storage/src/management_store/types.rs | 35 +++ crates/storage/src/metrics_conformance.rs | 179 ++++++++++++++ crates/storage/src/migrations/registry.rs | 12 +- docs/design/06-component-server.md | 12 +- docs/getting-started.md | 6 +- docs/manuals/04-quickstart-setup-guide.md | 4 +- docs/manuals/07-upgrade-manual.md | 16 +- docs/manuals/08-install-linux.md | 2 +- docs/manuals/09-install-macos.md | 2 +- docs/manuals/10-security-model.md | 2 +- docs/manuals/12-extending-extenddb-storage.md | 7 +- docs/schema/cassandra.txt | 21 +- docs/schema/mongodb.txt | 6 +- docs/schema/postgres.txt | 27 +-- docs/schema/sqlite.txt | 4 +- tests/test_console_metrics_admin_only.py | 7 +- tests/test_metrics_accounts.py | 221 ++++++++++++++++++ tests/test_resource_tag_migration.py | 4 + 68 files changed, 1759 insertions(+), 312 deletions(-) create mode 100644 crates/storage-cassandra/migrations/catalog/0010_account_metrics.cql create mode 100644 crates/storage-cassandra/tests/direct/metrics_store.rs create mode 100644 crates/storage-cassandra/tests/fixtures/0010_account_metrics.expected.json create mode 100644 crates/storage-mongodb/src/migrations/m0010_account_metrics.rs create mode 100644 crates/storage-mongodb/tests/fixtures/0010_account_metrics.expected.json create mode 100644 crates/storage-mongodb/tests/metrics_store.rs create mode 100644 crates/storage-postgres/migrations/0010_account_metrics.sql create mode 100644 crates/storage-postgres/tests/metrics_store.rs create mode 100644 crates/storage-sqlite/migrations/0010_account_metrics.sql create mode 100644 crates/storage/src/metrics_conformance.rs create mode 100644 tests/test_metrics_accounts.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f32f5ec26..4cabe5354 100755 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -375,28 +375,28 @@ SQLite, PostgreSQL, Cassandra and MongoDB migrate through this framework. ### Example: a nullable column on `tables` -Migration 10, `table_note` (10 being the next free id at the time of writing; -migrations 6, 7, 8 and 9 are `backup_catalog_index`, `stream_pending`, -`restore_progress` and `warm_throughput`), adds a nullable `note TEXT` column -to the catalog's `tables` table on SQLite and PostgreSQL. +Migration 11, `table_note` (11 being the next free id at the time of writing; +migrations 6, 7, 8, 9 and 10 are `backup_catalog_index`, `stream_pending`, +`restore_progress`, `warm_throughput` and `account_metrics`), adds a nullable +`note TEXT` column to the catalog's `tables` table on SQLite and PostgreSQL. In `crates/storage/src/migrations/registry.rs`, at the end of the `migration_ids!` list: ```rust /// Adds a nullable `note` column to the `tables` catalog table. - TableNote = 10, "table_note", + TableNote = 11, "table_note", "add a nullable note column to the tables catalog"; ``` PostgreSQL has `ADD COLUMN IF NOT EXISTS`, so its body is a file, -`crates/storage-postgres/migrations/0010_table_note.sql`: +`crates/storage-postgres/migrations/0011_table_note.sql`: ```sql -- Copyright 2026 ExtendDB contributors -- SPDX-License-Identifier: Apache-2.0 -- --- Migration 10, table_note, PostgreSQL catalog database: a nullable note +-- Migration 11, table_note, PostgreSQL catalog database: a nullable note -- column on tables. IF NOT EXISTS makes a second run change nothing. ALTER TABLE tables ADD COLUMN IF NOT EXISTS note TEXT; @@ -411,7 +411,7 @@ In `crates/storage-postgres/src/migrations/mod.rs`, the arm in `migration()`: bodies: vec![file_body!( StoreKind::Catalog, "migrations", - "0010_table_note.sql" + "0011_table_note.sql" )], }, ``` @@ -427,19 +427,20 @@ and the id in the transactional arm of `runs_in()`: | MigrationId::StreamPending | MigrationId::RestoreProgress | MigrationId::WarmThroughput + | MigrationId::AccountMetrics | MigrationId::TableNote => RunsIn::Transaction, ``` SQLite has no `ADD COLUMN IF NOT EXISTS`, and a file with a plain `ADD COLUMN` fails when it runs a second time, so the SQLite body is code that checks for the column first. -`crates/storage-sqlite/src/migrations/m0010_table_note.rs`: +`crates/storage-sqlite/src/migrations/m0011_table_note.rs`: ```rust // Copyright 2026 ExtendDB contributors // SPDX-License-Identifier: Apache-2.0 -//! Migration 10, `table_note`, SQLite: a nullable `note` column on `tables`. +//! Migration 11, `table_note`, SQLite: a nullable `note` column on `tables`. //! //! SQLite has no `ADD COLUMN IF NOT EXISTS`, so the body reads the table's //! columns first and adds the column only when it is missing. A second run @@ -451,10 +452,10 @@ use extenddb_storage::migrations::{CodeBody, Done, MigrationError}; use super::SqliteStores; /// This file, whose SHA-256 the checksum test pins. -pub(crate) const SOURCE: &str = include_str!("m0010_table_note.rs"); +pub(crate) const SOURCE: &str = include_str!("m0011_table_note.rs"); /// Recorded as this migration's checksum. Changes only with a new migration. -const FINGERPRINT: &str = "0010_table_note/sqlite/1"; +const FINGERPRINT: &str = "0011_table_note/sqlite/1"; pub(crate) struct TableNote; @@ -493,7 +494,7 @@ beside the others: ```rust #[rustfmt::skip] #[allow(clippy::all, deprecated)] -mod m0010_table_note; +mod m0011_table_note; ``` and the arm in `migration()`: @@ -504,8 +505,8 @@ and the arm in `migration()`: convergent: true, bodies: vec![StoreBody::code( StoreKind::Catalog, - &m0010_table_note::TableNote, - m0010_table_note::SOURCE, + &m0011_table_note::TableNote, + m0011_table_note::SOURCE, )], }, ``` @@ -517,8 +518,8 @@ rows of `tables`: ```rust #[tokio::test] -async fn migration_10_adds_the_note_column_to_tables() { - let db = temp_db("m10"); +async fn migration_11_adds_the_note_column_to_tables() { + let db = temp_db("m11"); load_dump(&db, DEV_0_1_14).await; run(&db).await.expect("migrate"); let mut text = dump( @@ -528,12 +529,12 @@ async fn migration_10_adds_the_note_column_to_tables() { ) .await; text.push_str(&dump(&db, "SELECT table_name, note FROM tables ORDER BY table_name").await); - golden("0010_table_note", &text); + golden("0011_table_note", &text); } ``` -`EXTENDDB_BLESS_EXPECTED=1 cargo test -p extenddb-storage-sqlite --lib migration_10` -writes `crates/storage-sqlite/tests/fixtures/golden/0010_table_note.txt`, one +`EXTENDDB_BLESS_EXPECTED=1 cargo test -p extenddb-storage-sqlite --lib migration_11` +writes `crates/storage-sqlite/tests/fixtures/golden/0011_table_note.txt`, one tab-separated line per row under each query: ``` @@ -548,15 +549,15 @@ The PostgreSQL body is a file, so it has no golden test. The example declares `Kind::Breaking`. A nullable column can be additive by the rule in step 4, but declaring it additive needs the old-binary job, which this example does not add. As a breaking migration it makes a 1.x binary -without migration 10 refuse a catalog that has it, and a rollback restores the +without migration 11 refuse a catalog that has it, and a rollback restores the backup. The same PR changes the code that reads and writes `tables.note` in each backend's catalog store, and commits the `docs/schema/sqlite.txt` and `docs/schema/postgres.txt` that the integration jobs regenerate. It changes no per-table DDL, because `tables` is a catalog table, and adds no `RELEASED` -entry. `extenddb migrate --list` then shows migration 10, and on a catalog at -migration 9 `extenddb migrate` lists migration 10 as pending and exits 3. +entry. `extenddb migrate --list` then shows migration 11, and on a catalog at +migration 10 `extenddb migrate` lists migration 11 as pending and exits 3. ### Running the migration tests diff --git a/crates/core/src/metrics/collector.rs b/crates/core/src/metrics/collector.rs index f6a3e6f68..e0f52bc1b 100755 --- a/crates/core/src/metrics/collector.rs +++ b/crates/core/src/metrics/collector.rs @@ -27,6 +27,9 @@ pub(super) fn window_cutoff(window: TimeWindow, now: Instant) -> Option #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub(super) struct MetricKey { pub(super) metric: MetricName, + /// The account whose request or table the point belongs to; `None` for a + /// metric of the whole deployment (workers, pools, backup jobs). + pub(super) account_id: Option, pub(super) table_name: Option, pub(super) index_name: Option, pub(super) operation: Option, @@ -49,6 +52,8 @@ pub struct MetricsCollector { #[derive(Debug, Clone)] pub(super) struct SegmentPoint { pub(super) operation: String, + #[allow(dead_code)] // TODO(cleanup): used when console adds per-account latency breakdown + pub(super) account_id: String, #[allow(dead_code)] // TODO(cleanup): used when console adds table-scoped latency breakdown pub(super) table_name: Option, pub(super) segments: LatencySegments, @@ -67,16 +72,23 @@ impl MetricsCollector { } /// Record a counter metric (e.g., consumed capacity, error count). + /// + /// `account_id` is the account of the request or table the value belongs + /// to, and `None` for a metric of the whole deployment. Points of two + /// accounts never share a key, so two accounts' tables of the same name + /// are kept apart. pub fn record( &self, metric: MetricName, value: f64, + account_id: Option<&str>, table_name: Option<&str>, index_name: Option<&str>, operation: Option<&str>, ) { let key = MetricKey { metric, + account_id: account_id.map(ToOwned::to_owned), table_name: table_name.map(ToOwned::to_owned), index_name: index_name.map(ToOwned::to_owned), operation: operation.map(ToOwned::to_owned), @@ -101,10 +113,17 @@ impl MetricsCollector { } /// Record a latency observation in microseconds. - pub fn record_latency(&self, table_name: Option<&str>, operation: &str, latency_us: f64) { + pub fn record_latency( + &self, + account_id: &str, + table_name: Option<&str>, + operation: &str, + latency_us: f64, + ) { self.record( MetricName::SuccessfulRequestLatency, latency_us, + Some(account_id), table_name, None, Some(operation), @@ -112,10 +131,17 @@ impl MetricsCollector { } /// Record a successful request's consumed read capacity. - pub fn record_read_capacity(&self, table_name: &str, operation: &str, units: f64) { + pub fn record_read_capacity( + &self, + account_id: &str, + table_name: &str, + operation: &str, + units: f64, + ) { self.record( MetricName::ConsumedReadCapacityUnits, units, + Some(account_id), Some(table_name), None, Some(operation), @@ -123,10 +149,17 @@ impl MetricsCollector { } /// Record a successful request's consumed write capacity. - pub fn record_write_capacity(&self, table_name: &str, operation: &str, units: f64) { + pub fn record_write_capacity( + &self, + account_id: &str, + table_name: &str, + operation: &str, + units: f64, + ) { self.record( MetricName::ConsumedWriteCapacityUnits, units, + Some(account_id), Some(table_name), None, Some(operation), @@ -134,10 +167,11 @@ impl MetricsCollector { } /// Record a user error (4xx). - pub fn record_user_error(&self, table_name: Option<&str>, operation: &str) { + pub fn record_user_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { self.record( MetricName::UserErrors, 1.0, + Some(account_id), table_name, None, Some(operation), @@ -145,10 +179,11 @@ impl MetricsCollector { } /// Record a system error (5xx). - pub fn record_system_error(&self, table_name: Option<&str>, operation: &str) { + pub fn record_system_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { self.record( MetricName::SystemErrors, 1.0, + Some(account_id), table_name, None, Some(operation), @@ -156,10 +191,16 @@ impl MetricsCollector { } /// Record a conditional check failure. - pub fn record_conditional_check_failure(&self, table_name: Option<&str>, operation: &str) { + pub fn record_conditional_check_failure( + &self, + account_id: &str, + table_name: Option<&str>, + operation: &str, + ) { self.record( MetricName::ConditionalCheckFailedRequests, 1.0, + Some(account_id), table_name, None, Some(operation), @@ -167,10 +208,11 @@ impl MetricsCollector { } /// Record a transaction conflict. - pub fn record_transaction_conflict(&self, operation: &str) { + pub fn record_transaction_conflict(&self, account_id: &str, operation: &str) { self.record( MetricName::TransactionConflict, 1.0, + Some(account_id), None, None, Some(operation), @@ -179,10 +221,17 @@ impl MetricsCollector { /// Record returned item count from a query/scan. #[allow(clippy::cast_precision_loss)] // count values are small enough for f64 - pub fn record_returned_items(&self, table_name: &str, operation: &str, count: u64) { + pub fn record_returned_items( + &self, + account_id: &str, + table_name: &str, + operation: &str, + count: u64, + ) { self.record( MetricName::ReturnedItemCount, count as f64, + Some(account_id), Some(table_name), None, Some(operation), @@ -191,10 +240,17 @@ impl MetricsCollector { /// Record returned bytes from a query/scan. #[allow(clippy::cast_precision_loss)] // byte counts within 1 MB page limit - pub fn record_returned_bytes(&self, table_name: &str, operation: &str, bytes: u64) { + pub fn record_returned_bytes( + &self, + account_id: &str, + table_name: &str, + operation: &str, + bytes: u64, + ) { self.record( MetricName::ReturnedBytes, bytes as f64, + Some(account_id), Some(table_name), None, Some(operation), @@ -202,10 +258,11 @@ impl MetricsCollector { } /// Record a TTL deletion. - pub fn record_ttl_deletion(&self, table_name: &str) { + pub fn record_ttl_deletion(&self, account_id: &str, table_name: &str) { self.record( MetricName::TimeToLiveDeletedItemCount, 1.0, + Some(account_id), Some(table_name), None, None, @@ -213,10 +270,11 @@ impl MetricsCollector { } /// Record a queue registration the TTL audit found missing and recreated. - pub fn record_ttl_audit_repair(&self, table_name: &str) { + pub fn record_ttl_audit_repair(&self, account_id: &str, table_name: &str) { self.record( MetricName::TtlAuditRepairedEntryCount, 1.0, + Some(account_id), Some(table_name), None, None, @@ -228,10 +286,11 @@ impl MetricsCollector { /// Isolated failures are normal (contention, restarts); a sustained /// nonzero rate on one table means rows are chronically erroring and /// retrying without ever expiring, which a log line alone won't surface. - pub fn record_ttl_sweep_row_error(&self, table_name: &str) { + pub fn record_ttl_sweep_row_error(&self, account_id: &str, table_name: &str) { self.record( MetricName::TtlSweepRowErrorCount, 1.0, + Some(account_id), Some(table_name), None, None, @@ -240,14 +299,22 @@ impl MetricsCollector { /// Record the number of unresolved TTL destroy markers seen in one pass. pub fn record_ttl_repair_markers(&self, count: f64) { - self.record(MetricName::TtlRepairMarkerCount, count, None, None, None); + self.record( + MetricName::TtlRepairMarkerCount, + count, + None, + None, + None, + None, + ); } /// Record TTL deletion staleness (seconds past expiry). - pub fn record_ttl_staleness(&self, table_name: &str, staleness_secs: f64) { + pub fn record_ttl_staleness(&self, account_id: &str, table_name: &str, staleness_secs: f64) { self.record( MetricName::TtlDeletionStaleness, staleness_secs, + Some(account_id), Some(table_name), None, None, @@ -255,8 +322,15 @@ impl MetricsCollector { } /// P120c: Record an HTTP request (dimensions: operation). - pub fn record_request_count(&self, operation: &str) { - self.record(MetricName::RequestCount, 1.0, None, None, Some(operation)); + pub fn record_request_count(&self, account_id: &str, operation: &str) { + self.record( + MetricName::RequestCount, + 1.0, + Some(account_id), + None, + None, + Some(operation), + ); } /// P120c: Record a storage query execution (dimensions: source, category). @@ -273,6 +347,7 @@ impl MetricsCollector { MetricName::StorageQueryCount, 1.0, None, + None, Some(&source_str), Some(&category_str), ); @@ -281,6 +356,7 @@ impl MetricsCollector { MetricName::StorageQueryLatency, latency_us, None, + None, Some(&source_str), Some(&category_str), ); @@ -296,6 +372,7 @@ impl MetricsCollector { None, None, None, + None, ); self.record( MetricName::PoolIdleConnections, @@ -303,13 +380,21 @@ impl MetricsCollector { None, None, None, + None, ); } } /// P120d: Record pool acquire latency in microseconds. pub fn record_pool_acquire_latency(&self, latency_us: f64) { - self.record(MetricName::PoolAcquireLatency, latency_us, None, None, None); + self.record( + MetricName::PoolAcquireLatency, + latency_us, + None, + None, + None, + None, + ); } /// P120e: Record a worker's last successful cycle timestamp. @@ -325,6 +410,7 @@ impl MetricsCollector { now_secs as f64, None, None, + None, Some(&worker_str), ); self.record( @@ -332,6 +418,7 @@ impl MetricsCollector { latency_us, None, None, + None, Some(&worker_str), ); } @@ -344,6 +431,7 @@ impl MetricsCollector { 1.0, None, None, + None, Some(&worker_str), ); } @@ -356,12 +444,20 @@ impl MetricsCollector { /// one that failed. pub fn record_backup_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { let outcome = outcome.to_string(); - self.record(MetricName::BackupJobs, 1.0, None, None, Some(&outcome)); + self.record( + MetricName::BackupJobs, + 1.0, + None, + None, + None, + Some(&outcome), + ); self.record( MetricName::BackupJobDuration, duration_us, None, None, + None, Some(&outcome), ); } @@ -377,12 +473,20 @@ impl MetricsCollector { None, None, None, + None, ); } /// Record a running backup job's snapshot age in seconds (gauge). pub fn record_backup_snapshot_age(&self, age_secs: f64) { - self.record(MetricName::BackupSnapshotAge, age_secs, None, None, None); + self.record( + MetricName::BackupSnapshotAge, + age_secs, + None, + None, + None, + None, + ); } /// Record a failed store call of a backup job, a restore job or the @@ -390,7 +494,14 @@ impl MetricsCollector { /// `delete_prefix`, a restore's `get`, the reconciler's `get`, `list` /// and `delete_prefix`). pub fn record_backup_store_error(&self, op: &str) { - self.record(MetricName::BackupStoreErrors, 1.0, None, None, Some(op)); + self.record( + MetricName::BackupStoreErrors, + 1.0, + None, + None, + None, + Some(op), + ); } /// Record the end of a store-backed restore job: one `RestoreJobs` under @@ -400,12 +511,20 @@ impl MetricsCollector { /// catalog or store call. pub fn record_restore_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { let outcome = outcome.to_string(); - self.record(MetricName::RestoreJobs, 1.0, None, None, Some(&outcome)); + self.record( + MetricName::RestoreJobs, + 1.0, + None, + None, + None, + Some(&outcome), + ); self.record( MetricName::RestoreJobDuration, duration_us, None, None, + None, Some(&outcome), ); } @@ -420,18 +539,21 @@ impl MetricsCollector { None, None, None, + None, ); } /// Record per-segment latency breakdown for a request. pub fn record_segments( &self, + account_id: &str, operation: &str, table_name: Option<&str>, segments: LatencySegments, ) { let point = SegmentPoint { operation: operation.to_owned(), + account_id: account_id.to_owned(), table_name: table_name.map(ToOwned::to_owned), segments, timestamp: Instant::now(), diff --git a/crates/core/src/metrics/collector_query.rs b/crates/core/src/metrics/collector_query.rs index 7ce9af6ff..3e81fdd02 100755 --- a/crates/core/src/metrics/collector_query.rs +++ b/crates/core/src/metrics/collector_query.rs @@ -35,6 +35,12 @@ impl MetricsCollector { let mut results = Vec::new(); for (key, acc) in map.iter() { + // Filter by account if specified. + if let Some(ref account) = params.account_id + && key.account_id.as_deref() != Some(account.as_str()) + { + continue; + } // Filter by table name if specified. if let Some(ref tn) = params.table_name && key.table_name.as_deref() != Some(tn.as_str()) @@ -53,6 +59,9 @@ impl MetricsCollector { }; let mut dimensions = Vec::new(); + if let Some(ref account) = key.account_id { + dimensions.push(Dimension::AccountId(account.clone())); + } if let Some(ref tn) = key.table_name { dimensions.push(Dimension::TableName(tn.clone())); } @@ -282,6 +291,7 @@ pub(super) fn aggregate(drained: Drained) -> Vec { bucket: SystemTime::UNIX_EPOCH + Duration::from_secs(u64::try_from(minute).unwrap_or(0)), metric: key.metric, + account_id: key.account_id.clone().unwrap_or_default(), table_name: key.table_name.clone().unwrap_or_default(), index_name: key.index_name.clone().unwrap_or_default(), operation: key.operation.clone().unwrap_or_default(), diff --git a/crates/core/src/metrics/collector_tests.rs b/crates/core/src/metrics/collector_tests.rs index 1683aa25f..1f52d4ca5 100755 --- a/crates/core/src/metrics/collector_tests.rs +++ b/crates/core/src/metrics/collector_tests.rs @@ -6,11 +6,13 @@ use super::*; use crate::metrics::types::{Dimension, MetricName, MetricsQuery, TimeWindow}; +const ACCOUNT: &str = "111111111111"; + #[test] fn record_and_query_basic() { let c = MetricsCollector::new(); - c.record_user_error(Some("MyTable"), "PutItem"); - c.record_user_error(Some("MyTable"), "PutItem"); + c.record_user_error(ACCOUNT, Some("MyTable"), "PutItem"); + c.record_user_error(ACCOUNT, Some("MyTable"), "PutItem"); let results = c.query(&MetricsQuery { table_name: Some("MyTable".to_owned()), @@ -27,7 +29,7 @@ fn record_and_query_basic() { fn latency_percentiles() { let c = MetricsCollector::new(); for i in 1..=100 { - c.record_latency(Some("T"), "GetItem", i as f64); + c.record_latency(ACCOUNT, Some("T"), "GetItem", i as f64); } let results = c.query(&MetricsQuery { metric: Some(MetricName::SuccessfulRequestLatency), @@ -43,8 +45,8 @@ fn latency_percentiles() { #[test] fn filter_by_table() { let c = MetricsCollector::new(); - c.record_user_error(Some("A"), "PutItem"); - c.record_user_error(Some("B"), "PutItem"); + c.record_user_error(ACCOUNT, Some("A"), "PutItem"); + c.record_user_error(ACCOUNT, Some("B"), "PutItem"); let results = c.query(&MetricsQuery { table_name: Some("A".to_owned()), @@ -58,10 +60,90 @@ fn filter_by_table() { ); } +/// Two accounts' tables of the same name keep their own points: neither +/// adds into the other's, each carries its account as a dimension, and the +/// account filter returns only that account's. A metric of the whole +/// deployment has no account dimension and only an unfiltered query sees it. +#[test] +fn two_accounts_with_the_same_table_name_keep_separate_metrics() { + const OTHER: &str = "222222222222"; + let c = MetricsCollector::new(); + c.record_user_error(ACCOUNT, Some("Orders"), "PutItem"); + c.record_user_error(OTHER, Some("Orders"), "PutItem"); + c.record_user_error(OTHER, Some("Orders"), "PutItem"); + c.record_pool_acquire_latency(5.0); + + let all = c.query(&MetricsQuery { + metric: Some(MetricName::UserErrors), + window: Some(TimeWindow::AllTime), + ..Default::default() + }); + let mut counts: Vec<(Vec, u64)> = all + .iter() + .map(|s| (s.dimensions.clone(), s.count)) + .collect(); + counts.sort_by(|a, b| format!("{:?}", a.0).cmp(&format!("{:?}", b.0))); + let dims = |account: &str| { + vec![ + Dimension::AccountId(account.to_owned()), + Dimension::TableName("Orders".to_owned()), + Dimension::Operation("PutItem".to_owned()), + ] + }; + assert_eq!(counts, [(dims(ACCOUNT), 1), (dims(OTHER), 2)]); + + let other = c.query(&MetricsQuery { + account_id: Some(OTHER.to_owned()), + window: Some(TimeWindow::AllTime), + ..Default::default() + }); + assert_eq!(other.len(), 1); + assert_eq!(other[0].dimensions, dims(OTHER)); + assert_eq!(other[0].count, 2); + + let pool = c.query(&MetricsQuery { + metric: Some(MetricName::PoolAcquireLatency), + window: Some(TimeWindow::AllTime), + ..Default::default() + }); + assert_eq!(pool.len(), 1); + assert!(pool[0].dimensions.is_empty()); +} + +/// A drained bucket carries its account, and a metric of the whole +/// deployment drains with an empty one. +#[test] +fn a_drained_bucket_carries_its_account() { + let c = MetricsCollector::new(); + c.record_ttl_deletion(ACCOUNT, "Orders"); + c.record_ttl_repair_markers(3.0); + let mut buckets: Vec<(String, String, String)> = c + .drain(Duration::ZERO) + .into_iter() + .map(|b| (b.metric.to_string(), b.account_id, b.table_name)) + .collect(); + buckets.sort(); + assert_eq!( + buckets, + [ + ( + "TimeToLiveDeletedItemCount".to_owned(), + ACCOUNT.to_owned(), + "Orders".to_owned() + ), + ( + "TtlRepairMarkerCount".to_owned(), + String::new(), + String::new() + ), + ] + ); +} + #[test] fn prune_removes_old_data() { let c = MetricsCollector::new(); - c.record_user_error(Some("T"), "PutItem"); + c.record_user_error(ACCOUNT, Some("T"), "PutItem"); // All data is recent, prune should keep it. c.prune(); @@ -78,6 +160,7 @@ fn record_and_query_segments() { let c = MetricsCollector::new(); c.record_segments( + ACCOUNT, "GetItem", Some("T"), LatencySegments { @@ -90,6 +173,7 @@ fn record_and_query_segments() { }, ); c.record_segments( + ACCOUNT, "GetItem", Some("T"), LatencySegments { @@ -119,6 +203,7 @@ fn prune_removes_old_segments() { let c = MetricsCollector::new(); c.record_segments( + ACCOUNT, "PutItem", None, LatencySegments { @@ -144,6 +229,7 @@ fn segments_multiple_operations() { let c = MetricsCollector::new(); c.record_segments( + ACCOUNT, "GetItem", Some("T"), LatencySegments { @@ -156,6 +242,7 @@ fn segments_multiple_operations() { }, ); c.record_segments( + ACCOUNT, "PutItem", Some("T"), LatencySegments { diff --git a/crates/core/src/metrics/drain_tests.rs b/crates/core/src/metrics/drain_tests.rs index 2945d339c..b2a9a598d 100644 --- a/crates/core/src/metrics/drain_tests.rs +++ b/crates/core/src/metrics/drain_tests.rs @@ -42,6 +42,9 @@ fn keys(n: usize) -> Vec { (0..n) .map(|i| MetricKey { metric: MetricName::SuccessfulRequestLatency, + // Two accounts, so a drain that dropped or swapped the account + // would not match the points recorded. + account_id: Some(format!("{:012}", i % 2)), table_name: Some(format!("table-{i}")), index_name: None, operation: Some("GetItem".to_owned()), @@ -246,7 +249,7 @@ fn rows(buckets: Vec) -> Vec { .map(|b| { ( b.metric.to_string(), - b.table_name, + format!("{}/{}", b.account_id, b.table_name), b.index_name, b.operation, b.bucket, @@ -291,6 +294,7 @@ fn drain_buckets_match_a_per_minute_reference() { want.push(FlushBucket { bucket: SystemTime::UNIX_EPOCH + Duration::from_secs(minute as u64), metric: key.metric, + account_id: key.account_id.clone().unwrap_or_default(), table_name: key.table_name.clone().unwrap_or_default(), index_name: String::new(), operation: key.operation.clone().unwrap_or_default(), @@ -309,8 +313,8 @@ fn drain_buckets_match_a_per_minute_reference() { fn drain_zero_takes_everything() { let c = MetricsCollector::new(); for i in 0..(2 * CHUNK_CAP + 5) { - c.record_latency(Some("T"), "GetItem", i as f64); - c.record_request_count("GetItem"); + c.record_latency("111111111111", Some("T"), "GetItem", i as f64); + c.record_request_count("111111111111", "GetItem"); } let buckets = c.drain(Duration::ZERO); let count: u64 = buckets.iter().map(|b| b.count).sum(); @@ -324,7 +328,7 @@ fn drain_zero_takes_everything() { fn record_keeps_every_chunk_within_the_cap() { let c = MetricsCollector::new(); for i in 0..(10 * CHUNK_CAP + 3) { - c.record_latency(Some("T"), "GetItem", i as f64); + c.record_latency("111111111111", Some("T"), "GetItem", i as f64); } let map = c.data.read().expect("lock"); let acc = map.values().next().expect("one key"); diff --git a/crates/core/src/metrics/types.rs b/crates/core/src/metrics/types.rs index 0030ce1b2..bd5dad1b1 100755 --- a/crates/core/src/metrics/types.rs +++ b/crates/core/src/metrics/types.rs @@ -172,6 +172,9 @@ impl std::fmt::Display for MetricName { #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[non_exhaustive] pub enum Dimension { + /// The account the metric belongs to. Absent on a metric of the whole + /// deployment. + AccountId(String), TableName(String), GlobalSecondaryIndexName(String), Operation(String), @@ -320,6 +323,10 @@ pub struct Percentiles { /// Auto-granularity when not specified: ≤10m→1m, ≤2h→5m, ≤8h→15m, >8h→1h. #[derive(Debug, Clone, Default, Deserialize)] pub struct MetricsQuery { + /// Only the metrics of this account. Absent: every account's metrics and + /// the deployment's own. + #[serde(default)] + pub account_id: Option, #[serde(default)] pub table_name: Option, #[serde(default)] @@ -346,6 +353,8 @@ pub struct FlushBucket { pub bucket: std::time::SystemTime, /// Which metric this bucket aggregates. pub metric: MetricName, + /// Account dimension (empty string for a metric of the whole deployment). + pub account_id: String, /// Table name dimension (empty string if not table-scoped). pub table_name: String, /// Index name dimension (empty string if not index-scoped). diff --git a/crates/server/src/console/pages/account_scope_reads_tests.rs b/crates/server/src/console/pages/account_scope_reads_tests.rs index c26e59736..628eba2b3 100644 --- a/crates/server/src/console/pages/account_scope_reads_tests.rs +++ b/crates/server/src/console/pages/account_scope_reads_tests.rs @@ -26,7 +26,7 @@ use extenddb_storage::CatalogStore; use extenddb_storage::authorization_store::{AuthorizationStore, SessionData as RoleSession}; use extenddb_storage::management_store::{ AccessKeyCreated, AccountDetail, AdminEntry, AdminStore, GroupDetail, GroupListEntry, - ManagementStore, MetricsRow, MetricsStore, OpError, OpResult, PasswordHashStore, + ManagementStore, MetricsFilter, MetricsRow, MetricsStore, OpError, OpResult, PasswordHashStore, RateLimitStore, RoleDetail, RoleListEntry, SettingsStore, UserDetail, UserListEntry, }; use futures::future::BoxFuture; @@ -161,9 +161,7 @@ impl SettingsStore for CountingStore { impl MetricsStore for CountingStore { counted_calls! { insert_metrics(&[MetricsRow]) -> (); - query_metrics( - OffsetDateTime, OffsetDateTime, Option<&str>, Option<&str> - ) -> Vec; + query_metrics(OffsetDateTime, OffsetDateTime, &MetricsFilter) -> Vec; prune_metrics(Duration) -> (); } } diff --git a/crates/server/src/console/pages/metrics_pages.rs b/crates/server/src/console/pages/metrics_pages.rs index 2e99e7228..2993276c5 100755 --- a/crates/server/src/console/pages/metrics_pages.rs +++ b/crates/server/src/console/pages/metrics_pages.rs @@ -8,9 +8,9 @@ //! availability chart (D16), and CSV/JSON data export (D19). Charts use //! inline `` and no external dependencies. //! -//! The dashboard and its data feed are for admins only. The `metrics` table -//! has no account column, so the feed holds the table names, index names, -//! operations, request counts and latencies of every account. +//! The dashboard and its data feed are for admins only. The feed holds the +//! table names, index names, operations, request counts and latencies of +//! every account, each row under its `AccountId`. //! //! HTML and JS content live in `metrics_content` to stay under the 500-line //! file limit. diff --git a/crates/server/src/console/pages/metrics_pages_tests.rs b/crates/server/src/console/pages/metrics_pages_tests.rs index 4d8eb976d..21c2b269b 100644 --- a/crates/server/src/console/pages/metrics_pages_tests.rs +++ b/crates/server/src/console/pages/metrics_pages_tests.rs @@ -12,7 +12,7 @@ use axum::http::StatusCode; use axum::http::header::LOCATION; use axum::response::Response; use extenddb_core::metrics::{LatencySegments, MetricsCollector, MetricsQuery, TimeWindow}; -use extenddb_storage::management_store::{MetricsRow, MetricsStore, OpResult}; +use extenddb_storage::management_store::{MetricsFilter, MetricsRow, MetricsStore, OpResult}; use futures::future::BoxFuture; use super::{metrics_json, render_metrics_page}; @@ -20,9 +20,12 @@ use crate::console::html; use crate::console::pages::{SessionData, identity_label}; use crate::management::CallerIdentity; -/// A table in each of two accounts. The `metrics` table has no account -/// column, so both accounts' rows come back from one store read. -const TABLES: [&str; 2] = ["orders-account-a", "orders-account-b"]; +/// A table in each of two accounts, as (account, table). The admin feed reads +/// every account's rows in one store read. +const TABLES: [(&str, &str); 2] = [ + ("111111111111", "orders-account-a"), + ("222222222222", "orders-account-b"), +]; /// The index every fake row is recorded against, so a test can check that a /// response carries no index name. @@ -44,15 +47,15 @@ impl MetricsStore for FakeMetricsStore { &self, _start: time::OffsetDateTime, _end: time::OffsetDateTime, - _table_name: Option<&str>, - _metric: Option<&str>, + _filter: &MetricsFilter, ) -> BoxFuture<'_, OpResult>> { self.reads.fetch_add(1, Ordering::SeqCst); let bucket = time::OffsetDateTime::now_utc() - time::Duration::minutes(2); let rows = TABLES .iter() - .map(|table| MetricsRow { + .map(|(account, table)| MetricsRow { bucket, + account_id: Some((*account).to_owned()), metric: "SuccessfulRequestLatency".to_owned(), table_name: Some((*table).to_owned()), index_name: Some(INDEX.to_owned()), @@ -93,8 +96,9 @@ fn session(identity: CallerIdentity) -> SessionData { fn collector() -> Arc { let collector = Arc::new(MetricsCollector::new()); collector.record_segments( + TABLES[0].0, "GetItem", - Some(TABLES[0]), + Some(TABLES[0].1), LatencySegments { dispatch_us: 500.0, total_us: 500.0, @@ -127,8 +131,9 @@ async fn iam_user_gets_403_and_no_metrics_from_the_data_feed() { assert_eq!(response.status(), StatusCode::FORBIDDEN); let body = body_text(response).await; assert!(!body.contains("TableName"), "403 body: {body}"); - for table in TABLES { + for (account, table) in TABLES { assert!(!body.contains(table), "403 body names {table}: {body}"); + assert!(!body.contains(account), "403 body names an account: {body}"); } assert!(!body.contains(INDEX), "403 body names an index: {body}"); assert!( @@ -159,11 +164,23 @@ async fn admin_gets_every_accounts_rows_from_the_data_feed() { .flatten() .filter_map(|d| d["TableName"].as_str()) .collect(); - for table in TABLES { + let accounts: Vec<&str> = body["metrics"] + .as_array() + .expect("metrics array") + .iter() + .filter_map(|m| m["dimensions"].as_array()) + .flatten() + .filter_map(|d| d["AccountId"].as_str()) + .collect(); + for (account, table) in TABLES { assert!( tables.contains(&table), "admin feed lacks {table}: {tables:?}" ); + assert!( + accounts.contains(&account), + "admin feed lacks the account of {table}: {accounts:?}" + ); } let indexes: Vec<&str> = body["metrics"] .as_array() diff --git a/crates/server/src/handler.rs b/crates/server/src/handler.rs index 4cce8cde1..a482031e8 100755 --- a/crates/server/src/handler.rs +++ b/crates/server/src/handler.rs @@ -228,12 +228,18 @@ pub(crate) async fn handle_request( } else { extenddb_core::metrics::MetricName::WriteThrottleEvents }; - state - .metrics - .record(metric, 1.0, metric_table.as_deref(), None, Some(&operation)); + state.metrics.record( + metric, + 1.0, + Some(&ctx.account_id), + metric_table.as_deref(), + None, + Some(&operation), + ); state.metrics.record( extenddb_core::metrics::MetricName::ThrottledRequests, 1.0, + Some(&ctx.account_id), metric_table.as_deref(), None, Some(&operation), @@ -302,6 +308,7 @@ pub(crate) async fn handle_request( state.metrics.record( extenddb_core::metrics::MetricName::ConsumedReadCapacityUnits, m.read_capacity_units, + Some(&ctx.account_id), Some(tn), m.index_name.as_deref(), Some(&operation), @@ -311,20 +318,27 @@ pub(crate) async fn handle_request( state.metrics.record( extenddb_core::metrics::MetricName::ConsumedWriteCapacityUnits, m.write_capacity_units, + Some(&ctx.account_id), Some(tn), m.index_name.as_deref(), Some(&operation), ); } if m.returned_item_count > 0 { - state - .metrics - .record_returned_items(tn, &operation, m.returned_item_count); + state.metrics.record_returned_items( + &ctx.account_id, + tn, + &operation, + m.returned_item_count, + ); } if m.returned_bytes > 0 { - state - .metrics - .record_returned_bytes(tn, &operation, m.returned_bytes); + state.metrics.record_returned_bytes( + &ctx.account_id, + tn, + &operation, + m.returned_bytes, + ); } } // Throttle buckets stay keyed by the name the request sent, which @@ -343,7 +357,13 @@ pub(crate) async fn handle_request( success_response(&result.body, &request_id) } Err(e) => { - record_error_metrics(&state.metrics, &e, metric_table.as_deref(), &operation); + record_error_metrics( + &state.metrics, + &e, + &ctx.account_id, + metric_table.as_deref(), + &operation, + ); error_response(&e, &request_id) } }; @@ -356,12 +376,18 @@ pub(crate) async fn handle_request( // `SuccessfulRequestLatency` counts successful requests only, as // CloudWatch's does. if succeeded { - state - .metrics - .record_latency(metric_table.as_deref(), &operation, total_us); + state.metrics.record_latency( + &ctx.account_id, + metric_table.as_deref(), + &operation, + total_us, + ); } - state.metrics.record_request_count(&operation); + state + .metrics + .record_request_count(&ctx.account_id, &operation); state.metrics.record_segments( + &ctx.account_id, &operation, metric_table.as_deref(), extenddb_core::metrics::LatencySegments { diff --git a/crates/server/src/metrics_endpoint.rs b/crates/server/src/metrics_endpoint.rs index 0c24473bf..492e2c7ab 100755 --- a/crates/server/src/metrics_endpoint.rs +++ b/crates/server/src/metrics_endpoint.rs @@ -179,11 +179,14 @@ pub(crate) async fn query_metrics_from_store( .and_then(parse_granularity) .unwrap_or_else(|| auto_granularity(range_secs)); - let table_filter = params.table_name.as_deref().filter(|s| !s.is_empty()); - let metric_filter = params.metric.map(|m| m.to_string()); + let filter = extenddb_storage::management_store::MetricsFilter { + account_id: params.account_id.clone(), + table_name: params.table_name.clone(), + metric: params.metric.map(|m| m.to_string()), + }; let rows = store - .query_metrics(start_ts, end_ts, table_filter, metric_filter.as_deref()) + .query_metrics(start_ts, end_ts, &filter) .await .map_err(|e| { tracing::warn!("Failed to query metrics from store: {e:?}"); @@ -215,8 +218,10 @@ fn aggregate_rows( use extenddb_core::metrics::{Dimension, MetricName, MetricSnapshot, MetricsBucket}; use std::collections::HashMap; - // Key: (metric, table_name, index_name, operation) - type DimKey = (String, String, String, String); + // Key: (metric, account_id, table_name, index_name, operation). The + // account is part of the key, so two accounts' tables of the same name + // are never added together. + type DimKey = (String, String, String, String, String); // Aggregate snapshots (overall). let mut snap_map: HashMap = HashMap::new(); @@ -224,11 +229,13 @@ fn aggregate_rows( let mut bucket_map: HashMap<(DimKey, i64), (f64, u64, f64, f64)> = HashMap::new(); for row in rows { - // CloudWatch-style dimensions: metrics are keyed per table and per - // index. The endpoint is authenticated (admin Basic auth), so per-table - // dimensions are only returned to an authorized caller. + // CloudWatch-style dimensions: metrics are keyed per account, per + // table and per index. The endpoint is authenticated (admin Basic + // auth), so per-table dimensions are only returned to an authorized + // caller. let key = ( row.metric.clone(), + row.account_id.clone().unwrap_or_default(), row.table_name.clone().unwrap_or_default(), row.index_name.clone().unwrap_or_default(), row.operation.clone().unwrap_or_default(), @@ -269,13 +276,16 @@ fn aggregate_rows( let make_dims = |key: &DimKey| -> Vec { let mut dims = Vec::new(); if !key.1.is_empty() { - dims.push(Dimension::TableName(key.1.clone())); + dims.push(Dimension::AccountId(key.1.clone())); } if !key.2.is_empty() { - dims.push(Dimension::GlobalSecondaryIndexName(key.2.clone())); + dims.push(Dimension::TableName(key.2.clone())); } if !key.3.is_empty() { - dims.push(Dimension::Operation(key.3.clone())); + dims.push(Dimension::GlobalSecondaryIndexName(key.3.clone())); + } + if !key.4.is_empty() { + dims.push(Dimension::Operation(key.4.clone())); } dims }; @@ -337,6 +347,7 @@ mod tests { fn aggregate_rows_retains_table_and_index_dimensions() { let rows = vec![MetricsRow { bucket: time::OffsetDateTime::UNIX_EPOCH, + account_id: Some("111111111111".to_owned()), metric: "SystemErrors".to_owned(), table_name: Some("MyTable".to_owned()), index_name: Some("MyIndex".to_owned()), @@ -363,6 +374,61 @@ mod tests { )), "GSI-name dimension must be retained, got {dims:?}" ); + assert!( + dims.contains(&Dimension::AccountId("111111111111".to_owned())), + "AccountId dimension must be retained, got {dims:?}" + ); + } + + /// Rows of two accounts with a table of the same name stay two groups, + /// in the snapshots and in every bucket, each under its own account. A + /// row of the whole deployment has no account dimension. + #[test] + fn aggregate_rows_keeps_two_accounts_with_the_same_table_name_apart() { + let row = |account: Option<&str>, table: Option<&str>, sum: f64| MetricsRow { + bucket: time::OffsetDateTime::UNIX_EPOCH, + account_id: account.map(ToOwned::to_owned), + metric: "ConsumedWriteCapacityUnits".to_owned(), + table_name: table.map(ToOwned::to_owned), + index_name: None, + operation: Some("PutItem".to_owned()), + sum, + count: 1, + min: sum, + max: sum, + }; + let rows = vec![ + row(Some("111111111111"), Some("Orders"), 1.0), + row(Some("222222222222"), Some("Orders"), 5.0), + row(None, None, 7.0), + ]; + + let (snapshots, buckets) = aggregate_rows(&rows, 60, TimeWindow::LastHour); + + let account_of = |dims: &[Dimension]| { + dims.iter().find_map(|d| match d { + Dimension::AccountId(a) => Some(a.clone()), + _ => None, + }) + }; + let mut sums: Vec<(Option, f64)> = snapshots + .iter() + .map(|s| (account_of(&s.dimensions), s.sum)) + .collect(); + sums.sort_by(|a, b| a.0.cmp(&b.0)); + assert_eq!( + sums, + [ + (None, 7.0), + (Some("111111111111".to_owned()), 1.0), + (Some("222222222222".to_owned()), 5.0), + ] + ); + assert_eq!( + buckets.len(), + 3, + "one bucket per account and the deployment" + ); } /// The backup job's metric names round-trip through the store's string @@ -386,6 +452,7 @@ mod tests { .iter() .map(|name| MetricsRow { bucket: time::OffsetDateTime::UNIX_EPOCH, + account_id: None, metric: name.to_string(), table_name: None, index_name: None, diff --git a/crates/server/src/response.rs b/crates/server/src/response.rs index 45203dbf3..01c26448c 100755 --- a/crates/server/src/response.rs +++ b/crates/server/src/response.rs @@ -83,26 +83,28 @@ pub(crate) fn error_response(error: &DynamoDbError, request_id: &str) -> Respons /// rules. A failed condition counts in `ConditionalCheckFailedRequests` and not /// in `UserErrors`, as CloudWatch's metrics page says. `table_name` is the name /// `request_helpers::metric_table_name` checked, used by the metrics that carry -/// one. +/// one. Every metric carries `account_id`, the account the request was +/// authenticated for. pub(crate) fn record_error_metrics( metrics: &extenddb_core::metrics::MetricsCollector, error: &DynamoDbError, + account_id: &str, table_name: Option<&str>, operation: &str, ) { match error { DynamoDbError::InternalServerError(_) | DynamoDbError::ServiceUnavailable(_) => { - metrics.record_system_error(table_name, operation); + metrics.record_system_error(account_id, table_name, operation); } DynamoDbError::ConditionalCheckFailedException(..) => { - metrics.record_conditional_check_failure(table_name, operation); + metrics.record_conditional_check_failure(account_id, table_name, operation); } DynamoDbError::TransactionConflictException(_) => { - metrics.record_user_error(None, operation); - metrics.record_transaction_conflict(operation); + metrics.record_user_error(account_id, None, operation); + metrics.record_transaction_conflict(account_id, operation); } _ => { - metrics.record_user_error(None, operation); + metrics.record_user_error(account_id, None, operation); } } } @@ -115,6 +117,8 @@ mod tests { use super::*; + const ACCOUNT: &str = "111111111111"; + /// The dimensions of every snapshot of `metric` in `collector`. fn dimensions_of(collector: &MetricsCollector, metric: MetricName) -> Vec> { collector @@ -142,11 +146,16 @@ mod tests { DynamoDbError::ResourceNotFoundException("missing".to_owned()), DynamoDbError::TransactionConflictException("conflict".to_owned()), ] { - record_error_metrics(&collector, &error, Some("Orders"), "PutItem"); + record_error_metrics(&collector, &error, ACCOUNT, Some("Orders"), "PutItem"); } let user_errors = dimensions_of(&collector, MetricName::UserErrors); assert_eq!(user_errors.len(), 1, "{user_errors:?}"); assert!(!has_table(&user_errors[0]), "{user_errors:?}"); + // The error still names the account the request was made for. + assert!( + user_errors[0].contains(&Dimension::AccountId(ACCOUNT.to_owned())), + "{user_errors:?}" + ); } /// A failed condition counts in `ConditionalCheckFailedRequests` only. @@ -156,6 +165,7 @@ mod tests { record_error_metrics( &collector, &DynamoDbError::ConditionalCheckFailedException("failed".to_owned(), None), + ACCOUNT, Some("Orders"), "PutItem", ); @@ -172,12 +182,14 @@ mod tests { record_error_metrics( &collector, &DynamoDbError::ConditionalCheckFailedException("failed".to_owned(), None), + ACCOUNT, Some("Orders"), "PutItem", ); record_error_metrics( &collector, &DynamoDbError::InternalServerError("boom".to_owned()), + ACCOUNT, Some("Orders"), "PutItem", ); diff --git a/crates/server/src/workers.rs b/crates/server/src/workers.rs index 58e3a7133..b9bb1bfe5 100755 --- a/crates/server/src/workers.rs +++ b/crates/server/src/workers.rs @@ -225,6 +225,11 @@ pub(crate) async fn metrics_flush_worker( .unwrap_or(time::OffsetDateTime::UNIX_EPOCH); MetricsRow { bucket: bucket_ts, + account_id: if b.account_id.is_empty() { + None + } else { + Some(b.account_id.clone()) + }, metric: b.metric.to_string(), table_name: if b.table_name.is_empty() { None @@ -288,7 +293,9 @@ mod tests { use super::{login_attempt_cleanup_worker, metrics_flush_worker}; use extenddb_core::metrics::MetricsCollector; use extenddb_storage::CancellationToken; - use extenddb_storage::management_store::{MetricsRow, MetricsStore, OpResult, RateLimitStore}; + use extenddb_storage::management_store::{ + MetricsFilter, MetricsRow, MetricsStore, OpResult, RateLimitStore, + }; use futures::future::BoxFuture; use std::sync::Arc; use std::sync::Mutex; @@ -320,8 +327,7 @@ mod tests { &self, _start: time::OffsetDateTime, _end: time::OffsetDateTime, - _table_name: Option<&str>, - _metric: Option<&str>, + _filter: &MetricsFilter, ) -> BoxFuture<'_, OpResult>> { Box::pin(async { Ok(Vec::new()) }) } @@ -421,7 +427,7 @@ mod tests { // Buffer a data point younger than one flush interval — a steady-state // flush would deliberately leave this to accumulate. - metrics.record_latency(Some("test-table"), "PutItem", 1_234.0); + metrics.record_latency("111111111111", Some("test-table"), "PutItem", 1_234.0); let handle = tokio::spawn(metrics_flush_worker(metrics, store.clone(), token.clone())); @@ -443,5 +449,14 @@ mod tests { store.inserted_count() > 0, "final flush lost the in-flight bucket: nothing was persisted on shutdown" ); + // The row keeps the account it was recorded for. + let inserted = store.inserted.lock().expect("lock poisoned"); + assert!( + inserted + .iter() + .any(|r| r.account_id.as_deref() == Some("111111111111") + && r.table_name.as_deref() == Some("test-table")), + "the flushed row lost its account" + ); } } diff --git a/crates/storage-cassandra/migrations/catalog/0010_account_metrics.cql b/crates/storage-cassandra/migrations/catalog/0010_account_metrics.cql new file mode 100644 index 000000000..816e2cd7e --- /dev/null +++ b/crates/storage-cassandra/migrations/catalog/0010_account_metrics.cql @@ -0,0 +1,28 @@ +-- Copyright 2026 ExtendDB contributors +-- SPDX-License-Identifier: Apache-2.0 +-- +-- Migration 10, account_metrics, Cassandra catalog keyspace: replaces the +-- metrics table with account_metrics, whose partition key has the account a +-- row was recorded for, so two accounts' tables of the same name keep their +-- own rows (#728). A partition key cannot be changed and a table cannot be +-- renamed, so the table is new. account_id is the empty string on a metric of +-- the whole deployment. The rows of metrics are dropped, not copied: they +-- carry no account, and expire a day after they are written. IF NOT EXISTS +-- and IF EXISTS make a second run change nothing. The runner writes the +-- catalog keyspace in front of each table. + +CREATE TABLE IF NOT EXISTS account_metrics ( + bucket timestamp, + account_id text, + metric text, + table_name text, + index_name text, + operation text, + sum double, + count bigint, + min double, + max double, + PRIMARY KEY ((bucket, account_id, metric, table_name, index_name, operation)) +); + +DROP TABLE IF EXISTS metrics; diff --git a/crates/storage-cassandra/src/catalog_store.rs b/crates/storage-cassandra/src/catalog_store.rs index 50a3c8206..aeb4c941e 100644 --- a/crates/storage-cassandra/src/catalog_store.rs +++ b/crates/storage-cassandra/src/catalog_store.rs @@ -604,6 +604,7 @@ impl RateLimitStore for CassandraCatalogStore { use extenddb_storage::management_store::MetricsRow; +/// `account_metrics`: migration 10 replaced `metrics` with it. impl MetricsStore for CassandraCatalogStore { fn insert_metrics(&self, rows: &[MetricsRow]) -> BoxFuture<'_, OpResult<()>> { let rows = rows.to_vec(); @@ -612,9 +613,10 @@ impl MetricsStore for CassandraCatalogStore { let ttl = self.metrics_ttl_seconds; Box::pin(async move { let query = format!( - "INSERT INTO {catalog_keyspace}.metrics \ - (bucket, metric, table_name, index_name, operation, sum, count, min, max) \ - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) USING TTL {ttl}" + "INSERT INTO {catalog_keyspace}.account_metrics \ + (bucket, account_id, metric, table_name, index_name, operation, \ + sum, count, min, max) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) USING TTL {ttl}" ); for row in &rows { #[allow(clippy::cast_possible_truncation)] @@ -624,6 +626,7 @@ impl MetricsStore for CassandraCatalogStore { &query, cdrs_tokio::query_values!( bucket_ms, + row.account_id.as_deref().unwrap_or(""), row.metric.as_str(), row.table_name.as_deref().unwrap_or(""), row.index_name.as_deref().unwrap_or(""), @@ -647,11 +650,11 @@ impl MetricsStore for CassandraCatalogStore { &self, start: time::OffsetDateTime, end: time::OffsetDateTime, - table_name: Option<&str>, - metric: Option<&str>, + filter: &extenddb_storage::management_store::MetricsFilter, ) -> BoxFuture<'_, OpResult>> { - let table_name = table_name.map(str::to_owned); - let metric = metric.map(str::to_owned); + let account_id = filter.account_id().map(str::to_owned); + let table_name = filter.table_name().map(str::to_owned); + let metric = filter.metric().map(str::to_owned); let session = self.session.clone(); let catalog_keyspace = self.catalog_keyspace(); Box::pin(async move { @@ -663,9 +666,9 @@ impl MetricsStore for CassandraCatalogStore { // bucket is part of the composite partition key so a range scan // requires ALLOW FILTERING; the time window keeps the result bounded. let query = format!( - "SELECT bucket, metric, table_name, index_name, operation, \ + "SELECT bucket, account_id, metric, table_name, index_name, operation, \ sum, count, min, max \ - FROM {catalog_keyspace}.metrics \ + FROM {catalog_keyspace}.account_metrics \ WHERE bucket >= ? AND bucket <= ? ALLOW FILTERING" ); let result = session @@ -682,17 +685,22 @@ impl MetricsStore for CassandraCatalogStore { .into_rows() .unwrap_or_default(); + let present = |s: String| (!s.is_empty()).then_some(s); let mut out = Vec::with_capacity(rows.len()); for row in rows { use crate::cassandra_util::get_column; let bucket_ms: i64 = get_column(&row, "bucket", "query_metrics")?; let bucket = time::OffsetDateTime::from_unix_timestamp(bucket_ms / 1000) .unwrap_or(time::OffsetDateTime::UNIX_EPOCH); + let account: String = get_column(&row, "account_id", "query_metrics")?; let metric_val: String = get_column(&row, "metric", "query_metrics")?; let tn: String = get_column(&row, "table_name", "query_metrics")?; let idx: String = get_column(&row, "index_name", "query_metrics")?; let op: String = get_column(&row, "operation", "query_metrics")?; + if account_id.as_deref().is_some_and(|f| f != account) { + continue; + } if table_name.as_deref().is_some_and(|f| f != tn) { continue; } @@ -702,10 +710,11 @@ impl MetricsStore for CassandraCatalogStore { out.push(MetricsRow { bucket, + account_id: present(account), metric: metric_val, - table_name: if tn.is_empty() { None } else { Some(tn) }, - index_name: if idx.is_empty() { None } else { Some(idx) }, - operation: if op.is_empty() { None } else { Some(op) }, + table_name: present(tn), + index_name: present(idx), + operation: present(op), sum: get_column(&row, "sum", "query_metrics")?, count: get_column(&row, "count", "query_metrics")?, min: get_column(&row, "min", "query_metrics")?, diff --git a/crates/storage-cassandra/src/metadata_engine.rs b/crates/storage-cassandra/src/metadata_engine.rs index 94c9bfd58..0822ddd78 100755 --- a/crates/storage-cassandra/src/metadata_engine.rs +++ b/crates/storage-cassandra/src/metadata_engine.rs @@ -646,7 +646,7 @@ impl CassandraEngine { { Ok(true) => { repaired += 1; - metrics.record_ttl_audit_repair(table_name); + metrics.record_ttl_audit_repair(account_id, table_name); } Ok(false) => {} // Claimed work owns this key right now; skip rather diff --git a/crates/storage-cassandra/src/migrations/mod.rs b/crates/storage-cassandra/src/migrations/mod.rs index 85d10a1b4..42b6f04df 100644 --- a/crates/storage-cassandra/src/migrations/mod.rs +++ b/crates/storage-cassandra/src/migrations/mod.rs @@ -168,6 +168,15 @@ pub fn migration(id: MigrationId) -> Migration { "0009_warm_throughput.cql" )], }, + MigrationId::AccountMetrics => Migration::Applies { + kind: Kind::Breaking, + convergent: true, + bodies: vec![file_body!( + StoreKind::Catalog, + "migrations/catalog", + "0010_account_metrics.cql" + )], + }, } } diff --git a/crates/storage-cassandra/src/ttl_worker.rs b/crates/storage-cassandra/src/ttl_worker.rs index b85b9900b..c644f9a87 100644 --- a/crates/storage-cassandra/src/ttl_worker.rs +++ b/crates/storage-cassandra/src/ttl_worker.rs @@ -1279,8 +1279,9 @@ pub async fn sweep_once(storage: &CassandraEngine, metrics: &MetricsCollector) { match outcome { Ok(true) => { deleted += 1; - metrics.record_ttl_deletion(table_name); + metrics.record_ttl_deletion(account_id, table_name); metrics.record_ttl_staleness( + account_id, table_name, now_epoch.saturating_sub(expires_at) as f64, ); @@ -1289,7 +1290,7 @@ pub async fn sweep_once(storage: &CassandraEngine, metrics: &MetricsCollector) { // One row's failure must not abandon the wave or the // table: its durable queue state drives its own retry. Err(error) => { - metrics.record_ttl_sweep_row_error(table_name); + metrics.record_ttl_sweep_row_error(account_id, table_name); tracing::warn!("TTL worker: row failed in {table_name}: {error}"); } } diff --git a/crates/storage-cassandra/tests/direct/keyspace_migrations.rs b/crates/storage-cassandra/tests/direct/keyspace_migrations.rs index 2c5476647..ecd1706a7 100644 --- a/crates/storage-cassandra/tests/direct/keyspace_migrations.rs +++ b/crates/storage-cassandra/tests/direct/keyspace_migrations.rs @@ -44,12 +44,14 @@ struct Schema { } /// Migration 1's catalog body: the tables, index and columns of the 0.1.x -/// catalog migrations V001 to V005, apart from `continuous_backups`, which -/// migration 5 drops (see [`DROPPED`]). The runner's ledger and the serving -/// registrations are in neither list. +/// catalog migrations V001 to V005, apart from `continuous_backups` and +/// `metrics`, which migrations 5 and 10 drop (see [`DROPPED`]), and with +/// `account_metrics`, which migration 10 creates in place of `metrics`. The +/// runner's ledger and the serving registrations are in neither list. const CATALOG: Schema = Schema { tables: &[ "access_keys", + "account_metrics", "accounts", "admin_users", "backups_by_account", @@ -67,7 +69,6 @@ const CATALOG: Schema = Schema { "idempotency_tokens_by_account", "indexes", "login_attempts", - "metrics", "schema_history", "settings", "tables", @@ -195,8 +196,9 @@ async fn tables_in(session: &Arc, keyspace: &str) -> BTreeSet< } /// Tables that migration 1 creates and a later migration drops, which no -/// keyspace holds after a run: migration 5 drops `continuous_backups`. -const DROPPED: &[&str] = &["continuous_backups"]; +/// keyspace holds after a run: migration 5 drops `continuous_backups`, and +/// migration 10 `metrics`. +const DROPPED: &[&str] = &["continuous_backups", "metrics"]; /// One line for each table, index or added column of `schema` that `keyspace` /// lacks, for each table of `other` (the other store kind's schema) it diff --git a/crates/storage-cassandra/tests/direct/metrics_store.rs b/crates/storage-cassandra/tests/direct/metrics_store.rs new file mode 100644 index 000000000..71f7d8ca7 --- /dev/null +++ b/crates/storage-cassandra/tests/direct/metrics_store.rs @@ -0,0 +1,24 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Direct integration test for MetricsStore on a catalog keyspace that has +//! run every migration: `extenddb_storage::metrics_conformance` (#728). + +#[cfg(test)] +mod tests { + use crate::helpers::{create_catalog_store, setup_engine, test_config}; + + /// Two accounts' tables of the same name keep their own rows in + /// `account_metrics`, and the account filter returns only that + /// account's. The check's accounts are made up for the run, so the + /// shared test catalog's other rows stay out of its reads. + #[tokio::test] + async fn the_metrics_store_keeps_two_accounts_apart() { + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let store = create_catalog_store(&engine, &test_config()); + extenddb_storage::metrics_conformance::run(&store).await; + } +} diff --git a/crates/storage-cassandra/tests/direct_integration.rs b/crates/storage-cassandra/tests/direct_integration.rs index e95fca8d8..8f25787df 100644 --- a/crates/storage-cassandra/tests/direct_integration.rs +++ b/crates/storage-cassandra/tests/direct_integration.rs @@ -38,6 +38,8 @@ mod index; mod keyspace_migrations; #[path = "direct/metadata_engine.rs"] mod metadata_engine; +#[path = "direct/metrics_store.rs"] +mod metrics_store; #[path = "direct/policies.rs"] mod policies; #[path = "direct/pool_size.rs"] diff --git a/crates/storage-cassandra/tests/fixtures/0010_account_metrics.expected.json b/crates/storage-cassandra/tests/fixtures/0010_account_metrics.expected.json new file mode 100644 index 000000000..2d18b3a5b --- /dev/null +++ b/crates/storage-cassandra/tests/fixtures/0010_account_metrics.expected.json @@ -0,0 +1,65 @@ +{ + "columns": { + "account_metrics.account_id": { + "kind": "partition_key", + "position": 1, + "clustering_order": "none", + "type": "text" + }, + "account_metrics.bucket": { + "kind": "partition_key", + "position": 0, + "clustering_order": "none", + "type": "timestamp" + }, + "account_metrics.count": { + "kind": "regular", + "position": -1, + "clustering_order": "none", + "type": "bigint" + }, + "account_metrics.index_name": { + "kind": "partition_key", + "position": 4, + "clustering_order": "none", + "type": "text" + }, + "account_metrics.max": { + "kind": "regular", + "position": -1, + "clustering_order": "none", + "type": "double" + }, + "account_metrics.metric": { + "kind": "partition_key", + "position": 2, + "clustering_order": "none", + "type": "text" + }, + "account_metrics.min": { + "kind": "regular", + "position": -1, + "clustering_order": "none", + "type": "double" + }, + "account_metrics.operation": { + "kind": "partition_key", + "position": 5, + "clustering_order": "none", + "type": "text" + }, + "account_metrics.sum": { + "kind": "regular", + "position": -1, + "clustering_order": "none", + "type": "double" + }, + "account_metrics.table_name": { + "kind": "partition_key", + "position": 3, + "clustering_order": "none", + "type": "text" + } + }, + "indexes": {} +} diff --git a/crates/storage-cassandra/tests/migrations.rs b/crates/storage-cassandra/tests/migrations.rs index b173e131d..b942d76f2 100644 --- a/crates/storage-cassandra/tests/migrations.rs +++ b/crates/storage-cassandra/tests/migrations.rs @@ -107,8 +107,8 @@ async fn two_migrators_started_together_apply_a_legacy_catalog_once() { ) .await; // The second migrator waits for the first's whole run, every schema - // statement of migrations 1 to 6 and 9 with an agreement wait after each: - // about 16 s on an idle node, and past 60 s beside the rest of this + // statement of migrations 1 to 6, 9 and 10 with an agreement wait after + // each: about 16 s on an idle node, and past 60 s beside the rest of this // binary's tests on one node. let wait = 4 * TIMEOUT; let (first, other) = tokio::join!( diff --git a/crates/storage-cassandra/tests/migrations_support/mod.rs b/crates/storage-cassandra/tests/migrations_support/mod.rs index 47db05154..f49a90e05 100644 --- a/crates/storage-cassandra/tests/migrations_support/mod.rs +++ b/crates/storage-cassandra/tests/migrations_support/mod.rs @@ -20,10 +20,16 @@ pub const TIMEOUT: Duration = Duration::from_secs(60); pub const CATALOG_MIGRATION_6: &str = include_str!("../fixtures/0006_backup_catalog_index.expected.json"); +/// The columns of `account_metrics`, which migration 10 creates in the +/// catalog keyspace in place of `metrics`, which it drops. +pub const CATALOG_MIGRATION_10: &str = + include_str!("../fixtures/0010_account_metrics.expected.json"); + /// The definitions a keyspace of `dir` (`catalog` or `account`) holds after /// every migration: migration 1's, without `continuous_backups`, which -/// migration 5 drops, and in the catalog with what migration 6 adds and the -/// `warm_throughput` column migration 9 adds to `tables` and `indexes`. +/// migration 5 drops, and in the catalog with what migration 6 adds, the +/// `warm_throughput` column migration 9 adds to `tables` and `indexes`, and +/// `account_metrics` in place of `metrics` (migration 10). /// /// # Panics /// @@ -44,6 +50,13 @@ pub fn expected_definitions(dir: &str) -> postcheck::Definitions { serde_json::from_str(CATALOG_MIGRATION_6).expect("migration 6"); expected.columns.extend(added.columns); expected.indexes.extend(added.indexes); + expected + .columns + .retain(|name, _| !name.starts_with("metrics.")); + let added: postcheck::Definitions = + serde_json::from_str(CATALOG_MIGRATION_10).expect("migration 10"); + expected.columns.extend(added.columns); + expected.indexes.extend(added.indexes); for table in ["tables", "indexes"] { expected.columns.insert( format!("{table}.warm_throughput"), diff --git a/crates/storage-mongodb/src/catalog_store.rs b/crates/storage-mongodb/src/catalog_store.rs index b33530908..9439ced9e 100644 --- a/crates/storage-mongodb/src/catalog_store.rs +++ b/crates/storage-mongodb/src/catalog_store.rs @@ -45,6 +45,15 @@ impl MongoCatalogStore { } } + /// A store on `catalog_db`, for the crate's integration tests (`tests/`), + /// which work in scratch databases of their own. Compiled only with the + /// `test-util` feature, which production builds do not enable. + #[cfg(feature = "test-util")] + #[must_use] + pub fn on_database_for_tests(client: mongodb::Client, catalog_db: mongodb::Database) -> Self { + Self::on_database(client, catalog_db) + } + /// Get a reference to the catalog database. pub(crate) fn catalog_db(&self) -> &mongodb::Database { &self.catalog_db diff --git a/crates/storage-mongodb/src/management_store.rs b/crates/storage-mongodb/src/management_store.rs index 305ffb0fe..fe259753e 100644 --- a/crates/storage-mongodb/src/management_store.rs +++ b/crates/storage-mongodb/src/management_store.rs @@ -2143,6 +2143,7 @@ impl extenddb_storage::management_store::SettingsStore for MongoCatalogStore { // ── MetricsStore ──────────────────────────────────────────────────────── +/// `account_metrics`: migration 10 replaced `metrics` with it. impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { fn insert_metrics(&self, rows: &[MetricsRow]) -> BoxFuture<'_, OpResult<()>> { let rows = rows.to_vec(); @@ -2150,7 +2151,7 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { if rows.is_empty() { return Ok(()); } - let coll = self.catalog_db().collection::("metrics"); + let coll = self.catalog_db().collection::(METRICS_COLLECTION); let docs: Vec = rows .into_iter() .map(|r| { @@ -2162,6 +2163,9 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { "min": r.min, "max": r.max, }; + if let Some(ref account) = r.account_id { + d.insert("account_id", account.as_str()); + } if let Some(ref tn) = r.table_name { d.insert("table_name", tn.as_str()); } @@ -2186,24 +2190,26 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { &self, start: time::OffsetDateTime, end: time::OffsetDateTime, - table_name: Option<&str>, - metric: Option<&str>, + filter: &extenddb_storage::management_store::MetricsFilter, ) -> BoxFuture<'_, OpResult>> { - let table_name = table_name.map(std::borrow::ToOwned::to_owned); - let metric = metric.map(std::borrow::ToOwned::to_owned); + let fields: Vec<(&'static str, String)> = [ + ("account_id", filter.account_id()), + ("table_name", filter.table_name()), + ("metric", filter.metric()), + ] + .into_iter() + .filter_map(|(field, value)| value.map(|v| (field, v.to_owned()))) + .collect(); Box::pin(async move { - let coll = self.catalog_db().collection::("metrics"); + let coll = self.catalog_db().collection::(METRICS_COLLECTION); let start_bson = BsonDateTime::from_millis(start.unix_timestamp() * 1000); let end_bson = BsonDateTime::from_millis(end.unix_timestamp() * 1000); let mut filter = doc! { "bucket": { "$gte": start_bson, "$lte": end_bson } }; - if let Some(ref tn) = table_name { - filter.insert("table_name", tn.as_str()); - } - if let Some(ref m) = metric { - filter.insert("metric", m.as_str()); + for (field, value) in fields { + filter.insert(field, value); } let cursor = coll.find(filter).await.map_err(|e| { @@ -2214,24 +2220,17 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { tracing::error!("query_metrics cursor: {e}"); OpError::Internal("Database error".to_owned()) })?; + let text = |d: &Document, field: &str| d.get_str(field).ok().map(ToOwned::to_owned); Ok(docs .into_iter() .filter_map(|d| { Some(MetricsRow { bucket: to_offset_dt(d.get_datetime("bucket").ok()?.to_owned()), + account_id: text(&d, "account_id"), metric: d.get_str("metric").ok()?.to_owned(), - table_name: d - .get_str("table_name") - .ok() - .map(std::borrow::ToOwned::to_owned), - index_name: d - .get_str("index_name") - .ok() - .map(std::borrow::ToOwned::to_owned), - operation: d - .get_str("operation") - .ok() - .map(std::borrow::ToOwned::to_owned), + table_name: text(&d, "table_name"), + index_name: text(&d, "index_name"), + operation: text(&d, "operation"), sum: d.get_f64("sum").ok()?, count: d .get_i64("count") @@ -2247,7 +2246,7 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { fn prune_metrics(&self, retention: std::time::Duration) -> BoxFuture<'_, OpResult<()>> { Box::pin(async move { - let coll = self.catalog_db().collection::("metrics"); + let coll = self.catalog_db().collection::(METRICS_COLLECTION); let cutoff = time::OffsetDateTime::now_utc() - retention; let cutoff_bson = BsonDateTime::from_millis(cutoff.unix_timestamp() * 1000); coll.delete_many(doc! { "bucket": { "$lt": cutoff_bson } }) @@ -2261,6 +2260,10 @@ impl extenddb_storage::management_store::MetricsStore for MongoCatalogStore { } } +/// The catalog collection of the metrics rows, which migration 10 created +/// in place of `metrics`. +const METRICS_COLLECTION: &str = "account_metrics"; + // ── RateLimitStore ────────────────────────────────────────────────────── impl extenddb_storage::management_store::RateLimitStore for MongoCatalogStore { diff --git a/crates/storage-mongodb/src/migrations/m0010_account_metrics.rs b/crates/storage-mongodb/src/migrations/m0010_account_metrics.rs new file mode 100644 index 000000000..97a6e0622 --- /dev/null +++ b/crates/storage-mongodb/src/migrations/m0010_account_metrics.rs @@ -0,0 +1,91 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Migration 10, `account_metrics`, MongoDB: replace the catalog's `metrics` +//! collection with `account_metrics`, whose documents carry the account a +//! row was recorded for, so two accounts' tables of the same name keep their +//! own rows (#728). The body creates `account_metrics` with an index on +//! `bucket`, which the queries and the prune filter on, and drops `metrics` +//! when the catalog lists it. Its documents are dropped, not copied: they +//! carry no account, and the server keeps metrics for a day. A second run +//! changes nothing. +//! +//! The checksum test pins this file once the migration is released. The +//! module is excluded from rustfmt and clippy so neither can require an +//! edit. + +use async_trait::async_trait; +use bson::{Document, doc}; +use extenddb_storage::migrations::{CodeBody, Done, MigrationError}; +use mongodb::IndexModel; + +use super::MongoStores; + +/// This module, whose SHA-256 the checksum test pins. +pub(crate) const SOURCE: &str = include_str!("m0010_account_metrics.rs"); + +/// Recorded as this migration's checksum. Changes only with a new migration. +const FINGERPRINT: &str = "0010_account_metrics/mongodb/1"; + +/// The server's answer to creating a collection that exists. +const NAMESPACE_EXISTS: i32 = 48; + +/// The collection this migration creates. +const CREATED: &str = "account_metrics"; + +/// The collection this migration drops. +const DROPPED: &str = "metrics"; + +/// Migration 10's catalog body. +pub(crate) struct AccountMetrics; + +#[async_trait] +impl CodeBody for AccountMetrics { + fn fingerprint(&self) -> &'static str { + FINGERPRINT + } + + async fn detect(&self, _stores: &MongoStores) -> Result, MigrationError> { + Ok(Vec::new()) + } + + async fn run(&self, stores: &MongoStores) -> Result { + let db = &stores.catalog; + stores + .request(format!("create the collection {}.{CREATED}", db.name()), async { + match db.create_collection(CREATED).await { + Err(e) if command_code(&e) == Some(NAMESPACE_EXISTS) => Ok(()), + other => other, + } + }) + .await?; + let model = IndexModel::builder().keys(doc! { "bucket": 1 }).build(); + let created = db.collection::(CREATED); + stores + .request(format!("create the index {{ bucket: 1 }} on {}.{CREATED}", db.name()), async { + created.create_index(model).await.map(|_| ()) + }) + .await?; + let names = stores + .request(format!("list the collection {}.{DROPPED}", db.name()), async { + db.list_collection_names().filter(doc! { "name": DROPPED }).await + }) + .await?; + if !names.is_empty() { + let dropped = db.collection::(DROPPED); + stores + .request(format!("drop the collection {}.{DROPPED}", db.name()), async { + dropped.drop().await + }) + .await?; + } + Ok(Done::default()) + } +} + +fn command_code(error: &mongodb::error::Error) -> Option { + match error.kind.as_ref() { + mongodb::error::ErrorKind::Command(command) => Some(command.code), + _ => None, + } +} diff --git a/crates/storage-mongodb/src/migrations/mod.rs b/crates/storage-mongodb/src/migrations/mod.rs index 0c97c8972..981252eab 100644 --- a/crates/storage-mongodb/src/migrations/mod.rs +++ b/crates/storage-mongodb/src/migrations/mod.rs @@ -41,6 +41,9 @@ mod m0005_drop_continuous_backups; #[rustfmt::skip] #[allow(clippy::all, deprecated)] mod m0006_backup_catalog_index; +#[rustfmt::skip] +#[allow(clippy::all, deprecated)] +mod m0010_account_metrics; mod postcheck; use std::time::Duration; @@ -173,6 +176,15 @@ pub fn migration(id: MigrationId) -> Migration { "the catalog collections have no validator, so a table or index document carries \ a warm_throughput field without a schema change", ), + MigrationId::AccountMetrics => Migration::Applies { + kind: Kind::Breaking, + convergent: true, + bodies: vec![StoreBody::code( + StoreKind::Catalog, + &m0010_account_metrics::AccountMetrics, + m0010_account_metrics::SOURCE, + )], + }, } } diff --git a/crates/storage-mongodb/src/migrations/tests.rs b/crates/storage-mongodb/src/migrations/tests.rs index ad5d06b43..79e143249 100644 --- a/crates/storage-mongodb/src/migrations/tests.rs +++ b/crates/storage-mongodb/src/migrations/tests.rs @@ -59,6 +59,13 @@ fn every_body_is_code_and_three_four_seven_eight_and_nine_do_not_apply() { } assert!(migration(MigrationId::Baseline).convergent()); assert!(!migration(MigrationId::ResourceTagConditionKeys).convergent()); + // Migration 10 replaces the metrics collection, and runs again under + // --reconverge after migration 1 creates `metrics` again. + assert_eq!( + migration(MigrationId::AccountMetrics).store_kinds(), + [StoreKind::Catalog] + ); + assert!(migration(MigrationId::AccountMetrics).convergent()); } /// The lock, the registrations and the ledgers are read and written through diff --git a/crates/storage-mongodb/src/ttl_worker.rs b/crates/storage-mongodb/src/ttl_worker.rs index 81de24c02..3dd66139d 100644 --- a/crates/storage-mongodb/src/ttl_worker.rs +++ b/crates/storage-mongodb/src/ttl_worker.rs @@ -492,10 +492,10 @@ async fn sweep_expired_items(storage: &MongoEngine, metrics: &MetricsCollector, } Ok(_old_item) => { deleted += 1; - metrics.record_ttl_deletion(table_name); + metrics.record_ttl_deletion(account_id, table_name); if let Some(s) = staleness { #[allow(clippy::cast_precision_loss)] - metrics.record_ttl_staleness(table_name, s as f64); + metrics.record_ttl_staleness(account_id, table_name, s as f64); } } } diff --git a/crates/storage-mongodb/tests/fixtures/0010_account_metrics.expected.json b/crates/storage-mongodb/tests/fixtures/0010_account_metrics.expected.json new file mode 100644 index 000000000..7cb23b758 --- /dev/null +++ b/crates/storage-mongodb/tests/fixtures/0010_account_metrics.expected.json @@ -0,0 +1,26 @@ +{ + "account_metrics": { + "type": "collection", + "options": {}, + "indexes": { + "_id_": { + "key": [ + [ + "_id", + 1 + ] + ], + "options": {} + }, + "bucket_1": { + "key": [ + [ + "bucket", + 1 + ] + ], + "options": {} + } + } + } +} diff --git a/crates/storage-mongodb/tests/metrics_store.rs b/crates/storage-mongodb/tests/metrics_store.rs new file mode 100644 index 000000000..08a649dda --- /dev/null +++ b/crates/storage-mongodb/tests/metrics_store.rs @@ -0,0 +1,73 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 +//! The metrics store on a catalog that has run every migration (#728): +//! `extenddb_storage::metrics_conformance`, and migration 10's replacement of +//! the catalog's `metrics` collection with an empty `account_metrics`. They +//! need a replica set named by `EXTENDDB_TEST_MONGODB_URI`, and skip without +//! one. + +mod migrations_support; + +use bson::doc; +use extenddb_storage::migrations::MigrationId; +use extenddb_storage_mongodb::MongoCatalogStore; +use migrations_support::{Deployment, server}; + +/// Two accounts' tables of the same name keep their own rows, and the +/// account filter returns only that account's. +#[tokio::test(flavor = "multi_thread")] +async fn the_metrics_store_keeps_two_accounts_apart() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "metrics").await; + d.create().await; + let store = MongoCatalogStore::on_database_for_tests(d.client.clone(), d.catalog()); + extenddb_storage::metrics_conformance::run(&store).await; + d.drop_databases().await; +} + +/// Migration 10 drops the `metrics` collection, documents and all, and +/// creates `account_metrics`; running it again keeps what was written since. +#[tokio::test(flavor = "multi_thread")] +async fn migration_10_replaces_metrics_with_an_empty_account_metrics() { + let Some(uri) = server() else { return }; + let d = Deployment::new(&uri, "m10").await; + d.migrate_through(MigrationId::WarmThroughput) + .await + .expect("migrate through 9"); + let catalog = d.catalog(); + catalog + .collection::("metrics") + .insert_one(doc! { "bucket": bson::DateTime::now(), "metric": "UserErrors" }) + .await + .expect("a metrics document"); + d.migrate().await.expect("migrate"); + let collections = Deployment::collections(&catalog).await; + assert!( + !collections.contains(&"metrics".to_owned()), + "{collections:?}" + ); + assert!( + Deployment::documents(&catalog, "account_metrics") + .await + .is_empty() + ); + + catalog + .collection::("account_metrics") + .insert_one(doc! { "bucket": bson::DateTime::now(), "account_id": "111111111111" }) + .await + .expect("an account_metrics document"); + d.reconverge().await.expect("reconverge"); + let collections = Deployment::collections(&catalog).await; + assert!( + !collections.contains(&"metrics".to_owned()), + "{collections:?}" + ); + assert_eq!( + Deployment::documents(&catalog, "account_metrics") + .await + .len(), + 1 + ); + d.drop_databases().await; +} diff --git a/crates/storage-mongodb/tests/migrations_fresh.rs b/crates/storage-mongodb/tests/migrations_fresh.rs index a654ba341..871f75dda 100644 --- a/crates/storage-mongodb/tests/migrations_fresh.rs +++ b/crates/storage-mongodb/tests/migrations_fresh.rs @@ -325,6 +325,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { (7, d.target.catalog.clone()), (8, d.target.catalog.clone()), (9, d.target.catalog.clone()), + (10, d.target.catalog.clone()), ] ); let not_applicable: Vec = report @@ -354,6 +355,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { (7, Outcome::NotApplicable), (8, Outcome::NotApplicable), (9, Outcome::NotApplicable), + (10, Outcome::Applied), ] ); assert!( @@ -375,8 +377,9 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { let (catalog, data) = definitions(&d).await; if std::env::var("EXTENDDB_BLESS_EXPECTED").as_deref() == Ok("1") { let dir = env!("CARGO_MANIFEST_DIR"); - // Migration 1's file keeps the collection migration 5 drops and - // leaves out what migration 6 adds, which its own file holds. + // Migration 1's file keeps the collections migrations 5 and 10 drop + // and leaves out what migrations 6 and 10 add, which their own files + // hold. let committed: Definitions = serde_json::from_str(CATALOG_EXPECTED).expect("catalog"); let added: Definitions = serde_json::from_str(migrations_support::CATALOG_MIGRATION_6).expect("migration 6"); @@ -395,9 +398,20 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { } } } - if let Some(dropped) = committed.get("continuous_backups") { - catalog_at_1.insert("continuous_backups".to_owned(), dropped.clone()); + for name in ["continuous_backups", "metrics"] { + if let Some(dropped) = committed.get(name) { + catalog_at_1.insert(name.to_owned(), dropped.clone()); + } } + let at_10: Definitions = + serde_json::from_str::(migrations_support::CATALOG_MIGRATION_10) + .expect("migration 10") + .keys() + .map(|name| { + catalog_at_1.remove(name); + (name.clone(), catalog[name].clone()) + }) + .collect(); let mut at_6 = Definitions::new(); for (name, collection) in &added { let mut found = catalog[name].clone(); @@ -418,6 +432,7 @@ async fn a_fresh_store_gets_every_migration_and_the_expected_definitions() { "tests/fixtures/0006_backup_catalog_index.expected.json", &at_6, ), + ("tests/fixtures/0010_account_metrics.expected.json", &at_10), ] { let text = serde_json::to_string_pretty(defs).expect("json") + "\n"; std::fs::write(format!("{dir}/{path}"), text).expect("write"); @@ -448,8 +463,8 @@ async fn a_second_run_applies_nothing_and_reconverge_keeps_the_rows() { let reconverged = d.reconverge().await.expect("reconverge"); // Migration 1's steps in the catalog and the data database, and - // migrations 5's and 6's in the catalog: 2 is not convergent, and 3, 4, - // 7, 8 and 9 do not apply on MongoDB. + // migrations 5's, 6's and 10's in the catalog: 2 is not convergent, and + // 3, 4, 7, 8 and 9 do not apply on MongoDB. let ran: Vec<(u32, String)> = reconverged .applied .iter() @@ -462,6 +477,7 @@ async fn a_second_run_applies_nothing_and_reconverge_keeps_the_rows() { (1, d.target.data.clone()), (5, d.target.catalog.clone()), (6, d.target.catalog.clone()), + (10, d.target.catalog.clone()), ], "{reconverged:?}" ); @@ -600,24 +616,24 @@ async fn a_changed_checksum_a_newer_catalog_and_an_older_release_are_reported() "notes": "", "applied_at": bson::DateTime::now(), "applied_by": "9.9.9" } }; ledger - .insert_one(row(10, "breaking")) + .insert_one(row(11, "breaking")) .await .expect("insert"); let status = d.status().await; assert_eq!(status.label(), "AheadIncompatible"); assert_eq!(status.exit_code(), exit::AHEAD_INCOMPATIBLE); ledger - .delete_one(doc! { "_id": 10_i64 }) + .delete_one(doc! { "_id": 11_i64 }) .await .expect("delete"); ledger - .insert_one(row(10, "additive")) + .insert_one(row(11, "additive")) .await .expect("insert"); assert_eq!(d.status().await.label(), "AheadCompatible"); assert!(d.migrate().await.expect("migrate").applied.is_empty()); ledger - .delete_one(doc! { "_id": 10_i64 }) + .delete_one(doc! { "_id": 11_i64 }) .await .expect("delete"); @@ -698,6 +714,6 @@ async fn a_ledger_document_this_binary_cannot_read_stops_the_status() { .expect_err("an error"); migrations.close().await; assert!(error.to_string().contains("\"sideways\""), "{error}"); - assert_eq!(MigrationId::LATEST.number(), 9); + assert_eq!(MigrationId::LATEST.number(), 10); d.drop_databases().await; } diff --git a/crates/storage-mongodb/tests/migrations_guards.rs b/crates/storage-mongodb/tests/migrations_guards.rs index 8d68a404c..af1453cdc 100644 --- a/crates/storage-mongodb/tests/migrations_guards.rs +++ b/crates/storage-mongodb/tests/migrations_guards.rs @@ -66,6 +66,7 @@ async fn assert_fresh_definitions(d: &Deployment) { migrations.close().await; let expected_catalog = migrations_support::expected_catalog(); assert!(!catalog.contains_key("continuous_backups")); + assert!(!catalog.contains_key("metrics")); let expected_data: Definitions = serde_json::from_str(DATA_EXPECTED).expect("json"); for (name, want) in &expected_catalog { assert_eq!(catalog.get(name), Some(want), "{name}"); diff --git a/crates/storage-mongodb/tests/migrations_locks.rs b/crates/storage-mongodb/tests/migrations_locks.rs index bf8d5d911..934916533 100644 --- a/crates/storage-mongodb/tests/migrations_locks.rs +++ b/crates/storage-mongodb/tests/migrations_locks.rs @@ -53,7 +53,7 @@ async fn two_migrators_started_together_apply_once_and_the_other_finds_nothing() applied.sort_unstable(); assert_eq!( applied, - [0, 10], + [0, 11], "one applies every step and the other finds nothing" ); assert_eq!( diff --git a/crates/storage-mongodb/tests/migrations_released.rs b/crates/storage-mongodb/tests/migrations_released.rs index b03921406..02f4a4de5 100644 --- a/crates/storage-mongodb/tests/migrations_released.rs +++ b/crates/storage-mongodb/tests/migrations_released.rs @@ -161,7 +161,7 @@ async fn every_released_catalog_upgrades_to_the_definitions_of_a_fresh_install() panic!("{state} is not behind"); }; assert_eq!(version, None, "{state} has no ledger"); - assert_eq!(pending.len(), 10, "{state}"); + assert_eq!(pending.len(), 11, "{state}"); let report = d.migrate().await.expect("migrate"); assert_eq!( (report.from, report.to), @@ -180,10 +180,9 @@ async fn every_released_catalog_upgrades_to_the_definitions_of_a_fresh_install() for (name, want) in &expected_catalog { assert_eq!(catalog.get(name), Some(want), "{state}: {name}"); } - assert!( - !catalog.contains_key("continuous_backups"), - "{state}: continuous_backups" - ); + for dropped in ["continuous_backups", "metrics"] { + assert!(!catalog.contains_key(dropped), "{state}: {dropped}"); + } for (name, want) in &expected_data { assert_eq!(data.get(name), Some(want), "{state}: {name}"); } diff --git a/crates/storage-mongodb/tests/migrations_support/mod.rs b/crates/storage-mongodb/tests/migrations_support/mod.rs index 24e970bd0..ae507d3d5 100644 --- a/crates/storage-mongodb/tests/migrations_support/mod.rs +++ b/crates/storage-mongodb/tests/migrations_support/mod.rs @@ -37,9 +37,13 @@ pub const CATALOG_BASELINE: &str = pub const CATALOG_MIGRATION_6: &str = include_str!("../fixtures/0006_backup_catalog_index.expected.json"); +/// The collection migration 10 creates in place of `metrics`, which it drops. +pub const CATALOG_MIGRATION_10: &str = + include_str!("../fixtures/0010_account_metrics.expected.json"); + /// The catalog database's definitions after every migration: migration 1's, -/// without `continuous_backups`, which migration 5 drops, and with what -/// migration 6 adds. +/// without `continuous_backups`, which migration 5 drops, and `metrics`, +/// which migration 10 drops, with what migrations 6 and 10 add. /// /// # Panics /// @@ -48,12 +52,16 @@ pub const CATALOG_MIGRATION_6: &str = pub fn expected_catalog() -> Definitions { let mut expected: Definitions = serde_json::from_str(CATALOG_BASELINE).expect("migration 1"); expected.remove("continuous_backups"); - let added: Definitions = serde_json::from_str(CATALOG_MIGRATION_6).expect("migration 6"); - for (name, collection) in added { - match expected.get_mut(&name) { - Some(existing) => existing.indexes.extend(collection.indexes), - None => { - expected.insert(name, collection); + expected.remove("metrics"); + for (file, id) in [(CATALOG_MIGRATION_6, 6), (CATALOG_MIGRATION_10, 10)] { + let added: Definitions = + serde_json::from_str(file).unwrap_or_else(|e| panic!("migration {id}: {e}")); + for (name, collection) in added { + match expected.get_mut(&name) { + Some(existing) => existing.indexes.extend(collection.indexes), + None => { + expected.insert(name, collection); + } } } } diff --git a/crates/storage-postgres/migrations/0010_account_metrics.sql b/crates/storage-postgres/migrations/0010_account_metrics.sql new file mode 100644 index 000000000..9ec918992 --- /dev/null +++ b/crates/storage-postgres/migrations/0010_account_metrics.sql @@ -0,0 +1,28 @@ +-- Copyright 2026 ExtendDB contributors +-- SPDX-License-Identifier: Apache-2.0 +-- +-- Migration 10, account_metrics, PostgreSQL catalog database: replaces the +-- metrics table with account_metrics, whose key has the account a row was +-- recorded for, so two accounts' tables of the same name keep their own rows +-- (#728). account_id is the empty string on a metric of the whole +-- deployment. The rows of metrics are dropped, not copied: they carry no +-- account, and the server keeps metrics for a day. IF NOT EXISTS and +-- IF EXISTS make a second run change nothing. + +CREATE TABLE IF NOT EXISTS account_metrics ( + bucket TIMESTAMPTZ NOT NULL, + account_id TEXT NOT NULL DEFAULT '', + metric TEXT NOT NULL, + table_name TEXT NOT NULL DEFAULT '', + index_name TEXT NOT NULL DEFAULT '', + operation TEXT NOT NULL DEFAULT '', + sum DOUBLE PRECISION NOT NULL DEFAULT 0, + count BIGINT NOT NULL DEFAULT 0, + min DOUBLE PRECISION NOT NULL DEFAULT 'Infinity', + max DOUBLE PRECISION NOT NULL DEFAULT '-Infinity', + PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) +); + +CREATE INDEX IF NOT EXISTS idx_account_metrics_bucket ON account_metrics (bucket); + +DROP TABLE IF EXISTS metrics; diff --git a/crates/storage-postgres/src/catalog_store.rs b/crates/storage-postgres/src/catalog_store.rs index 834c88a5a..27b815988 100755 --- a/crates/storage-postgres/src/catalog_store.rs +++ b/crates/storage-postgres/src/catalog_store.rs @@ -197,22 +197,25 @@ impl extenddb_storage::diagnostics::DiagnosticsStore for PostgresCatalogStore { // ── MetricsStore ─────────────────────────────────────────────────────── +/// `account_metrics`: migration 10 replaced `metrics` with it. impl extenddb_storage::management_store::MetricsStore for PostgresCatalogStore { fn insert_metrics(&self, rows: &[MetricsRow]) -> BoxFuture<'_, OpResult<()>> { let rows = rows.to_vec(); Box::pin(async move { for row in &rows { let result = sqlx::query( - "INSERT INTO metrics \ - (bucket, metric, table_name, index_name, operation, sum, count, min, max) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) \ - ON CONFLICT (bucket, metric, table_name, index_name, operation) \ - DO UPDATE SET sum = metrics.sum + EXCLUDED.sum, \ - count = metrics.count + EXCLUDED.count, \ - min = LEAST(metrics.min, EXCLUDED.min), \ - max = GREATEST(metrics.max, EXCLUDED.max)", + "INSERT INTO account_metrics \ + (bucket, account_id, metric, table_name, index_name, operation, \ + sum, count, min, max) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) \ + ON CONFLICT (bucket, account_id, metric, table_name, index_name, operation) \ + DO UPDATE SET sum = account_metrics.sum + EXCLUDED.sum, \ + count = account_metrics.count + EXCLUDED.count, \ + min = LEAST(account_metrics.min, EXCLUDED.min), \ + max = GREATEST(account_metrics.max, EXCLUDED.max)", ) .bind(row.bucket) + .bind(row.account_id.as_deref().unwrap_or("")) .bind(&row.metric) .bind(row.table_name.as_deref().unwrap_or("")) .bind(row.index_name.as_deref().unwrap_or("")) @@ -235,41 +238,35 @@ impl extenddb_storage::management_store::MetricsStore for PostgresCatalogStore { &self, start: time::OffsetDateTime, end: time::OffsetDateTime, - table_name: Option<&str>, - metric: Option<&str>, + filter: &extenddb_storage::management_store::MetricsFilter, ) -> BoxFuture<'_, OpResult>> { - let table_name = table_name.map(std::borrow::ToOwned::to_owned); - let metric = metric.map(std::borrow::ToOwned::to_owned); + let filters: Vec<(&'static str, String)> = [ + ("account_id", filter.account_id()), + ("table_name", filter.table_name()), + ("metric", filter.metric()), + ] + .into_iter() + .filter_map(|(column, value)| value.map(|v| (column, v.to_owned()))) + .collect(); Box::pin(async move { use std::fmt::Write as _; let mut sql = String::from( - "SELECT bucket, metric, table_name, index_name, operation, \ + "SELECT bucket, account_id, metric, table_name, index_name, operation, \ sum, count, min, max \ - FROM metrics WHERE bucket >= $1 AND bucket <= $2", + FROM account_metrics WHERE bucket >= $1 AND bucket <= $2", ); - let mut param_idx = 3u32; - - let table_filter = table_name.as_deref().filter(|s| !s.is_empty()); - if table_filter.is_some() { - let _ = write!(sql, " AND table_name = ${param_idx}"); - param_idx += 1; - } - if metric.is_some() { - let _ = write!(sql, " AND metric = ${param_idx}"); + for (i, (column, _)) in filters.iter().enumerate() { + let _ = write!(sql, " AND {column} = ${}", i + 3); } - let _ = param_idx; sql.push_str(" ORDER BY bucket"); // Build the query with dynamic binds. let mut query = sqlx::query_as::<_, DbMetricsRow>(&sql) .bind(start) .bind(end); - if let Some(tn) = table_filter { - query = query.bind(tn); - } - if let Some(mn) = metric.as_deref() { - query = query.bind(mn); + for (_, value) in &filters { + query = query.bind(value); } let rows = query.fetch_all(&self.pool).await.map_err(|e| { @@ -277,26 +274,16 @@ impl extenddb_storage::management_store::MetricsStore for PostgresCatalogStore { OpError::Internal("Database error".to_owned()) })?; + let present = |s: String| (!s.is_empty()).then_some(s); Ok(rows .into_iter() .map(|r| MetricsRow { bucket: r.bucket, + account_id: present(r.account_id), metric: r.metric, - table_name: if r.table_name.is_empty() { - None - } else { - Some(r.table_name) - }, - index_name: if r.index_name.is_empty() { - None - } else { - Some(r.index_name) - }, - operation: if r.operation.is_empty() { - None - } else { - Some(r.operation) - }, + table_name: present(r.table_name), + index_name: present(r.index_name), + operation: present(r.operation), sum: r.sum, count: r.count, min: r.min, @@ -311,7 +298,7 @@ impl extenddb_storage::management_store::MetricsStore for PostgresCatalogStore { #[allow(clippy::cast_possible_wrap)] // retention seconds fit in i64 let cutoff = time::OffsetDateTime::now_utc() - time::Duration::seconds(retention.as_secs() as i64); - sqlx::query("DELETE FROM metrics WHERE bucket < $1") + sqlx::query("DELETE FROM account_metrics WHERE bucket < $1") .bind(cutoff) .execute(&self.pool) .await @@ -328,6 +315,7 @@ impl extenddb_storage::management_store::MetricsStore for PostgresCatalogStore { #[derive(sqlx::FromRow)] struct DbMetricsRow { bucket: time::OffsetDateTime, + account_id: String, metric: String, table_name: String, index_name: String, diff --git a/crates/storage-postgres/src/migrations/mod.rs b/crates/storage-postgres/src/migrations/mod.rs index 0f360ac1d..38400723e 100644 --- a/crates/storage-postgres/src/migrations/mod.rs +++ b/crates/storage-postgres/src/migrations/mod.rs @@ -167,6 +167,15 @@ pub fn migration(id: MigrationId) -> Migration { "0009_warm_throughput.sql" )], }, + MigrationId::AccountMetrics => Migration::Applies { + kind: Kind::Breaking, + convergent: true, + bodies: vec![file_body!( + StoreKind::Catalog, + "migrations", + "0010_account_metrics.sql" + )], + }, } } @@ -192,7 +201,8 @@ pub(crate) const fn runs_in(id: MigrationId) -> RunsIn { | MigrationId::BackupCatalogIndex | MigrationId::StreamPending | MigrationId::RestoreProgress - | MigrationId::WarmThroughput => RunsIn::Transaction, + | MigrationId::WarmThroughput + | MigrationId::AccountMetrics => RunsIn::Transaction, MigrationId::IndexTableBaseKey => RunsIn::Autocommit, } } diff --git a/crates/storage-postgres/src/ttl_worker.rs b/crates/storage-postgres/src/ttl_worker.rs index c40a59b5e..2a5033006 100644 --- a/crates/storage-postgres/src/ttl_worker.rs +++ b/crates/storage-postgres/src/ttl_worker.rs @@ -156,10 +156,10 @@ async fn sweep_expired_items( } Ok(_old_item) => { deleted += 1; - metrics.record_ttl_deletion(table_name); + metrics.record_ttl_deletion(account_id, table_name); if let Some(s) = staleness { #[allow(clippy::cast_precision_loss)] - metrics.record_ttl_staleness(table_name, s as f64); + metrics.record_ttl_staleness(account_id, table_name, s as f64); } } } diff --git a/crates/storage-postgres/tests/metrics_store.rs b/crates/storage-postgres/tests/metrics_store.rs new file mode 100644 index 000000000..6eed98e21 --- /dev/null +++ b/crates/storage-postgres/tests/metrics_store.rs @@ -0,0 +1,81 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 +//! The metrics store on a catalog that has run every migration (#728): +//! `extenddb_storage::metrics_conformance`, and migration 10's replacement of +//! a released catalog's `metrics` table with an empty `account_metrics`. See +//! `migrations_support` for the database these tests need. + +mod migrations_support; + +use extenddb_storage::migrations::MigrationId; +use extenddb_storage_postgres::PostgresCatalogStore; +use migrations_support::{Scratch, base_conn, count, skip}; + +/// Two accounts' tables of the same name keep their own rows, and the +/// account filter returns only that account's. +#[tokio::test] +async fn the_metrics_store_keeps_two_accounts_apart() { + let Some(base) = base_conn() else { + return skip("metrics store"); + }; + let s = Scratch::new(&base, "pgm_metrics").await; + s.migrate_through(MigrationId::LATEST).await; + let catalog = s.catalog().await; + let store = PostgresCatalogStore::new(catalog.clone()); + extenddb_storage::metrics_conformance::run(&store).await; + catalog.close().await; +} + +/// Migration 10 drops a released catalog's `metrics` table, rows and all, +/// and leaves an empty `account_metrics`; a second run of its body keeps +/// the rows written since. +#[tokio::test] +async fn migration_10_replaces_metrics_with_an_empty_account_metrics() { + let Some(base) = base_conn() else { + return skip("migration 10"); + }; + let s = Scratch::new(&base, "pgm_m10").await; + s.migrate_through(MigrationId::WarmThroughput).await; + let catalog = s.catalog().await; + sqlx::query( + "INSERT INTO metrics (bucket, metric, table_name, operation, sum, count, min, max) \ + VALUES (now(), 'UserErrors', 'Orders', 'PutItem', 1, 1, 1, 1)", + ) + .execute(&catalog) + .await + .expect("insert a metrics row"); + s.migrate_through(MigrationId::AccountMetrics).await; + assert_eq!( + count( + &catalog, + "SELECT COUNT(*) FROM pg_tables WHERE schemaname = 'public' AND tablename = 'metrics'" + ) + .await, + 0, + "metrics is dropped" + ); + assert_eq!( + count(&catalog, "SELECT COUNT(*) FROM account_metrics").await, + 0 + ); + for account in ["111111111111", "222222222222"] { + sqlx::query( + "INSERT INTO account_metrics \ + (bucket, account_id, metric, table_name, operation, sum, count, min, max) \ + VALUES (now(), $1, 'UserErrors', 'Orders', 'PutItem', 1, 1, 1, 1)", + ) + .bind(account) + .execute(&catalog) + .await + .expect("insert an account_metrics row"); + } + sqlx::raw_sql(include_str!("../migrations/0010_account_metrics.sql")) + .execute(&catalog) + .await + .expect("the body again"); + assert_eq!( + count(&catalog, "SELECT COUNT(*) FROM account_metrics").await, + 2 + ); + catalog.close().await; +} diff --git a/crates/storage-sqlite/migrations/0010_account_metrics.sql b/crates/storage-sqlite/migrations/0010_account_metrics.sql new file mode 100644 index 000000000..7c26396fb --- /dev/null +++ b/crates/storage-sqlite/migrations/0010_account_metrics.sql @@ -0,0 +1,28 @@ +-- Copyright 2026 ExtendDB contributors +-- SPDX-License-Identifier: Apache-2.0 +-- +-- Migration 10, account_metrics, SQLite: replaces the metrics table with +-- account_metrics, whose key has the account a row was recorded for, so two +-- accounts' tables of the same name keep their own rows (#728). account_id is +-- the empty string on a metric of the whole deployment. The rows of metrics +-- are dropped, not copied: they carry no account, and the server keeps +-- metrics for a day. IF NOT EXISTS and IF EXISTS make a second run change +-- nothing. ±9e999 store as ±Infinity in SQLite REAL, as in migration 1. + +CREATE TABLE IF NOT EXISTS account_metrics ( + bucket TEXT NOT NULL, + account_id TEXT NOT NULL DEFAULT '', + metric TEXT NOT NULL, + table_name TEXT NOT NULL DEFAULT '', + index_name TEXT NOT NULL DEFAULT '', + operation TEXT NOT NULL DEFAULT '', + sum REAL NOT NULL DEFAULT 0, + count INTEGER NOT NULL DEFAULT 0, + min REAL NOT NULL DEFAULT 9e999, + max REAL NOT NULL DEFAULT -9e999, + PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) +); + +CREATE INDEX IF NOT EXISTS idx_account_metrics_bucket ON account_metrics (bucket); + +DROP TABLE IF EXISTS metrics; diff --git a/crates/storage-sqlite/src/catalog_store.rs b/crates/storage-sqlite/src/catalog_store.rs index 17af9a254..651fdb5cc 100644 --- a/crates/storage-sqlite/src/catalog_store.rs +++ b/crates/storage-sqlite/src/catalog_store.rs @@ -12,7 +12,7 @@ use std::sync::Arc; use extenddb_storage::CatalogStore; use extenddb_storage::diagnostics::{DiagError, DiagResult, DiagnosticsStore}; use extenddb_storage::management_store::{ - MetricsRow, MetricsStore, OpError, OpResult, RateLimitStore, SettingsStore, + MetricsFilter, MetricsRow, MetricsStore, OpError, OpResult, RateLimitStore, SettingsStore, }; use futures::future::BoxFuture; use sqlx::SqlitePool; @@ -138,6 +138,7 @@ impl SettingsStore for SqliteCatalogStore { #[derive(sqlx::FromRow)] struct DbMetricsRow { bucket: String, + account_id: String, metric: String, table_name: String, index_name: String, @@ -148,6 +149,7 @@ struct DbMetricsRow { max: f64, } +/// `account_metrics`: migration 10 replaced `metrics` with it. impl MetricsStore for SqliteCatalogStore { fn insert_metrics(&self, rows: &[MetricsRow]) -> BoxFuture<'_, OpResult<()>> { let rows = rows.to_vec(); @@ -158,16 +160,19 @@ impl MetricsStore for SqliteCatalogStore { for row in &rows { let bucket = format_timestamp(row.bucket); let result = sqlx::query( - "INSERT INTO metrics \ - (bucket, metric, table_name, index_name, operation, sum, count, min, max) \ - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) \ - ON CONFLICT(bucket, metric, table_name, index_name, operation) DO UPDATE SET \ - sum = metrics.sum + excluded.sum, \ - count = metrics.count + excluded.count, \ - min = MIN(metrics.min, excluded.min), \ - max = MAX(metrics.max, excluded.max)", + "INSERT INTO account_metrics \ + (bucket, account_id, metric, table_name, index_name, operation, \ + sum, count, min, max) \ + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) \ + ON CONFLICT(bucket, account_id, metric, table_name, index_name, operation) \ + DO UPDATE SET \ + sum = account_metrics.sum + excluded.sum, \ + count = account_metrics.count + excluded.count, \ + min = MIN(account_metrics.min, excluded.min), \ + max = MAX(account_metrics.max, excluded.max)", ) .bind(&bucket) + .bind(row.account_id.as_deref().unwrap_or("")) .bind(&row.metric) .bind(row.table_name.as_deref().unwrap_or("")) .bind(row.index_name.as_deref().unwrap_or("")) @@ -190,20 +195,23 @@ impl MetricsStore for SqliteCatalogStore { &self, start: OffsetDateTime, end: OffsetDateTime, - table_name: Option<&str>, - metric: Option<&str>, + filter: &MetricsFilter, ) -> BoxFuture<'_, OpResult>> { - let table_name = table_name.map(str::to_owned); - let metric = metric.map(str::to_owned); + let account_id = filter.account_id().map(str::to_owned); + let table_name = filter.table_name().map(str::to_owned); + let metric = filter.metric().map(str::to_owned); let start_str = format_timestamp(start); let end_str = format_timestamp(end); Box::pin(async move { let mut sql = String::from( - "SELECT bucket, metric, table_name, index_name, operation, sum, count, min, max \ - FROM metrics WHERE bucket >= ? AND bucket <= ?", + "SELECT bucket, account_id, metric, table_name, index_name, operation, \ + sum, count, min, max \ + FROM account_metrics WHERE bucket >= ? AND bucket <= ?", ); - let table_filter = table_name.as_deref().filter(|s| !s.is_empty()); - if table_filter.is_some() { + if account_id.is_some() { + sql.push_str(" AND account_id = ?"); + } + if table_name.is_some() { sql.push_str(" AND table_name = ?"); } if metric.is_some() { @@ -214,11 +222,8 @@ impl MetricsStore for SqliteCatalogStore { let mut q = sqlx::query_as::<_, DbMetricsRow>(&sql) .bind(&start_str) .bind(&end_str); - if let Some(tn) = table_filter { - q = q.bind(tn.to_owned()); - } - if let Some(m) = metric.as_deref() { - q = q.bind(m.to_owned()); + for value in [&account_id, &table_name, &metric].into_iter().flatten() { + q = q.bind(value.clone()); } let rows = q @@ -231,6 +236,7 @@ impl MetricsStore for SqliteCatalogStore { .filter_map(|r| { Some(MetricsRow { bucket: parse_timestamp(&r.bucket).ok()?, + account_id: (!r.account_id.is_empty()).then_some(r.account_id), metric: r.metric, table_name: (!r.table_name.is_empty()).then_some(r.table_name), index_name: (!r.index_name.is_empty()).then_some(r.index_name), @@ -252,7 +258,7 @@ impl MetricsStore for SqliteCatalogStore { let cutoff_str = format_timestamp(cutoff); // D1: every writer holds the write lock. let _writer = self.lock_writes().await; - sqlx::query("DELETE FROM metrics WHERE bucket < ?") + sqlx::query("DELETE FROM account_metrics WHERE bucket < ?") .bind(&cutoff_str) .execute(&self.pool) .await diff --git a/crates/storage-sqlite/src/migrations/mod.rs b/crates/storage-sqlite/src/migrations/mod.rs index 62211a0c3..212a7a97f 100644 --- a/crates/storage-sqlite/src/migrations/mod.rs +++ b/crates/storage-sqlite/src/migrations/mod.rs @@ -138,6 +138,15 @@ pub fn migration(id: MigrationId) -> Migration { m0009_warm_throughput::SOURCE, )], }, + MigrationId::AccountMetrics => Migration::Applies { + kind: Kind::Breaking, + convergent: true, + bodies: vec![file_body!( + StoreKind::Catalog, + "migrations", + "0010_account_metrics.sql" + )], + }, } } diff --git a/crates/storage-sqlite/src/migrations/released_tests.rs b/crates/storage-sqlite/src/migrations/released_tests.rs index e453df2ef..df565220e 100644 --- a/crates/storage-sqlite/src/migrations/released_tests.rs +++ b/crates/storage-sqlite/src/migrations/released_tests.rs @@ -133,8 +133,9 @@ async fn settings(db: &Path) -> BTreeMap { } /// Tables that a released catalog has and a migration drops: migration 5 -/// drops `continuous_backups`. -const DROPPED_TABLES: &[&str] = &["continuous_backups"]; +/// drops `continuous_backups`, and migration 10 `metrics`, which +/// `account_metrics` replaces. +const DROPPED_TABLES: &[&str] = &["continuous_backups", "metrics"]; /// Row counts of every table but the ones a migration writes. async fn row_counts(db: &Path) -> BTreeMap { @@ -361,6 +362,57 @@ async fn every_released_catalog_upgrades_to_the_fresh_schema() { } } +/// Migration 10 replaces `metrics`, whose rows carry no account, with an +/// empty `account_metrics` keyed by account: the released rows are dropped, +/// not copied. Two accounts' rows for a table of the same name are then two +/// rows, and a second run of the body changes nothing. +#[tokio::test] +async fn migration_10_replaces_metrics_with_an_empty_account_metrics() { + let db = temp_db("m10"); + load_dump(&db, DEV_0_1_14).await; + assert!(count(&db, "SELECT COUNT(*) FROM metrics").await > 0); + run(&db).await.expect("migrate"); + assert_eq!( + count( + &db, + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'metrics'" + ) + .await, + 0, + "metrics is dropped" + ); + assert_eq!(count(&db, "SELECT COUNT(*) FROM account_metrics").await, 0); + + let insert = |account: &str| { + format!( + "INSERT INTO account_metrics (bucket, account_id, metric, table_name, operation, \ + sum, count, min, max) \ + VALUES ('2026-11-11T00:00:00Z', '{account}', 'UserErrors', 'Orders', 'PutItem', \ + 1, 1, 1, 1)" + ) + }; + exec(&db, &insert("111111111111")).await; + exec(&db, &insert("222222222222")).await; + assert_eq!(count(&db, "SELECT COUNT(*) FROM account_metrics").await, 2); + + // The body again, as --reconverge runs it: the rows stay. + let body = include_str!("../../migrations/0010_account_metrics.sql"); + exec(&db, body).await; + assert_eq!(count(&db, "SELECT COUNT(*) FROM account_metrics").await, 2); +} + +/// The metrics store on a migrated catalog keeps two accounts' tables of +/// the same name apart, and filters on the account. +#[tokio::test] +async fn the_metrics_store_keeps_two_accounts_apart() { + let db = temp_db("metrics-store"); + run(&db).await.expect("migrate"); + let pool = plain_pool(&db).await; + let store = crate::SqliteCatalogStore::new(pool.clone()); + extenddb_storage::metrics_conformance::run(&store).await; + pool.close().await; +} + /// A release before 1.0 that starts on a v1.0 catalog rewrites /// `catalog_version` (SQLite dev builds) or the marker (`migrate --yes`). /// v1.0 then reports it, refuses to migrate, and `--reconverge` repairs it, diff --git a/crates/storage-sqlite/src/migrations/tests.rs b/crates/storage-sqlite/src/migrations/tests.rs index 6a54dbd5d..1c7ffdbb4 100644 --- a/crates/storage-sqlite/src/migrations/tests.rs +++ b/crates/storage-sqlite/src/migrations/tests.rs @@ -5,9 +5,9 @@ //! protocol, the copy, the entry points and the in-memory case. The lock //! is in `serve_tests`, beside the server's refusal while a migration //! holds it. -//! Released catalogs are in `released_tests`, migrations 2 to 4, 6 and 9 in -//! `golden_tests`, refusals and rollbacks in `refusal_tests`, and the killed -//! migrator in `crash_tests`. +//! Released catalogs and migration 10 are in `released_tests`, migrations 2 +//! to 4, 6 and 9 in `golden_tests`, refusals and rollbacks in +//! `refusal_tests`, and the killed migrator in `crash_tests`. use std::path::{Path, PathBuf}; use std::time::Duration; diff --git a/crates/storage-sqlite/src/workers.rs b/crates/storage-sqlite/src/workers.rs index a518457f1..7992e1c37 100644 --- a/crates/storage-sqlite/src/workers.rs +++ b/crates/storage-sqlite/src/workers.rs @@ -289,12 +289,12 @@ async fn sweep_expired_items( Err(e) => tracing::warn!("TTL worker: delete failed for {table_name}: {e}"), Ok(_) => { deleted += 1; - metrics.record_ttl_deletion(table_name); + metrics.record_ttl_deletion(account_id, table_name); if let Some(s) = staleness { // Staleness seconds are small and well within f64's // exact-integer range; used only for telemetry. #[allow(clippy::cast_precision_loss)] - metrics.record_ttl_staleness(table_name, s as f64); + metrics.record_ttl_staleness(account_id, table_name, s as f64); } } } diff --git a/crates/storage/src/lib.rs b/crates/storage/src/lib.rs index 255c2a049..c098868be 100755 --- a/crates/storage/src/lib.rs +++ b/crates/storage/src/lib.rs @@ -19,6 +19,9 @@ pub mod error; pub mod hooks; pub mod management_store; pub mod management_store_factory; +#[doc(hidden)] +#[cfg(feature = "test-util")] +pub mod metrics_conformance; pub mod migrations; pub mod operations; pub mod password; diff --git a/crates/storage/src/management_store/mod.rs b/crates/storage/src/management_store/mod.rs index 06a66fa21..be1efa488 100755 --- a/crates/storage/src/management_store/mod.rs +++ b/crates/storage/src/management_store/mod.rs @@ -25,8 +25,8 @@ mod types; pub use types::{ - AccessKeyCreated, AccountDetail, AdminEntry, GroupDetail, MetricsRow, OpError, OpResult, - RoleDetail, SessionPolicyEntry, UserDetail, + AccessKeyCreated, AccountDetail, AdminEntry, GroupDetail, MetricsFilter, MetricsRow, OpError, + OpResult, RoleDetail, SessionPolicyEntry, UserDetail, }; use futures::future::BoxFuture; @@ -76,12 +76,15 @@ pub trait MetricsStore: Send + Sync { fn insert_metrics(&self, rows: &[MetricsRow]) -> BoxFuture<'_, OpResult<()>>; /// Query metrics rows within a time range, with optional filters. + /// + /// A filter that is `None` matches every row; `filter.account_id` set + /// returns only the rows recorded for that account, never a row of the + /// whole deployment (which has no account). fn query_metrics( &self, start: time::OffsetDateTime, end: time::OffsetDateTime, - table_name: Option<&str>, - metric: Option<&str>, + filter: &MetricsFilter, ) -> BoxFuture<'_, OpResult>>; /// Delete metrics rows older than the retention period. diff --git a/crates/storage/src/management_store/types.rs b/crates/storage/src/management_store/types.rs index 26ba653b7..0b143bc01 100755 --- a/crates/storage/src/management_store/types.rs +++ b/crates/storage/src/management_store/types.rs @@ -122,6 +122,9 @@ pub struct AdminEntry { #[derive(Debug, Clone)] pub struct MetricsRow { pub bucket: time::OffsetDateTime, + /// The account the row was recorded for; `None` for a metric of the + /// whole deployment. Stored as the empty string. + pub account_id: Option, pub metric: String, pub table_name: Option, pub index_name: Option, @@ -131,3 +134,35 @@ pub struct MetricsRow { pub min: f64, pub max: f64, } + +/// The optional filters of [`super::MetricsStore::query_metrics`]. A `None` +/// field matches every row, and an empty string counts as `None`. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct MetricsFilter { + /// Only the rows recorded for this account. + pub account_id: Option, + /// Only the rows of this table name. + pub table_name: Option, + /// Only the rows of this metric. + pub metric: Option, +} + +impl MetricsFilter { + /// The account filter, with an empty string read as no filter. + #[must_use] + pub fn account_id(&self) -> Option<&str> { + self.account_id.as_deref().filter(|s| !s.is_empty()) + } + + /// The table name filter, with an empty string read as no filter. + #[must_use] + pub fn table_name(&self) -> Option<&str> { + self.table_name.as_deref().filter(|s| !s.is_empty()) + } + + /// The metric filter, with an empty string read as no filter. + #[must_use] + pub fn metric(&self) -> Option<&str> { + self.metric.as_deref().filter(|s| !s.is_empty()) + } +} diff --git a/crates/storage/src/metrics_conformance.rs b/crates/storage/src/metrics_conformance.rs new file mode 100644 index 000000000..1bc31bfc3 --- /dev/null +++ b/crates/storage/src/metrics_conformance.rs @@ -0,0 +1,179 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Backend-independent check for [`MetricsStore`], run by each backend's own +//! test suite against a real catalog that has run every migration (#728). +//! +//! The check writes rows for two accounts that each have a table of the same +//! name, under the same metric, operation and minute, and one row of the +//! whole deployment. It then reads them back: two accounts' rows are two +//! rows, each with its own account and values, and the account filter +//! returns only that account's rows, never the deployment's. +//! +//! Every row is in one minute 23 hours back, inside the day the server keeps +//! metrics for, and the accounts are made up for the run, so rows another +//! test writes to the same catalog stay out of the reads. It does not check +//! that two rows of one key add up: the Cassandra store writes a row of the +//! same key over the previous one. + +use futures::future::BoxFuture; + +use crate::management_store::{MetricsFilter, MetricsRow, MetricsStore}; + +/// The values the check reads back for a row: (account, table, metric, sum). +type Seen = (Option, Option, String, f64); + +/// Runs the check against `store`. +/// +/// # Panics +/// +/// When the store fails a call or returns rows other than the ones the +/// check expects. +pub async fn run(store: &dyn MetricsStore) { + let bucket = minute_a_day_back(); + let unique = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |d| d.as_nanos()) + % 1_000_000_000_000; + let first = format!("{unique:012}"); + let second = format!("{:012}", (unique + 1) % 1_000_000_000_000); + let row = |account: Option<&str>, table: Option<&str>, metric: &str, sum: f64| MetricsRow { + bucket, + account_id: account.map(ToOwned::to_owned), + metric: metric.to_owned(), + table_name: table.map(ToOwned::to_owned), + index_name: None, + operation: Some("PutItem".to_owned()), + sum, + count: 1, + min: sum, + max: sum, + }; + store + .insert_metrics(&[ + row( + Some(&first), + Some("Orders"), + "ConsumedWriteCapacityUnits", + 1.0, + ), + row( + Some(&second), + Some("Orders"), + "ConsumedWriteCapacityUnits", + 5.0, + ), + row(Some(&first), None, "UserErrors", 2.0), + row(None, None, "WorkerCycleLatency", 7.0), + ]) + .await + .expect("insert_metrics"); + + let ours = |rows: Vec| -> Vec { + let mut seen: Vec = rows + .into_iter() + .filter(|r| { + r.account_id + .as_deref() + .is_none_or(|a| a == first || a == second) + && r.bucket == bucket + }) + .map(|r| (r.account_id, r.table_name, r.metric, r.sum)) + .collect(); + seen.sort_by(|a, b| format!("{a:?}").cmp(&format!("{b:?}"))); + seen + }; + let read = |filter: MetricsFilter| -> BoxFuture<'_, Vec> { + Box::pin(async move { + store + .query_metrics(bucket, bucket, &filter) + .await + .expect("query_metrics") + }) + }; + let seen = |account: &str, table: Option<&str>, metric: &str, sum: f64| -> Seen { + ( + Some(account.to_owned()), + table.map(ToOwned::to_owned), + metric.to_owned(), + sum, + ) + }; + let deployment: Seen = (None, None, "WorkerCycleLatency".to_owned(), 7.0); + + // No filter: every account's rows and the deployment's, apart. + let all = ours(read(MetricsFilter::default()).await); + let mut want = vec![ + seen(&first, Some("Orders"), "ConsumedWriteCapacityUnits", 1.0), + seen(&second, Some("Orders"), "ConsumedWriteCapacityUnits", 5.0), + seen(&first, None, "UserErrors", 2.0), + ]; + let rows_of_deployment: Vec<&Seen> = all.iter().filter(|s| s.0.is_none()).collect(); + assert!( + rows_of_deployment.contains(&&deployment), + "the row of the deployment is missing: {all:?}" + ); + let accounts: Vec = all.iter().filter(|s| s.0.is_some()).cloned().collect(); + want.sort_by(|a, b| format!("{a:?}").cmp(&format!("{b:?}"))); + assert_eq!(accounts, want, "two accounts' rows of one table name"); + + // The account filter: that account's rows only, no deployment row. + let first_only = ours( + read(MetricsFilter { + account_id: Some(first.clone()), + ..MetricsFilter::default() + }) + .await, + ); + let mut want_first = vec![ + seen(&first, Some("Orders"), "ConsumedWriteCapacityUnits", 1.0), + seen(&first, None, "UserErrors", 2.0), + ]; + want_first.sort_by(|a, b| format!("{a:?}").cmp(&format!("{b:?}"))); + assert_eq!(first_only, want_first, "the first account's rows"); + + // Every filter together. + let narrow = ours( + read(MetricsFilter { + account_id: Some(second.clone()), + table_name: Some("Orders".to_owned()), + metric: Some("ConsumedWriteCapacityUnits".to_owned()), + }) + .await, + ); + assert_eq!( + narrow, + [seen( + &second, + Some("Orders"), + "ConsumedWriteCapacityUnits", + 5.0 + )] + ); + + // An account with no rows reads nothing, and empty filters read as none. + let unknown = read(MetricsFilter { + account_id: Some(format!("{:012}", (unique + 2) % 1_000_000_000_000)), + ..MetricsFilter::default() + }) + .await; + assert!(unknown.is_empty(), "{unknown:?}"); + let empty = ours( + read(MetricsFilter { + account_id: Some(String::new()), + table_name: Some(String::new()), + metric: Some(String::new()), + }) + .await, + ); + assert_eq!(empty, all, "empty filters read as no filter"); +} + +/// The start of the minute 23 hours back, in whole seconds: every store +/// keeps the bucket at second precision. +fn minute_a_day_back() -> time::OffsetDateTime { + let t = time::OffsetDateTime::now_utc() - time::Duration::hours(23); + let secs = t.unix_timestamp(); + time::OffsetDateTime::from_unix_timestamp(secs - secs.rem_euclid(60)) + .expect("a timestamp in range") +} diff --git a/crates/storage/src/migrations/registry.rs b/crates/storage/src/migrations/registry.rs index 95ff9be2e..55f4d17ae 100644 --- a/crates/storage/src/migrations/registry.rs +++ b/crates/storage/src/migrations/registry.rs @@ -93,11 +93,21 @@ migration_ids! { /// a global secondary index is stored. WarmThroughput = 9, "warm_throughput", "add a nullable warm_throughput column to the tables and indexes catalogs"; + /// Replaces the catalog's `metrics` table with `account_metrics`, whose + /// key has the account a row was recorded for, so two accounts' tables + /// of the same name keep their own rows (#728). The rows of `metrics` + /// are dropped, not copied: they carry no account, and the server keeps + /// metrics for a day. A later migration must not create a table named + /// `metrics`: `--reconverge` runs migration 1, which creates it, and then + /// this drop again before the later one. + AccountMetrics = 10, "account_metrics", + "replace the metrics table with account_metrics, keyed by account; drops the rows \ + recorded without one"; } /// Highest id that has shipped in a release. Bodies at or below it are frozen: /// each backend pins their checksums, kinds and convergent flags, and -/// `forget` refuses them. The v1.0 release PR raises it to 9. +/// `forget` refuses them. The v1.0 release PR raises it to 10. pub const RELEASED_THROUGH: u32 = 0; impl MigrationId { diff --git a/docs/design/06-component-server.md b/docs/design/06-component-server.md index 37e2529b6..e0ca19445 100755 --- a/docs/design/06-component-server.md +++ b/docs/design/06-component-server.md @@ -312,6 +312,8 @@ schema that uses DynamoDB CloudWatch-style metric names and dimensions. - `window`: `LastMinute` | `Last5Minutes` | `LastHour` | `LastDay` | `AllTime` - `start`, `end`: ISO 8601 (custom range, alternative to `window`) - `granularity`: `1m` | `5m` | `15m` | `1h` (auto-selected if omitted) +- `account_id`: only the metrics of this account (leaves out the metrics of + the whole deployment, which have no account) - `table_name`: filter by table - `metric`: filter by metric name @@ -320,13 +322,15 @@ schema that uses DynamoDB CloudWatch-style metric names and dimensions. ```json { "metrics": [ { "metric": "SuccessfulRequestLatency", - "dimensions": [ { "TableName": "Users" }, + "dimensions": [ { "AccountId": "123456789012" }, + { "TableName": "Users" }, { "Operation": "GetItem" } ], "window": "Last5Minutes", "sum": 26033.0, "count": 50, "min": 321.0, "max": 714.0 } ], "buckets": [ { "timestamp": "2026-05-29T00:02:00Z", "metric": "SuccessfulRequestLatency", - "dimensions": [ { "TableName": "Users" } ], + "dimensions": [ { "AccountId": "123456789012" }, + { "TableName": "Users" } ], "sum": 1234.0, "count": 5, "min": 200.0, "max": 410.0 } ], "segments": [ { "operation": "GetItem", "count": 50, "avg": { "auth_us": 12.0, "authz_us": 4.0, @@ -363,6 +367,10 @@ ExtendDB-internal: **Dimensions** (from the `Dimension` enum): +- `AccountId(String)`: the account of the request or table the metric + belongs to; absent on a metric of the whole deployment (pool, worker, + storage query and backup job metrics). Each stored row is keyed by it, so + two accounts' tables of the same name have their own rows. - `TableName(String)` - `GlobalSecondaryIndexName(String)` - `Operation(String)` diff --git a/docs/getting-started.md b/docs/getting-started.md index 1978740ba..60910c579 100755 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -269,7 +269,7 @@ Config: extenddb.toml Catalog database: extenddb_catalog --- Checking catalog migrations... - OK: Current: the catalog is at migration 9 + OK: Current: the catalog is at migration 10 --- Checking catalog connection... OK: Connected to catalog. --- Checking data database... @@ -287,7 +287,7 @@ extenddb runs as a daemon (background process) and logs to syslog. On startup it ```bash ./target/release/extenddb serve --config extenddb.toml -# extenddb 0.1.14 (catalog migration 9) starting on 127.0.0.1:18443 +# extenddb 0.1.14 (catalog migration 10) starting on 127.0.0.1:18443 # storage: postgres (postgresql://extenddb:***@localhost:5432/extenddb_catalog) ``` @@ -1386,7 +1386,7 @@ Each runner requires its tools to be installed. The runner checks prerequisites ```bash ./target/release/extenddb version # extenddb 0.1.14 -# catalog migration 9 (postgres) +# catalog migration 10 (postgres) # commit abc1234 # built 2026-04-17T12:00:00Z ``` diff --git a/docs/manuals/04-quickstart-setup-guide.md b/docs/manuals/04-quickstart-setup-guide.md index d3d197414..fd96343d1 100755 --- a/docs/manuals/04-quickstart-setup-guide.md +++ b/docs/manuals/04-quickstart-setup-guide.md @@ -130,7 +130,7 @@ Check the version: ```bash ./target/release/extenddb version # extenddb 0.1.14 -# catalog migration 9 (postgres) +# catalog migration 10 (postgres) # commit abc1234 # built 2026-04-17T12:00:00Z # test-hooks disabled @@ -175,7 +175,7 @@ Config: extenddb.toml Catalog database: extenddb_catalog --- Checking catalog migrations... - OK: Current: the catalog is at migration 9 + OK: Current: the catalog is at migration 10 --- Checking catalog connection... OK: Connected to catalog. --- Checking data database... diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index 9de90ef69..dc21c538a 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -4,10 +4,12 @@ ## Current Status -This release versions the PostgreSQL, SQLite, Cassandra and MongoDB catalogs by a migration ledger and carries catalog migrations 1 to 9, which `extenddb migrate --list` prints. **Every deployment created by a 0.1.x release runs `extenddb migrate --yes` once** before a server of this release starts on it: the server refuses a catalog with pending migrations. Migration 1 brings the catalog of every 0.1.x release to the v1.0 schema, migration 5 drops `continuous_backups`, a table and on MongoDB a collection (see [Catalog 0.0.5](#catalog-005)), and migration 6 adds the backup catalog index and the restore job records ([below](#migration-6-the-backup-catalog-index)). Migration 7 adds the `stream_pending` table to the PostgreSQL data database, where a write stores its stream record until the record gets its sequence number after the write commits; it does not apply to the other backends. Migration 8 adds the `restore_progress` table to the PostgreSQL data database, where a store-backed restore records each data file it applied; it does not apply on the other backends either. +This release versions the PostgreSQL, SQLite, Cassandra and MongoDB catalogs by a migration ledger and carries catalog migrations 1 to 10, which `extenddb migrate --list` prints. **Every deployment created by a 0.1.x release runs `extenddb migrate --yes` once** before a server of this release starts on it: the server refuses a catalog with pending migrations. Migration 1 brings the catalog of every 0.1.x release to the v1.0 schema, migration 5 drops `continuous_backups`, a table and on MongoDB a collection (see [Catalog 0.0.5](#catalog-005)), and migration 6 adds the backup catalog index and the restore job records ([below](#migration-6-the-backup-catalog-index)). Migration 7 adds the `stream_pending` table to the PostgreSQL data database, where a write stores its stream record until the record gets its sequence number after the write commits; it does not apply to the other backends. Migration 8 adds the `restore_progress` table to the PostgreSQL data database, where a store-backed restore records each data file it applied; it does not apply on the other backends either. Migration 9, `warm_throughput`, adds a nullable `warm_throughput` column to the `tables` and `indexes` catalog tables on PostgreSQL, SQLite and Cassandra. It holds the `WarmThroughput` a client sets on a table or a global secondary index, which `DescribeTable` reports; nothing acts on the value (see [Differences from DynamoDB](../differences-from-dynamodb.md)). Tables created before the migration report the defaults for their billing mode. On MongoDB it changes nothing, since the catalog collections have no validator, and the ledger records it as not applicable. Migration 9 is breaking: to go back to a release without it, restore the backup taken before the upgrade (see [Rollback Procedure](#rollback-procedure)). +Migration 10, `account_metrics`, replaces the catalog's `metrics` table (on MongoDB, collection) with `account_metrics`, whose key has the account a row was recorded for, on every backend. Two accounts that each have a table of the same name now keep their own metrics rows, where before they wrote to one (see [Metrics carry their account](#v0115-metrics-carry-their-account-catalog-migration-required)). The rows of `metrics` are dropped, not copied: they carry no account, and the server keeps metrics for a day, so `/metrics` and the console dashboard show no history from before the upgrade. Migration 10 is breaking: a server of an earlier release writes and reads `metrics`, so stop every server of an earlier release before running `extenddb migrate --yes`, and to go back restore the backup taken before the upgrade (see [Rollback Procedure](#rollback-procedure)). + Configure `[backup.store]` before the release that stops creating in-catalog backups. The [Backup and Restore manual](14-backup-and-restore.md) covers choosing and securing a store, the recovery figures, the restore drill, catalog-loss recovery and what to do with in-catalog backups. This section says what the upgrade changes. In this release, CreateBackup and RestoreTableFromBackup work as follows: - On PostgreSQL, SQLite and MongoDB, CreateBackup writes to the configured store. There it is asynchronous, as on the service. The response is `CREATING`, and the backup becomes `AVAILABLE` when the upload ends. So a client that creates a backup and then deletes it has to poll DescribeBackup first. The previous release returned `AVAILABLE`. @@ -66,7 +68,7 @@ Exit status 6 means the migration lock, or a running server, was still held when `verify` also checks the connection to the data database. When that check fails, `verify` exits 1 on a Behind catalog instead of 3, because `migrate --yes` cannot apply the pending migrations while the data database cannot be reached. It still exits 4 or 5 for the statuses that need a person. -A migration is additive when the previous release serves the catalog correctly with it applied, and breaking otherwise. Migrations 1 to 7 and 9 are breaking. Migration 8 creates a table in the PostgreSQL data database that the previous release never reads; on PostgreSQL it is declared breaking only because the job that proves an additive declaration against the old binary does not yet exist (CONTRIBUTING, step 4 of adding a migration), and on SQLite, MongoDB and Cassandra it does not apply (`migrate --list` prints it as not applicable, and `migrate` counts it as additive). Do not run a 0.1.x server against a migrated catalog. A 0.1.x binary that writes to a migrated catalog (its `migrate --yes`, or a SQLite dev server) leaves it WrittenByOlderRelease. `migrate --yes --reconverge` repairs it by running the convergent migrations again. Migration 2, which rewrites stored policies, is not convergent: the repair skips it and prints a line that says so (see the v0.1.14 notes below). +A migration is additive when the previous release serves the catalog correctly with it applied, and breaking otherwise. Migrations 1 to 7, 9 and 10 are breaking. Migration 8 creates a table in the PostgreSQL data database that the previous release never reads; on PostgreSQL it is declared breaking only because the job that proves an additive declaration against the old binary does not yet exist (CONTRIBUTING, step 4 of adding a migration), and on SQLite, MongoDB and Cassandra it does not apply (`migrate --list` prints it as not applicable, and `migrate` counts it as additive). Do not run a 0.1.x server against a migrated catalog. A 0.1.x binary that writes to a migrated catalog (its `migrate --yes`, or a SQLite dev server) leaves it WrittenByOlderRelease. `migrate --yes --reconverge` repairs it by running the convergent migrations again. Migration 2, which rewrites stored policies, is not convergent: the repair skips it and prints a line that says so (see the v0.1.14 notes below). ### Migration 6: the backup catalog index @@ -362,6 +364,16 @@ Catalog upgrades change the schema; behavior changes alter how the running serve interprets existing data or configuration. Review these before upgrading a live deployment, even when no catalog migration is required. +### v0.1.15: metrics carry their account (catalog migration required) + +**What changed.** Every metric of a request, and the TTL metrics of a table, now carry the account they were recorded for. The catalog stores the account in the key of each metrics row (migration 10, `account_metrics`), so two accounts that each have a table of the same name keep their own rows: before this release they wrote to one row, and the counts and latencies `/metrics` reported for that name were the two tables' added together. `/metrics` and the console's `/console/metrics-data` report the account as an `AccountId` dimension, beside `TableName`, `GlobalSecondaryIndexName` and `Operation`, and `/metrics` takes an `account_id` query parameter that returns one account's metrics only. Metrics of the whole deployment, such as the connection pool, worker and backup job metrics, have no `AccountId` dimension, and the `account_id` filter leaves them out. The console dashboard still serves admin sessions only and shows every account's metrics; its charts add the accounts together. + +**Who is affected.** A reader of `/metrics` that groups rows by their dimensions now gets one row per account where it got one row per table name, and a deployment with one account gets an `AccountId` dimension on every request metric. A reader that sums the rows of a table name across accounts gets the same totals as before for tables whose names no other account uses. + +**Migration.** Run `extenddb migrate --yes`. Migration 10 drops the `metrics` rows, which carry no account; metrics recorded before the upgrade are not shown afterwards. Stop every server of an earlier release first: such a server writes its metrics to `metrics`, which no longer exists, so they are lost (on MongoDB its first write creates the collection again, and `account_metrics` never sees those rows). + +--- + ### Streams: disabling closes the shards, enabling creates a new stream **What changed.** `UpdateTable` with `StreamEnabled: false` now closes the diff --git a/docs/manuals/08-install-linux.md b/docs/manuals/08-install-linux.md index cf6a977c8..a4980368f 100755 --- a/docs/manuals/08-install-linux.md +++ b/docs/manuals/08-install-linux.md @@ -118,7 +118,7 @@ Expected: ``` === extenddb verify === ... - OK: Current: the catalog is at migration 9 + OK: Current: the catalog is at migration 10 ... === HEALTHY: All checks passed === ``` diff --git a/docs/manuals/09-install-macos.md b/docs/manuals/09-install-macos.md index a87f6c685..8421d506c 100755 --- a/docs/manuals/09-install-macos.md +++ b/docs/manuals/09-install-macos.md @@ -96,7 +96,7 @@ Expected: ``` === extenddb verify === ... - OK: Current: the catalog is at migration 9 + OK: Current: the catalog is at migration 10 ... === HEALTHY: All checks passed === ``` diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index a6ded4cd4..a722a7d44 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -236,7 +236,7 @@ The management web console (`/console/*`) implements: - **Security headers**: X-Content-Type-Options (nosniff), X-Frame-Options (DENY), Referrer-Policy (strict-origin-when-cross-origin) - **HSTS**: Sent automatically (TLS is always enabled) - **Login rate limiting**: Failed login attempts are counted per user and source address, and per source address, as described under [Login Rate Limiting](#login-rate-limiting). -- **Admin-only metrics**: The metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) serve admin sessions only. The metrics table has no account column, so the feed carries the table names, index names and traffic of every account. An IAM-user session is redirected from the page and gets HTTP 403 with no metric data from the feed. +- **Admin-only metrics**: The metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) serve admin sessions only. The feed carries the table names, index names and traffic of every account, each row under the account it was recorded for. An IAM-user session is redirected from the page and gets HTTP 403 with no metric data from the feed. ## Login Rate Limiting diff --git a/docs/manuals/12-extending-extenddb-storage.md b/docs/manuals/12-extending-extenddb-storage.md index 7905409fc..214de4894 100755 --- a/docs/manuals/12-extending-extenddb-storage.md +++ b/docs/manuals/12-extending-extenddb-storage.md @@ -209,8 +209,11 @@ Defined in `crates/storage/src/management_store/mod.rs`. Historical metrics pers | Method | Purpose | |--------|---------| -| `record_metrics` | Insert a metrics snapshot | -| `query_metrics` | Query metrics by time range, operation, and table filters | +| `insert_metrics` | Insert a batch of one-minute rows, each with the account it was recorded for (none for a metric of the whole deployment) | +| `query_metrics` | Query rows by time range, filtered by `MetricsFilter`: account, table name and metric | +| `prune_metrics` | Delete rows older than the retention period | + +Two accounts' rows for a table of the same name must stay two rows: the account is part of each row's key. `extenddb_storage::metrics_conformance::run` (feature `test-util`) checks a store against a migrated catalog. ### RateLimitStore diff --git a/docs/schema/cassandra.txt b/docs/schema/cassandra.txt index 48fabb934..b45a000b1 100644 --- a/docs/schema/cassandra.txt +++ b/docs/schema/cassandra.txt @@ -1,5 +1,6 @@ # catalog keyspace table access_keys +table account_metrics table accounts table admin_users table backup_index_by_status @@ -21,7 +22,6 @@ table iam_users table idempotency_tokens_by_account table indexes table login_attempts -table metrics table schema_history table settings table tables @@ -32,6 +32,16 @@ column access_keys.created_at: timestamp regular column access_keys.is_active: boolean regular column access_keys.secret_key_encrypted: blob regular column access_keys.user_name: text regular +column account_metrics.bucket: timestamp partition_key 0 +column account_metrics.account_id: text partition_key 1 +column account_metrics.metric: text partition_key 2 +column account_metrics.table_name: text partition_key 3 +column account_metrics.index_name: text partition_key 4 +column account_metrics.operation: text partition_key 5 +column account_metrics.count: bigint regular +column account_metrics.max: double regular +column account_metrics.min: double regular +column account_metrics.sum: double regular column accounts.account_id: text partition_key 0 column accounts.account_name: text regular column accounts.created_at: timestamp regular @@ -171,15 +181,6 @@ column login_attempts.principal: text partition_key 0 column login_attempts.attempted_at: timestamp clustering 0 desc column login_attempts.source_ip: text regular column login_attempts.success: boolean regular -column metrics.bucket: timestamp partition_key 0 -column metrics.metric: text partition_key 1 -column metrics.table_name: text partition_key 2 -column metrics.index_name: text partition_key 3 -column metrics.operation: text partition_key 4 -column metrics.count: bigint regular -column metrics.max: double regular -column metrics.min: double regular -column metrics.sum: double regular column schema_history.version: int partition_key 0 column schema_history.applied_at: timestamp regular column schema_history.description: text regular diff --git a/docs/schema/mongodb.txt b/docs/schema/mongodb.txt index 518bce418..bc7a2676b 100644 --- a/docs/schema/mongodb.txt +++ b/docs/schema/mongodb.txt @@ -1,6 +1,9 @@ collection extenddb_catalog.access_keys type collection options {} index extenddb_catalog.access_keys _id_ key {"_id":1} options {} index extenddb_catalog.access_keys account_id_1_user_name_1 key {"account_id":1,"user_name":1} options {} +collection extenddb_catalog.account_metrics type collection options {} +index extenddb_catalog.account_metrics _id_ key {"_id":1} options {} +index extenddb_catalog.account_metrics bucket_1 key {"bucket":1} options {} collection extenddb_catalog.accounts type collection options {} index extenddb_catalog.accounts _id_ key {"_id":1} options {} index extenddb_catalog.accounts account_name_1 key {"account_name":1} options {"unique":true} @@ -44,9 +47,6 @@ index extenddb_catalog.indexes _id_ key {"_id":1} options {} collection extenddb_catalog.login_attempts type collection options {} index extenddb_catalog.login_attempts _id_ key {"_id":1} options {} index extenddb_catalog.login_attempts principal_1_attempted_at_1 key {"principal":1,"attempted_at":1} options {} -collection extenddb_catalog.metrics type collection options {} -index extenddb_catalog.metrics _id.bucket_1 key {"_id.bucket":1} options {} -index extenddb_catalog.metrics _id_ key {"_id":1} options {} collection extenddb_catalog.schema_history type collection options {} index extenddb_catalog.schema_history _id_ key {"_id":1} options {} collection extenddb_catalog.settings type collection options {} diff --git a/docs/schema/postgres.txt b/docs/schema/postgres.txt index 0054b32ed..19a949bf2 100644 --- a/docs/schema/postgres.txt +++ b/docs/schema/postgres.txt @@ -1,5 +1,6 @@ # catalog database table access_keys +table account_metrics table accounts table admin_users table backup_definitions @@ -20,7 +21,6 @@ table iam_users table idempotency_tokens table indexes table login_attempts -table metrics table schema_history table settings table stream_records @@ -35,6 +35,16 @@ column access_keys.account_id: text NOT NULL column access_keys.user_name: text NOT NULL column access_keys.is_active: boolean NOT NULL DEFAULT true column access_keys.created_at: timestamp with time zone NOT NULL DEFAULT now() +column account_metrics.bucket: timestamp with time zone NOT NULL +column account_metrics.account_id: text NOT NULL DEFAULT ''::text +column account_metrics.metric: text NOT NULL +column account_metrics.table_name: text NOT NULL DEFAULT ''::text +column account_metrics.index_name: text NOT NULL DEFAULT ''::text +column account_metrics.operation: text NOT NULL DEFAULT ''::text +column account_metrics.sum: double precision NOT NULL DEFAULT 0 +column account_metrics.count: bigint NOT NULL DEFAULT 0 +column account_metrics.min: double precision NOT NULL DEFAULT 'Infinity'::double precision +column account_metrics.max: double precision NOT NULL DEFAULT '-Infinity'::double precision column accounts.account_id: text NOT NULL column accounts.account_name: text NOT NULL column accounts.created_at: timestamp with time zone NOT NULL DEFAULT now() @@ -156,15 +166,6 @@ column login_attempts.principal: text NOT NULL column login_attempts.attempted_at: timestamp with time zone NOT NULL DEFAULT now() column login_attempts.success: boolean NOT NULL column login_attempts.source_ip: text -column metrics.bucket: timestamp with time zone NOT NULL -column metrics.metric: text NOT NULL -column metrics.table_name: text NOT NULL DEFAULT ''::text -column metrics.index_name: text NOT NULL DEFAULT ''::text -column metrics.operation: text NOT NULL DEFAULT ''::text -column metrics.sum: double precision NOT NULL DEFAULT 0 -column metrics.count: bigint NOT NULL DEFAULT 0 -column metrics.min: double precision NOT NULL DEFAULT 'Infinity'::double precision -column metrics.max: double precision NOT NULL DEFAULT '-Infinity'::double precision column schema_history.filename: text NOT NULL column schema_history.applied_at: timestamp with time zone NOT NULL DEFAULT now() column settings.key: text NOT NULL @@ -236,6 +237,7 @@ column vector_indexes.skipped_item_count: bigint column vector_indexes.build_owner: text column vector_indexes.build_heartbeat_at: timestamp with time zone index access_keys_pkey: CREATE UNIQUE INDEX access_keys_pkey ON public.access_keys USING btree (access_key_id) +index account_metrics_pkey: CREATE UNIQUE INDEX account_metrics_pkey ON public.account_metrics USING btree (bucket, account_id, metric, table_name, index_name, operation) index accounts_account_name_key: CREATE UNIQUE INDEX accounts_account_name_key ON public.accounts USING btree (account_name) index accounts_pkey: CREATE UNIQUE INDEX accounts_pkey ON public.accounts USING btree (account_id) index admin_users_pkey: CREATE UNIQUE INDEX admin_users_pkey ON public.admin_users USING btree (admin_name) @@ -258,6 +260,7 @@ index iam_user_tags_pkey: CREATE UNIQUE INDEX iam_user_tags_pkey ON public.iam_u index iam_users_pkey: CREATE UNIQUE INDEX iam_users_pkey ON public.iam_users USING btree (account_id, user_name) index iam_users_user_arn_key: CREATE UNIQUE INDEX iam_users_user_arn_key ON public.iam_users USING btree (user_arn) index idempotency_tokens_pkey: CREATE UNIQUE INDEX idempotency_tokens_pkey ON public.idempotency_tokens USING btree (token) +index idx_account_metrics_bucket: CREATE INDEX idx_account_metrics_bucket ON public.account_metrics USING btree (bucket) index idx_backup_index_account_table: CREATE INDEX idx_backup_index_account_table ON public.backup_index USING btree (account_id, table_name) index idx_backup_index_backup_id: CREATE UNIQUE INDEX idx_backup_index_backup_id ON public.backup_index USING btree (backup_id) index idx_backup_index_store_status: CREATE INDEX idx_backup_index_store_status ON public.backup_index USING btree (store_kind, store_root, store_id, status) @@ -266,7 +269,6 @@ index idx_backups_table: CREATE INDEX idx_backups_table ON public.backups USING index idx_idempotency_tokens_created: CREATE INDEX idx_idempotency_tokens_created ON public.idempotency_tokens USING btree (created_at) index idx_login_attempts_principal_time: CREATE INDEX idx_login_attempts_principal_time ON public.login_attempts USING btree (principal, attempted_at DESC) index idx_login_attempts_source_ip_time: CREATE INDEX idx_login_attempts_source_ip_time ON public.login_attempts USING btree (source_ip, attempted_at DESC) WHERE (source_ip IS NOT NULL) -index idx_metrics_bucket: CREATE INDEX idx_metrics_bucket ON public.metrics USING btree (bucket) index idx_stream_records_created: CREATE INDEX idx_stream_records_created ON public.stream_records USING btree (created_at) index idx_stream_shards_table: CREATE INDEX idx_stream_shards_table ON public.stream_shards USING btree (table_id) index idx_table_restores_backup: CREATE INDEX idx_table_restores_backup ON public.table_restores USING btree (source_backup_arn) @@ -274,7 +276,6 @@ index idx_table_restores_status: CREATE INDEX idx_table_restores_status ON publi index idx_tables_pending_transition: CREATE INDEX idx_tables_pending_transition ON public.tables USING btree (status_transition_at) WHERE (status_transition_at IS NOT NULL) index idx_vector_indexes_index_id: CREATE UNIQUE INDEX idx_vector_indexes_index_id ON public.vector_indexes USING btree (index_id) index indexes_pkey: CREATE UNIQUE INDEX indexes_pkey ON public.indexes USING btree (table_id, index_name) -index metrics_pkey: CREATE UNIQUE INDEX metrics_pkey ON public.metrics USING btree (bucket, metric, table_name, index_name, operation) index schema_history_pkey: CREATE UNIQUE INDEX schema_history_pkey ON public.schema_history USING btree (filename) index settings_pkey: CREATE UNIQUE INDEX settings_pkey ON public.settings USING btree (key) index stream_records_pkey: CREATE UNIQUE INDEX stream_records_pkey ON public.stream_records USING btree (shard_id, sequence_number) @@ -286,6 +287,7 @@ index tags_pkey: CREATE UNIQUE INDEX tags_pkey ON public.tags USING btree (resou index vector_indexes_pkey: CREATE UNIQUE INDEX vector_indexes_pkey ON public.vector_indexes USING btree (table_id, index_name) constraint access_keys.access_keys_account_id_user_name_fkey: FOREIGN KEY (account_id, user_name) REFERENCES iam_users(account_id, user_name) ON DELETE CASCADE constraint access_keys.access_keys_pkey: PRIMARY KEY (access_key_id) +constraint account_metrics.account_metrics_pkey: PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) constraint accounts.accounts_account_name_key: UNIQUE (account_name) constraint accounts.accounts_pkey: PRIMARY KEY (account_id) constraint admin_users.admin_users_pkey: PRIMARY KEY (admin_name) @@ -326,7 +328,6 @@ constraint idempotency_tokens.idempotency_tokens_pkey: PRIMARY KEY (token) constraint indexes.chk_propagation_delay_ms_non_negative: CHECK (((propagation_delay_ms IS NULL) OR (propagation_delay_ms >= 0))) constraint indexes.indexes_pkey: PRIMARY KEY (table_id, index_name) constraint indexes.indexes_table_id_fkey: FOREIGN KEY (table_id) REFERENCES tables(table_id) ON DELETE CASCADE -constraint metrics.metrics_pkey: PRIMARY KEY (bucket, metric, table_name, index_name, operation) constraint schema_history.schema_history_pkey: PRIMARY KEY (filename) constraint settings.settings_pkey: PRIMARY KEY (key) constraint stream_records.stream_records_pkey: PRIMARY KEY (shard_id, sequence_number) diff --git a/docs/schema/sqlite.txt b/docs/schema/sqlite.txt index 33a97d696..c32620e06 100644 --- a/docs/schema/sqlite.txt +++ b/docs/schema/sqlite.txt @@ -1,3 +1,4 @@ +index idx_account_metrics_bucket on account_metrics: CREATE INDEX idx_account_metrics_bucket ON account_metrics (bucket) index idx_backup_index_account_table on backup_index: CREATE INDEX idx_backup_index_account_table ON backup_index (account_id, table_name) index idx_backup_index_backup_id on backup_index: CREATE UNIQUE INDEX idx_backup_index_backup_id ON backup_index (backup_id) index idx_backup_index_store_status on backup_index: CREATE INDEX idx_backup_index_store_status ON backup_index (store_kind, store_root, store_id, status) @@ -7,7 +8,6 @@ index idx_gsi_pending_claim on gsi_pending: CREATE INDEX idx_gsi_pending_claim O index idx_idempotency_tokens_created on idempotency_tokens: CREATE INDEX idx_idempotency_tokens_created ON idempotency_tokens (created_at) index idx_login_attempts_principal_time on login_attempts: CREATE INDEX idx_login_attempts_principal_time ON login_attempts (principal, attempted_at DESC) index idx_login_attempts_source_ip_time on login_attempts: CREATE INDEX idx_login_attempts_source_ip_time ON login_attempts (source_ip, attempted_at DESC) WHERE source_ip IS NOT NULL -index idx_metrics_bucket on metrics: CREATE INDEX idx_metrics_bucket ON metrics (bucket) index idx_stream_records_created on stream_records: CREATE INDEX idx_stream_records_created ON stream_records (created_at) index idx_stream_shards_table on stream_shards: CREATE INDEX idx_stream_shards_table ON stream_shards (table_id) index idx_table_restores_backup on table_restores: CREATE INDEX idx_table_restores_backup ON table_restores (source_backup_arn) @@ -15,6 +15,7 @@ index idx_table_restores_status on table_restores: CREATE INDEX idx_table_restor index idx_tables_pending_transition on tables: CREATE INDEX idx_tables_pending_transition ON tables (status_transition_at) WHERE status_transition_at IS NOT NULL index idx_vector_indexes_index_id on vector_indexes: CREATE UNIQUE INDEX idx_vector_indexes_index_id ON vector_indexes (index_id) table access_keys on access_keys: CREATE TABLE access_keys ( access_key_id TEXT PRIMARY KEY, secret_key_encrypted BLOB NOT NULL, account_id TEXT NOT NULL, user_name TEXT NOT NULL, is_active INTEGER NOT NULL DEFAULT 1, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), FOREIGN KEY (account_id, user_name) REFERENCES iam_users(account_id, user_name) ON DELETE CASCADE ) +table account_metrics on account_metrics: CREATE TABLE account_metrics ( bucket TEXT NOT NULL, account_id TEXT NOT NULL DEFAULT '', metric TEXT NOT NULL, table_name TEXT NOT NULL DEFAULT '', index_name TEXT NOT NULL DEFAULT '', operation TEXT NOT NULL DEFAULT '', sum REAL NOT NULL DEFAULT 0, count INTEGER NOT NULL DEFAULT 0, min REAL NOT NULL DEFAULT 9e999, max REAL NOT NULL DEFAULT -9e999, PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) ) table accounts on accounts: CREATE TABLE accounts ( account_id TEXT PRIMARY KEY, account_name TEXT NOT NULL UNIQUE, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')) ) table admin_users on admin_users: CREATE TABLE admin_users ( admin_name TEXT PRIMARY KEY, password_hash TEXT NOT NULL, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')) ) table backup_definitions on backup_definitions: CREATE TABLE backup_definitions ( backup_arn TEXT PRIMARY KEY REFERENCES backups(backup_arn) ON DELETE CASCADE, definition TEXT NOT NULL ) @@ -36,7 +37,6 @@ table iam_users on iam_users: CREATE TABLE iam_users ( account_id TEXT NOT NULL table idempotency_tokens on idempotency_tokens: CREATE TABLE idempotency_tokens ( account_id TEXT NOT NULL, token TEXT NOT NULL, fingerprint TEXT NOT NULL, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), PRIMARY KEY (account_id, token) ) table indexes on indexes: CREATE TABLE indexes ( table_id TEXT NOT NULL, index_id TEXT NOT NULL, index_name TEXT NOT NULL, index_type TEXT NOT NULL, key_schema TEXT NOT NULL, projection TEXT NOT NULL, index_status TEXT NOT NULL DEFAULT 'ACTIVE', provisioned_throughput TEXT, propagation_delay_ms INTEGER, warm_throughput TEXT, PRIMARY KEY (table_id, index_name), CONSTRAINT indexes_table_id_fkey FOREIGN KEY (table_id) REFERENCES tables(table_id) ON DELETE CASCADE, CONSTRAINT chk_propagation_delay_ms_non_negative CHECK (propagation_delay_ms IS NULL OR propagation_delay_ms >= 0) ) table login_attempts on login_attempts: CREATE TABLE login_attempts ( principal TEXT NOT NULL, attempted_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), success INTEGER NOT NULL, source_ip TEXT ) -table metrics on metrics: CREATE TABLE metrics ( bucket TEXT NOT NULL, metric TEXT NOT NULL, table_name TEXT NOT NULL DEFAULT '', index_name TEXT NOT NULL DEFAULT '', operation TEXT NOT NULL DEFAULT '', sum REAL NOT NULL DEFAULT 0, count INTEGER NOT NULL DEFAULT 0, min REAL NOT NULL DEFAULT 9e999, max REAL NOT NULL DEFAULT -9e999, PRIMARY KEY (bucket, metric, table_name, index_name, operation) ) table schema_history on schema_history: CREATE TABLE schema_history ( filename TEXT PRIMARY KEY, applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')) ) table seq_counters on seq_counters: CREATE TABLE seq_counters ( name TEXT PRIMARY KEY, value INTEGER NOT NULL DEFAULT 0 ) table settings on settings: CREATE TABLE settings ( key TEXT PRIMARY KEY, value TEXT NOT NULL ) diff --git a/tests/test_console_metrics_admin_only.py b/tests/test_console_metrics_admin_only.py index 2538cc21b..905d1c10c 100644 --- a/tests/test_console_metrics_admin_only.py +++ b/tests/test_console_metrics_admin_only.py @@ -3,10 +3,9 @@ """The console metrics dashboard and its data feed serve admins only. -The metrics table behind the dashboard has no account column, so its feed -carries the table names and traffic of every account. An IAM user who logs in -to the console gets neither the page nor the feed. An admin gets both, with -every account's tables. +The feed behind the dashboard carries the table names and traffic of every +account. An IAM user who logs in to the console gets neither the page nor the +feed. An admin gets both, with every account's tables. The Settings and Cache pages are admin only too. Each of the three pages redirects an IAM-user session to the console dashboard, and the navigation bar diff --git a/tests/test_metrics_accounts.py b/tests/test_metrics_accounts.py new file mode 100644 index 000000000..21db26e75 --- /dev/null +++ b/tests/test_metrics_accounts.py @@ -0,0 +1,221 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Metrics are kept per account (issue #728). + +The metrics used to have no account dimension, so two accounts that each +had a table of the same name wrote to one row, and the counts an admin read +for that name were the two tables' added together. Each metric of a request +now carries the account the request was made for, as an ``AccountId`` +dimension, and ``/metrics`` takes an ``account_id`` filter. + +The test creates two accounts, each with a user that may call every +DynamoDB action, and a table of the same name in each. It writes one item +to the first account's table and three to the second's, waits until the +flush has persisted both accounts' ``ConsumedWriteCapacityUnits`` rows, then +reads until two reads in a row are the same (see ``flushed``): the two +accounts' rows are two rows with their own sums, and the account filter +returns one account's rows only. + +Prerequisites: + - extenddb running on EXTENDDB_TEST_ENDPOINT + - Admin credentials in EXTENDDB_ADMIN_USER / EXTENDDB_ADMIN_PASSWORD +""" + +from __future__ import annotations + +import json +import os +import time +import uuid +from typing import Any + +import boto3 +import pytest +import requests +from botocore.config import Config as BotoConfig + +from conftest import wait_for_active, wait_for_deleted +from management_helpers import ManagementClient + +# The flush worker persists a data point once it is older than the worker's +# 60-second interval, at the next tick, so a point can take up to two +# intervals to reach the metrics table. +FLUSH_DEADLINE_SECONDS = 180 +POLL_SECONDS = 5 +# After the awaited rows, the fixture reads the metrics every SETTLE_SECONDS +# until two reads in a row are the same: one flush writes its rows one by +# one, and a read can land partway through it. +SETTLE_SECONDS = 5 + +# Items each account writes to its table; a PutItem of a small item on an +# on-demand table consumes one write capacity unit. +WRITES = (1, 3) + +pytestmark = pytest.mark.slow + +KEY = { + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "BillingMode": "PAY_PER_REQUEST", +} + + +def _require_env() -> tuple[str, str, str]: + endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() + admin_user = os.environ.get("EXTENDDB_ADMIN_USER", "").strip() + admin_pass = os.environ.get("EXTENDDB_ADMIN_PASSWORD", "").strip() + if not endpoint or not admin_user or not admin_pass: + pytest.fail( + "MISCONFIGURED: the per-account metrics test needs " + "EXTENDDB_TEST_ENDPOINT, EXTENDDB_ADMIN_USER and " + "EXTENDDB_ADMIN_PASSWORD, which devtools/run-tests sets." + ) + return endpoint, admin_user, admin_pass + + +def _admin_metrics(env: tuple[str, str, str], **params: str) -> dict[str, Any]: + endpoint, admin_user, admin_pass = env + resp = requests.get( + f"{endpoint}/metrics", + params=params, + auth=(admin_user, admin_pass), + timeout=10, + verify=False, + ) + assert resp.status_code == 200, f"/metrics: {resp.status_code} {resp.text[:200]}" + return resp.json() + + +def _dimension(metric: dict[str, Any], key: str) -> str | None: + for d in metric["dimensions"]: + if isinstance(d, dict) and key in d: + return d[key] + return None + + +def _write_capacity(data: dict[str, Any], table: str) -> dict[str | None, float]: + """The PutItem write capacity under ``table``, summed per AccountId.""" + sums: dict[str | None, float] = {} + for m in data["metrics"]: + if ( + m["metric"] == "ConsumedWriteCapacityUnits" + and _dimension(m, "Operation") == "PutItem" + and _dimension(m, "TableName") == table + ): + account = _dimension(m, "AccountId") + sums[account] = sums.get(account, 0.0) + m["sum"] + return sums + + +def _canonical(data: dict[str, Any]) -> list[str]: + return sorted(json.dumps(m, sort_keys=True) for m in data["metrics"]) + + +def _account(mgmt: ManagementClient, endpoint: str, label: str): + """A new account with a user that may call every DynamoDB action. + + Returns the account id, the user's DynamoDB client and the user's name. + """ + account_id = f"{uuid.uuid4().int % 10**12:012d}" + resp = mgmt.create_account(account_id, f"metrics-{label}-{account_id}") + assert resp.status_code == 201, resp.text + user = f"metrics-{label}" + resp = mgmt.create_user(account_id, user) + assert resp.status_code == 201, resp.text + resp = mgmt.create_access_key(account_id, user) + assert resp.status_code == 201, resp.text + creds = resp.json() + policy = { + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + } + resp = mgmt.put_user_policy(account_id, user, "full", policy) + assert resp.status_code == 204, resp.text + kwargs: dict[str, Any] = dict( + service_name="dynamodb", + endpoint_url=endpoint, + aws_access_key_id=creds["access_key_id"], + aws_secret_access_key=creds["secret_access_key"], + region_name=os.environ.get("AWS_DEFAULT_REGION", "us-east-1"), + config=BotoConfig(retries={"max_attempts": 0}), + ) + if endpoint.startswith("https://"): + kwargs["verify"] = False + return account_id, boto3.client(**kwargs), user + + +@pytest.fixture(scope="module") +def flushed(): + """Both accounts' writes, persisted by the flush. + + Yields the admin environment, the table name, the two account ids in + the order of WRITES, and the metrics an admin reads once both accounts' + rows are in them. + """ + env = _require_env() + mgmt = ManagementClient(*env) + table = f"metrics-accounts-{uuid.uuid4().hex[:8]}" + accounts = [] + try: + for label in ("first", "second"): + accounts.append(_account(mgmt, env[0], label)) + for _, client, _ in accounts: + client.create_table(TableName=table, **KEY) + for (_, client, _), writes in zip(accounts, WRITES): + wait_for_active(client, table) + for i in range(writes): + client.put_item(TableName=table, Item={"pk": {"S": str(i)}}) + + ids = [account_id for account_id, _, _ in accounts] + + def both_flushed(data: dict[str, Any]) -> bool: + return set(ids) <= set(_write_capacity(data, table)) + + deadline = time.monotonic() + FLUSH_DEADLINE_SECONDS + while True: + data = _admin_metrics(env, window="LastHour") + if both_flushed(data) or time.monotonic() > deadline: + break + time.sleep(POLL_SECONDS) + assert both_flushed(data), ( + f"within {FLUSH_DEADLINE_SECONDS}s the metrics lacked a PutItem " + f"write capacity row under {table} for each account: " + f"{_write_capacity(data, table)}" + ) + # The server lists the rows in no fixed order, so compare them sorted. + while True: + time.sleep(SETTLE_SECONDS) + again = _admin_metrics(env, window="LastHour") + if _canonical(again) == _canonical(data): + break + assert time.monotonic() < deadline, ( + f"the metrics were still changing {FLUSH_DEADLINE_SECONDS}s " + f"after the test's requests" + ) + data = again + yield env, table, ids, data + finally: + for account_id, client, user in accounts: + try: + client.delete_table(TableName=table) + wait_for_deleted(client, table) + except client.exceptions.ResourceNotFoundException: + pass + finally: + mgmt.delete_user(account_id, user) + mgmt.delete_account(account_id) + + +def test_two_accounts_with_the_same_table_name_get_separate_rows(flushed): + _, table, ids, data = flushed + sums = _write_capacity(data, table) + assert sums == {ids[0]: float(WRITES[0]), ids[1]: float(WRITES[1])}, sums + + +def test_the_account_filter_returns_that_accounts_rows_only(flushed): + env, table, ids, _ = flushed + data = _admin_metrics(env, window="LastHour", account_id=ids[1]) + accounts = {_dimension(m, "AccountId") for m in data["metrics"]} + assert accounts == {ids[1]}, accounts + assert _write_capacity(data, table) == {ids[1]: float(WRITES[1])} diff --git a/tests/test_resource_tag_migration.py b/tests/test_resource_tag_migration.py index 6ae02872b..0fc6a546f 100644 --- a/tests/test_resource_tag_migration.py +++ b/tests/test_resource_tag_migration.py @@ -309,6 +309,8 @@ def _as_released_catalog(sqlite_path: str) -> None: (warm_throughput) add are removed, since no release before 1.0 had them and the check after migration 1 refuses a `table_restores` carrying its twelve job columns and a `tables` or `indexes` carrying warm_throughput. + The `account_metrics` table migration 10 creates is removed too; migration + 1 creates the `metrics` table it replaced again. The runner then applies every migration, the condition-key rewrite included, as it does on a released catalog.""" conn = sqlite3.connect(sqlite_path) @@ -322,6 +324,8 @@ def _as_released_catalog(sqlite_path: str) -> None: # Migration 9 adds warm_throughput to tables and indexes. for table in ("tables", "indexes"): conn.execute(f"ALTER TABLE {table} DROP COLUMN warm_throughput") + # Migration 10 replaces metrics with account_metrics. + conn.execute("DROP TABLE IF EXISTS account_metrics") conn.execute("DELETE FROM extenddb_migrations") conn.execute( "UPDATE settings SET value = '0.0.3' WHERE key = 'catalog_version'" From 3ec94e1c552557886bbed8fe50a8d14d5d3b6f99 Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Tue, 10 Nov 2026 22:58:07 -0800 Subject: [PATCH 2/4] fix(metrics): Filter segments by account, name the account Review of #955: - The latency segments of /metrics and /console/metrics-data take the account_id filter: query_segments keeps the requests of that account only. Before, a response filtered by account still held every account's per-operation counts and latencies. - The console's raw metrics table has an Account column. - An empty account_id is no filter in the in-memory collector, as in the stores. - The named record_* methods move to collector_record.rs, keeping collector.rs under the 500-line limit. - Tests: every one of the 14 per-account recorders keeps its account; segments are filtered by account; SystemErrors, failed conditions and transaction conflicts carry the account. The store check gives its deployment row a key of its own run, and checks that two inserts of one key add up and that the prune deletes an old row (not on Cassandra, which overwrites and expires by TTL). The wire test sends a GetItem, a Query, a Scan, a failed condition and a request on a missing table as each account, puts an expired item for the TTL sweep, and waits for the PutItem sums to reach their final values; it checks the account on each metric, the TTL deletions and the filtered segments. The MongoDB metrics store test runs with EXTENDDB_TEST_MONGODB_REQUIRED in CI. - Docs: the account is part of the row key on SQLite, PostgreSQL and Cassandra; on MongoDB each document records it. AccountId is ExtendDB's own dimension. The storage query metrics carry no account. Dropping metrics on Cassandra keeps a snapshot when auto_snapshot is on. Stale text about the metrics table having no account is updated. Part of #728. --- crates/core/src/metrics/collector.rs | 433 +---------------- crates/core/src/metrics/collector_query.rs | 21 +- crates/core/src/metrics/collector_record.rs | 444 ++++++++++++++++++ crates/core/src/metrics/collector_tests.rs | 151 +++++- crates/core/src/metrics/mod.rs | 1 + .../src/console/pages/metrics_content.rs | 4 +- .../server/src/console/pages/metrics_pages.rs | 10 +- .../src/console/pages/metrics_pages_tests.rs | 5 +- crates/server/src/metrics_endpoint.rs | 25 +- crates/server/src/response.rs | 13 +- .../tests/direct/metrics_store.rs | 11 +- crates/storage-mongodb/tests/metrics_store.rs | 9 +- .../storage-postgres/tests/metrics_store.rs | 9 +- .../src/migrations/released_tests.rs | 28 +- crates/storage/src/metrics_conformance.rs | 104 +++- devtools/run-mongodb-tests | 11 +- docs/design/06-component-server.md | 15 +- docs/manuals/07-upgrade-manual.md | 6 +- docs/manuals/10-security-model.md | 2 +- docs/manuals/12-extending-extenddb-storage.md | 2 +- tests/test_metrics_accounts.py | 215 +++++++-- 21 files changed, 971 insertions(+), 548 deletions(-) create mode 100644 crates/core/src/metrics/collector_record.rs diff --git a/crates/core/src/metrics/collector.rs b/crates/core/src/metrics/collector.rs index e0f52bc1b..52fb30af7 100755 --- a/crates/core/src/metrics/collector.rs +++ b/crates/core/src/metrics/collector.rs @@ -52,7 +52,7 @@ pub struct MetricsCollector { #[derive(Debug, Clone)] pub(super) struct SegmentPoint { pub(super) operation: String, - #[allow(dead_code)] // TODO(cleanup): used when console adds per-account latency breakdown + /// The account of the request, which `query_segments` filters on. pub(super) account_id: String, #[allow(dead_code)] // TODO(cleanup): used when console adds table-scoped latency breakdown pub(super) table_name: Option, @@ -112,437 +112,6 @@ impl MetricsCollector { .record(value, now, wall_time); } - /// Record a latency observation in microseconds. - pub fn record_latency( - &self, - account_id: &str, - table_name: Option<&str>, - operation: &str, - latency_us: f64, - ) { - self.record( - MetricName::SuccessfulRequestLatency, - latency_us, - Some(account_id), - table_name, - None, - Some(operation), - ); - } - - /// Record a successful request's consumed read capacity. - pub fn record_read_capacity( - &self, - account_id: &str, - table_name: &str, - operation: &str, - units: f64, - ) { - self.record( - MetricName::ConsumedReadCapacityUnits, - units, - Some(account_id), - Some(table_name), - None, - Some(operation), - ); - } - - /// Record a successful request's consumed write capacity. - pub fn record_write_capacity( - &self, - account_id: &str, - table_name: &str, - operation: &str, - units: f64, - ) { - self.record( - MetricName::ConsumedWriteCapacityUnits, - units, - Some(account_id), - Some(table_name), - None, - Some(operation), - ); - } - - /// Record a user error (4xx). - pub fn record_user_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { - self.record( - MetricName::UserErrors, - 1.0, - Some(account_id), - table_name, - None, - Some(operation), - ); - } - - /// Record a system error (5xx). - pub fn record_system_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { - self.record( - MetricName::SystemErrors, - 1.0, - Some(account_id), - table_name, - None, - Some(operation), - ); - } - - /// Record a conditional check failure. - pub fn record_conditional_check_failure( - &self, - account_id: &str, - table_name: Option<&str>, - operation: &str, - ) { - self.record( - MetricName::ConditionalCheckFailedRequests, - 1.0, - Some(account_id), - table_name, - None, - Some(operation), - ); - } - - /// Record a transaction conflict. - pub fn record_transaction_conflict(&self, account_id: &str, operation: &str) { - self.record( - MetricName::TransactionConflict, - 1.0, - Some(account_id), - None, - None, - Some(operation), - ); - } - - /// Record returned item count from a query/scan. - #[allow(clippy::cast_precision_loss)] // count values are small enough for f64 - pub fn record_returned_items( - &self, - account_id: &str, - table_name: &str, - operation: &str, - count: u64, - ) { - self.record( - MetricName::ReturnedItemCount, - count as f64, - Some(account_id), - Some(table_name), - None, - Some(operation), - ); - } - - /// Record returned bytes from a query/scan. - #[allow(clippy::cast_precision_loss)] // byte counts within 1 MB page limit - pub fn record_returned_bytes( - &self, - account_id: &str, - table_name: &str, - operation: &str, - bytes: u64, - ) { - self.record( - MetricName::ReturnedBytes, - bytes as f64, - Some(account_id), - Some(table_name), - None, - Some(operation), - ); - } - - /// Record a TTL deletion. - pub fn record_ttl_deletion(&self, account_id: &str, table_name: &str) { - self.record( - MetricName::TimeToLiveDeletedItemCount, - 1.0, - Some(account_id), - Some(table_name), - None, - None, - ); - } - - /// Record a queue registration the TTL audit found missing and recreated. - pub fn record_ttl_audit_repair(&self, account_id: &str, table_name: &str) { - self.record( - MetricName::TtlAuditRepairedEntryCount, - 1.0, - Some(account_id), - Some(table_name), - None, - None, - ); - } - - /// Record a sweep row that failed and was left for its queue-state retry. - /// - /// Isolated failures are normal (contention, restarts); a sustained - /// nonzero rate on one table means rows are chronically erroring and - /// retrying without ever expiring, which a log line alone won't surface. - pub fn record_ttl_sweep_row_error(&self, account_id: &str, table_name: &str) { - self.record( - MetricName::TtlSweepRowErrorCount, - 1.0, - Some(account_id), - Some(table_name), - None, - None, - ); - } - - /// Record the number of unresolved TTL destroy markers seen in one pass. - pub fn record_ttl_repair_markers(&self, count: f64) { - self.record( - MetricName::TtlRepairMarkerCount, - count, - None, - None, - None, - None, - ); - } - - /// Record TTL deletion staleness (seconds past expiry). - pub fn record_ttl_staleness(&self, account_id: &str, table_name: &str, staleness_secs: f64) { - self.record( - MetricName::TtlDeletionStaleness, - staleness_secs, - Some(account_id), - Some(table_name), - None, - None, - ); - } - - /// P120c: Record an HTTP request (dimensions: operation). - pub fn record_request_count(&self, account_id: &str, operation: &str) { - self.record( - MetricName::RequestCount, - 1.0, - Some(account_id), - None, - None, - Some(operation), - ); - } - - /// P120c: Record a storage query execution (dimensions: source, category). - pub fn record_storage_query( - &self, - source: super::types::QuerySource, - category: super::types::QueryCategory, - latency_us: f64, - ) { - let source_str = source.to_string(); - let category_str = category.to_string(); - // Count - self.record( - MetricName::StorageQueryCount, - 1.0, - None, - None, - Some(&source_str), - Some(&category_str), - ); - // Latency - self.record( - MetricName::StorageQueryLatency, - latency_us, - None, - None, - Some(&source_str), - Some(&category_str), - ); - } - - /// P120d: Record pool connection gauge values. - pub fn record_pool_state(&self, active: u32, idle: u32) { - #[allow(clippy::cast_precision_loss)] - { - self.record( - MetricName::PoolActiveConnections, - f64::from(active), - None, - None, - None, - None, - ); - self.record( - MetricName::PoolIdleConnections, - f64::from(idle), - None, - None, - None, - None, - ); - } - } - - /// P120d: Record pool acquire latency in microseconds. - pub fn record_pool_acquire_latency(&self, latency_us: f64) { - self.record( - MetricName::PoolAcquireLatency, - latency_us, - None, - None, - None, - None, - ); - } - - /// P120e: Record a worker's last successful cycle timestamp. - #[allow(clippy::cast_precision_loss)] - pub fn record_worker_success(&self, source: super::types::QuerySource, latency_us: f64) { - let worker_str = source.to_string(); - let now_secs = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_secs(); - self.record( - MetricName::WorkerLastSuccess, - now_secs as f64, - None, - None, - None, - Some(&worker_str), - ); - self.record( - MetricName::WorkerCycleLatency, - latency_us, - None, - None, - None, - Some(&worker_str), - ); - } - - /// P120e: Record a worker error. - pub fn record_worker_error(&self, source: super::types::QuerySource) { - let worker_str = source.to_string(); - self.record( - MetricName::WorkerErrorCount, - 1.0, - None, - None, - None, - Some(&worker_str), - ); - } - - /// Record the end of a store-backed backup job: one `BackupJobs` under - /// the outcome and its duration in microseconds under the same - /// dimension. The job measures the duration from the snapshot's open to - /// the end of its last catalog or store call: the AVAILABLE write for a - /// job that succeeded, the FAILED write and the prefix's deletion for - /// one that failed. - pub fn record_backup_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { - let outcome = outcome.to_string(); - self.record( - MetricName::BackupJobs, - 1.0, - None, - None, - None, - Some(&outcome), - ); - self.record( - MetricName::BackupJobDuration, - duration_us, - None, - None, - None, - Some(&outcome), - ); - } - - /// Record bytes a backup job wrote to the store; the job records them - /// once it ended AVAILABLE, so a failed job's uploaded and then deleted - /// bytes are not counted. - #[allow(clippy::cast_precision_loss)] // byte counts of one backup - pub fn record_backup_bytes_uploaded(&self, bytes: u64) { - self.record( - MetricName::BackupBytesUploaded, - bytes as f64, - None, - None, - None, - None, - ); - } - - /// Record a running backup job's snapshot age in seconds (gauge). - pub fn record_backup_snapshot_age(&self, age_secs: f64) { - self.record( - MetricName::BackupSnapshotAge, - age_secs, - None, - None, - None, - None, - ); - } - - /// Record a failed store call of a backup job, a restore job or the - /// backup reconciler; `op` names the call (a backup job's `put` and - /// `delete_prefix`, a restore's `get`, the reconciler's `get`, `list` - /// and `delete_prefix`). - pub fn record_backup_store_error(&self, op: &str) { - self.record( - MetricName::BackupStoreErrors, - 1.0, - None, - None, - None, - Some(op), - ); - } - - /// Record the end of a store-backed restore job: one `RestoreJobs` under - /// the outcome and its duration in microseconds under the same - /// dimension, as [`Self::record_backup_job`] records a backup's. The job - /// measures the duration from its registration to the end of its last - /// catalog or store call. - pub fn record_restore_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { - let outcome = outcome.to_string(); - self.record( - MetricName::RestoreJobs, - 1.0, - None, - None, - None, - Some(&outcome), - ); - self.record( - MetricName::RestoreJobDuration, - duration_us, - None, - None, - None, - Some(&outcome), - ); - } - - /// Record bytes a restore job read from the store, as each download - /// arrived. - #[allow(clippy::cast_precision_loss)] // byte counts of one backup - pub fn record_restore_bytes_downloaded(&self, bytes: u64) { - self.record( - MetricName::RestoreBytesDownloaded, - bytes as f64, - None, - None, - None, - None, - ); - } - /// Record per-segment latency breakdown for a request. pub fn record_segments( &self, diff --git a/crates/core/src/metrics/collector_query.rs b/crates/core/src/metrics/collector_query.rs index 3e81fdd02..373d49fc7 100755 --- a/crates/core/src/metrics/collector_query.rs +++ b/crates/core/src/metrics/collector_query.rs @@ -35,9 +35,10 @@ impl MetricsCollector { let mut results = Vec::new(); for (key, acc) in map.iter() { - // Filter by account if specified. - if let Some(ref account) = params.account_id - && key.account_id.as_deref() != Some(account.as_str()) + // Filter by account if specified; an empty string is no filter, + // as in the stores. + if let Some(account) = params.account_id.as_deref().filter(|a| !a.is_empty()) + && key.account_id.as_deref() != Some(account) { continue; } @@ -151,9 +152,16 @@ impl MetricsCollector { /// Query average latency segments for the console deep-dive. /// - /// Returns per-operation averages over the given time window. + /// Returns per-operation averages over the given time window, of the + /// requests of `account_id` only when it is given (an empty string is no + /// filter). #[must_use] - pub fn query_segments(&self, window: TimeWindow) -> Vec { + pub fn query_segments( + &self, + window: TimeWindow, + account_id: Option<&str>, + ) -> Vec { + let account_id = account_id.filter(|a| !a.is_empty()); let now = Instant::now(); let cutoff = window_cutoff(window, now); let Ok(vec) = self.segments.read() else { @@ -166,6 +174,9 @@ impl MetricsCollector { { continue; } + if account_id.is_some_and(|a| a != p.account_id) { + continue; + } let e = by_op.entry(p.operation.clone()).or_default(); e.0 += p.segments.auth_us; e.1 += p.segments.authz_us; diff --git a/crates/core/src/metrics/collector_record.rs b/crates/core/src/metrics/collector_record.rs new file mode 100644 index 000000000..91b0a5176 --- /dev/null +++ b/crates/core/src/metrics/collector_record.rs @@ -0,0 +1,444 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The named `record_*` methods of `MetricsCollector`, one per metric. +//! +//! Split from `collector.rs` to keep both files under the 500-line limit. A +//! metric of a request or a table takes the account it belongs to; a metric +//! of the whole deployment takes none. + +use super::collector::MetricsCollector; +use super::types::MetricName; + +impl MetricsCollector { + /// Record a latency observation in microseconds. + pub fn record_latency( + &self, + account_id: &str, + table_name: Option<&str>, + operation: &str, + latency_us: f64, + ) { + self.record( + MetricName::SuccessfulRequestLatency, + latency_us, + Some(account_id), + table_name, + None, + Some(operation), + ); + } + + /// Record a successful request's consumed read capacity. + pub fn record_read_capacity( + &self, + account_id: &str, + table_name: &str, + operation: &str, + units: f64, + ) { + self.record( + MetricName::ConsumedReadCapacityUnits, + units, + Some(account_id), + Some(table_name), + None, + Some(operation), + ); + } + + /// Record a successful request's consumed write capacity. + pub fn record_write_capacity( + &self, + account_id: &str, + table_name: &str, + operation: &str, + units: f64, + ) { + self.record( + MetricName::ConsumedWriteCapacityUnits, + units, + Some(account_id), + Some(table_name), + None, + Some(operation), + ); + } + + /// Record a user error (4xx). + pub fn record_user_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { + self.record( + MetricName::UserErrors, + 1.0, + Some(account_id), + table_name, + None, + Some(operation), + ); + } + + /// Record a system error (5xx). + pub fn record_system_error(&self, account_id: &str, table_name: Option<&str>, operation: &str) { + self.record( + MetricName::SystemErrors, + 1.0, + Some(account_id), + table_name, + None, + Some(operation), + ); + } + + /// Record a conditional check failure. + pub fn record_conditional_check_failure( + &self, + account_id: &str, + table_name: Option<&str>, + operation: &str, + ) { + self.record( + MetricName::ConditionalCheckFailedRequests, + 1.0, + Some(account_id), + table_name, + None, + Some(operation), + ); + } + + /// Record a transaction conflict. + pub fn record_transaction_conflict(&self, account_id: &str, operation: &str) { + self.record( + MetricName::TransactionConflict, + 1.0, + Some(account_id), + None, + None, + Some(operation), + ); + } + + /// Record returned item count from a query/scan. + #[allow(clippy::cast_precision_loss)] // count values are small enough for f64 + pub fn record_returned_items( + &self, + account_id: &str, + table_name: &str, + operation: &str, + count: u64, + ) { + self.record( + MetricName::ReturnedItemCount, + count as f64, + Some(account_id), + Some(table_name), + None, + Some(operation), + ); + } + + /// Record returned bytes from a query/scan. + #[allow(clippy::cast_precision_loss)] // byte counts within 1 MB page limit + pub fn record_returned_bytes( + &self, + account_id: &str, + table_name: &str, + operation: &str, + bytes: u64, + ) { + self.record( + MetricName::ReturnedBytes, + bytes as f64, + Some(account_id), + Some(table_name), + None, + Some(operation), + ); + } + + /// Record a TTL deletion. + pub fn record_ttl_deletion(&self, account_id: &str, table_name: &str) { + self.record( + MetricName::TimeToLiveDeletedItemCount, + 1.0, + Some(account_id), + Some(table_name), + None, + None, + ); + } + + /// Record a queue registration the TTL audit found missing and recreated. + pub fn record_ttl_audit_repair(&self, account_id: &str, table_name: &str) { + self.record( + MetricName::TtlAuditRepairedEntryCount, + 1.0, + Some(account_id), + Some(table_name), + None, + None, + ); + } + + /// Record a sweep row that failed and was left for its queue-state retry. + /// + /// Isolated failures are normal (contention, restarts); a sustained + /// nonzero rate on one table means rows are chronically erroring and + /// retrying without ever expiring, which a log line alone won't surface. + pub fn record_ttl_sweep_row_error(&self, account_id: &str, table_name: &str) { + self.record( + MetricName::TtlSweepRowErrorCount, + 1.0, + Some(account_id), + Some(table_name), + None, + None, + ); + } + + /// Record the number of unresolved TTL destroy markers seen in one pass. + pub fn record_ttl_repair_markers(&self, count: f64) { + self.record( + MetricName::TtlRepairMarkerCount, + count, + None, + None, + None, + None, + ); + } + + /// Record TTL deletion staleness (seconds past expiry). + pub fn record_ttl_staleness(&self, account_id: &str, table_name: &str, staleness_secs: f64) { + self.record( + MetricName::TtlDeletionStaleness, + staleness_secs, + Some(account_id), + Some(table_name), + None, + None, + ); + } + + /// P120c: Record an HTTP request (dimensions: operation). + pub fn record_request_count(&self, account_id: &str, operation: &str) { + self.record( + MetricName::RequestCount, + 1.0, + Some(account_id), + None, + None, + Some(operation), + ); + } + + /// P120c: Record a storage query execution (dimensions: source, category). + pub fn record_storage_query( + &self, + source: super::types::QuerySource, + category: super::types::QueryCategory, + latency_us: f64, + ) { + let source_str = source.to_string(); + let category_str = category.to_string(); + // Count + self.record( + MetricName::StorageQueryCount, + 1.0, + None, + None, + Some(&source_str), + Some(&category_str), + ); + // Latency + self.record( + MetricName::StorageQueryLatency, + latency_us, + None, + None, + Some(&source_str), + Some(&category_str), + ); + } + + /// P120d: Record pool connection gauge values. + pub fn record_pool_state(&self, active: u32, idle: u32) { + #[allow(clippy::cast_precision_loss)] + { + self.record( + MetricName::PoolActiveConnections, + f64::from(active), + None, + None, + None, + None, + ); + self.record( + MetricName::PoolIdleConnections, + f64::from(idle), + None, + None, + None, + None, + ); + } + } + + /// P120d: Record pool acquire latency in microseconds. + pub fn record_pool_acquire_latency(&self, latency_us: f64) { + self.record( + MetricName::PoolAcquireLatency, + latency_us, + None, + None, + None, + None, + ); + } + + /// P120e: Record a worker's last successful cycle timestamp. + #[allow(clippy::cast_precision_loss)] + pub fn record_worker_success(&self, source: super::types::QuerySource, latency_us: f64) { + let worker_str = source.to_string(); + let now_secs = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + self.record( + MetricName::WorkerLastSuccess, + now_secs as f64, + None, + None, + None, + Some(&worker_str), + ); + self.record( + MetricName::WorkerCycleLatency, + latency_us, + None, + None, + None, + Some(&worker_str), + ); + } + + /// P120e: Record a worker error. + pub fn record_worker_error(&self, source: super::types::QuerySource) { + let worker_str = source.to_string(); + self.record( + MetricName::WorkerErrorCount, + 1.0, + None, + None, + None, + Some(&worker_str), + ); + } + + /// Record the end of a store-backed backup job: one `BackupJobs` under + /// the outcome and its duration in microseconds under the same + /// dimension. The job measures the duration from the snapshot's open to + /// the end of its last catalog or store call: the AVAILABLE write for a + /// job that succeeded, the FAILED write and the prefix's deletion for + /// one that failed. + pub fn record_backup_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { + let outcome = outcome.to_string(); + self.record( + MetricName::BackupJobs, + 1.0, + None, + None, + None, + Some(&outcome), + ); + self.record( + MetricName::BackupJobDuration, + duration_us, + None, + None, + None, + Some(&outcome), + ); + } + + /// Record bytes a backup job wrote to the store; the job records them + /// once it ended AVAILABLE, so a failed job's uploaded and then deleted + /// bytes are not counted. + #[allow(clippy::cast_precision_loss)] // byte counts of one backup + pub fn record_backup_bytes_uploaded(&self, bytes: u64) { + self.record( + MetricName::BackupBytesUploaded, + bytes as f64, + None, + None, + None, + None, + ); + } + + /// Record a running backup job's snapshot age in seconds (gauge). + pub fn record_backup_snapshot_age(&self, age_secs: f64) { + self.record( + MetricName::BackupSnapshotAge, + age_secs, + None, + None, + None, + None, + ); + } + + /// Record a failed store call of a backup job, a restore job or the + /// backup reconciler; `op` names the call (a backup job's `put` and + /// `delete_prefix`, a restore's `get`, the reconciler's `get`, `list` + /// and `delete_prefix`). + pub fn record_backup_store_error(&self, op: &str) { + self.record( + MetricName::BackupStoreErrors, + 1.0, + None, + None, + None, + Some(op), + ); + } + + /// Record the end of a store-backed restore job: one `RestoreJobs` under + /// the outcome and its duration in microseconds under the same + /// dimension, as [`Self::record_backup_job`] records a backup's. The job + /// measures the duration from its registration to the end of its last + /// catalog or store call. + pub fn record_restore_job(&self, outcome: super::types::BackupJobOutcome, duration_us: f64) { + let outcome = outcome.to_string(); + self.record( + MetricName::RestoreJobs, + 1.0, + None, + None, + None, + Some(&outcome), + ); + self.record( + MetricName::RestoreJobDuration, + duration_us, + None, + None, + None, + Some(&outcome), + ); + } + + /// Record bytes a restore job read from the store, as each download + /// arrived. + #[allow(clippy::cast_precision_loss)] // byte counts of one backup + pub fn record_restore_bytes_downloaded(&self, bytes: u64) { + self.record( + MetricName::RestoreBytesDownloaded, + bytes as f64, + None, + None, + None, + None, + ); + } +} diff --git a/crates/core/src/metrics/collector_tests.rs b/crates/core/src/metrics/collector_tests.rs index 1f52d4ca5..0075cbaff 100755 --- a/crates/core/src/metrics/collector_tests.rs +++ b/crates/core/src/metrics/collector_tests.rs @@ -186,7 +186,7 @@ fn record_and_query_segments() { }, ); - let segs = c.query_segments(TimeWindow::AllTime); + let segs = c.query_segments(TimeWindow::AllTime, None); assert_eq!(segs.len(), 1); let s = &segs[0]; assert_eq!(s.operation, "GetItem"); @@ -218,7 +218,7 @@ fn prune_removes_old_segments() { // All data is recent, prune should keep it. c.prune(); - let segs = c.query_segments(TimeWindow::AllTime); + let segs = c.query_segments(TimeWindow::AllTime, None); assert_eq!(segs.len(), 1); assert_eq!(segs[0].count, 1); } @@ -255,7 +255,7 @@ fn segments_multiple_operations() { }, ); - let segs = c.query_segments(TimeWindow::AllTime); + let segs = c.query_segments(TimeWindow::AllTime, None); assert_eq!(segs.len(), 2); // Both operations should have count=1. for s in &segs { @@ -377,3 +377,148 @@ fn restore_job_recorders_land_under_the_outcome_and_render_their_names() { ); assert_eq!(QuerySource::RestoreJob.to_string(), "RestoreJob"); } + +/// Every recorder of a request's or a table's metric keys its point by the +/// account it is given, so the account filter finds it and reports it as the +/// `AccountId` dimension. +#[test] +fn every_scoped_recorder_keeps_its_account() { + type Recorder = fn(&MetricsCollector, &str); + let recorders: [(&str, MetricName, Recorder); 14] = [ + ("latency", MetricName::SuccessfulRequestLatency, |c, a| { + c.record_latency(a, Some("T"), "GetItem", 1.0); + }), + ( + "read capacity", + MetricName::ConsumedReadCapacityUnits, + |c, a| { + c.record_read_capacity(a, "T", "GetItem", 1.0); + }, + ), + ( + "write capacity", + MetricName::ConsumedWriteCapacityUnits, + |c, a| { + c.record_write_capacity(a, "T", "PutItem", 1.0); + }, + ), + ("user error", MetricName::UserErrors, |c, a| { + c.record_user_error(a, None, "PutItem"); + }), + ("system error", MetricName::SystemErrors, |c, a| { + c.record_system_error(a, Some("T"), "PutItem"); + }), + ( + "condition", + MetricName::ConditionalCheckFailedRequests, + |c, a| { + c.record_conditional_check_failure(a, Some("T"), "PutItem"); + }, + ), + ("conflict", MetricName::TransactionConflict, |c, a| { + c.record_transaction_conflict(a, "TransactWriteItems"); + }), + ("returned items", MetricName::ReturnedItemCount, |c, a| { + c.record_returned_items(a, "T", "Query", 2); + }), + ("returned bytes", MetricName::ReturnedBytes, |c, a| { + c.record_returned_bytes(a, "T", "Query", 20); + }), + ( + "ttl deletion", + MetricName::TimeToLiveDeletedItemCount, + |c, a| { + c.record_ttl_deletion(a, "T"); + }, + ), + ( + "ttl audit", + MetricName::TtlAuditRepairedEntryCount, + |c, a| { + c.record_ttl_audit_repair(a, "T"); + }, + ), + ( + "ttl row error", + MetricName::TtlSweepRowErrorCount, + |c, a| { + c.record_ttl_sweep_row_error(a, "T"); + }, + ), + ("ttl staleness", MetricName::TtlDeletionStaleness, |c, a| { + c.record_ttl_staleness(a, "T", 3.0); + }), + ("request count", MetricName::RequestCount, |c, a| { + c.record_request_count(a, "GetItem"); + }), + ]; + for (label, metric, record) in recorders { + let c = MetricsCollector::new(); + record(&c, ACCOUNT); + record(&c, "222222222222"); + let mine = c.query(&MetricsQuery { + account_id: Some(ACCOUNT.to_owned()), + metric: Some(metric), + window: Some(TimeWindow::AllTime), + ..Default::default() + }); + assert_eq!(mine.len(), 1, "{label}: {mine:?}"); + assert!( + mine[0] + .dimensions + .contains(&Dimension::AccountId(ACCOUNT.to_owned())), + "{label}: {:?}", + mine[0].dimensions + ); + assert_eq!(mine[0].count, 1, "{label}"); + } +} + +/// The latency segments of one account leave out the other account's +/// requests; without an account, or with an empty one, they hold both. +#[test] +fn segments_are_filtered_by_account() { + use crate::metrics::types::LatencySegments; + + let c = MetricsCollector::new(); + let point = LatencySegments { + dispatch_us: 10.0, + total_us: 10.0, + ..LatencySegments::default() + }; + c.record_segments(ACCOUNT, "GetItem", Some("T"), point.clone()); + c.record_segments("222222222222", "Scan", Some("T"), point.clone()); + c.record_segments("222222222222", "GetItem", Some("T"), point); + + let ops = |account: Option<&str>| { + let mut ops: Vec<(String, u64)> = c + .query_segments(TimeWindow::AllTime, account) + .into_iter() + .map(|s| (s.operation, s.count)) + .collect(); + ops.sort(); + ops + }; + assert_eq!(ops(Some(ACCOUNT)), [("GetItem".to_owned(), 1)]); + assert_eq!( + ops(Some("222222222222")), + [("GetItem".to_owned(), 1), ("Scan".to_owned(), 1)] + ); + let all = [("GetItem".to_owned(), 2), ("Scan".to_owned(), 1)]; + assert_eq!(ops(None), all); + assert_eq!(ops(Some("")), all); +} + +/// An empty account filter is no filter, as in the stores. +#[test] +fn an_empty_account_filter_is_no_filter() { + let c = MetricsCollector::new(); + c.record_user_error(ACCOUNT, None, "PutItem"); + c.record_pool_acquire_latency(1.0); + let all = c.query(&MetricsQuery { + account_id: Some(String::new()), + window: Some(TimeWindow::AllTime), + ..Default::default() + }); + assert_eq!(all.len(), 2, "{all:?}"); +} diff --git a/crates/core/src/metrics/mod.rs b/crates/core/src/metrics/mod.rs index 637a97c29..9a680fd0d 100755 --- a/crates/core/src/metrics/mod.rs +++ b/crates/core/src/metrics/mod.rs @@ -13,6 +13,7 @@ mod accumulator; mod collector; mod collector_query; +mod collector_record; mod types; pub use collector::MetricsCollector; diff --git a/crates/server/src/console/pages/metrics_content.rs b/crates/server/src/console/pages/metrics_content.rs index fed5cc1eb..3107b5f9f 100755 --- a/crates/server/src/console/pages/metrics_content.rs +++ b/crates/server/src/console/pages/metrics_content.rs @@ -106,7 +106,7 @@ Per-operation average latency by request phase. Segments sum to total.

Raw Metrics

- +
MetricTableOperationPlaneSumCountMinMax
MetricAccountTableOperationPlaneSumCountMinMax
"#; @@ -177,7 +177,7 @@ async function refresh(){ buildLatencyOps(m); renderSegments(segs); const body=document.getElementById('metrics-body'); - body.innerHTML=m.map(x=>{const dims=x.dimensions||[],tbl=(dims.find(d=>d.TableName)||{}).TableName||'',op=(dims.find(d=>d.Operation)||{}).Operation||'',pl=isDataOp(op)?'Data':'Control';return''+esc(x.metric)+''+esc(tbl)+''+esc(op)+''+pl+''+x.sum.toFixed(2)+''+x.count+''+x.min.toFixed(2)+''+x.max.toFixed(2)+'';}).join(''); + body.innerHTML=m.map(x=>{const dims=x.dimensions||[],acct=(dims.find(d=>d.AccountId)||{}).AccountId||'',tbl=(dims.find(d=>d.TableName)||{}).TableName||'',op=(dims.find(d=>d.Operation)||{}).Operation||'',pl=isDataOp(op)?'Data':'Control';return''+esc(x.metric)+''+esc(acct)+''+esc(tbl)+''+esc(op)+''+pl+''+x.sum.toFixed(2)+''+x.count+''+x.min.toFixed(2)+''+x.max.toFixed(2)+'';}).join(''); if(bkts.length>0)renderBkt(bkts);else{['c-latency','c-availability','c-rcu','c-wcu','c-cp-latency','c-cp-errors'].forEach(id=>drawLine(id,[],[]));}; }catch(e){console.error('metrics fetch failed',e);} } diff --git a/crates/server/src/console/pages/metrics_pages.rs b/crates/server/src/console/pages/metrics_pages.rs index 2993276c5..4d4818f1c 100755 --- a/crates/server/src/console/pages/metrics_pages.rs +++ b/crates/server/src/console/pages/metrics_pages.rs @@ -139,10 +139,12 @@ async fn metrics_json( .window .unwrap_or(extenddb_core::metrics::TimeWindow::Last5Minutes); let seg_metrics = Arc::clone(collector); - response.segments = - tokio::task::spawn_blocking(move || seg_metrics.query_segments(window)) - .await - .unwrap_or_default(); + let account = params.account_id.clone(); + response.segments = tokio::task::spawn_blocking(move || { + seg_metrics.query_segments(window, account.as_deref()) + }) + .await + .unwrap_or_default(); axum::Json(serde_json::to_value(response).unwrap_or_default()).into_response() } Err(msg) => { diff --git a/crates/server/src/console/pages/metrics_pages_tests.rs b/crates/server/src/console/pages/metrics_pages_tests.rs index 21c2b269b..09324da15 100644 --- a/crates/server/src/console/pages/metrics_pages_tests.rs +++ b/crates/server/src/console/pages/metrics_pages_tests.rs @@ -179,7 +179,7 @@ async fn admin_gets_every_accounts_rows_from_the_data_feed() { ); assert!( accounts.contains(&account), - "admin feed lacks the account of {table}: {accounts:?}" + "admin feed lacks the account of {table}" ); } let indexes: Vec<&str> = body["metrics"] @@ -218,6 +218,9 @@ async fn admin_gets_the_metrics_page() { assert!(body.contains("

Metrics

"), "{body}"); assert!(body.contains("/console/metrics-data"), "{body}"); assert!(!body.contains("accountFilter"), "{body}"); + // The raw metrics table names each row's account. + assert!(body.contains("MetricAccountTable")); + assert!(body.contains("d.AccountId")); } #[test] diff --git a/crates/server/src/metrics_endpoint.rs b/crates/server/src/metrics_endpoint.rs index 492e2c7ab..c8f4fe3cc 100755 --- a/crates/server/src/metrics_endpoint.rs +++ b/crates/server/src/metrics_endpoint.rs @@ -82,10 +82,12 @@ pub(crate) async fn render_metrics( .window .unwrap_or(extenddb_core::metrics::TimeWindow::Last5Minutes); let seg_metrics = state.metrics.clone(); - response.segments = - tokio::task::spawn_blocking(move || seg_metrics.query_segments(window)) - .await - .unwrap_or_default(); + let account = params.account_id.clone(); + response.segments = tokio::task::spawn_blocking(move || { + seg_metrics.query_segments(window, account.as_deref()) + }) + .await + .unwrap_or_default(); return ( StatusCode::OK, axum::Json(serde_json::to_value(response).unwrap_or_default()), @@ -106,6 +108,7 @@ pub(crate) async fn render_metrics( .window .unwrap_or(extenddb_core::metrics::TimeWindow::Last5Minutes); let metrics_for_segments = state.metrics.clone(); + let account = params.account_id.clone(); let snapshots = match tokio::task::spawn_blocking(move || metrics.query(¶ms)).await { Ok(s) => s, Err(e) => { @@ -113,9 +116,11 @@ pub(crate) async fn render_metrics( Vec::new() } }; - let segments = tokio::task::spawn_blocking(move || metrics_for_segments.query_segments(window)) - .await - .unwrap_or_default(); + let segments = tokio::task::spawn_blocking(move || { + metrics_for_segments.query_segments(window, account.as_deref()) + }) + .await + .unwrap_or_default(); let response = MetricsResponse { metrics: snapshots, buckets: Vec::new(), @@ -229,9 +234,9 @@ fn aggregate_rows( let mut bucket_map: HashMap<(DimKey, i64), (f64, u64, f64, f64)> = HashMap::new(); for row in rows { - // CloudWatch-style dimensions: metrics are keyed per account, per - // table and per index. The endpoint is authenticated (admin Basic - // auth), so per-table dimensions are only returned to an authorized + // CloudWatch-style dimensions per table and per index, and ExtendDB's + // own account dimension. The endpoint is authenticated (admin Basic + // auth), so these dimensions are only returned to an authorized // caller. let key = ( row.metric.clone(), diff --git a/crates/server/src/response.rs b/crates/server/src/response.rs index 01c26448c..5c3007b24 100755 --- a/crates/server/src/response.rs +++ b/crates/server/src/response.rs @@ -151,11 +151,18 @@ mod tests { let user_errors = dimensions_of(&collector, MetricName::UserErrors); assert_eq!(user_errors.len(), 1, "{user_errors:?}"); assert!(!has_table(&user_errors[0]), "{user_errors:?}"); - // The error still names the account the request was made for. + // The error still names the account the request was made for, and + // so does the conflict. assert!( user_errors[0].contains(&Dimension::AccountId(ACCOUNT.to_owned())), "{user_errors:?}" ); + let conflicts = dimensions_of(&collector, MetricName::TransactionConflict); + assert_eq!(conflicts.len(), 1, "{conflicts:?}"); + assert!( + conflicts[0].contains(&Dimension::AccountId(ACCOUNT.to_owned())), + "{conflicts:?}" + ); } /// A failed condition counts in `ConditionalCheckFailedRequests` only. @@ -205,6 +212,10 @@ mod tests { .any(|d| matches!(d, Dimension::TableName(t) if t == "Orders")), "{metric:?}: {dims:?}" ); + assert!( + dims[0].contains(&Dimension::AccountId(ACCOUNT.to_owned())), + "{metric:?}: {dims:?}" + ); } } } diff --git a/crates/storage-cassandra/tests/direct/metrics_store.rs b/crates/storage-cassandra/tests/direct/metrics_store.rs index 71f7d8ca7..04db607b4 100644 --- a/crates/storage-cassandra/tests/direct/metrics_store.rs +++ b/crates/storage-cassandra/tests/direct/metrics_store.rs @@ -19,6 +19,15 @@ mod tests { } let engine = setup_engine().await; let store = create_catalog_store(&engine, &test_config()); - extenddb_storage::metrics_conformance::run(&store).await; + extenddb_storage::metrics_conformance::run( + &store, + // Cassandra writes a row of the same key over the previous one, + // and its TTL, not prune_metrics, expires the rows. + extenddb_storage::metrics_conformance::Expectations { + accumulates: false, + prunes: false, + }, + ) + .await; } } diff --git a/crates/storage-mongodb/tests/metrics_store.rs b/crates/storage-mongodb/tests/metrics_store.rs index 08a649dda..8ee4921dd 100644 --- a/crates/storage-mongodb/tests/metrics_store.rs +++ b/crates/storage-mongodb/tests/metrics_store.rs @@ -21,7 +21,14 @@ async fn the_metrics_store_keeps_two_accounts_apart() { let d = Deployment::new(&uri, "metrics").await; d.create().await; let store = MongoCatalogStore::on_database_for_tests(d.client.clone(), d.catalog()); - extenddb_storage::metrics_conformance::run(&store).await; + extenddb_storage::metrics_conformance::run( + &store, + extenddb_storage::metrics_conformance::Expectations { + accumulates: true, + prunes: true, + }, + ) + .await; d.drop_databases().await; } diff --git a/crates/storage-postgres/tests/metrics_store.rs b/crates/storage-postgres/tests/metrics_store.rs index 6eed98e21..1d324d9a1 100644 --- a/crates/storage-postgres/tests/metrics_store.rs +++ b/crates/storage-postgres/tests/metrics_store.rs @@ -22,7 +22,14 @@ async fn the_metrics_store_keeps_two_accounts_apart() { s.migrate_through(MigrationId::LATEST).await; let catalog = s.catalog().await; let store = PostgresCatalogStore::new(catalog.clone()); - extenddb_storage::metrics_conformance::run(&store).await; + extenddb_storage::metrics_conformance::run( + &store, + extenddb_storage::metrics_conformance::Expectations { + accumulates: true, + prunes: true, + }, + ) + .await; catalog.close().await; } diff --git a/crates/storage-sqlite/src/migrations/released_tests.rs b/crates/storage-sqlite/src/migrations/released_tests.rs index df565220e..b452ddd10 100644 --- a/crates/storage-sqlite/src/migrations/released_tests.rs +++ b/crates/storage-sqlite/src/migrations/released_tests.rs @@ -383,16 +383,15 @@ async fn migration_10_replaces_metrics_with_an_empty_account_metrics() { ); assert_eq!(count(&db, "SELECT COUNT(*) FROM account_metrics").await, 0); - let insert = |account: &str| { - format!( - "INSERT INTO account_metrics (bucket, account_id, metric, table_name, operation, \ - sum, count, min, max) \ - VALUES ('2026-11-11T00:00:00Z', '{account}', 'UserErrors', 'Orders', 'PutItem', \ - 1, 1, 1, 1)" - ) - }; - exec(&db, &insert("111111111111")).await; - exec(&db, &insert("222222222222")).await; + // Two rows that differ only in their account. + exec( + &db, + "INSERT INTO account_metrics (bucket, account_id, metric, table_name, operation, \ + sum, count, min, max) VALUES \ + ('2026-11-11T00:00:00Z', '111111111111', 'UserErrors', 'Orders', 'PutItem', 1, 1, 1, 1), \ + ('2026-11-11T00:00:00Z', '222222222222', 'UserErrors', 'Orders', 'PutItem', 1, 1, 1, 1)", + ) + .await; assert_eq!(count(&db, "SELECT COUNT(*) FROM account_metrics").await, 2); // The body again, as --reconverge runs it: the rows stay. @@ -409,7 +408,14 @@ async fn the_metrics_store_keeps_two_accounts_apart() { run(&db).await.expect("migrate"); let pool = plain_pool(&db).await; let store = crate::SqliteCatalogStore::new(pool.clone()); - extenddb_storage::metrics_conformance::run(&store).await; + extenddb_storage::metrics_conformance::run( + &store, + extenddb_storage::metrics_conformance::Expectations { + accumulates: true, + prunes: true, + }, + ) + .await; pool.close().await; } diff --git a/crates/storage/src/metrics_conformance.rs b/crates/storage/src/metrics_conformance.rs index 1bc31bfc3..5fdc70085 100644 --- a/crates/storage/src/metrics_conformance.rs +++ b/crates/storage/src/metrics_conformance.rs @@ -11,10 +11,12 @@ //! returns only that account's rows, never the deployment's. //! //! Every row is in one minute 23 hours back, inside the day the server keeps -//! metrics for, and the accounts are made up for the run, so rows another -//! test writes to the same catalog stay out of the reads. It does not check -//! that two rows of one key add up: the Cassandra store writes a row of the -//! same key over the previous one. +//! metrics for, and the accounts and the deployment row's operation are made +//! up for the run, so rows another test or an earlier run writes to the same +//! catalog stay out of the reads. [`Expectations`] says whether the store adds +//! two rows of one key together and whether it prunes: the Cassandra store +//! writes a row of the same key over the previous one and leaves expiry to +//! its TTL, so it does neither. use futures::future::BoxFuture; @@ -23,13 +25,23 @@ use crate::management_store::{MetricsFilter, MetricsRow, MetricsStore}; /// The values the check reads back for a row: (account, table, metric, sum). type Seen = (Option, Option, String, f64); +/// What a store does beyond keeping accounts apart. +#[derive(Debug, Clone, Copy)] +pub struct Expectations { + /// Two inserts of one key read back as their sum, whether the store adds + /// them in one row or keeps two rows. + pub accumulates: bool, + /// `prune_metrics` deletes the rows older than the retention it is given. + pub prunes: bool, +} + /// Runs the check against `store`. /// /// # Panics /// /// When the store fails a call or returns rows other than the ones the /// check expects. -pub async fn run(store: &dyn MetricsStore) { +pub async fn run(store: &dyn MetricsStore, expect: Expectations) { let bucket = minute_a_day_back(); let unique = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -37,13 +49,20 @@ pub async fn run(store: &dyn MetricsStore) { % 1_000_000_000_000; let first = format!("{unique:012}"); let second = format!("{:012}", (unique + 1) % 1_000_000_000_000); + // The deployment row's key is the run's own, so a second run in the same + // minute neither adds to it nor reads it. + let deployment_op = format!("conformance-{unique}"); let row = |account: Option<&str>, table: Option<&str>, metric: &str, sum: f64| MetricsRow { bucket, account_id: account.map(ToOwned::to_owned), metric: metric.to_owned(), table_name: table.map(ToOwned::to_owned), index_name: None, - operation: Some("PutItem".to_owned()), + operation: Some(if account.is_some() { + "PutItem".to_owned() + } else { + deployment_op.clone() + }), sum, count: 1, min: sum, @@ -73,10 +92,11 @@ pub async fn run(store: &dyn MetricsStore) { let mut seen: Vec = rows .into_iter() .filter(|r| { - r.account_id - .as_deref() - .is_none_or(|a| a == first || a == second) - && r.bucket == bucket + let mine = match r.account_id.as_deref() { + Some(a) => a == first || a == second, + None => r.operation.as_deref() == Some(deployment_op.as_str()), + }; + mine && r.bucket == bucket }) .map(|r| (r.account_id, r.table_name, r.metric, r.sum)) .collect(); @@ -109,9 +129,10 @@ pub async fn run(store: &dyn MetricsStore) { seen(&first, None, "UserErrors", 2.0), ]; let rows_of_deployment: Vec<&Seen> = all.iter().filter(|s| s.0.is_none()).collect(); - assert!( - rows_of_deployment.contains(&&deployment), - "the row of the deployment is missing: {all:?}" + assert_eq!( + rows_of_deployment, + [&deployment], + "the row of the deployment: {all:?}" ); let accounts: Vec = all.iter().filter(|s| s.0.is_some()).cloned().collect(); want.sort_by(|a, b| format!("{a:?}").cmp(&format!("{b:?}"))); @@ -167,6 +188,63 @@ pub async fn run(store: &dyn MetricsStore) { .await, ); assert_eq!(empty, all, "empty filters read as no filter"); + + // A second insert of one key reads back as the sum of the two. + if expect.accumulates { + store + .insert_metrics(&[row( + Some(&first), + Some("Orders"), + "ConsumedWriteCapacityUnits", + 2.0, + )]) + .await + .expect("insert_metrics again"); + let rows = read(MetricsFilter { + account_id: Some(first.clone()), + table_name: Some("Orders".to_owned()), + metric: Some("ConsumedWriteCapacityUnits".to_owned()), + }) + .await; + let sum: f64 = rows.iter().map(|r| r.sum).sum(); + let count: i64 = rows.iter().map(|r| r.count).sum(); + assert_eq!((sum, count), (3.0, 2), "two inserts of one key: {rows:?}"); + } + + // The prune deletes a row older than the retention and keeps the rows + // inside it. + if expect.prunes { + let old = bucket - time::Duration::hours(25); + let mut stale = row(Some(&first), Some("Orders"), "UserErrors", 1.0); + stale.bucket = old; + store + .insert_metrics(&[stale]) + .await + .expect("insert an old row"); + let filter = MetricsFilter { + account_id: Some(first.clone()), + ..MetricsFilter::default() + }; + let before = store + .query_metrics(old, old, &filter) + .await + .expect("query the old row"); + assert_eq!(before.len(), 1, "{before:?}"); + store + .prune_metrics(std::time::Duration::from_secs(36 * 3600)) + .await + .expect("prune_metrics"); + let after = store + .query_metrics(old, old, &filter) + .await + .expect("query after the prune"); + assert!(after.is_empty(), "the prune kept {after:?}"); + let kept = ours(read(MetricsFilter::default()).await); + assert!( + !kept.is_empty(), + "the prune deleted rows inside the retention" + ); + } } /// The start of the minute 23 hours back, in whole seconds: every store diff --git a/devtools/run-mongodb-tests b/devtools/run-mongodb-tests index 57eaba215..4b4449782 100755 --- a/devtools/run-mongodb-tests +++ b/devtools/run-mongodb-tests @@ -353,15 +353,16 @@ if [[ " ${RUN_TESTS_ARGS[*]} " == *" --rust"* ]]; then EXTENDDB_TEST_MONGODB_URI="mongodb://localhost:$MONGO_PORT/?replicaSet=rs0&directConnection=true" \ cargo test -p extenddb-storage-mongodb --lib -- --ignored records_live echo "" - # The catalog migration tests: each runs in databases of its own on this - # server, and EXTENDDB_TEST_MONGODB_REQUIRED turns a skip for a missing - # server into a failure. - echo "=== Running MongoDB catalog migration tests (migrations_*) ===" + # The catalog migration tests and the metrics store test: each runs in + # databases of its own on this server, and EXTENDDB_TEST_MONGODB_REQUIRED + # turns a skip for a missing server into a failure. + echo "=== Running MongoDB catalog migration tests (migrations_*, metrics_store) ===" echo "" EXTENDDB_TEST_MONGODB_URI="mongodb://localhost:$MONGO_PORT/?replicaSet=rs0&directConnection=true" \ EXTENDDB_TEST_MONGODB_REQUIRED=1 \ cargo test -p extenddb-storage-mongodb --test migrations_fresh --test migrations_guards \ - --test migrations_holders --test migrations_locks --test migrations_released + --test migrations_holders --test migrations_locks --test migrations_released \ + --test metrics_store echo "" fi echo "=== Running test suite via devtools/run-tests --backend mongodb ===" diff --git a/docs/design/06-component-server.md b/docs/design/06-component-server.md index e0ca19445..bf9802250 100755 --- a/docs/design/06-component-server.md +++ b/docs/design/06-component-server.md @@ -305,7 +305,8 @@ GET /metrics → 200 OK (application/json) ``` This is **not** Prometheus exposition format. The response is a custom JSON -schema that uses DynamoDB CloudWatch-style metric names and dimensions. +schema that uses DynamoDB CloudWatch-style metric names and dimensions, and +ExtendDB's own `AccountId` dimension. **Query parameters** (`MetricsQuery` in `crates/core/src/metrics/types.rs`): @@ -360,17 +361,19 @@ DynamoDB CloudWatch-aligned: - `TimeToLiveDeletedItemCount`, `TtlDeletionStaleness` ExtendDB-internal: -- `RequestCount` (HTTP request count, dimension: `Operation`) +- `RequestCount` (HTTP request count, dimensions: `AccountId`, `Operation`) - `StorageQueryCount`, `StorageQueryLatency` (dimensions: source, category) - `PoolActiveConnections`, `PoolIdleConnections`, `PoolAcquireLatency` - `WorkerLastSuccess`, `WorkerCycleLatency`, `WorkerErrorCount` **Dimensions** (from the `Dimension` enum): -- `AccountId(String)`: the account of the request or table the metric - belongs to; absent on a metric of the whole deployment (pool, worker, - storage query and backup job metrics). Each stored row is keyed by it, so - two accounts' tables of the same name have their own rows. +- `AccountId(String)`, ExtendDB's own: the account of the request or table + the metric belongs to; absent on a metric of the whole deployment (pool, + worker, storage query and backup job metrics). Each stored row records it + (on SQLite, PostgreSQL and Cassandra as part of the key), so two accounts' + tables of the same name have their own rows. The `account_id` filter also + applies to `segments`. - `TableName(String)` - `GlobalSecondaryIndexName(String)` - `Operation(String)` diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index dc21c538a..14d879000 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -8,7 +8,7 @@ This release versions the PostgreSQL, SQLite, Cassandra and MongoDB catalogs by Migration 9, `warm_throughput`, adds a nullable `warm_throughput` column to the `tables` and `indexes` catalog tables on PostgreSQL, SQLite and Cassandra. It holds the `WarmThroughput` a client sets on a table or a global secondary index, which `DescribeTable` reports; nothing acts on the value (see [Differences from DynamoDB](../differences-from-dynamodb.md)). Tables created before the migration report the defaults for their billing mode. On MongoDB it changes nothing, since the catalog collections have no validator, and the ledger records it as not applicable. Migration 9 is breaking: to go back to a release without it, restore the backup taken before the upgrade (see [Rollback Procedure](#rollback-procedure)). -Migration 10, `account_metrics`, replaces the catalog's `metrics` table (on MongoDB, collection) with `account_metrics`, whose key has the account a row was recorded for, on every backend. Two accounts that each have a table of the same name now keep their own metrics rows, where before they wrote to one (see [Metrics carry their account](#v0115-metrics-carry-their-account-catalog-migration-required)). The rows of `metrics` are dropped, not copied: they carry no account, and the server keeps metrics for a day, so `/metrics` and the console dashboard show no history from before the upgrade. Migration 10 is breaking: a server of an earlier release writes and reads `metrics`, so stop every server of an earlier release before running `extenddb migrate --yes`, and to go back restore the backup taken before the upgrade (see [Rollback Procedure](#rollback-procedure)). +Migration 10, `account_metrics`, replaces the catalog's `metrics` table (on MongoDB, collection) with `account_metrics`, whose rows record the account they were recorded for. On SQLite, PostgreSQL and Cassandra the account is part of each row's key. Two accounts that each have a table of the same name now keep their own metrics rows, where before they wrote to one (see [Metrics carry their account](#v0115-metrics-carry-their-account-catalog-migration-required)). The rows of `metrics` are dropped, not copied: they carry no account, and the server keeps metrics for a day, so `/metrics` and the console dashboard show no history from before the upgrade. On Cassandra with `auto_snapshot` on (Cassandra's default), dropping `metrics` keeps a snapshot of it on disk; `nodetool clearsnapshot` removes it. Migration 10 is breaking: a server of an earlier release writes and reads `metrics`, so stop every server of an earlier release before running `extenddb migrate --yes`, and to go back restore the backup taken before the upgrade (see [Rollback Procedure](#rollback-procedure)). Configure `[backup.store]` before the release that stops creating in-catalog backups. The [Backup and Restore manual](14-backup-and-restore.md) covers choosing and securing a store, the recovery figures, the restore drill, catalog-loss recovery and what to do with in-catalog backups. This section says what the upgrade changes. In this release, CreateBackup and RestoreTableFromBackup work as follows: @@ -366,7 +366,7 @@ deployment, even when no catalog migration is required. ### v0.1.15: metrics carry their account (catalog migration required) -**What changed.** Every metric of a request, and the TTL metrics of a table, now carry the account they were recorded for. The catalog stores the account in the key of each metrics row (migration 10, `account_metrics`), so two accounts that each have a table of the same name keep their own rows: before this release they wrote to one row, and the counts and latencies `/metrics` reported for that name were the two tables' added together. `/metrics` and the console's `/console/metrics-data` report the account as an `AccountId` dimension, beside `TableName`, `GlobalSecondaryIndexName` and `Operation`, and `/metrics` takes an `account_id` query parameter that returns one account's metrics only. Metrics of the whole deployment, such as the connection pool, worker and backup job metrics, have no `AccountId` dimension, and the `account_id` filter leaves them out. The console dashboard still serves admin sessions only and shows every account's metrics; its charts add the accounts together. +**What changed.** The metrics of a request that have a table or an operation dimension (capacity, latency, errors, throttles, returned items and bytes, `RequestCount`), and the TTL metrics of a table, now carry the account they were recorded for. The catalog stores the account with each metrics row (migration 10, `account_metrics`), so two accounts that each have a table of the same name keep their own rows: before this release they wrote to one row, and the counts and latencies `/metrics` reported for that name were the two tables' added together. `/metrics` and the console's `/console/metrics-data` report the account as an `AccountId` dimension, beside `TableName`, `GlobalSecondaryIndexName` and `Operation`, and `/metrics` takes an `account_id` query parameter that returns one account's metrics only. Metrics of the whole deployment, such as the connection pool, worker, storage query and backup job metrics, have no `AccountId` dimension, and the `account_id` filter leaves them out. The filter also applies to the latency `segments` of the response. `AccountId` is ExtendDB's own dimension, not one of CloudWatch's DynamoDB dimensions. The console dashboard still serves admin sessions only and shows every account's metrics: its raw metrics table names each row's account, and its charts add the accounts together. **Who is affected.** A reader of `/metrics` that groups rows by their dimensions now gets one row per account where it got one row per table name, and a deployment with one account gets an `AccountId` dimension on every request metric. A reader that sums the rows of a table name across accounts gets the same totals as before for tables whose names no other account uses. @@ -980,7 +980,7 @@ This change needs no catalog migration of its own. Before upgrading a deployment ### v0.1.15: the console metrics dashboard is admin only, and admin-only console pages redirect IAM users -**What changed.** The console metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) now serve admin sessions only. An IAM-user session that opens the dashboard is redirected to `/console`, the data feed answers it with HTTP 403 and no metric data, and the navigation bar no longer shows IAM users the Metrics link. Before this release any console session could read the feed. The metrics table has no account column, so the feed held the table names, index names, operations, request counts and latencies of every account. The page put an IAM user's account id in a script variable that no script read, and the server sent every row. The Settings and Cache pages already served admin sessions only. They now redirect an IAM-user session to `/console` instead of answering HTTP 403, and the navigation bar no longer shows IAM users the Settings and Cache links. +**What changed.** The console metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) now serve admin sessions only. An IAM-user session that opens the dashboard is redirected to `/console`, the data feed answers it with HTTP 403 and no metric data, and the navigation bar no longer shows IAM users the Metrics link. Before this release any console session could read the feed. The metrics table had no account column then, so the feed held the table names, index names, operations, request counts and latencies of every account. The page put an IAM user's account id in a script variable that no script read, and the server sent every row. The Settings and Cache pages already served admin sessions only. They now redirect an IAM-user session to `/console` instead of answering HTTP 403, and the navigation bar no longer shows IAM users the Settings and Cache links. **Who is affected.** IAM users who opened the console's metrics dashboard. They lose the dashboard. A client that expected HTTP 403 from the Settings page, the Cache page or the cache form for an IAM-user session now gets a redirect to `/console`. Admin sessions get the same dashboard and data as before, and the admin-authenticated `/metrics` endpoint is unchanged. diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index a722a7d44..cced7a475 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -236,7 +236,7 @@ The management web console (`/console/*`) implements: - **Security headers**: X-Content-Type-Options (nosniff), X-Frame-Options (DENY), Referrer-Policy (strict-origin-when-cross-origin) - **HSTS**: Sent automatically (TLS is always enabled) - **Login rate limiting**: Failed login attempts are counted per user and source address, and per source address, as described under [Login Rate Limiting](#login-rate-limiting). -- **Admin-only metrics**: The metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) serve admin sessions only. The feed carries the table names, index names and traffic of every account, each row under the account it was recorded for. An IAM-user session is redirected from the page and gets HTTP 403 with no metric data from the feed. +- **Admin-only metrics**: The metrics dashboard (`/console/metrics`) and its data feed (`/console/metrics-data`) serve admin sessions only. The feed carries the table names, index names and traffic of every account, each request's or table's row under the account it was recorded for. An IAM-user session is redirected from the page and gets HTTP 403 with no metric data from the feed. ## Login Rate Limiting diff --git a/docs/manuals/12-extending-extenddb-storage.md b/docs/manuals/12-extending-extenddb-storage.md index 214de4894..d583ad778 100755 --- a/docs/manuals/12-extending-extenddb-storage.md +++ b/docs/manuals/12-extending-extenddb-storage.md @@ -213,7 +213,7 @@ Defined in `crates/storage/src/management_store/mod.rs`. Historical metrics pers | `query_metrics` | Query rows by time range, filtered by `MetricsFilter`: account, table name and metric | | `prune_metrics` | Delete rows older than the retention period | -Two accounts' rows for a table of the same name must stay two rows: the account is part of each row's key. `extenddb_storage::metrics_conformance::run` (feature `test-util`) checks a store against a migrated catalog. +Two accounts' rows for a table of the same name must never be read back as one: a store that adds rows of one key together keeps the account in the key. `extenddb_storage::metrics_conformance::run` (feature `test-util`) checks a store against a migrated catalog. ### RateLimitStore diff --git a/tests/test_metrics_accounts.py b/tests/test_metrics_accounts.py index 21db26e75..d732febdb 100644 --- a/tests/test_metrics_accounts.py +++ b/tests/test_metrics_accounts.py @@ -6,16 +6,19 @@ The metrics used to have no account dimension, so two accounts that each had a table of the same name wrote to one row, and the counts an admin read for that name were the two tables' added together. Each metric of a request -now carries the account the request was made for, as an ``AccountId`` -dimension, and ``/metrics`` takes an ``account_id`` filter. +and each TTL metric of a table now carries the account it was recorded for, +as an ``AccountId`` dimension, and ``/metrics`` takes an ``account_id`` +filter, which also applies to the latency segments. The test creates two accounts, each with a user that may call every -DynamoDB action, and a table of the same name in each. It writes one item -to the first account's table and three to the second's, waits until the -flush has persisted both accounts' ``ConsumedWriteCapacityUnits`` rows, then -reads until two reads in a row are the same (see ``flushed``): the two -accounts' rows are two rows with their own sums, and the account filter -returns one account's rows only. +DynamoDB action, and a table of the same name with TTL in each. Each +account sends one PutItem (one write capacity unit for the first account, +three for the second, so a sum of the two is visible), a GetItem, a Query, +a DeleteItem whose condition fails, and a GetItem on a table that does not +exist; the second account also sends a Scan. Each table holds one expired +item for the TTL sweep. The fixture waits until the flush has persisted +every row the tests read, with the PutItem sums at their final values, +then reads until two reads in a row are the same (see ``flushed``). Prerequisites: - extenddb running on EXTENDDB_TEST_ENDPOINT @@ -34,23 +37,26 @@ import pytest import requests from botocore.config import Config as BotoConfig +from botocore.exceptions import ClientError from conftest import wait_for_active, wait_for_deleted from management_helpers import ManagementClient -# The flush worker persists a data point once it is older than the worker's -# 60-second interval, at the next tick, so a point can take up to two -# intervals to reach the metrics table. -FLUSH_DEADLINE_SECONDS = 180 +# A TTL sweep runs once a minute, a new TTL attribute can take one more +# sweep to be ready, and the flush persists a data point at most two +# minutes after it was recorded. +FLUSH_DEADLINE_SECONDS = 420 POLL_SECONDS = 5 # After the awaited rows, the fixture reads the metrics every SETTLE_SECONDS -# until two reads in a row are the same: one flush writes its rows one by -# one, and a read can land partway through it. +# until two reads in a row are the same, for at most SETTLE_DEADLINE_SECONDS: +# one flush writes its rows one by one, and a read can land partway through. SETTLE_SECONDS = 5 +SETTLE_DEADLINE_SECONDS = 180 -# Items each account writes to its table; a PutItem of a small item on an -# on-demand table consumes one write capacity unit. -WRITES = (1, 3) +# The write capacity each account's one PutItem consumes: a small item is +# one unit, an item of about 2.5 KB three. +WRITE_UNITS = (1.0, 3.0) +PADDING = (0, 2500) pytestmark = pytest.mark.slow @@ -78,7 +84,7 @@ def _admin_metrics(env: tuple[str, str, str], **params: str) -> dict[str, Any]: endpoint, admin_user, admin_pass = env resp = requests.get( f"{endpoint}/metrics", - params=params, + params={"window": "LastHour", **params}, auth=(admin_user, admin_pass), timeout=10, verify=False, @@ -94,20 +100,34 @@ def _dimension(metric: dict[str, Any], key: str) -> str | None: return None -def _write_capacity(data: dict[str, Any], table: str) -> dict[str | None, float]: - """The PutItem write capacity under ``table``, summed per AccountId.""" +def _per_account( + data: dict[str, Any], + metric: str, + operation: str | None, + table: str | None, + field: str = "sum", +) -> dict[str | None, float]: + """``field`` of the rows of ``metric``, summed per AccountId. + + Only rows whose Operation is ``operation`` and whose TableName is + ``table`` count; None asks for a row without that dimension. + """ sums: dict[str | None, float] = {} for m in data["metrics"]: if ( - m["metric"] == "ConsumedWriteCapacityUnits" - and _dimension(m, "Operation") == "PutItem" + m["metric"] == metric + and _dimension(m, "Operation") == operation and _dimension(m, "TableName") == table ): account = _dimension(m, "AccountId") - sums[account] = sums.get(account, 0.0) + m["sum"] + sums[account] = sums.get(account, 0.0) + m[field] return sums +def _ours(sums: dict[str | None, float], ids: list[str]) -> dict[str | None, float]: + return {account: value for account, value in sums.items() if account in ids} + + def _canonical(data: dict[str, Any]) -> list[str]: return sorted(json.dumps(m, sort_keys=True) for m in data["metrics"]) @@ -145,13 +165,54 @@ def _account(mgmt: ManagementClient, endpoint: str, label: str): return account_id, boto3.client(**kwargs), user +def _send_requests(client, table: str, padding: int, scan: bool) -> None: + """The requests whose metrics the tests read, as one account.""" + # The expired item goes in by BatchWriteItem, whose metrics carry no + # table name, so it adds nothing to the PutItem rows. + client.batch_write_item( + RequestItems={ + table: [ + { + "PutRequest": { + "Item": { + "pk": {"S": "expired"}, + "expires": {"N": str(int(time.time()) - 600)}, + } + } + } + ] + } + ) + client.put_item( + TableName=table, Item={"pk": {"S": "1"}, "pad": {"S": "x" * padding or "x"}} + ) + client.get_item(TableName=table, Key={"pk": {"S": "1"}}) + client.query( + TableName=table, + KeyConditionExpression="pk = :pk", + ExpressionAttributeValues={":pk": {"S": "1"}}, + ) + if scan: + client.scan(TableName=table) + with pytest.raises(ClientError) as failed: + client.delete_item( + TableName=table, + Key={"pk": {"S": "absent"}}, + ConditionExpression="attribute_exists(pk)", + ) + assert failed.value.response["Error"]["Code"] == "ConditionalCheckFailedException" + with pytest.raises(ClientError) as missing: + client.get_item(TableName=f"{table}-missing", Key={"pk": {"S": "1"}}) + assert missing.value.response["Error"]["Code"] == "ResourceNotFoundException" + + @pytest.fixture(scope="module") def flushed(): - """Both accounts' writes, persisted by the flush. + """Both accounts' requests, persisted by the flush. Yields the admin environment, the table name, the two account ids in - the order of WRITES, and the metrics an admin reads once both accounts' - rows are in them. + the order of WRITE_UNITS, and the metrics an admin reads once every row + the tests read is in them. """ env = _require_env() mgmt = ManagementClient(*env) @@ -162,36 +223,55 @@ def flushed(): accounts.append(_account(mgmt, env[0], label)) for _, client, _ in accounts: client.create_table(TableName=table, **KEY) - for (_, client, _), writes in zip(accounts, WRITES): + for _, client, _ in accounts: wait_for_active(client, table) - for i in range(writes): - client.put_item(TableName=table, Item={"pk": {"S": str(i)}}) + client.update_time_to_live( + TableName=table, + TimeToLiveSpecification={"Enabled": True, "AttributeName": "expires"}, + ) + for i, ((_, client, _), padding) in enumerate(zip(accounts, PADDING)): + _send_requests(client, table, padding, scan=i == 1) ids = [account_id for account_id, _, _ in accounts] + every = set(ids) - def both_flushed(data: dict[str, Any]) -> bool: - return set(ids) <= set(_write_capacity(data, table)) + def all_flushed(data: dict[str, Any]) -> bool: + writes = _ours( + _per_account(data, "ConsumedWriteCapacityUnits", "PutItem", table), ids + ) + present = [ + _per_account(data, "ConsumedReadCapacityUnits", "GetItem", table), + _per_account(data, "ReturnedItemCount", "Query", table), + _per_account(data, "ConditionalCheckFailedRequests", "DeleteItem", table), + _per_account(data, "UserErrors", "GetItem", None), + _per_account(data, "TimeToLiveDeletedItemCount", None, table), + ] + return writes == dict(zip(ids, WRITE_UNITS)) and all( + every <= set(p) for p in present + ) deadline = time.monotonic() + FLUSH_DEADLINE_SECONDS while True: - data = _admin_metrics(env, window="LastHour") - if both_flushed(data) or time.monotonic() > deadline: + data = _admin_metrics(env) + if all_flushed(data) or time.monotonic() > deadline: break time.sleep(POLL_SECONDS) - assert both_flushed(data), ( - f"within {FLUSH_DEADLINE_SECONDS}s the metrics lacked a PutItem " - f"write capacity row under {table} for each account: " - f"{_write_capacity(data, table)}" + assert all_flushed(data), ( + f"within {FLUSH_DEADLINE_SECONDS}s the metrics lacked a row the " + f"tests read for one of the two accounts under {table}; PutItem " + f"write capacity per account: " + f"{_per_account(data, 'ConsumedWriteCapacityUnits', 'PutItem', table)}" ) # The server lists the rows in no fixed order, so compare them sorted. + settle = time.monotonic() + SETTLE_DEADLINE_SECONDS while True: time.sleep(SETTLE_SECONDS) - again = _admin_metrics(env, window="LastHour") + again = _admin_metrics(env) if _canonical(again) == _canonical(data): break - assert time.monotonic() < deadline, ( - f"the metrics were still changing {FLUSH_DEADLINE_SECONDS}s " - f"after the test's requests" + assert time.monotonic() < settle, ( + f"the metrics were still changing {SETTLE_DEADLINE_SECONDS}s " + f"after the awaited rows arrived" ) data = again yield env, table, ids, data @@ -203,19 +283,60 @@ def both_flushed(data: dict[str, Any]) -> bool: except client.exceptions.ResourceNotFoundException: pass finally: - mgmt.delete_user(account_id, user) - mgmt.delete_account(account_id) + resp = mgmt.delete_user(account_id, user) + assert resp.status_code in (200, 204), resp.text + resp = mgmt.delete_account(account_id) + assert resp.status_code in (200, 204), resp.text def test_two_accounts_with_the_same_table_name_get_separate_rows(flushed): _, table, ids, data = flushed - sums = _write_capacity(data, table) - assert sums == {ids[0]: float(WRITES[0]), ids[1]: float(WRITES[1])}, sums + writes = _per_account(data, "ConsumedWriteCapacityUnits", "PutItem", table) + assert _ours(writes, ids) == dict(zip(ids, WRITE_UNITS)), writes + + +@pytest.mark.parametrize( + "metric, operation, on_table", + [ + ("ConsumedReadCapacityUnits", "GetItem", True), + ("ReturnedItemCount", "Query", True), + ("ReturnedBytes", "Query", True), + ("ConditionalCheckFailedRequests", "DeleteItem", True), + ("UserErrors", "GetItem", False), + ("RequestCount", "PutItem", False), + ], +) +def test_each_request_metric_names_its_account(flushed, metric, operation, on_table): + _, table, ids, data = flushed + rows = _per_account(data, metric, operation, table if on_table else None, "count") + assert set(ids) <= set(rows), f"{metric} {operation}: {rows}" + + +def test_latency_counts_each_accounts_requests_apart(flushed): + _, table, ids, data = flushed + counts = _per_account(data, "SuccessfulRequestLatency", "PutItem", table, "count") + assert _ours(counts, ids) == {ids[0]: 1, ids[1]: 1}, counts + + +def test_ttl_deletions_name_their_account(flushed): + _, table, ids, data = flushed + deleted = _per_account(data, "TimeToLiveDeletedItemCount", None, table) + assert _ours(deleted, ids) == {ids[0]: 1.0, ids[1]: 1.0}, deleted def test_the_account_filter_returns_that_accounts_rows_only(flushed): env, table, ids, _ = flushed - data = _admin_metrics(env, window="LastHour", account_id=ids[1]) + data = _admin_metrics(env, account_id=ids[1]) accounts = {_dimension(m, "AccountId") for m in data["metrics"]} assert accounts == {ids[1]}, accounts - assert _write_capacity(data, table) == {ids[1]: float(WRITES[1])} + writes = _per_account(data, "ConsumedWriteCapacityUnits", "PutItem", table) + assert writes == {ids[1]: WRITE_UNITS[1]}, writes + + +def test_the_account_filter_applies_to_the_latency_segments(flushed): + env, _, ids, _ = flushed + # Only the second account sent a Scan. + first = {s["operation"] for s in _admin_metrics(env, account_id=ids[0])["segments"]} + second = {s["operation"] for s in _admin_metrics(env, account_id=ids[1])["segments"]} + assert "PutItem" in first and "Scan" not in first, first + assert {"PutItem", "Scan"} <= second, second From 5a44c5c4b02305f784e8690a88d836274e9888ad Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Tue, 10 Nov 2026 23:05:45 -0800 Subject: [PATCH 3/4] fix(metrics): Test each store filter alone, drop a redundant index Review of #955: - The store check gives the second account a row under another table name and a row under another metric, and reads with the table filter alone and the metric filter alone, so a store that ignored either filter fails it. - account_metrics has no separate bucket index on SQLite and PostgreSQL: the primary key starts with bucket, so its index serves the range reads and the prune. - MetricsRow says that a deployment row has no account_id field on MongoDB. Part of #728. --- .../migrations/0010_account_metrics.sql | 8 +-- .../migrations/0010_account_metrics.sql | 8 +-- crates/storage/src/management_store/types.rs | 3 +- crates/storage/src/metrics_conformance.rs | 49 +++++++++++++++++++ docs/schema/postgres.txt | 1 - docs/schema/sqlite.txt | 1 - 6 files changed, 59 insertions(+), 11 deletions(-) diff --git a/crates/storage-postgres/migrations/0010_account_metrics.sql b/crates/storage-postgres/migrations/0010_account_metrics.sql index 9ec918992..26e41e808 100644 --- a/crates/storage-postgres/migrations/0010_account_metrics.sql +++ b/crates/storage-postgres/migrations/0010_account_metrics.sql @@ -6,8 +6,10 @@ -- recorded for, so two accounts' tables of the same name keep their own rows -- (#728). account_id is the empty string on a metric of the whole -- deployment. The rows of metrics are dropped, not copied: they carry no --- account, and the server keeps metrics for a day. IF NOT EXISTS and --- IF EXISTS make a second run change nothing. +-- account, and the server keeps metrics for a day. The primary key starts +-- with bucket, so its index serves the reads and the prune, which filter on a +-- range of buckets. IF NOT EXISTS and IF EXISTS make a second run change +-- nothing. CREATE TABLE IF NOT EXISTS account_metrics ( bucket TIMESTAMPTZ NOT NULL, @@ -23,6 +25,4 @@ CREATE TABLE IF NOT EXISTS account_metrics ( PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) ); -CREATE INDEX IF NOT EXISTS idx_account_metrics_bucket ON account_metrics (bucket); - DROP TABLE IF EXISTS metrics; diff --git a/crates/storage-sqlite/migrations/0010_account_metrics.sql b/crates/storage-sqlite/migrations/0010_account_metrics.sql index 7c26396fb..96e2d4f1c 100644 --- a/crates/storage-sqlite/migrations/0010_account_metrics.sql +++ b/crates/storage-sqlite/migrations/0010_account_metrics.sql @@ -6,8 +6,10 @@ -- accounts' tables of the same name keep their own rows (#728). account_id is -- the empty string on a metric of the whole deployment. The rows of metrics -- are dropped, not copied: they carry no account, and the server keeps --- metrics for a day. IF NOT EXISTS and IF EXISTS make a second run change --- nothing. ±9e999 store as ±Infinity in SQLite REAL, as in migration 1. +-- metrics for a day. The primary key starts with bucket, so its index serves +-- the reads and the prune, which filter on a range of buckets. IF NOT EXISTS +-- and IF EXISTS make a second run change nothing. ±9e999 store as ±Infinity +-- in SQLite REAL, as in migration 1. CREATE TABLE IF NOT EXISTS account_metrics ( bucket TEXT NOT NULL, @@ -23,6 +25,4 @@ CREATE TABLE IF NOT EXISTS account_metrics ( PRIMARY KEY (bucket, account_id, metric, table_name, index_name, operation) ); -CREATE INDEX IF NOT EXISTS idx_account_metrics_bucket ON account_metrics (bucket); - DROP TABLE IF EXISTS metrics; diff --git a/crates/storage/src/management_store/types.rs b/crates/storage/src/management_store/types.rs index 0b143bc01..4d5b2fbf4 100755 --- a/crates/storage/src/management_store/types.rs +++ b/crates/storage/src/management_store/types.rs @@ -123,7 +123,8 @@ pub struct AdminEntry { pub struct MetricsRow { pub bucket: time::OffsetDateTime, /// The account the row was recorded for; `None` for a metric of the - /// whole deployment. Stored as the empty string. + /// whole deployment, stored as the empty string on SQLite, PostgreSQL + /// and Cassandra and as a document without the field on MongoDB. pub account_id: Option, pub metric: String, pub table_name: Option, diff --git a/crates/storage/src/metrics_conformance.rs b/crates/storage/src/metrics_conformance.rs index 5fdc70085..33de7c65c 100644 --- a/crates/storage/src/metrics_conformance.rs +++ b/crates/storage/src/metrics_conformance.rs @@ -83,6 +83,15 @@ pub async fn run(store: &dyn MetricsStore, expect: Expectations) { 5.0, ), row(Some(&first), None, "UserErrors", 2.0), + // Rows that only the table filter or only the metric filter + // leaves out of the narrow reads below. + row( + Some(&second), + Some("Other"), + "ConsumedWriteCapacityUnits", + 4.0, + ), + row(Some(&second), Some("Orders"), "UserErrors", 6.0), row(None, None, "WorkerCycleLatency", 7.0), ]) .await @@ -127,6 +136,8 @@ pub async fn run(store: &dyn MetricsStore, expect: Expectations) { seen(&first, Some("Orders"), "ConsumedWriteCapacityUnits", 1.0), seen(&second, Some("Orders"), "ConsumedWriteCapacityUnits", 5.0), seen(&first, None, "UserErrors", 2.0), + seen(&second, Some("Other"), "ConsumedWriteCapacityUnits", 4.0), + seen(&second, Some("Orders"), "UserErrors", 6.0), ]; let rows_of_deployment: Vec<&Seen> = all.iter().filter(|s| s.0.is_none()).collect(); assert_eq!( @@ -172,6 +183,44 @@ pub async fn run(store: &dyn MetricsStore, expect: Expectations) { )] ); + // The table filter alone, and the metric filter alone. + let sorted = |mut v: Vec| { + v.sort_by(|a, b| format!("{a:?}").cmp(&format!("{b:?}"))); + v + }; + let by_table = ours( + read(MetricsFilter { + table_name: Some("Orders".to_owned()), + ..MetricsFilter::default() + }) + .await, + ); + assert_eq!( + by_table, + sorted(vec![ + seen(&first, Some("Orders"), "ConsumedWriteCapacityUnits", 1.0), + seen(&second, Some("Orders"), "ConsumedWriteCapacityUnits", 5.0), + seen(&second, Some("Orders"), "UserErrors", 6.0), + ]), + "the table filter" + ); + let by_metric = ours( + read(MetricsFilter { + metric: Some("ConsumedWriteCapacityUnits".to_owned()), + ..MetricsFilter::default() + }) + .await, + ); + assert_eq!( + by_metric, + sorted(vec![ + seen(&first, Some("Orders"), "ConsumedWriteCapacityUnits", 1.0), + seen(&second, Some("Orders"), "ConsumedWriteCapacityUnits", 5.0), + seen(&second, Some("Other"), "ConsumedWriteCapacityUnits", 4.0), + ]), + "the metric filter" + ); + // An account with no rows reads nothing, and empty filters read as none. let unknown = read(MetricsFilter { account_id: Some(format!("{:012}", (unique + 2) % 1_000_000_000_000)), diff --git a/docs/schema/postgres.txt b/docs/schema/postgres.txt index 19a949bf2..74a2e4e05 100644 --- a/docs/schema/postgres.txt +++ b/docs/schema/postgres.txt @@ -260,7 +260,6 @@ index iam_user_tags_pkey: CREATE UNIQUE INDEX iam_user_tags_pkey ON public.iam_u index iam_users_pkey: CREATE UNIQUE INDEX iam_users_pkey ON public.iam_users USING btree (account_id, user_name) index iam_users_user_arn_key: CREATE UNIQUE INDEX iam_users_user_arn_key ON public.iam_users USING btree (user_arn) index idempotency_tokens_pkey: CREATE UNIQUE INDEX idempotency_tokens_pkey ON public.idempotency_tokens USING btree (token) -index idx_account_metrics_bucket: CREATE INDEX idx_account_metrics_bucket ON public.account_metrics USING btree (bucket) index idx_backup_index_account_table: CREATE INDEX idx_backup_index_account_table ON public.backup_index USING btree (account_id, table_name) index idx_backup_index_backup_id: CREATE UNIQUE INDEX idx_backup_index_backup_id ON public.backup_index USING btree (backup_id) index idx_backup_index_store_status: CREATE INDEX idx_backup_index_store_status ON public.backup_index USING btree (store_kind, store_root, store_id, status) diff --git a/docs/schema/sqlite.txt b/docs/schema/sqlite.txt index c32620e06..60a15d398 100644 --- a/docs/schema/sqlite.txt +++ b/docs/schema/sqlite.txt @@ -1,4 +1,3 @@ -index idx_account_metrics_bucket on account_metrics: CREATE INDEX idx_account_metrics_bucket ON account_metrics (bucket) index idx_backup_index_account_table on backup_index: CREATE INDEX idx_backup_index_account_table ON backup_index (account_id, table_name) index idx_backup_index_backup_id on backup_index: CREATE UNIQUE INDEX idx_backup_index_backup_id ON backup_index (backup_id) index idx_backup_index_store_status on backup_index: CREATE INDEX idx_backup_index_store_status ON backup_index (store_kind, store_root, store_id, status) From 4d8eb8d99732a8e0dbc698df4b8f8d11a68c211a Mon Sep 17 00:00:00 2001 From: Hitesh Sharma Date: Wed, 11 Nov 2026 00:53:39 -0800 Subject: [PATCH 4/4] test(metrics): Check the account of the throttle metrics Review of #955: - test_metrics_accounts.py gives each account a table provisioned with one write capacity unit and writes to it until a PutItem is throttled, then checks the AccountId of WriteThrottleEvents and ThrottledRequests under that table. - The upgrade manual's list of the request metrics that carry the account no longer has a qualifier that also covered the storage query metrics, which carry none. Part of #728. --- docs/manuals/07-upgrade-manual.md | 2 +- tests/test_metrics_accounts.py | 59 +++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/docs/manuals/07-upgrade-manual.md b/docs/manuals/07-upgrade-manual.md index 14d879000..837688ae2 100755 --- a/docs/manuals/07-upgrade-manual.md +++ b/docs/manuals/07-upgrade-manual.md @@ -366,7 +366,7 @@ deployment, even when no catalog migration is required. ### v0.1.15: metrics carry their account (catalog migration required) -**What changed.** The metrics of a request that have a table or an operation dimension (capacity, latency, errors, throttles, returned items and bytes, `RequestCount`), and the TTL metrics of a table, now carry the account they were recorded for. The catalog stores the account with each metrics row (migration 10, `account_metrics`), so two accounts that each have a table of the same name keep their own rows: before this release they wrote to one row, and the counts and latencies `/metrics` reported for that name were the two tables' added together. `/metrics` and the console's `/console/metrics-data` report the account as an `AccountId` dimension, beside `TableName`, `GlobalSecondaryIndexName` and `Operation`, and `/metrics` takes an `account_id` query parameter that returns one account's metrics only. Metrics of the whole deployment, such as the connection pool, worker, storage query and backup job metrics, have no `AccountId` dimension, and the `account_id` filter leaves them out. The filter also applies to the latency `segments` of the response. `AccountId` is ExtendDB's own dimension, not one of CloudWatch's DynamoDB dimensions. The console dashboard still serves admin sessions only and shows every account's metrics: its raw metrics table names each row's account, and its charts add the accounts together. +**What changed.** The metrics of a request (capacity, latency, errors, throttles, returned items and bytes, `RequestCount`), and the TTL metrics of a table, now carry the account they were recorded for. The catalog stores the account with each metrics row (migration 10, `account_metrics`), so two accounts that each have a table of the same name keep their own rows: before this release they wrote to one row, and the counts and latencies `/metrics` reported for that name were the two tables' added together. `/metrics` and the console's `/console/metrics-data` report the account as an `AccountId` dimension, beside `TableName`, `GlobalSecondaryIndexName` and `Operation`, and `/metrics` takes an `account_id` query parameter that returns one account's metrics only. Metrics of the whole deployment, such as the connection pool, worker, storage query and backup job metrics, have no `AccountId` dimension, and the `account_id` filter leaves them out. The filter also applies to the latency `segments` of the response. `AccountId` is ExtendDB's own dimension, not one of CloudWatch's DynamoDB dimensions. The console dashboard still serves admin sessions only and shows every account's metrics: its raw metrics table names each row's account, and its charts add the accounts together. **Who is affected.** A reader of `/metrics` that groups rows by their dimensions now gets one row per account where it got one row per table name, and a deployment with one account gets an `AccountId` dimension on every request metric. A reader that sums the rows of a table name across accounts gets the same totals as before for tables whose names no other account uses. diff --git a/tests/test_metrics_accounts.py b/tests/test_metrics_accounts.py index d732febdb..c59ceb8fa 100644 --- a/tests/test_metrics_accounts.py +++ b/tests/test_metrics_accounts.py @@ -14,14 +14,16 @@ DynamoDB action, and a table of the same name with TTL in each. Each account sends one PutItem (one write capacity unit for the first account, three for the second, so a sum of the two is visible), a GetItem, a Query, -a DeleteItem whose condition fails, and a GetItem on a table that does not -exist; the second account also sends a Scan. Each table holds one expired +a DeleteItem whose condition fails, a GetItem on a table that does not +exist, and PutItems to a table provisioned with one write capacity unit +until one is throttled; the second account also sends a Scan. Each table holds one expired item for the TTL sweep. The fixture waits until the flush has persisted every row the tests read, with the PutItem sums at their final values, then reads until two reads in a row are the same (see ``flushed``). Prerequisites: - - extenddb running on EXTENDDB_TEST_ENDPOINT + - extenddb running on EXTENDDB_TEST_ENDPOINT, with the throttling_enabled + setting on, as devtools/run-tests sets it - Admin credentials in EXTENDDB_ADMIN_USER / EXTENDDB_ADMIN_PASSWORD """ @@ -66,6 +68,16 @@ "BillingMode": "PAY_PER_REQUEST", } +# A table that throttles writes: one write capacity unit. +SLOW_KEY = { + "AttributeDefinitions": KEY["AttributeDefinitions"], + "KeySchema": KEY["KeySchema"], + "ProvisionedThroughput": {"ReadCapacityUnits": 1, "WriteCapacityUnits": 1}, +} +# How long PutItems go to that table before one must be throttled: the +# server reads the throttling_enabled setting every 30 seconds. +THROTTLE_DEADLINE_SECONDS = 60 + def _require_env() -> tuple[str, str, str]: endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() @@ -165,6 +177,25 @@ def _account(mgmt: ManagementClient, endpoint: str, label: str): return account_id, boto3.client(**kwargs), user +def _throttle(client, table: str) -> None: + """PutItems to ``table`` until one is throttled.""" + deadline = time.monotonic() + THROTTLE_DEADLINE_SECONDS + i = 0 + while time.monotonic() < deadline: + try: + client.put_item(TableName=table, Item={"pk": {"S": f"w{i}"}}) + except ClientError as e: + code = e.response["Error"]["Code"] + assert code == "ProvisionedThroughputExceededException", code + return + i += 1 + time.sleep(0.02) + pytest.fail( + f"PutItems to a table of one write capacity unit were not throttled " + f"within {THROTTLE_DEADLINE_SECONDS}s; is the throttling_enabled setting on?" + ) + + def _send_requests(client, table: str, padding: int, scan: bool) -> None: """The requests whose metrics the tests read, as one account.""" # The expired item goes in by BatchWriteItem, whose metrics carry no @@ -204,6 +235,7 @@ def _send_requests(client, table: str, padding: int, scan: bool) -> None: with pytest.raises(ClientError) as missing: client.get_item(TableName=f"{table}-missing", Key={"pk": {"S": "1"}}) assert missing.value.response["Error"]["Code"] == "ResourceNotFoundException" + _throttle(client, f"{table}-slow") @pytest.fixture(scope="module") @@ -223,8 +255,10 @@ def flushed(): accounts.append(_account(mgmt, env[0], label)) for _, client, _ in accounts: client.create_table(TableName=table, **KEY) + client.create_table(TableName=f"{table}-slow", **SLOW_KEY) for _, client, _ in accounts: wait_for_active(client, table) + wait_for_active(client, f"{table}-slow") client.update_time_to_live( TableName=table, TimeToLiveSpecification={"Enabled": True, "AttributeName": "expires"}, @@ -245,6 +279,8 @@ def all_flushed(data: dict[str, Any]) -> bool: _per_account(data, "ConditionalCheckFailedRequests", "DeleteItem", table), _per_account(data, "UserErrors", "GetItem", None), _per_account(data, "TimeToLiveDeletedItemCount", None, table), + _per_account(data, "WriteThrottleEvents", "PutItem", f"{table}-slow"), + _per_account(data, "ThrottledRequests", "PutItem", f"{table}-slow"), ] return writes == dict(zip(ids, WRITE_UNITS)) and all( every <= set(p) for p in present @@ -278,10 +314,12 @@ def all_flushed(data: dict[str, Any]) -> bool: finally: for account_id, client, user in accounts: try: - client.delete_table(TableName=table) - wait_for_deleted(client, table) - except client.exceptions.ResourceNotFoundException: - pass + for name in (table, f"{table}-slow"): + try: + client.delete_table(TableName=name) + wait_for_deleted(client, name) + except client.exceptions.ResourceNotFoundException: + pass finally: resp = mgmt.delete_user(account_id, user) assert resp.status_code in (200, 204), resp.text @@ -312,6 +350,13 @@ def test_each_request_metric_names_its_account(flushed, metric, operation, on_ta assert set(ids) <= set(rows), f"{metric} {operation}: {rows}" +@pytest.mark.parametrize("metric", ["WriteThrottleEvents", "ThrottledRequests"]) +def test_each_throttle_metric_names_its_account(flushed, metric): + _, table, ids, data = flushed + rows = _per_account(data, metric, "PutItem", f"{table}-slow", "count") + assert set(ids) <= set(rows), f"{metric}: {rows}" + + def test_latency_counts_each_accounts_requests_apart(flushed): _, table, ids, data = flushed counts = _per_account(data, "SuccessfulRequestLatency", "PutItem", table, "count")