Skip to content

feat(metrics): profile local sqlite storage - #5612

Open
NathanFlurry wants to merge 8 commits into
mainfrom
sql-metrics
Open

feat(metrics): profile local sqlite storage#5612
NathanFlurry wants to merge 8 commits into
mainfrom
sql-metrics

Conversation

@NathanFlurry

Copy link
Copy Markdown
Member
  • Add bounded local SQLite statement, transaction, VFS I/O, and amplification profiling.
  • Add named transaction and profiling configuration APIs across Rust, TypeScript, NAPI, and wasm.
  • Verify native VFS-to-Prometheus reporting, fingerprint admission, disabled behavior, and API parity.

@railway-app

railway-app Bot commented Aug 28, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5612 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink 😴 Sleeping (View Logs) Web Aug 28, 2026 at 8:55 am
frontend-cloud 😴 Sleeping (View Logs) Web Aug 28, 2026 at 8:52 am
frontend-inspector 😴 Sleeping (View Logs) Web Aug 28, 2026 at 8:50 am
website ❌ Build Failed (View Logs) Web Aug 28, 2026 at 8:42 am
ladle ✅ Success (View Logs) Web Aug 28, 2026 at 12:26 am
mcp-hub ✅ Success (View Logs) Web Aug 28, 2026 at 12:24 am

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review: feat(metrics): profile local sqlite storage

This is a large, well-tested addition (bounded SQLite statement/transaction/VFS profiling, wired through Rust core -> NAPI -> wasm -> TS). Overall the design is careful about the things CLAUDE.md calls out most (bounded cardinality via admission control, scc::HashMap/HashSet instead of Mutex<HashMap>, structured tracing fields, saturating arithmetic everywhere, parking_lot justified by the sync VFS-callback context). Two things stood out that are worth a look before merge, plus a couple of smaller notes.

Findings

1. Fingerprint/series admission budget is process-global, not per-actor (rivetkit-rust/packages/rivetkit-core/src/actor/metrics.rs)

SQLITE_PROFILE_ADMISSION (the statements/transactions scc::HashSets and the series: AtomicUsize budget) is a single process-wide LazyLock static. The actual metric handles (Prometheus series) are correctly keyed per actor_name via the tuple/candidate_key strings, but the admission counters that gate whether a new fingerprint/series is allowed at all are shared across every actor instance and every actor kind in the process, while the caps checked against those shared counters (max_tracked_statement_fingerprints, max_tracked_transaction_fingerprints, max_prometheus_series) are read from each call's own self.inner.sqlite_profiling (i.e. that particular actor's config).

Concretely:

  • select_sqlite_fingerprint checks set.contains_sync(fingerprint) against the global SQLITE_PROFILE_ADMISSION.statements/.transactions, and count.fetch_update(...) against the global statement_count/transaction_count, using cap sourced from the calling actor's local config.
  • reserve_sqlite_series does the same against the global series counter using self.inner.sqlite_profiling.max_prometheus_series.

If two actor kinds in the same process configure different sqlite_profiling limits (or even just run concurrently with the same default limits), a busy/high-cardinality actor kind can exhaust the shared fingerprint/series budget, silently pushing an unrelated, low-traffic actor kind's SQL into the other bucket even though that actor kind is well under its own configured limit. Enforcement also becomes order-dependent when limits differ, since whichever actor's config happens to run the fetch_update first is what gets enforced against the shared counter.

Given metrics labels are already namespaced by actor_name (per the metrics cardinality rules in CLAUDE.md), the natural expectation is that these caps bound cardinality per actor kind. If a process-wide ceiling is intentional (e.g. to protect the Prometheus registry as a whole regardless of actor mix), that's reasonable, but it should probably be a separate, explicitly-global knob rather than silently reusing each actor's local config value against shared state, since as written it's easy to misconfigure without any signal other than more things landing in other.

2. Substantial duplicated profiling boilerplate that's easy to let drift

  • engine/packages/depot-client/src/query.rs: execute_single_statement_profiled/exec_statements_profiled are near-total copies of execute_single_statement/exec_statements (~90 and ~110 lines respectively) with profiling calls interleaved. A future fix to the SQLite statement-stepping/error-handling logic in one twin can easily be missed in the other.
  • rivetkit-rust/packages/rivetkit-core/src/actor/sqlite/mod.rs: exec/query/run/execute each repeat the same ~25-line #[cfg(feature = "sqlite-local")] let started_at = ... / transaction_wait / branch-on-started_at.is_some() / observe_statement_profile(...) sequence.
  • rivetkit-rust/packages/rivetkit-core/src/actor/sqlite/tx.rs: the same pattern repeats again for transaction_exec_inner, transaction_execute_inner, transaction begin, and commit/rollback.

None of this is wrong, but it's a lot of copy-pasted control flow (not "three similar lines") gating on the same started_at/profiled/fallback shape each time. A shared helper (something like a closure-based "run + optionally profile" wrapper) would cut the duplication substantially and remove the risk of one call site's profiled/non-profiled branches silently diverging (e.g. missing a transaction_mode argument, or forgetting to call observe_statement_profile on one path).

Smaller notes (not blocking)

  • rivetkit-rust/packages/rivetkit-core/src/actor/sqlite/profiling.rs: statement fingerprints hash the exact SQL text (not normalized), so any code that inlines literals into SQL instead of using bind params will fragment across many fingerprints. This is intentional and now correctly documented (docs/content/docs/sqlite-profiling.mdx "Too many fingerprints" section, plus the fix(docs): describe normalized SQL fingerprints commit), just flagging it's a real footgun for anyone who doesn't read that section since the failure mode is silent (everything falls into other).
  • Docs also note "Prometheus series remain allocated for the life of the process after admission", which combined with finding [SVC-2555] Set up issue templates #1 means the shared budget only grows, never shrinks (aside from the reservation-rollback-on-failure path), for the lifetime of the process. Worth confirming that's the intended long-run behavior for long-lived worker processes hosting many distinct actor kinds (e.g. preview/ephemeral namespaces).
  • rivetkit-rust/packages/rivetkit/src/sqlite.rs (SqliteDbExt::transaction): on commit failure it calls transaction.rollback().await after the commit already failed, and discards the rollback result. That mirrors the existing TS db.transaction() semantics, so likely fine, just double-checking it's deliberate given commit failures (e.g. connection loss) may make the follow-up rollback a guaranteed no-op/error.

Positives

  • Good end-to-end test coverage: tests/metrics.rs includes a real native-stack test (sqlite_operations_report_profiles_through_the_native_stack) that exercises statements, a named transaction, and a cold reopen through an in-memory SqliteTransport, not just unit-level assertions.
  • Config validation (ActorConfig::validate) correctly bounds baseline_sample_rate to [0,1], max_get_pages_requests_per_trace to the fixed array size, etc., and this is exercised in tests/config.rs.
  • The SqliteVfsMetrics trait's no-op default methods were removed in favor of required methods (matching the "require explicit metrics hooks" refactor commit); both implementors (ActorMetrics, and the test-only WorkerTestMetrics) were updated in the same change, so nothing is silently left as a no-op.
  • Saturating arithmetic is used consistently throughout the new counters, and the VFS operation-profile guard (SqliteOperationProfileGuard) correctly finishes-or-drops so a profile is never leaked across operations.
  • Config plumbing (Rust core -> NAPI -> wasm -> TS) is consistent and mirrored correctly across all four layers, matching the RivetKit layer-architecture rules in CLAUDE.md.

@NathanFlurry

Copy link
Copy Markdown
Member Author

Review follow-up:

  • Fixed the oversized-SQL panic by bounding the transaction trace copy and added a regression test.
  • Removed the TypeScript hardcoded 128-byte limit; the configured core limit is now authoritative.
  • Added an atomic fast-path guard so disabled profiling skips the VFS profile mutex.
  • Kept remote/wasm profiling as a no-op because the implementation spec explicitly scopes profiling to local-native SQLite; profiling is enabled by default, so rejecting remote actors would be a breaking change.
  • Kept distinct profiled/unprofiled traversal paths intentionally to meet the spec requirement that disabled profiling perform no per-bind or per-result profiling branch.
  • Kept tests inline in the new profiling module; the source-shim rule applies when moving existing inline tests out of src.

Copy link
Copy Markdown
Member Author

@codex

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4a75ea36e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

"fingerprint_source",
"transaction_mode",
"storage_transport",
"outcome_class",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use result for operation outcome labels

Rename this label, and the new outcome label on rivetkit_sqlite_outcome_total, to the repository-standard result. As written, the new SQLite operation metrics will not compose with dashboards and alerts that consistently group operation success/failure by result, requiring one-off queries for this subsystem.

AGENTS.md reference: AGENTS.md:L306-L310

Useful? React with 👍 / 👎.

Comment on lines +605 to +612
.buckets(vec![
512.0,
4096.0,
16_384.0,
65_536.0,
262_144.0,
1_048_576.0,
]),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Move the response-size buckets into the shared constants

Replace this newly invented bucket vector with an existing constant from engine/packages/metrics/src/buckets.rs, or add an appropriate shared constant there. Keeping a private bucket scheme here makes histogram aggregation and dashboard thresholds inconsistent with the rest of the repository.

AGENTS.md reference: AGENTS.md:L311-L313

Useful? React with 👍 / 👎.

Comment on lines +1887 to +1889
if set.contains_sync(fingerprint) {
return (fingerprint.to_owned(), false);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce fingerprint caps per profiling configuration

When another actor type has already admitted the same fingerprint into this process-global set, this early return bypasses the current actor's maxTrackedStatementFingerprints or maxTrackedTransactionFingerprints. For example, after actor A admits a query, actor B configured with a cap of zero still creates fingerprint-specific series for it; differing actor configurations therefore behave according to startup history rather than their own limits. Scope admission membership by actor/configuration (as the candidate and tuple keys already partly do), or apply a single explicitly process-global cap.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant