refactor(telemetry): retire the duplicate nodestore_latency gauge

nodestore_latency published six values that nodestore_state already
publishes from the same Database accessors, so the two gauges were
duplicate readings of the same atomics:

  write_count       -> node_writes             getStoreCount()
  read_count        -> node_reads_total        getFetchTotalCount()
  write_duration_us -> node_writes_duration_us getStoreDurationUs()
  read_duration_us  -> node_reads_duration_us  getFetchDurationUs()
  write_mean_us     -> write_mean_us           store duration / count
  read_mean_us      -> read_mean_us            fetch duration / count

nodestore_state is kept because its means go through scaledMean(), which
saturates at INT64_MAX instead of wrapping and omits a mean when the
denominator is zero rather than reporting a misleading 0 us.

Removes registerNodeStoreLatencyGauge, its instrument member, the
metric::nodestoreLatency constant and the lval::nodestore_latency label
namespace. The gauge-over-histogram rationale and the "p99 is not
obtainable" consequence are folded into observeNodeStoreTotals' docs.

Retargets the gauge-contract test onto nodestore_state rather than
deleting it: the scaledMean arithmetic is covered by the static_asserts
in tests/libxrpl/telemetry/MetricsRegistry.cpp, but nothing else asserts
that these named series multiplex onto one instrument keyed by `metric`.
The test now calls the production scaledMean instead of a copy of the
division, and its sub-microsecond case asserts scaledMean's actual
behaviour (a genuine mean of 0 on a zero numerator with a non-zero
count), which differs from the retired gauge's extra numerator guard.

Rewrites both ledger-sync-health copies' panel 38/39 queries and drops
the obsolete claim that the write numerator was never written: all three
concrete store paths call recordStoreDuration, so write_mean_us is live
on an ordinary node. The same stale [import_db] caveat is removed from
the runbook, the 09 reference row and the workload validator's note.
This commit is contained in:
Pratik Mankawde
2026-07-28 11:52:44 +01:00
parent 05f337c686
commit c4e434d520
12 changed files with 196 additions and 343 deletions

View File

@@ -2534,11 +2534,9 @@ whose extra writes need an archive to read from. Both are absent by construction
on a fresh node, which is what makes them candidate explanations for this branch's
symptom rather than general slowness.
Three limits to respect here. The node-store numbers are **means, not
percentiles**, and `write_mean_us` is currently emitted only for the
`[import_db]` admin import path — on an ordinary node `write_count` climbs with
no `write_mean_us` line, which is a known instrumentation gap, not a healthy
zero. And the trim's fault counter is scoped to the **trim call only**: it shows
Two limits to respect here. The node-store numbers are **means, not
percentiles**, so a tail that matters will move them but there is no p99. And
the trim's fault counter is scoped to the **trim call only**: it shows
that the trim itself faults, and it cannot show the faults paid later as the
caches refill and touch the pages the trim returned. That later re-fault cost is
the actual mechanism by which a trim would slow a sync, and no metric here
@@ -2974,9 +2972,10 @@ panel it reads.
write-bound, so no read-side panel can show it; check this step whenever a
node with existing history is the slow one. Both panels live in the
collapsed **Back-fill & persistence** row — expand it.
Panel _NodeStore Write vs Read Latency (us/op)_ (`nodestore_latency`,
`metric=write_mean_us` and `read_mean_us`) with _NodeStore Operation Rate
(writes vs reads)_ (`metric=write_count` / `read_count`) beside it:
Panel _NodeStore Write vs Read Latency (us/op)_ (`nodestore_state`,
`metric=node_writes_duration_us` / `node_reads_duration_us` rated against
their counts) with _NodeStore Operation Rate (writes vs reads)_
(`metric=node_writes` / `node_reads_total`) beside it:
- **Write line rising during history back-fill** — the backend cannot
absorb writes fast enough. Sync will stay slow however many peers are
available, so adding peers will not help. Check storage IOPS, the
@@ -2999,12 +2998,10 @@ panel it reads.
a tail that matters will move it, but there is no p99 here. That is a
deliberate cost trade — a histogram would need one `Record()` per node
object, and a single ledger write walks thousands of SHAMap nodes.
Second, `write_mean_us` is currently emitted only for store paths that
record their duration, which today is the `[import_db]` admin import.
On an ordinary node you will see `write_count` climbing with **no**
`write_mean_us` line: that is a known instrumentation gap, not a healthy
zero, and the mean is deliberately omitted rather than drawn as 0 so it
cannot be misread as "writes are instantaneous".
Second, a mean is **omitted rather than drawn as 0** when nothing has
been stored or fetched yet, so an absent line means "no samples", not
"instantaneous". All three concrete store paths time themselves, so
`write_mean_us` is present on any node that has written at all.
15. **Is replay-based back-fill silently falling back to the slow path?**
Only relevant when `[ledger_replay]` is enabled. Panels _Replay Fallback to