docs(analytics): document transaction-commit-time metric - #572
Conversation
Add a Storage Metrics section documenting transaction-commit-time (write commit submit->durable duration distribution) and how its upper percentiles serve as the leading indicator for the storage overload (503) rejection. Companion to harper#592. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-572 This preview will update automatically when you push new commits. |
![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 adds documentation for a new transaction-commit-time storage metric in reference/analytics/overview.md, detailing its distribution and its relationship with the overload guard configuration. The feedback suggests correcting a grammatical typo in the documented error message and adding backticks around a configuration parameter for markdown consistency.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-572 This preview will update automatically when you push new commits. |
- Correct transaction-commit-time description: the recorder fires on both fulfilment and rejection (submit -> settle), not only on durable commit — a failed/retried commit still consumes queue time and should not be undercounted. - Restore the storage. prefix on maxTransactionQueueTime for consistency with its earlier mention. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…tle-only Cross-model review (codex) flagged the original framing as overstating the metric's reliability as a leading indicator: it is RocksDB-only, records one sample per commit attempt (not per logical transaction — conflict retries each get their own sample), and only records once an attempt settles, so a wedged commit contributes nothing while still tripping the 503 guard. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…ding - median/p90/p95/p99/p999 are only present on the per-minute aggregate (hdb_analytics); raw per-second entries only carry mean/distribution/count. - The overload guard tracks a commit it happened to arm on, not reliably the oldest outstanding one (retries/chained commits can wedge unarmed). - ASCII "to" instead of the arrow glyph per the content style guide. Co-Authored-By: Claude Opus <noreply@anthropic.com>
… signal - Add a transaction-commit-time entry to the v5.2 release notes so it's discoverable from the changelog. - Replace the vague "queue-depth or stuck-commit observability" pointer (no such metric is documented yet) with the actual overload-guard log line operators can watch today. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…cy-592 # Conflicts: # release-notes/v5-lincoln/5.2.md
Cross-model review (codex + grok + harper-domain adjudication) found the initial framing overstated this metric's reliability as a leading indicator. Verified each claim against harper's DatabaseTransaction.ts and analytics/write.ts before applying: - RocksDB-only; never emitted for LMDB-backed databases. - One sample per commit attempt, not per logical transaction (conflict retries each record their own sample). - Only records once an attempt settles — a still-outstanding commit contributes nothing. - Raw entries (hdb_raw_analytics) carry mean/distribution/count only; percentiles are computed on the per-minute aggregate (hdb_analytics). Fixed the page's own raw-entry JSON example, which showed percentile fields that don't exist on raw rows. - The overload guard rejects record updates and publishes only — deletes and canonical-source writes (replication, caching sources) bypass it. - The guard tracks at most one outstanding commit per thread: a retry issued while the prior attempt still holds that slot is never armed (so a wedge there won't trip the 503); a later backoff-delayed retry re-arms fresh. - Added a v5.2 release-note entry and VersionBadge. Co-Authored-By: Claude Opus <noreply@anthropic.com>
recordCommitLatency wraps the async transaction.commit() path only; replayLogs.ts's directCommitSync() calls commitSync() directly and isn't instrumented, so replay/startup commits emit no sample either. Co-Authored-By: Claude Opus <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-572 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.
Reviewed against the harper source at v5.2.0 (c11e0976) and origin/main. The metric name, emission path, units, the raw/aggregate field split, the 503 error string, and the bypass rules all check out — I verified each rather than assuming. Four things to fix, plus one sequencing decision shared with #573.
Sequencing with #573 — read this first. #573 documents the same transaction-commit-time metric in the same file, in the Resource Usage Metrics table, with a contradictory attribute list (mean, median, p90, p95, p99, p999 — i.e. exactly the raw/aggregate confusion this PR correctly fixes). I ran the trial merge: git merges the two cleanly, with zero conflicts, so nothing warns you. The merged page documents the metric in two different tables with contradictory guidance, and 5.2.md ends up with two ## Analytics H2s.
Merge order: this PR (#572) first, then #573. This PR should own transaction-commit-time — it has the purpose-built ### Storage Metrics section, its table shape (path/method/type) matches how the metric is actually emitted via recordAction() with no byThread flag, and it fixes the stale raw-entry example. #573 then rebases onto this, drops its transaction-commit-time row and duplicated prose, cross-links #storage-metrics, and folds its release note under the ## Analytics heading this PR creates. I've left the matching comments on #573.
One thing that is not a problem, so it doesn't get re-litigated: the Rejecting writes on this thread log string at line 201 is real. I grepped it at both v5.2.0 (DatabaseTransaction.ts:394) and origin/main (:485), and the source comment there confirms it fires once per stuck commit. Keep it.
Aggregate example left stale while the raw one was fixed (can't suggest inline — unchanged, so not in the diff). You correctly rewrote the raw example, but the aggregate example just below it (reference/analytics/overview.md lines ~107-120) still shows only median/mean/p95/p90, omitting the p99/p999 this PR's new prose tells operators to alert on. A reader who follows the guidance and looks at the example won't find the fields they were just told to query. Same drive-by class as the fix already in here.
Two smaller ones:
- The alerting advice doesn't mention that the aggregate has a 60s floor (
analytics.aggregatePeriod, default 60), which bounds how fast a percentile alert can fire. One clause would make the guidance actionable. storage-tuning.md'sstorage.maxTransactionQueueTimesection says "Harper rejects new writes with HTTP 503" and now has an observable signal to point at — worth adding the reverse link to#storage-metrics.
sent with Claude Opus 5
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-572 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-572 This preview will update automatically when you push new commits. |
3 participants
Documents the
transaction-commit-timemetric added by Harper PR #1688, "feat(analytics): record write-transaction commit latency", including its RocksDB-only submit-to-settle scope, raw/aggregate fields, aggregation delay, and relationship to the versioned overload guard. It also corrects the duplicated analytics examples and links the metric from storage tuning and the 5.2 release notes.For the human reviewer
transaction-commit-time. Documentation PR #573, "docs(analytics): document transaction queue depth metrics" should merge afterward, rebase, drop its duplicate commit-time row/prose, and fold its release note under this PR's Analytics heading.mean, compresseddistribution, andcount; aggregates have all nine percentiles. This was already true in Harper v5.0.0, so the old v5 docs were stale rather than describing a 5.2 schema change. Source:resources/analytics/write.tsat v5.0.0.hdb_analyticsquerying because it already teaches both system tables.get_analyticsremains available through the Operations API; a reviewer may prefer linking that operation as the recommended alerting path.Verification
npm run buildcompleted successfully and validated the MDX, anchors, and links. It reported only the pre-existing broken#deployment-operationsanchor warning in the v5.1 release notes.npm run format:writefollowed bynpm run format:checkpassed.npm run typecheckandnpm run lintpassed.path/method/type, both fulfilled and rejected async attempts, current oldest-attempt guard behavior, 503/log strings, and update/publish bypass rules.Review coverage
Authored by GPT-5.6 Codex. Full review: Claude Opus 5, Cursor Composer 2.5, and Claude Opus 5 Harper-domain adjudication. Final delta: Claude Opus 5. Gemini via agy returned no output in all rounds; Cursor Composer and domain adjudication were policy-pruned on the final narrow delta. Receipt @
175121c5.Generated by GPT-5.6 Codex.
Human-Review-Need: 4 @ 94bf4e4