docs(analytics): document transaction queue depth metrics - #573
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the analytics documentation to include two new metrics: read-transaction-queue-depth and write-transaction-queue-depth, along with a detailed explanation of transaction queue depth metrics. Feedback is provided to remove the hyphen from 'write-transaction' in the table description for consistency with other entries.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
Address review feedback on PR #573: fix hyphenation inconsistency, remove the overclaim that write-transaction-queue-depth predicts the write-queue 503 rejection (that check is duration-based on a single outstanding commit, not a function of concurrent commit count), and add the missing caveat that both queue-depth metrics are RocksDB-only and always read zero on LMDB-backed databases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
Add write-transaction-queue-depth and read-transaction-queue-depth to the Resource Usage metrics reference, with a section explaining the depth/maxDepth fields and alerting guidance (alert on the per-period maxDepth peak). Companion to harper#592. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on PR #573: fix hyphenation inconsistency, remove the overclaim that write-transaction-queue-depth predicts the write-queue 503 rejection (that check is duration-based on a single outstanding commit, not a function of concurrent commit count), and add the missing caveat that both queue-depth metrics are RocksDB-only and always read zero on LMDB-backed databases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Independent pre-push review (codex + harper-domain) verified against resources/analytics/write.ts:897-956 that the aggregate hdb_analytics maxDepth is a sum of per-thread period averages, not per-thread peaks as previously documented — the doc's spike-detection advice pointed at the wrong table. Also: add the required v5.2.0 version badge and release-notes entry, soften the write/read depth semantics (settled commit promise != durable under storage.writeAsync; open snapshot count can't distinguish long-lived from many short-lived), and link storage.maxTransactionQueueTime to its documented section instead of a generic options list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ndicator Independent review (codex + grok + harper-domain, unanimous across three lenses) flagged that the doc named the 503 rejection four times while omitting the metric that actually predicts it. Verified against resources/analytics/metadata.ts and DatabaseTransaction.ts: enterWriteQueue/ leaveWriteQueue and the outstandingCommit arm/clear share the same commitResolution promise, so a wedged commit pins write-transaction-queue-depth at >=1 for its whole duration — the opposite of what the prior wording claimed. Add the transaction-commit-time metric row and point to it as the direct duration signal; correct the depth/duration relationship; soften the read-depth "(snapshot)" wording (disableSnapshot reads are counted too); and fix storage-tuning.md's maxTransactionQueueTime description, which read as a queue-length threshold rather than the per-commit duration check it is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t rising Domain review caught an inversion in the prior commit: recordCommitLatency only fires on commitResolution.then(record, record), so a commit that never settles produces zero samples for transaction-commit-time — the opposite of "rising p99/p999 predicts the 503" for that failure mode. Reframe: commit-time is the leading indicator for gradual slowdowns toward the storage.maxTransactionQueueTime bound; write-transaction-queue-depth's depth staying elevated (plus Harper's own stuck-commit log line) is the signal for a genuine wedge. Also fix the transaction-commit-time table row (percentiles are aggregate-only per resources/analytics/write.ts, not present on hdb_raw_analytics) and note that per-thread analytics rows are activity-gated, so a quiet thread emits no row at all rather than an implicit zero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…duplicate wording Verified against resources/Table.ts:2218,4407 (checkOverloaded guarded by !context?.source) and the 503 log text in DatabaseTransaction.ts:403: deletes and canonical-source writes (replication, caching sources) bypass the queue-time check entirely. Note that in storage-tuning.md, and sync configuration/options.md's duplicate one-line description (previously "Max write queue time before 503", which re-introduced the queue-length framing this branch corrects elsewhere) to point at the same section instead of drifting. Also tighten the maxDepth field description: the high-water mark resets on emit, which is activity-gated, not a fixed sampling interval. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
e11f9bf to
91d9fec
Compare
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Re-reviewed at 91d9fecd. I verified the substantive claims against the merged implementation in harper, and the hard ones are right: the "sum of per-thread averages" aggregation semantics (write.ts:942-957), the "no row at all on a quiet thread" caveat (the analytics timer is only armed from inside recordAction(), so a thread with zero recorded actions never reaches the gauge push), and the storage.maxTransactionQueueTime rewrite (both checkOverloaded() call sites are gated on !context?.source; deletes never reach it). Those are the valuable parts of this PR and I'm not asking you to touch them.
The blocker is coordination with #572, not correctness.
Blocker: semantic collision with #572 — and git merges the two cleanly, so nothing warns you. Both PRs document transaction-commit-time in reference/analytics/overview.md. #572 puts it in a new ### Storage Metrics section; this PR adds it to ### Resource Usage Metrics. I ran the trial merge: "Automatic merge went well", zero conflicts. The merged page documents one metric in two tables with contradictory guidance, and 5.2.md ends up with two ## Analytics H2s.
Merge order: #572 first, then this one. #572 should own transaction-commit-time — it built a purpose-built section, its table shape (path/method/type) matches how the metric is actually emitted through recordAction() with no byThread flag, and it also fixes the stale raw-entry example. This PR should keep only the two queue-depth gauges and cross-link. The inline suggestions below do exactly that, and assume #572 has landed so #storage-metrics resolves. I've left the matching note on #572.
The Key attributes cell is a second reason to drop the row rather than relocate it: it lists mean, median, p90, p95, p99, p999, which both omits the raw fields (distribution, count) and silently drops four of the nine percentiles the aggregator actually emits — and it contradicts your own prose four lines down, which correctly says percentiles "aren't present on hdb_raw_analytics".
Release note. Once #572 lands, fold this entry under the ## Analytics heading it creates (after ## Security) instead of adding a second one between ## Configuration and ## CLI. Only one ## Analytics should exist in 5.2.md.
One claim I checked that holds — keep it. "Harper also logs once per stuck commit when the 503 check itself fires" is accurate. The log exists at DatabaseTransaction.ts:394 (v5.2.0) and :485 (origin/main), emitting Rejecting writes on this thread: a commit has been outstanding for ..., and the source comment literally reads "Log once per stuck commit (not once per rejected request, harper#2001)". Flagging this explicitly because an earlier grep of mine against an older ref missed it and I don't want it re-raised. One forward-looking note: on main that log has picked up an episode-level cooldown (OVERLOAD_LOG_MIN_INTERVAL_MS), so "once per stuck commit" weakens slightly next minor — nothing to change here, just worth knowing.
PR body is stale. It says "secondary/optional metric — see that PR for the open question on whether it ships." It shipped: fe58546ec is on harper main, along with unitTests/resources/transactionQueueDepth.test.js. Worth updating so the body doesn't read as speculative docs.
sent with Claude Opus 5
| | `resource-usage` | (see below) | | various | Node.js process resource usage (see [resource-usage](#resource-usage-metric)) | | ||
| | `storage-volume` | `available`, `free`, `size` | `database` | bytes | Storage volume size breakdown | | ||
| | `table-size` | `size` | `database`, `table` | bytes | Table file size | | ||
| | `transaction-commit-time` | `mean`, `median`, `p90`, `p95`, `p99`, `p999` | | ms | Duration from write commit submission to settlement (see [transaction queue depth](#transaction-queue-depth-metrics)) | |
There was a problem hiding this comment.
transaction-commit-time belongs in #572's ### Storage Metrics section, not in this table — it isn't a resource-usage gauge. It flows through recordAction() with no byThread flag, so it aggregates as a cross-thread distribution, unlike every other row here. Dropping the row also resolves the incomplete and self-contradictory attribute list.
| | `transaction-commit-time` | `mean`, `median`, `p90`, `p95`, `p99`, `p999` | | ms | Duration from write commit submission to settlement (see [transaction queue depth](#transaction-queue-depth-metrics)) | |
| `transaction-commit-time` records each commit's submit-to-settle duration on that same clock, and a | ||
| rising `p99`/`p999` (in the `hdb_analytics` aggregate table, where percentiles are computed — they | ||
| aren't present on `hdb_raw_analytics`) is a leading indicator of _gradual_ slowdowns approaching that | ||
| limit. It doesn't help with a single commit that hangs indefinitely, though: the metric only records | ||
| once a commit settles, so a genuinely wedged commit contributes no sample at all, while | ||
| `write-transaction-queue-depth`'s `depth` stays elevated on that thread for as long as the commit | ||
| remains outstanding. Harper also logs once per stuck commit when the 503 check itself fires, which is | ||
| the authoritative signal for that specific failure. |
There was a problem hiding this comment.
With #572 owning the metric, this paragraph should shrink to the part that is genuinely specific to queue depth: the contrast between a settled-only distribution and a gauge that stays elevated while a commit is wedged. That contrast is the most useful thing in this section and it is not in #572, so keep it — just drop the raw/aggregate percentile aside, which #572 covers more completely (and which correctly lists all nine percentiles).
The log sentence stays; I verified it against the source.
| `transaction-commit-time` records each commit's submit-to-settle duration on that same clock, and a | |
| rising `p99`/`p999` (in the `hdb_analytics` aggregate table, where percentiles are computed — they | |
| aren't present on `hdb_raw_analytics`) is a leading indicator of _gradual_ slowdowns approaching that | |
| limit. It doesn't help with a single commit that hangs indefinitely, though: the metric only records | |
| once a commit settles, so a genuinely wedged commit contributes no sample at all, while | |
| `write-transaction-queue-depth`'s `depth` stays elevated on that thread for as long as the commit | |
| remains outstanding. Harper also logs once per stuck commit when the 503 check itself fires, which is | |
| the authoritative signal for that specific failure. | |
| [`transaction-commit-time`](#storage-metrics) records each commit's submit-to-settle duration on that | |
| same clock, and a rising `p99`/`p999` is a leading indicator of _gradual_ slowdowns approaching that | |
| limit. It doesn't help with a single commit that hangs indefinitely, though: the metric only records | |
| once a commit settles, so a genuinely wedged commit contributes no sample at all, while | |
| `write-transaction-queue-depth`'s `depth` stays elevated on that thread for as long as the commit | |
| remains outstanding. Harper also logs once per stuck commit when the 503 check itself fires, which is | |
| the authoritative signal for that specific failure. |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-573 This preview will update automatically when you push new commits. |
|
Reviewed — |
3 participants
Summary
Documents the v5.2
write-transaction-queue-depthandread-transaction-queue-depthmetrics, including their per-thread sampling and aggregation caveats, and corrects thestorage.maxTransactionQueueTimedescription.This PR must follow #572, "docs(analytics): document transaction-commit-time metric". After #572 lands, the remaining review threads require removing this branch's duplicate
transaction-commit-timerow and prose, cross-linking the Storage Metrics section, and folding the queue-depth release note into #572's Analytics section.For the human reviewer
storage.maxTransactionQueueTimecorrection needs a changed-version badge, and whether that correction should remain bundled here. These remain for human review after the coordination blocker clears.Verification
npm run build- passed; it reports the pre-existing broken#deployment-operationsanchor in the v5.1 release notes as a warning.npm run format:check- passed.npm run typecheck- passed.npm run lint- passed (the repository script is a no-op).Review coverage
Original change authored by Claude; current review fixes authored by GPT-5 Codex. Full review: Claude Opus 5 and Harper-domain adjudication succeeded; Gemini produced no output. Final delta review: Claude Opus 5 succeeded, Gemini via agy returned no findings, and the domain pass was pruned as a narrow low-risk delta. Cursor legs were disabled.
Implements HarperFast/harper#592, "Metrics on in-flight transactions" through the shipped HarperFast/harper#1689, "feat(analytics): expose write/read transaction queue depth (count) metrics".
Updated by GPT-5 Codex.
Human-Review-Need: 4 @ a7a4070