[hitesh96db]

What

Metrics are kept per account. Two accounts that each have a table of the same name now get their own metrics rows.

• Collector and flush. MetricKey, FlushBucket and MetricsRow carry the account a point was recorded for. MetricsCollector::record takes an account_id, and the table-scoped record_* methods take one too (latency, capacity, the error metrics, throttles, returned items and bytes, request count, segments, and the TTL metrics). The request handler passes the account the request was authenticated for, and each backend's TTL worker passes the table's account. Metrics of the whole deployment (pool, worker, storage query, backup and restore jobs, TtlRepairMarkerCount) have no account.

• Migration 10, account_metrics, on every backend. It creates account_metrics, whose key is (bucket, account_id, metric, table_name, index_name, operation), and drops metrics with its rows. account_id is the empty string on a metric of the whole deployment on SQLite, PostgreSQL and Cassandra; on MongoDB such a document has no account_id field.

  • SQLite, PostgreSQL and Cassandra: a file body, CREATE TABLE IF NOT EXISTS account_metrics, then DROP TABLE IF EXISTS metrics. The primary key starts with bucket, so its index serves the range reads and the prune.
  • MongoDB: a code body that creates the collection with a {bucket: 1} index and drops metrics when the catalog lists it.

  The table is new rather than altered: a Cassandra partition key cannot be changed and a table cannot be renamed, and SQLite cannot alter a primary key. A new name also keeps the bodies safe to run again: on a catalog that has the migration, both statements do nothing. The old rows are dropped, not copied, because they carry no account, and the server keeps metrics for a day. The migration is Kind::Breaking and convergent: true. The registry entry says that no later migration may create a table named metrics: --reconverge runs migration 1, which creates it, and then this drop again.

• MetricsStore. query_metrics(start, end, &MetricsFilter) replaces the two positional filters. The filter has an account, a table name and a metric, and an empty string reads as no filter. With an account set, the store returns that account's rows only, never a row of the whole deployment. All four stores read and write account_metrics.

• /metrics. Each row has an AccountId dimension, beside TableName, GlobalSecondaryIndexName and Operation, and aggregate_rows keys on the account, so two accounts' same-named tables are two rows in the snapshots and in every bucket. The account_id query parameter returns one account's rows. The filter also applies to the latency segments. /console/metrics-data returns the same payload, so it carries AccountId too.

• Console. The dashboard is still admin-only. Its raw metrics table has an Account column, and its charts add the accounts together.

• Docs.

  • The upgrade manual: migration 10 in Current Status and a behaviour-change entry.
  • The component design doc's metrics section, the storage-extension manual's MetricsStore table, and a line of the security model.
  • docs/schema/*.txt, and the "catalog is at migration 10" sample output lines.
  • CONTRIBUTING's worked example, renumbered from 10 to 11.

This is the second part of #728. #922 was the first.

Why

Part of #728.

The metrics table had no account column on any backend, so two accounts with a table of the same name wrote to one row. The counts and latencies an admin read for that name were the two tables' added together.

To reproduce on main (9c3fbfb):

1. Create two accounts, each with a user that may call dynamodb:*.
2. Create a table of the same name in each.
3. PutItem one item in the first and three in the second.
4. After the flush, /metrics?window=LastHour has one ConsumedWriteCapacityUnits PutItem row for that name, summing 4.

Testing done

• extenddb_storage::metrics_conformance (new, feature test-util). It writes rows for two accounts with a table of the same name, plus a row of the whole deployment, then checks:

  • the two accounts' rows read back as two rows with their own sums;
  • the account filter returns one account's rows and no deployment row;
  • the table filter alone and the metric filter alone each leave out a row the other keeps, and all three filters work together;
  • an unknown account reads nothing, and empty filters read as none;
  • two inserts of one key read back as their sum, and the prune deletes a row older than the retention (both skipped on Cassandra, which writes a row of the same key over the previous one and expires rows by TTL).

  Each backend runs it against a migrated catalog:

  • SQLite: migrations::released_tests::the_metrics_store_keeps_two_accounts_apart
  • PostgreSQL: tests/metrics_store.rs
  • MongoDB: tests/metrics_store.rs, through MongoCatalogStore::on_database_for_tests
  • Cassandra: tests/direct/metrics_store.rs

  On SQLite the check fails with the account filter removed, the table filter removed, the metric filter removed, the upsert setting the sum instead of adding to it, or the prune deleting nothing.

• Migration 10. On SQLite, PostgreSQL and MongoDB, a catalog with metrics rows migrates to an empty account_metrics without metrics, and a second run of the body keeps the rows written since. On MongoDB the second run is through --reconverge, after migration 1 recreated metrics.

• Existing migration tests, updated for the dropped table and the new one:

  • SQLite: DROPPED_TABLES.
  • MongoDB: expected_catalog() plus a 0010_account_metrics.expected.json fixture, and the step lists in migrations_fresh, migrations_locks and migrations_released.
  • Cassandra: expected_definitions() plus a fixture, and keyspace_migrations' CATALOG.tables and DROPPED.
  • tests/test_resource_tag_migration.py: _as_released_catalog removes account_metrics.

• Unit tests.

  • Collector: two accounts kept apart, the account filter, an empty account filter, each of the 14 per-account record_* methods keeping its account, and the latency segments filtered by account.
  • Drain: buckets carry their account. The drain property test now uses keys of two accounts.
  • aggregate_rows: two accounts with the same table name stay two groups, and AccountId is retained.
  • record_error_metrics: UserErrors carries the account and no table name; SystemErrors, ConditionalCheckFailedRequests and TransactionConflict carry the account.
  • Flush worker: the persisted row keeps its account.
  • Console: the admin feed names each row's account, the 403 body names none, and the raw metrics table has an Account column.

• tests/test_metrics_accounts.py (new, slow): two accounts, the same table name with TTL on, one expired item in each. Each account sends one PutItem (1 write capacity unit in the first, 3 in the second), a GetItem, a Query, a DeleteItem whose condition fails and a GetItem on a missing table; the second also sends a Scan. Once the flush has written every row read below, with the PutItem sums at their final values, and two reads 5 s apart agree:

  • the PutItem write capacity under that name is {first: 1.0, second: 3.0};
  • read capacity, returned items and bytes, failed conditions, UserErrors, RequestCount and the PutItem latency count each have a row for each account;
  • WriteThrottleEvents and ThrottledRequests have a row for each account, under a table of one write capacity unit that each account writes to until a PutItem is throttled;
  • TimeToLiveDeletedItemCount is 1 for each account;
  • account_id=<second> returns only rows whose AccountId is the second account;
  • the segments filtered by the first account have PutItem and no Scan, and those of the second have both.

  Against rebuilt servers on fresh databases, the fixture fails with the account dropped at flush ({None: 4.0}), with the handler recording read capacity, a throttle event or a throttled request without the account, and with the SQLite TTL worker recording deletions without it.

• Against a SQLite release server built from this branch, on a fresh database: test_metrics_accounts.py, test_metrics_table_names.py, test_metrics.py, test_console_metrics_admin_only.py, test_cross_account_isolation.py and test_resource_tag_migration.py pass.

• cargo test --locked for extenddb-core, -storage, -server, -storage-sqlite, -app and extenddb passes, as do extenddb-storage-cassandra --lib and extenddb-storage-mongodb --lib. The one MongoDB library failure is a_standalone_server_is_unsupported, which needs EXTENDDB_TEST_MONGODB_STANDALONE_URI, not set here.

• PostgreSQL tests/metrics_store.rs and migrations_{crash,fresh,golden,guards,owners,released,runs,serving,tag_blobs} pass against a local server. The exception is migrations_guards::a_role_that_does_not_own_the_tables_is_told_what_to_run, which also fails locally on main: the local superuser owns the tables.

• MongoDB tests/metrics_store.rs and migrations_{fresh,guards,locks,released} pass against a local replica set.

• Cassandra: --lib and compile checks only. The migrations, migrations_stopped and direct_integration suites, including the new test and the updated schema lists, run first in CI.

• Schema docs. docs/schema/sqlite.txt and docs/schema/postgres.txt are devtools/schema-dump of fresh installs from this branch, and match exactly. mongodb.txt and cassandra.txt are edited by hand, because their dumps need Docker; the integration jobs compare them.

• Lint. cargo fmt --all -- --check is clean, and both CI clippy commands on 1.97.0 are clean with -D warnings. Locally they also needed -A unused-variables, for the existing line_len in crates/core/tests/data_file_decode_peak.rs, which is used on Linux only. cargo +1.88.0 check --workspace --all-targets passes.

Checklist

• I have read CONTRIBUTING.md
• All tests pass (cargo test --workspace)
• Code is formatted (cargo fmt --check)
• Clippy is clean (cargo clippy -- -W clippy::pedantic)
• I have added or updated tests for new functionality
• I have updated documentation if behavior changed
• Breaking changes are noted below (if any)
• If this changes the wire protocol, Storage trait, auth model, on-disk
  format, or public CLI surface, an RFC has been accepted or is linked
  below. Otherwise, an ADR captures the decision (link below).

cargo test ran for the crates listed above, not the whole workspace. Clippy ran with -D warnings, as CI does, without the pedantic group.

ADR / RFC: the catalog change is a numbered migration under ADR-0023. MetricsStore::query_metrics changes signature: the trait is internal to the workspace, and every implementation is in this PR.

Migration number: #934, #833 and #823 also add migration 10. Whichever merges later renumbers.

Overlap with #923: MongoCatalogStore::on_database_for_tests is added with the same text as in #923.

Breaking changes

• Catalog migration 10 is required. serve refuses a catalog at migration 9. A build that reads the migration ledger and lacks migration 10 refuses a catalog at migration 10 (exit 4); a 0.1.x release must not be run against a migrated catalog at all. Rolling back means restoring the backup taken before migrate --yes. Stop every server of an earlier release before migrating: such a server writes to metrics, which no longer exists.
• Metrics recorded before the upgrade are dropped.
• /metrics and /console/metrics-data:
  • every request metric has an AccountId dimension, so a reader that groups rows by dimensions gets one row per account where it got one per table name;
  • metrics of the whole deployment have none;
  • /metrics takes account_id.
• MetricsStore::query_metrics takes a &MetricsFilter, and MetricsCollector::record and the table-scoped record_* methods take an account.

---

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

[LeeroyHannigan]

I read the change against main at 9c3fbfb. I ran nothing, so each point below comes from reading the code. CI passes apart from CodeQL, which fails on the new alert at metrics_pages_tests.rs:182.

After this change the feed returns one row per account. The console's Raw Metrics table (metrics_content.rs:109 and :180) shows only Metric, Table and Operation. Two accounts' rows for Orders therefore show as two rows with the same labels and different sums. The charts sum the rows of each timestamp, which adds the accounts together, and the table prints each row as it arrives. Please add an Account column from the AccountId dimension, or sum the rows of one table name as the charts do.

docs/manuals/07-upgrade-manual.md:983, in the v0.1.15 entry on the admin-only dashboard, still says "The metrics table has no account column". The new v0.1.15 entry at line 369 says the opposite. Please put that sentence in the past tense, for example "Before migration 10 the metrics table had no account column".

[LeeroyHannigan]

/metrics?account_id=X reads only X's rows from the store. render_metrics then sets segments from query_segments(window), which takes no account, and the console feed at metrics_pages.rs:143 does the same. The filtered response carries the request count and the average auth, authz, throttle, dispatch and response latencies per operation of every account. The upgrade manual at line 369 says the parameter "returns one account's metrics only". The design doc (06-component-server.md:315) says "only the metrics of this account". record_segments sets this field, and nothing reads it. Please filter the segment points by account in query_segments, with a test that records segments for two accounts and reads one. test_the_account_filter_returns_that_accounts_rows_only could check segments too. If segments should stay deployment-wide, please say so in both docs and drop this field.

[hitesh96db]

Fixed in 3ec94e1. query_segments takes the account and keeps that account's points only; /metrics and /console/metrics-data pass account_id to it, and an empty one is no filter. segments_are_filtered_by_account records segments for two accounts and reads each, and test_the_account_filter_applies_to_the_latency_segments checks that the first account's segments have no Scan, which only the second account sent. With the filter removed from query_segments, the unit test fails.

[LeeroyHannigan]

This read still passes if a store drops its table filter or its metric filter. The second account has one row, for Orders and ConsumedWriteCapacityUnits, and those two filters keep it anyway. This PR rewrote the table and metric filters in all four stores, and this module is the only store-level test of them. Please give the second account a row under another table name and a row under another metric, so this read has rows to exclude. A read with only table_name and one with only metric would test each filter alone.

[hitesh96db]

Fixed in 5a44c5c. The second account also has a row under Other and a row under UserErrors, and the check reads with the table filter alone and the metric filter alone. With either filter removed from the SQLite query, the check fails on that read.

[LeeroyHannigan]

Only PutItem's write capacity has a test of the account the handler records, in test_metrics_accounts.py. By my reading, no test fails if this line passes None, or if the handler passes "" to record_latency, record_request_count or record_error_metrics. Nothing in the tests calls handle_request. The TTL metrics have no test of their account, and record_ttl_deletion(table_name, table_name) compiles because both parameters are &str. Please add a test that sends requests for two accounts through the router and checks the AccountId of each metric the handler records. Please also add a TTL test on one backend that checks the account of TimeToLiveDeletedItemCount.

[hitesh96db]

In 3ec94e1 test_metrics_accounts.py sends, as each account, a PutItem, a GetItem, a Query, a DeleteItem whose condition fails and a GetItem on a missing table, and puts an expired item for the TTL sweep. It checks the AccountId of read and write capacity, returned items and bytes, failed conditions, UserErrors, RequestCount, the latency count and TimeToLiveDeletedItemCount. The wire suites run it on all four backends. Against a rebuilt SQLite server, it fails with this line passing None, and with the SQLite TTL worker passing "" to record_ttl_deletion. every_scoped_recorder_keeps_its_account covers each of the 14 per-account recorders, and the record_error_metrics tests now check the account on SystemErrors, failed conditions and TransactionConflict. I did not add a router-level Rust test, and the throttle site has no test.

[LeeroyHannigan]

On MongoDB a deployment metric has no account_id field, because management_store.rs:2166 inserts it only for Some. "Stored as the empty string" holds on SQLite, PostgreSQL and Cassandra. Please say that the field is absent on MongoDB, here and in the PR body.

[hitesh96db]

Fixed in 5a44c5c: the comment says that the field is empty on SQLite, PostgreSQL and Cassandra and absent on MongoDB. The PR body says the same.

[LeeroyHannigan]

The primary key starts with bucket, so its index already serves the bucket >= ? AND bucket <= ? read and the prune's bucket < ?. This index is a second B-tree that each flush also writes. The PostgreSQL body has the same index at line 26. The table is new, so dropping the index now needs no later migration. Please drop it from both bodies and the schema dumps, or say in the comment which query needs it.

[hitesh96db]

Removed in 5a44c5c from the SQLite and PostgreSQL bodies and from both schema docs. The bodies' comments say that the primary key's index serves the reads and the prune. docs/schema/sqlite.txt matches a fresh install from 5a44c5c.

[LeeroyHannigan]

CodeQL fails on this message (alert 219, rust/cleartext-logging) because it prints account ids. The ids are the made-up values in TABLES, so the message logs nothing sensitive. Please drop {accounts:?} from the message, so main does not carry the alert after the merge.

[hitesh96db]

Fixed in 3ec94e1: the message no longer prints the ids. The two other alerts on this PR, in the SQLite migration test, are fixed the same commit: the test inserts with a literal statement.

[hitesh96db]

3ec94e1 and 5a44c5c address the review. The Raw Metrics table has an Account column, read from the AccountId dimension, and admin_gets_the_metrics_page checks it. The v0.1.15 entry at docs/manuals/07-upgrade-manual.md:983 now reads "The metrics table had no account column then". The inline threads have the other replies.

[LeeroyHannigan]

I checked the round-1 items against 3ec94e1 and 5a44c5c, by reading the code this round. The two commits fix seven of the eight. For the handler comment, each metric the handler records with an account now has a test, except the three throttle metrics (handler.rs:234). CodeQL shows no open alert on this PR now. The other comment is on text the new commits rewrote.

[LeeroyHannigan]

ReadThrottleEvents, WriteThrottleEvents and ThrottledRequests still have no account check. Line 369 of the upgrade manual now lists throttles among the metrics that carry the account. No test fails if this line or line 242 passes None. tests/rust/src/capacity_throttling.rs already throttles a table provisioned with 1 write capacity unit by sending 50 PutItems. Please give each account in test_metrics_accounts.py such a table, write to it until the first ProvisionedThroughputExceededException, and check the AccountId of WriteThrottleEvents and ThrottledRequests.

[hitesh96db]

Fixed in 4d8eb8d. Each account gets a table provisioned with one write capacity unit and sends PutItems to it until one answers ProvisionedThroughputExceededException; test_each_throttle_metric_names_its_account checks that WriteThrottleEvents and ThrottledRequests under that table have a row for each account. Against a rebuilt SQLite server with throttling_enabled on, the fixture fails with line 234 passing None, and again with line 242 passing None. ReadThrottleEvents comes from the same record call as WriteThrottleEvents, and the test sends no read that throttles.

[LeeroyHannigan]

The handler records StorageQueryCount and StorageQueryLatency for each request, with the query category as their Operation dimension, and they carry no account. The rule at the start of this paragraph, "metrics of a request that have a table or an operation dimension", includes them. A later sentence lists them among the deployment's metrics. Please drop "that have a table or an operation dimension", so the list in parentheses names the metrics that carry the account.

[hitesh96db]

Fixed in 4d8eb8d: the sentence now reads "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".

[LeeroyHannigan]

I checked 4d8eb8d against the two comments. Each account now sends PutItems to a table with one write capacity unit until the server throttles one. test_each_throttle_metric_names_its_account then checks the account of WriteThrottleEvents and ThrottledRequests, and ReadThrottleEvents comes from the same record call at handler.rs:231. The test passes in the wire jobs of all four backends. The upgrade manual sentence now lists the metrics without the rule.