feat(analytics): record write-transaction commit latency#1688
Open
kriszyp wants to merge 2 commits into
Open
Conversation
Emit transaction-commit-time as a distribution metric (mean + percentiles via the standard analytics action path) capturing each write commit's submit->settle duration. This is the same clock the overload check compares against, so a rising p99/p999 is the leading indicator for the 'Outstanding write transactions have too long of queue' (503) rejection — and unlike an in-flight count it reflects transaction size/duration and the shared storage-engine write path, not just per-thread submission concurrency. Recorded per commit attempt (retries re-arm the outstanding-commit clock, so per-attempt timing matches the overload semantics). Async commit path only; synchronous commits do not queue. Closes #592 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 7, 2026
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
Reviewed; no blockers found. |
…ni hardening - Skip the commit-recording assertion under HARPER_STORAGE_ENGINE=lmdb: LMDB writes route through the separate LMDBTransaction.commit() override (resources/LMDBTransaction.ts), which does not call the RocksDB-path latency hook — matching the existing RocksDB-only carve-outs in transaction.test.js. This is what failed CI (all 3 Node versions, via the npm run test:unit:all -> test:unit:lmdb pass). - setCommitLatencyRecorder now types the recorder as optional, matching actual call sites (tests unregister it with undefined). - recordCommitLatency guards commitResolution as thenable before calling .then(), per Gemini review feedback -- defensive against a future caller passing a non-Promise value, even though today's only caller (rocksdb-js's async Transaction.commit()) always returns a real Promise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
transaction-commit-time, a distribution metric (mean + p90/p95/p99/p999 via the standard analytics action path) recording each write transaction's submit→settle duration.Purpose
#592 asks for a leading indicator before write-heavy workloads hit
Outstanding write transactions have too long of queue, please try again later(503). That rejection fires when the oldest outstanding commit has been outstanding longer thanstorage.maxTransactionQueueTime(45s default) — a latency condition, not a count.transaction-commit-timeis measured on that exact clock (outstandingCommitStart→ settle), so a rising p99/p999 is the direct leading indicator: it reflects transaction size/duration and the shared storage-engine write path, not just per-thread submission concurrency.Recorded per commit attempt — a transient-conflict retry rejects and re-issues
commit(), and the overload check's ownoutstandingCommitre-arms per attempt the same way, so this matches the failure semantics exactly.Design note (dependency inversion)
resources/DatabaseTransaction.tsdoes not import the analytics/server modules. It exposessetCommitLatencyRecorder(recorder);resources/analytics/write.tsregisters the recorder at load. This mirrors the existingreplicationConfirmationcallback-registration pattern in the same file and avoids a circular import between the storage layer and the analytics/server layer. The recorder is wrapped so a metrics failure can never throw into the floating.then()or disturb the commit path.Attention
.then(), a circular-import risk (DatabaseTransaction→Server), and a test binding issue. All three are resolved by the registration-based redesign above (verified by re-running build/lint/tests); no open concerns remain from that review.Generated by Claude Opus 4.8.