feat: add more instrumentation to xrpld - #7875
Draft
pratikmankawde wants to merge 59 commits into
Draft
Conversation
Adds the anchors the sync-diagnostics signals attach to, with no signals emitted yet: - New "Ledger Sync Health" dashboard (uid ledger-sync-health) with the standard template-variable block copied from an existing board, plus empty "Bootstrap (Domain 0)" and "Sync pipeline" rows. - Signal index section in the data-collection reference, an operator-flow stub in the telemetry runbook, and a glossary anchor. - A sync_diagnostics group in expected_metrics.json and a matching assertion helper in validate_telemetry.py so CI fails when a signal regresses to absent. Also registers the new dashboard uid with the harness so the board is covered by validation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…el-sync-diagnostics
A freshly started node most often stalls before it ever peers or reaches quorum, and that whole chain had no telemetry. Adds the six signals that make it observable: - dns_resolve_total / dns_resolve_latency_ms: configured-peer hostname resolution, emitted from OverlayImpl so libxrpl stays independent. - overlay_connect_total / overlay_dial_latency_ms: outbound dial outcome by terminal reason, plus dial duration. - handshake_negotiation_fail_total: protocol and network-id negotiation rejections, labelled by reason, so a misconfigured network is no longer indistinguishable from unreachable peers. - unl_fetch_total and the unl_quorum gauge: validator-list fetch outcome per site and trusted key count against the required quorum. Without these a bad validators.txt leaves the node syncing forever with no signal. - clock_close_offset_seconds: network close-time offset, which server_info hides below 60s but which stalls consensus participation. Panels land in the Bootstrap row of the Ledger Sync Health dashboard, the metrics are asserted by the workload validator, and both the reference and the runbook flow describe them. Levelization baseline regenerated: overlay now includes MetricMacros.h, so the overlay/telemetry pair is reported one-way instead of bidirectional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects found reviewing the WP-A1 commit: - Handshake.cpp moved a std::string into std::runtime_error, which has no rvalue constructor. The move never happened and clang-tidy rejects it under performance-move-const-arg, so CI would fail even though the local hook only runs clang-tidy with TIDY=1. Takes the message by const reference instead, and drops the <utility> include that existed only for that move. - ValidatorList disables quorum by returning SIZE_MAX. Casting that to int64_t wrapped it to -1, so the headroom panel computed 0 - (-1) = +1 and coloured yellow on a node that can never validate: the sign inverted in exactly the bootstrap failure these signals exist to catch. Reports the disabled state as int64 max so headroom goes strongly negative instead. - The runbook claimed an expired list loads no keys. Expired counts as accepted, so its keys are loaded and then dropped by the expiry sweep, which calls for a different fix than replacing validators.txt. Pending is likewise a future-dated refresh, not a rejection. Documents both, plus how the quorum-disabled state now reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five signals that explain why a node is not advancing toward full, none of
which were observable before:
- state_changes_total now carries {from,to} mode labels, emitted at
setMode using the existing strOperatingMode helper. A bare count could
not distinguish a healthy climb from a node flapping between tracking
and connected. Removes the now-unused incrementStateChanges wrapper.
- sync_state{initial_full_duration_us}: time to first reach full, which
StateAccounting already computed but exposed only in server_info.
- sync_state{network_ledger_gate}: whether the node is still refusing to
build ledgers because it has no network ledger.
- sync_state{server_stall_seconds} and server_stall_events_total: how
long the main thread has been unresponsive. LoadManager computed this
and only logged it, so a stall was invisible until the fatal threshold.
The episode rule is a pure function so it can be tested without adding
a test-only mutator to LoadManager.
- sync_state{ledgers_behind}: how far our validated sequence trails the
best sequence any peer advertises, read from already-cached peer ranges
so no extra network traffic is added.
Also fixes the naming checker: it derived only the first label of a
multi-label instrument, so a dashboard querying the second label was
wrongly rejected.
Note: the clang-tidy hook cannot run in this worktree (no build
directory); the remaining pre-commit hooks, the naming check, dashboard
schema and harness syntax all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signals that separate a sync that is merely slow from one that will never
finish:
- sync_acquire{missing_state_nodes_max, missing_tx_nodes_max, in_flight,
received_data_depth}: how many SHAMap nodes each in-flight acquire is
still waiting for. getMissingNodes already computed this and the callers
discarded it after a trace log. A count that stays flat means the
acquire is wedged; a shrinking count means it is progressing. Recorded
once per sweep, never inside the per-node walk, and reset when a tree
completes so a finished acquire does not read as stuck forever.
- shamap_cache_hit_rate{treenode}: hit rate of the in-memory tree-node
cache, which sits above the node store, so it is distinct from the
existing NuDB ratio. A cold cache on a fresh node sends every traversal
step to disk.
- sync_acquire_no_progress_total: timer ticks where an acquire made no
progress, previously only logged.
- sync_addnode_total{good,duplicate,invalid}: whether arriving nodes are
useful, duplicated or rejected, so wasted fetch work is visible.
- sync_acquire_source_total{local,network}: whether a ledger was served
from the local store or had to be fetched.
Adds getBad()/getDuplicate() to SHAMapAddNode and an acquireProgress()
accessor on InboundLedgers so the xrpld gauge can read these without
libxrpl depending on telemetry.
ledger_seq is deliberately not a metric label: it is unbounded. Per-ledger
identity stays on the ledger.acquire span; the metrics expose bounded
aggregates instead.
The full-below cache hit rate is not exported: KeyCache updates different
counters than getHitRate() reads, so it would always report zero. That
libxrpl bug is documented rather than papered over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sync-critical job types run at very low concurrency limits (ledgerRequest
and ledgerData allow 3 each), so a node can stall simply because those
jobs are held back behind other work. Nothing exposed that until now:
the existing job metrics are rates and quantiles of jobs that already
moved, or a single queue-wide depth.
- jobq_backlog{metric,job_type}: instantaneous waiting, running and
deferred counts per job type. Deferred is the starvation signal and had
no exposure anywhere; it is set when a type is at its concurrency limit.
- jobq_saturation{metric}: running tasks, worker-thread count and total
waiting, so a slowdown spanning several subsystems can be attributed to
worker-pool exhaustion instead of being diagnosed once per victim.
Both read through two new const accessors on JobQueue that take the
existing mutex once and copy integers, so a single reading is internally
consistent and no per-job cost is added. The job_type label reuses the
same JobTypes name helper the existing job counters use, so the two label
sets join.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-A7)
Whether the network can even serve this node, and whether the node is
about to be shut out of validation, were both invisible:
- peer_ledger_supply: how many connected peers advertise a range covering
the sequence being fetched. Peers each track a range from status
changes, but nothing aggregated them, so "nobody has what I need" looked
identical to "peers are slow".
- peerfinder_slot_census: outbound active against capacity, connection
attempts, inbound, fixed configured against active, and the bootcache
and livecache sizes. All were computed already; only two were exported,
read at unrelated instants, so they could not be compared.
- peer_disconnect_total{reason,direction} and peer_accept_total{outcome}:
every disconnect previously collapsed into one number, so our own
backpressure could not be told from topology or network faults. Reasons
are a fixed set of literals recorded on the peer and emitted once at
close, never data supplied by the remote end.
- serve_refused_total{request,reason}: the other half of the sync
exchange, when this node declines to serve a peer.
- amendment_block: whether an unsupported amendment is expected and how
long until it activates. Amendment-blocked is terminal for validation,
so the countdown is the only leading indicator. The amendment id is
deliberately not a label, since the network can vote an id this build
has never heard of; it is already logged.
- ledger_jump_total: repeated last-closed-ledger switches, which mean the
node is thrashing between chains.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, WP-A6) Quorum and publish (A5): - ledger_quorum_publish gauge: the trusted-validation tally against the quorum the candidate ledger must reach, plus the gap between the validated and published sequences. The published sequence was never exported, so a publish pipeline falling behind healthy validation was invisible. - ledger_quorum_shortfall_total: counts the pre-accept early return where a node has peers and validators yet still declines to declare a ledger validated. That path was log-only, and it is the difference between accumulating toward quorum and never reaching it. Back-fill and persistence (A6): - nodestore_latency: read and write service time. storeDurationUs_ was declared but never written and had no accessor, so there was no write latency signal at all. This is the direct fingerprint of a node with an existing database syncing slower than a fresh one, where the node cache is cold and every tree step reaches disk. Distinct from the existing NuDB read panels, which show volume and hit ratio rather than service time. - ledger_replay_fallback_total and ledger_replay_outcome_total: the replay path silently falls back to plain acquisition on timeout or failure, so a defeated optimisation looked like ordinary slow back-fill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The span only recorded an outcome on the normal completion path. An acquire that stalled and was later swept ended with no outcome at all, and its duration stretched to the sweep interval rather than the real fetch time. So the one case these signals exist to catch, a fetch that never finishes, was the one case that could not be traced, and aggregate outcome and timeout rates read low exactly when nodes are stuck. - Adds an abandoned outcome value for the swept-while-fetching case. - Routes every exit through one idempotent finalizer, so a span is finalized exactly once whether it completes, fails, short-circuits on local data, or is destroyed mid-fetch. The destructor path cannot throw. - Adds the ledger hash to the span and backfills the sequence once known, since by-hash acquires start without one and could not otherwise be tied to a specific ledger. - Record layer: outcome stays a span-metrics dimension in both collector configs, which drift apart if only one is edited. The ledger hash is indexed in Tempo for trace search instead, because a per-ledger value as a metric dimension would mint a new series every ledger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registers the new gauges, renders them, asserts them and documents them, so each signal reaches an operator rather than stopping at the emit site: - MetricsRegistry: gauge registration for ledger_quorum_publish, nodestore_latency, peer_ledger_supply, peerfinder_slot_census and amendment_block, each guarded by the detached-callbacks check and tolerant of services that are not ready yet. - Ledger Sync Health dashboard: panels for the new signals, filtered by the node template variable like every other board. - Workload validation: the new series are asserted, so a signal that regresses to absent fails CI. Signals the local cluster structurally cannot produce, such as a replay fallback or an amendment block, are noted rather than asserted, which would fail red on a healthy run. - Reference, runbook and glossary entries, including the diagnosis order for a node that has peers and validators but never validates. - Regenerated levelization baseline: three new one-way edges from the telemetry and test modules, no new cycles. Also drops an unused cstddef include from the macro tests, which the include checker rejects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The quorum and publish gauges were emitted but never surfaced: no panel, no harness assertion, no reference entry. Completes those layers. - Four panels: trusted validations against the quorum target on one axis so a tally climbing toward quorum is visually distinct from one flat below it; publish lag; pre-accept shortfall rate; and time to first validated ledger. - Both signals are asserted by the workload validator. The shortfall counter does fire on a healthy cluster, because this node validates and then immediately re-enters the accept gate before its peers' validations arrive, so the first evaluation of every round tallies short. The panel and note say so, and give the fault signature instead: the shortfall rate outpacing the ledger-close rate while the tally stays flat and nothing ever reaches first-validated. - The quorum target is deliberately drawn as its own line rather than as a headroom stat, so the disabled-quorum sentinel reads as an unreachable target instead of an unreadable negative number. Also removes three reference rows that were appended twice when two agents each documented the same back-fill signals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four blind spots in the sync exchange, each now a span:
- txset.acquire: transaction-set acquisition had no span at all, though it
is the sibling of ledger.acquire and runs every consensus round. A round
that falls behind because its tx set never arrived was indistinguishable
from one that deliberated slowly.
- ledger.acquire.{header,astree,txtree}: the acquire span was flat, so the
account-state tree, which dominates a fresh sync, could not be separated
from the transaction tree. These are children, closed before the parent.
- peer.dial: the outbound dial already had outcome counters; the span adds
the per-attempt timeline, so a slow stage is visible rather than only its
terminal reason.
- ledger.serve: serving a peer's ledger request was uninstrumented, so this
node's contribution to someone else's sync was invisible.
Every span finalizes exactly once. Outcomes come from shared compile-time
rules rather than a literal per branch, so no exit can mislabel itself and
an exit added later cannot omit one. Destructor paths are noexcept.
One rule needed care: the timeout path also sets the failure flag, because
that is how the timeout loop stops, so precedence puts timeout ahead of
failure or a timed-out acquire would read as a data fault.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ogram (WP-B3) A slow fresh-sync ledger produced spans scattered across threads with no way to relate them. They now share a trace id derived from the ledger's own hash, the one value every participating site already holds, so nothing new is plumbed across threads. This is the pattern the transaction pipeline already uses for its tx id. Joined: ledger.validate, ledger.store, and a new consensus.validation.accept recorded when a trusted validation arrives. In Tempo, searching one ledger hash returns them together, so an operator can tell whether the ledger was slow to arrive, slow to be accepted, or slow to be stored. They are siblings rather than a chain because the accept gate is entered from three different threads, so no fixed parent order exists. consensus.validation.accept also records why an arriving validation did or did not advance the gate, which makes "validations arrive but are all rejected" visible for the first time. consensus_round_duration_ms turns the existing round-time span attribute into a histogram, so a fleet trend needs a metric query rather than raw trace inspection. An explicit bucket view is required, not optional: the SDK default tops out at ten seconds while consensus abandons a round at two minutes, so slow rounds would all fall in one bucket and every quantile would read exactly ten seconds. Cost is one record per round. Record layer: the histogram is native and needs no collector change. The two new bounded attributes are added as span-metric dimensions to both collector configs. The ledger hash stays out of them, since a per-ledger dimension mints a series per ledger; it is indexed in Tempo as the join key. The ledger.acquire span is not joined yet, because that file was being changed concurrently. It is registered as an optional member of the join group so nothing fails, and switching it is a one-line follow-up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the join left open when the acquire and consensus work landed in parallel. The acquire span now derives its trace id from the ledger hash it already records, so a fetch shares one trace with that ledger's validation, acceptance and store, and its three phase children inherit the same id. Reading one trace now answers the whole question for a slow ledger: whether the data was slow to arrive, slow to be accepted, or slow to persist. The acquire span stays an optional member of the join group, for the reason its own entry already gives: a healthy cluster agreeing from genesis rarely back-fills, so the span need not appear on every run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tes (WP-B4) The board and runbook had grown by append across eight work packages, so they read in the order the work was done rather than the order a node progresses. This is the coherence pass; it adds no new instrumentation. - Dashboard: 52 panels regrouped from two rows into nine that follow the fresh-start sequence — bootstrap, peer supply, sync state, acquire and SHAMap fetch, job queue, quorum and publish, terminal blockers, then back-fill and spans collapsed since they answer conditional questions. Layout only: no title, query or description changed. - Runbook: the flat step list becomes a decision tree branching on the observed symptom, with the amendment-block check first because it is terminal. Each branch names the panels, what healthy and unhealthy look like, and what to conclude. The existing steps are kept as the detail bodies. - Reference table: every signal name re-checked against the code and every named panel against the board; four stale panel references fixed. - Validation: every signal is now either asserted or covered by a note explaining why a five-node local cluster cannot produce it. Also fixes the write-latency signal, which was inert on a real node: the store duration was only recorded on the database-import path, while the two production store implementations did not time themselves, so an ordinary node reported a write count with no latency. Both now time the backend write, which is the disk work this signal exists to expose. Without it the "existing database syncs slower than a fresh one" diagnosis had no primary signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it (WP-A8) Metric names and label keys were bare string literals, repeated across the emit site, the gauge registration, the unit test, the workload manifest, the dashboard queries and the reference table. A rename touched six places and a typo in any one of them failed silently: a metric that never appears, or a label that never joins. The span side already had this right, with names and attribute keys declared once in the *SpanNames.h headers and a CI rule rejecting literals at call sites. That rule only ever covered spans, so the metric side had no equivalent and no suffix convention was enforced by anything. - Adds MetricNames.h declaring every instrument name, label key and bounded label value this story emits, grouped by subsystem, following the existing span-name header layout. - Converts the call sites subsystem by subsystem. The emitted strings are unchanged: 75 names before, the same 75 after, verified by extracting the wire strings from both trees and diffing the sets. - Extends the naming check with three rules: no literal instrument name or label key at an emit site, the duration and counter suffix conventions, and every name in the workload manifest resolving to a constant. The first rule is ratcheted per metric family so the pre-existing families warn rather than block, keeping the remaining work visible instead of forcing one unreviewable change. Constants are character arrays rather than the span headers' StaticStr, because the metrics API takes a string view that will not construct from it. Two things the conversion exposed: a serve-refusal reason that the original inventory missed because it is passed through a ternary, and a label whose constant made it invisible to the checker's literal scan, which would have failed a dashboard rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…el-sync-diagnostics Phase-10 independently instrumented the peer object-fetch path while this branch instrumented fresh-node sync, so the two overlapped in three places. Resolved by keeping each side's stronger implementation rather than shipping both. Per-job-type waiting/running/deferred existed twice. Phase-10's version survives: it publishes per-type gauges from JobQueue::collect(), which snapshots under the queue lock and publishes after releasing it, a deliberate lock-order fix against the collector's own lock. This branch's jobq_backlog gauge and the JobQueue::getJobTypeCounts() accessor that fed it are removed, along with their panels, assertions and reference rows. jobq_saturation stays: it reports the whole worker pool, which phase-10 has no equivalent for. The histogram view helper also existed twice with identical bodies under two names; one survives, and the microsecond ladder is now the named array rather than boundaries repeated inline. The job_type label was declared twice, once as a file-local constant invisible to the naming check; both it and handler now come from the constants header. Two things phase-10 adds are complementary, not duplicates, and are kept as they are: the handler label, which separates the two request kinds that both report as the same job type, and getobject_rejected_total, which counts malformed requests where this branch's serve_refused_total counts requests this node declined to serve. Also fixes two naming-check failures that pre-date this merge on phase-10. The check derived label keys only from namespaced constants, so it could not see the per-subsystem headers' flat k-prefixed style and rejected dashboards querying labels the code really emits. It now reads both styles, with the enforcement rules unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These are the first failures found by actually building and running the new telemetry tests. All three were defects in the tests, not the metrics. - state_changes_total expected six series from seven transitions, but two edges were traversed twice, so there are five distinct from/to pairs. The repeat is the point of the label: flapping raises the count on one edge rather than minting a new series, so the assertion now says five and explains why. - shamap_cache_hit_rate compared for exact double equality against a value that reaches the gauge through a float hit rate, so 0.9 arrives as 0.89999997. Now compares within a tolerance far tighter than any threshold a dashboard reads, since asserting exact equality was only asserting the float representation. - nodestore_latency shared one provider across four scenarios. The reader reports cumulative temporality, so a mean observed by an earlier scenario was still present in the next collection, which defeated the two assertions that a mean is absent when its denominator is zero. Each scenario now collects from its own provider. 139 of 139 telemetry tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The write-duration test encoded the behaviour from before the store path was timed. It populated the source database through store(), then asserted that database's write duration was still zero, which was only true while importDatabase was the sole timed path. Both concrete store overrides now time their backend write, so an ordinary store run accumulates a duration and the assertion failed. Turned that stale assertion into a positive one: an ordinary storeBatch must produce a non-zero duration, which is the case a real node actually exercises. The negative half of the test, proving the accumulation is per-database rather than a shared global, now checks that the import leaves the source's store COUNT unchanged, since the source's duration is legitimately non-zero from its own writes. 80552 tests across the six affected suites pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pratikmankawde
added
the
DraftRunCI
pratikmankawde
changed the base branch from
develop
to
pratik/otel-phase10-workload-validation
July 27, 2026 13:13
Found by reviewing what each metric actually measures, with attention to the derived and bucketed ones. All five could report healthy while the node was not, or the reverse. - The nodestore latency panel took rate() of a mean. The gauge already divides duration by count in code, so rating it produced a figure with no unit, and Prometheus discards a gauge's decreases, so a heavy back-fill read as roughly zero microseconds per operation. The cumulative duration totals are now exported alongside the means, and the panel divides the rate of the total by the rate of the count, which is the latency over the panel's own window rather than a since-boot average that flattens with uptime. - The DNS-resolve and outbound-dial histograms had no explicit buckets, so they inherited a ladder that stops at ten seconds while the dial timer is fifteen. Every timed-out dial fell in the overflow bucket and p95 read exactly ten seconds however bad it got. Both now have a ladder reaching thirty seconds with fifteen on its own boundary, so a timeout is distinguishable from merely slow. - The missing-node counts only cleared when a tree completed, so a timed-out or failed acquire left its last count latched. Since the gauge reports the maximum across everything still in the collection, and eviction waits on a grace period plus the sweep interval, a finished node reported as stuck for minutes. That inverts the one signal that separates stuck from slow. Cleared unconditionally on the terminal path instead. - A disabled quorum published a sentinel so large that, on a timeseries axis shared with the trusted-key count, it flattened the key line to the baseline and hid the outage it was meant to mark. The series is now omitted and a quorum_disabled flag carries the state. - Two panel descriptions claimed a one-second export cycle. The reader is configured for ten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pratikmankawde
marked this pull request as ready for review
July 27, 2026 14:31
Two suspects from the 3.3.0 slowdown investigation had no signal. Both were already computing the numbers and throwing them away, so this exposes them rather than adding measurement. Per-sweep heap trim. The trim runs after every cache sweep, and its cost scales with resident heap, so it is the leading explanation for a node with a populated database syncing slower than a fresh one. The report already carried duration, fault deltas and reclaimed pages, but the whole measurement sat behind a debug-journal check, so an ordinary node measured nothing, and the call site discarded the result. The measurement now always runs and only the log line stays gated. Records trim duration, minor faults and reclaimed kilobytes. Measured cost of the always-on path is about six microseconds per sweep against a trim costing milliseconds, at a cadence of ten to a hundred and twenty seconds. Honest limit, stated in the runbook: the fault delta spans only the trim call, so it shows the trim itself faulting but not the faults that follow as caches refill. The duration is the signal to correlate against sweep-job queueing. Rotation writes. Rotation copies archive-served reads forward and re-stores nodes missing from both backends, both of which compete with sync I/O and only happen on a populated online_delete database. The copy-forward count existed but was reset by the rotation's own log line, so a metric reading it would drop to zero on every swap; a never-reset total sits beside it now. The re-store count was not measured at all. Rotation duration is deliberately not recorded: the health throttle sleeps at eight points inside the sequence and dominates exactly when the node is unhealthy, so the number would conflate work with waiting. Nothing added for the other two suspects. Get-object serving is already covered by the handler label, the lookup histogram and the deferred and saturation gauges; peer churn by the disconnect-reason counter. Also replaces nine per-file cspell ignores with one ignoreRegExpList entry for the telemetry macro names, and picks up the levelization baseline for the consensus span-name test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All eleven were reproduced locally against the same checks before fixing. Six unused includes, left behind when the merge unioned two sets of includes and later edits removed their only users: algorithm, functional, numeric and thread in the job-queue test, and cstdint in the sync-state test and the load manager. Each verified unused by grepping for every symbol the header provides, so none is a still-needed include being dropped. The telemetry registry header included ranges for a std::ranges::all_of call, but that algorithm comes from algorithm, which the header already included. Two consteval handler-name loops became std::ranges::all_of, which reads as the predicate it is, and the two flagged fixtures are const. Also picks up the levelization baseline the check asked for: the consensus span-name test adds one edge from the libxrpl tests to xrpld.consensus, which is the exact line CI's diff requested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gcc debug-coverage job failed where clang, macos and windows passed: gcc's -Werror=type-limits rejects comparing an unsigned expression against zero with >=, because it is always true. The assertion was vacuous anyway. Both operands are unsigned, so the check proved nothing, while the comment beside it says the intent was a non-zero microsecond figure. Now asserts strictly positive, which is what it meant. The same job also logs a CMake LTO capability probe failing on a missing compiler-ar and prints the code-generation guard's echo line; neither is related to this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gcc debug-coverage job rejected an unbraced `if` whose body is a GTest assertion: EXPECT_EQ expands to an if/else, so the outer `if` leaves an else that could bind either way, and -Werror=dangling-else refuses it. clang does not warn, which is why only that one job failed. Braced the span-names case that failed, then swept every test file this branch touches for the same shape and braced the two others found, so the next gcc run does not fail on the next one down the list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
json::ValueConstIterator and ValueIterator declared difference_type, reference and pointer but not value_type or iterator_category. Under C++23, std::iterator_traits then classifies them as output iterators, so std::all_of over a Value's members (isValidJson2 in RPCCall.cpp) fails to instantiate on GCC 13/14 with: cannot convert 'output_iterator_tag' to 'std::input_iterator_tag' GCC 15 masks this via LWG-3798/P2609, but the perf CI image ships GCC 13, so the source needs the traits regardless. The iterators wrap a std::map iterator (++/-- only), so the category is bidirectional. Add value_type + iterator_category to both iterators, include <iterator>, and add a regression test asserting the traits and that std::all_of / std::count_if compile and run over Value members. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The isValidJson2 call site in RPCCall.cpp already forces instantiation of std::all_of over json::ValueConstIterator, so a regression that removed the iterator traits would fail the real build. A dedicated static_assert test is redundant; remove it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four defects from the automated review on PR #7875, each verified against the current tree before fixing (one further comment, the row-63 dashboard overlap, was already fixed by an earlier commit and needed nothing). 1. Rule J could not detect an instrument-kind mismatch. instrument_kinds() wrote `kinds[wire] = ...`, so a wire name created through two different factories kept only the kind visited last and whichever emit site the file walk reached last silently decided the verdict. It now collects a set per name and reports the conflict itself -- one name exporting two instruments is the defect, and no suffix can be correct for both. Added a regression test that builds a name as both a counter and an observable gauge and asserts the message names both. 2. A duplicate connection was reported as `tls_fail`. The TLS handshake had in fact succeeded; PeerFinder simply already held a slot for that address, which is ordinary churn on a healthy node. Conflating the two made a rising `tls_fail` unreadable -- it could mean unreachable peers or merely a busy PeerFinder, and those need opposite responses. Added a distinct `duplicate` outcome and carried the widened vocabulary through every place that enumerates it: the panel description, both filter descriptions, the runbook branch table, the runbook outcome list and the expected_spans note. The `dial_outcome` template variable is a label_values() query, so it picks the new value up on its own. 3. ConnectAttempt::onShutdown had no `operation_aborted` guard, unlike the five other handlers in the same file. A clean teardown was therefore counted as `upgrade_fail`, inflating that outcome on any node shutting down with dials in flight. 4. ValidatorSite used the raw configured URI as a Prometheus label. [validator_list_sites] accepts credentials in the URI and ParsedUrl keeps them in username/password, so a configured `https://user:pass@host` would have copied the secret into a metric label and on into the collector, Prometheus and every dashboard. The label is now rebuilt from scheme, host, port and path -- everything needed to tell one site apart, and nothing more. Verified: naming checker exits 0 with Rule J still passing all 40 real instrument names; its unit tests now number 139 and all pass; 15 dashboards validate; both workload JSON files parse; clang-tidy over the full compile database reports no finding on either changed .cpp; pre-commit passes. Not verified: not compiled. Item 4 introduces string concatenation and item 2 a new constexpr, so CI's build is the first real check on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
…e label
Adversarial validation of the previous commit found one of its two code fixes
was diagnosed wrongly and the other incomplete. Both are corrected here, along
with the layers the first pass missed.
1. The new dial outcome was named for the wrong condition. It was added as
`duplicate` on the belief that PeerFinder had already granted a slot for the
address. It has not: `Logic::onConnected` contains exactly ONE false-returning
path and it is the self-connect check, which logs "Logic dropping as self
connect" (include/xrpl/peerfinder/detail/Logic.h). The duplicate check lives
in `newOutboundSlot`, evaluated before a ConnectAttempt exists, so a real
duplicate can never reach this branch.
That mattered beyond the name: the previous commit told operators the outcome
was benign churn to ignore, when it actually reports a local misconfiguration
-- this node has its own address in [ips_fixed] or behind its advertised
endpoint, and every dial to it is wasted. Renamed to `self_connection`,
reusing the slug `handshake_negotiation_fail_total` already publishes for the
same fault so it reads identically on both signals, and every description
corrected to say so. The fail() string now reads "Self connection" too.
The first pass also missed three enforcement and contract sites: the
ConnectAttempt.h Doxygen state machine (which still mapped the slot branch
onto tls_fail), the LedgerSpanNames unit test (which pinned exactly five
values over a std::array<..., 5> and so left the new member untested), and the
span-derived twin panel plus two reference docs that still published the old
five-value domain.
2. The credential-free site label was incomplete twice over.
- It appended the port, and `Resource::Resource` DEFAULTS that to 443/https
and 80/http when the config omits one. The label would have become
`https://vl.ripple.com:443/` where Grafana Cloud currently holds
`https://vl.ripple.com`, silently renaming the series for every deployment
already scraping this metric. Verified against live label values before and
after; the port is now omitted.
- parseUrl's path group is `(/.*)?`, greedy to end of string, so a query or
fragment lands inside `path`. A list URL authenticated by `?token=...` would
have leaked exactly as userinfo did. The path is now truncated at the first
'?' or '#'.
Also updated the MetricNames.h usage example, which still taught the raw-URI
pattern to the next author, and the 09-doc row that described the label as the
configured URI.
3. Rule J hardening from the same review: `classify_instrument_kind` returns an
`other` sentinel for a non-factory macro, and storing it in the kind set could
render a future conflict as "created as counter and other". The sentinel is
now skipped, keeping it doing what it already did -- matching no shape rule.
Added a second regression test whose input the pre-fix code reported as CLEAN
(gauge-then-histogram on a `_us` name), so the guard is proven by a 0-vs-1
difference and not only by a changed message. Both new tests were run against
a reconstructed last-wins implementation and both fail against it.
Documented the conflict class in the Rule J rows of the checker README and
CONTRIBUTING, which previously described only the suffix conventions.
Verified: naming checker exits 0 with Rule J passing all 40 real names; 140
checker tests pass; 15 dashboards validate; both workload JSON files parse;
clang-tidy over the full compile database reports no finding on any changed line
of ConnectAttempt.cpp or ValidatorSite.cpp; pre-commit passes.
Not verified: not compiled. The label change adds string truncation and the
outcome rename touches a constexpr used across three translation units, so CI's
build remains the first real check on both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cludes Two findings from the CI clang-tidy job, both mechanical: - MetricsRegistry.h no longer includes <ranges>. The only ranges use in the header is std::ranges::all_of, which is declared in <algorithm> and already included, so the include was genuinely unused. - The computeGetObjectByHashFee() expectation table now uses designated initializers. FeeCase has five fields, two of which are plain ints in adjacent positions, so naming them at each row also makes a requested/found transposition impossible to write by accident. No behaviour change.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…el-sync-diagnostics # Conflicts: # src/xrpld/app/ledger/detail/InboundLedger.cpp
…te label Four findings from a review pass over the PR. The "Spans & traces" row was empty. Moving the row header down to clear the back-fill panels was only half the change -- the seven span-derived panels stayed at their old y, one unit below the native panels, so every pair overlapped and Grafana parented all fifteen to "Back-fill & persistence". The panels now sit below the row header, which restores the split the runbook already describes: eight native panels answer "how much", seven span-derived ones answer "which". Both rows stay expanded, so the docs no longer call them collapsed. metric_constants() excises each namespaced block before the flat prefix pass. The flat pass classifies by identifier prefix and is meant for headers that name the role in the identifier because they have no `namespace metric`/`label`/`lval`; it was running over the whole header, so a `kLabel`-prefixed constant written inside `namespace metric` landed in both buckets and an instrument name became a valid label key for Rule D. Nothing in the tree does that today, which is why it went unnoticed, and why the guard is a test rather than a fix for an observed failure. The `site` label now keeps a non-default port and drops userinfo residue from the host. Omitting the port unconditionally merged two local sites that differ only by port; printing it unconditionally would have renamed the existing `https://vl.ripple.com` series. Comparing against the scheme default distinguishes a configured port from the one the Resource constructor fills in. parseUrl's host group also permits '@', so a malformed URI with two of them leaves part of the userinfo in `domain`.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
getSlotCensus() had no test that called it. The existing telemetry tests cover the gauge callback by re-implementing it against a mock meter, so they never exercise the accessor itself and would not notice a field added to SlotCensus without a matching census assignment. Four tests drive a real Logic and assert every field exactly: - a fresh Logic publishes its configured ceilings and nothing else, so "no slots configured" is distinguishable from "configured but empty"; - inPeers with wantIncoming=false yields an inbound ceiling of 0, per the rule in Counts::onConfig; - one fixed and one ordinary peer walked through dial, connect, handshake and close, asserting all seven intermediate states. This pins the non-obvious part: Counts::adjust guards the outActive_/inActive_ branch with !s.fixed(), so a fixed peer raises fixedActive and never outActive. A census that conflated them would overstate slot pressure. fixedConfigured stays at 1 after the disconnect, which is the "configured fixed peer I cannot reach" reading an operator needs; - bootcache and livecache depths come from separate sources, so filling one leaves the other at 0.
HashRouter.h declared `class Config;` and never referred to it: the only other mentions of the name in the file are two prose comments. Nothing that includes this header depends on it either -- of the eighteen files that do, the only one that names the type needs the full definition and includes it itself. The declaration is vestigial, left behind when setupHashRouter() moved to setup_HashRouter.h, which carries its own. Left alone it is now an error rather than dead code. This branch's SlotCensus work made Overlay.h include PeerfinderManager.h, because Overlay::slotCensus() returns a SlotCensus by value. ValidatorList.cpp includes both Overlay.h and HashRouter.h, and is the one translation unit that reaches them without also pulling in xrpld/core/Config.h -- so PeerFinder::Config and this declaration meet there, and bugprone-forward-declaration-namespace reports a declaration with no definition beside a same-named class in another namespace. Removing it rather than including the real header: xrpl::Config lives in xrpld, so an include would point the xrpl.libxrpl.core layer at the layer above it.
pratikmankawde
force-pushed
the
pratik/otel-sync-diagnostics
branch
from
July 28, 2026 17:56
53d157b to
78411c6
Compare
…el-sync-diagnostics
…el-sync-diagnostics
pratikmankawde
commented
Jul 28, 2026
pratikmankawde
commented
Jul 28, 2026
pratikmankawde
commented
Jul 28, 2026
pratikmankawde
commented
Jul 28, 2026
pratikmankawde
commented
Jul 28, 2026
Five review findings. The validation_accept span started inside the "outcome is Current" branch, so validation_status could only ever read "current" and the four rejected values were unreachable. That defeated the attribute: a node whose trusted validations are all rejected emitted no span at all, so it looked the same as a quiet node. The span now starts before the outcome is checked, still for trusted validations only, so a rejected one is recorded with its real status. The rest are consistency fixes: ValidatorSite passes the parse_error constant instead of the literal, the 28 gauge callbacks that spelled the `metric` label key as a literal now use label::metric like the other two, a stray blank first line is gone from six files, and MetricsRegistry.cpp loses a duplicate Doxygen block that documented addHistogramView but sat above an unrelated constant.
…tors Alloy declared 24 spanmetrics dimensions where both collector configs declare 28. The four missing ones -- timed_out, object_type, validation_status and accept_gated -- are emitted by the code and queried by ledger-sync-health panels "Ledger Serve Rate by Object Type" and "Trusted Validation Accept Rate by Status", so on an Alloy deployment those panels collapsed to one undifferentiated series. deploy-run-otel-xrpld.md already requires the dimension sets to stay in sync; the config had drifted from it. Alloy's other differences from the collectors (no filelog, no tail_sampling) are deliberate -- it is cloud-only by design.
Someone filling in .env.grafanacloud-alloy has no reason to know that deployment.environment and xrpl.network.type are not env vars: they are literals inside the tier transform, because OTTL statements do not expand sys.env(). Left alone they ship as prod/mainnet and every dashboard environment and network filter reads the wrong tier. The example now says so, and notes service_instance_id needs nothing here.
…el-sync-diagnostics
…el-sync-diagnostics
Contributor
There was a problem hiding this comment.
Pull request overview
This PR substantially expands xrpld’s OpenTelemetry instrumentation to make fresh-node ledger sync stalls diagnosable end-to-end (bootstrap → peering → quorum → acquire/store), adding new spans/metrics plus supporting libxrpl read-only accessors, tests, and dashboard/config updates.
Changes:
- Add new native metrics and tracing spans across overlay/handshake/dial, UNL fetch, sync state, acquire/serve, quorum/publish, nodestore persistence, and consensus timing/validation acceptance.
- Introduce/extend libxrpl accessors (JobQueue saturation, NodeStore store-duration totals, SHAMapAddNode tallies, PeerFinder slot census, online-delete rotation copy-forward totals) so xrpld telemetry can observe internal state safely.
- Update workload/collector/Tempo configs, dashboards, naming rules, and add targeted unit tests to pin contracts/decision rules.
Reviewed changes
Copilot reviewed 87 out of 90 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/telemetry/MetricMacros.h | Remove local cspell ignore comments (handled globally now). |
| src/xrpld/overlay/Overlay.h | Add PeerLedgerSupply + new Overlay accessors for peer ledger supply and PeerFinder slot census. |
| src/xrpld/overlay/detail/PeerSpanNames.h | Add peer.dial span constants + attrs/values for outbound dial attempts. |
| src/xrpld/overlay/detail/PeerImp.h | Add disconnect-reason plumbing, serve-refusal reporting, and ledger.serve span helpers. |
| src/xrpld/overlay/detail/OverlayImpl.h | Add peer-ledger supply aggregation, slot census forwarding, inbound accept outcome + DNS resolve reporting helpers. |
| src/xrpld/overlay/detail/OverlayImpl.cpp | Implement inbound accept outcome counters, DNS resolve metrics, and peer supply aggregation. |
| src/xrpld/overlay/detail/Handshake.cpp | Add labeled metric for handshake negotiation failures via centralized helper. |
| src/xrpld/overlay/detail/ConnectAttempt.h | Track dial start/outcome; add peer.dial span lifecycle + unified reportOutcome funnel. |
| src/xrpld/app/misc/ValidatorSite.h | Add per-fetch UNL outcome reporting helper signature. |
| src/xrpld/app/misc/ValidatorList.h | Add trustedKeyCount() accessor for cheap periodic polling. |
| src/xrpld/app/misc/SHAMapStoreImp.cpp | Add counter for rotation-time “restore missing node” events. |
| src/xrpld/app/misc/NetworkOPs.cpp | Add accessors for initial sync duration and “ledgers behind network”; add mode-transition edge counter and ledger-jump counter. |
| src/xrpld/app/misc/detail/ValidatorSite.cpp | Emit unl_fetch_total with sanitized site label; record outcomes for failures and successes. |
| src/xrpld/app/misc/detail/ValidatorList.cpp | Implement trustedKeyCount(). |
| src/xrpld/app/main/LoadManager.h | Add relaxed-atomic stall accessors + pure constexpr stall decision rule (evaluateStall). |
| src/xrpld/app/main/LoadManager.cpp | Publish stall state each tick via updateStallState(). |
| src/xrpld/app/main/Application.cpp | Factor sweep heap trim into trimHeapAndRecord() and emit trim metrics. |
| src/xrpld/app/ledger/LedgerReplayTask.h | Add telemetry terminal-outcome recording hook. |
| src/xrpld/app/ledger/InboundLedgers.h | Add aggregated AcquireProgress snapshot API for metrics polling. |
| src/xrpld/app/ledger/detail/TransactionAcquire.h | Add txset.acquire span state, timeout distinction, and finalize helper. |
| src/xrpld/app/ledger/detail/TransactionAcquire.cpp | Implement span lifecycle (including destructor finalization) and activate span for outcome logs. |
| src/xrpld/app/ledger/detail/SkipListAcquire.cpp | Emit replay fallback metric for skiplist stage. |
| src/xrpld/app/ledger/detail/LedgerReplayTask.cpp | Emit replay terminal-outcome counters (success/build_failed/parameter_failed/timeout). |
| src/xrpld/app/ledger/detail/LedgerMaster.cpp | Add per-ledger hash-joined spans (ledger.validate/ledger.store), quorum-gate metrics, and time-to-first-validated tracking. |
| src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp | Emit replay fallback metric for delta stage. |
| src/xrpld/app/ledger/detail/InboundLedgers.cpp | Implement acquireProgress() by aggregating missing nodes + stash depth across inflight acquires. |
| src/xrpld/app/consensus/RCLValidations.cpp | Add consensus.validation.accept span keyed to validated-ledger trace, with bounded attrs. |
| src/xrpld/app/consensus/RCLConsensus.cpp | Record native histogram for consensus round duration (in addition to span attr). |
| src/tests/libxrpl/telemetry/SyncStateSignals.cpp | Add unit tests pinning LoadManager stall rule (compile-time + boundary semantics). |
| src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp | Add unit tests pinning consensus.validation.accept span naming + value mapping contract. |
| src/tests/libxrpl/nodestore/Database.cpp | Add test ensuring import path accumulates store-duration totals. |
| src/tests/libxrpl/basics/MallocTrim.cpp | Strengthen tests to ensure measurement doesn’t depend on debug logging; validate fields on supported platforms. |
| src/test/overlay/TMGetObjectByHash_test.cpp | Minor brace addition in test for clarity/structure. |
| src/test/core/JobQueue_test.cpp | Minor brace addition in test for clarity/structure. |
| src/test/app/LedgerReplay_test.cpp | Update mock InboundLedgers to implement new acquireProgress() API. |
| src/libxrpl/peerfinder/PeerfinderManager.cpp | Expose getSlotCensus() through PeerFinder Manager implementation. |
| src/libxrpl/nodestore/DatabaseRotatingImp.cpp | Use recordStoreDuration() helper; add monotonic copy-forward total updates. |
| src/libxrpl/nodestore/DatabaseNodeImp.cpp | Use recordStoreDuration() helper for store timing. |
| src/libxrpl/nodestore/Database.cpp | Time import batch stores and feed store-duration accumulator. |
| src/libxrpl/core/detail/JobQueue.cpp | Implement JobQueue::getWorkerSaturation() aggregation. |
| src/libxrpl/basics/MallocTrim.cpp | Make measurement unconditional (log remains debug-gated); refactor into measuredTrim helper. |
| include/xrpl/telemetry/GetObjectMetricNames.h | Remove local cspell ignore comments (handled globally now). |
| include/xrpl/shamap/SHAMapAddNode.h | Add getBad()/getDuplicate() accessors for bounded telemetry counters. |
| include/xrpl/server/NetworkOPs.h | Add new accessors for initial sync duration and ledgers-behind-network. |
| include/xrpl/peerfinder/PeerfinderManager.h | Introduce SlotCensus struct and Manager::getSlotCensus() API. |
| include/xrpl/peerfinder/detail/Logic.h | Implement census snapshot construction under PeerFinder lock. |
| include/xrpl/nodestore/detail/DatabaseRotatingImp.h | Add isRotationInFlight() and copyForwardTotal() accessors. |
| include/xrpl/nodestore/DatabaseRotating.h | Add public accessors for rotation-in-flight + monotonic copy-forward total. |
| include/xrpl/nodestore/Database.h | Add store/fetch duration accessors w/ relaxed atomics + recordStoreDuration(duration) helper. |
| include/xrpl/json/json_value.h | Fix iterator traits by adding value_type and iterator_category (standard algorithm compatibility). |
| include/xrpl/core/JobQueue.h | Add WorkerSaturation struct + getWorkerSaturation() API. |
| include/xrpl/core/HashRouter.h | Remove stale forward declaration (minor header cleanup). |
| include/xrpl/consensus/ConsensusSpanNames.h | Add consensus.validation.accept span contract and bounded status mapping helper. |
| include/xrpl/basics/MallocTrim.h | Update notes + cspell ignore to reflect always-on measurement behavior. |
| docker/telemetry/workload/README.md | Update dashboard count reference (10 → 15). |
| docker/telemetry/tempo.yaml | Add dedicated Parquet columns for high-value span attrs (ledger_hash/txset_hash/remote_endpoint). |
| docker/telemetry/otel-collector-config.yaml | Add spanmetrics dimensions for new bounded attrs and explain cardinality choices. |
| docker/telemetry/otel-collector-config.grafanacloud.yaml | Mirror spanmetrics dimension updates for Grafana Cloud collector config. |
| docker/telemetry/grafana/dashboards/node-health.json | Update job queue panel description (but export-cycle statement needs correction). |
| docker/telemetry/grafana/dashboards/ledger-data-sync.json | Update job queue panel description (but export-cycle statement needs correction). |
| docker/telemetry/alloy/config.alloy | Add spanmetrics dimensions for new bounded attrs (timed_out/object_type/validation_status/accept_gated). |
| docker/telemetry/.env.grafanacloud-alloy.example | Clarify which attributes aren’t configurable via env vars. |
| CONTRIBUTING.md | Document metric naming rules + checker rules I/J/K; update “adding a metric” references. |
| .github/scripts/otel-naming/README.md | Document new metric rules (I/J/K) and warning rule L. |
| .github/scripts/levelization/results/ordering.txt | Update layerization ordering outputs for new dependencies. |
| .github/scripts/levelization/results/loops.txt | Update loop results to reflect dependency direction change. |
| .cspell.config.yaml | Ignore XRPL_METRIC_* macro names; add txset word. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "title": "Job Queue Saturation (Running vs Limit)", | ||
| "description": "###### What this is:\n*How close each concurrency-capped job type is to its ceiling. JobQueue enforces a per-type limit on how many jobs of that type may run at once, and the tight ones carry ledger-sync traffic: makeFetchPack 1, ledgerRequest 3, ledgerData 3, updatePaths 1, fetchTxnData 5. A type at its ceiling cannot start more work no matter how many workers are idle, so this is a different kind of limit from CPU or disk.*\n\n###### How it's computed:\n*Each jobq_<jobtype>_running gauge divided by that type's own limit from JobTypes.h, so every line shares one 0-to-1 axis. 1.0 means running equals the limit. Multiply a reading by the limit shown in its legend to recover the raw job count. JobQueue::collect snapshots all three per-type counters under the queue's own lock and publishes them after releasing it, on the 1-second export cycle.*\n\n###### Reading it:\n*Read the distance to 1.0, not the absolute height. Below 1.0 the type has spare slots and its queue wait is not the limit's fault. Touching 1.0 briefly is normal work. Sitting at 1.0 means the type is pinned at its ceiling and every further job of that type is being deferred rather than started, which is what turns into queue wait downstream. Because the limits differ, a raw count of 3 is saturation for ledgerRequest but only 60 percent for fetchTxnData; normalizing is what makes the lines comparable.*\n\n###### Healthy range:\n*Below 1.0, with brief touches under load.*\n\n###### Watch for:\n*A line flat at 1.0: that type is the binding constraint. ledgerRequest pinned means the 3 slots shared by RcvGetLedger and RcvGetObjByHash are full, so peer ledger and object requests are queueing behind each other; the Ledger Data and Sync dashboard splits that wait by handler and shows the matching deferred depth. ledgerData or fetchTxnData pinned means inbound ledger data cannot be absorbed and validated ledger age will grow. makeFetchPack or updatePaths pinned at their limit of 1 means a single long job is blocking the whole type. These are sampled gauges, so a line that never reaches 1.0 is not proof the type was never momentarily saturated.*\n\n###### Keywords:\n- **Job queue / job type** *(per node)* — xrpld's worker-thread pool; every unit of background work is enqueued under a named job type.\n- **Concurrency limit** *(per node)* — the cap on how many jobs of one type may run at once; a type at its cap cannot start more work.\n- **Deferred job** *(per node)* — a job held back because its type is already at its concurrency limit; the leading indicator of queue backpressure.\n\n###### Computation boundary:\n*Result: Per node — each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[core/JobQueue.cpp](https://github.com/XRPLF/rippled/blob/develop/src/libxrpl/core/detail/JobQueue.cpp)\n\n###### Function:\n`JobQueue::getNextJob (limit enforcement) / JobQueue::collect (publication)`\n\n###### References:\n[Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#concurrency-limit)", | ||
| "description": "###### What this is:\n*How close each concurrency-capped job type is to its ceiling. JobQueue enforces a per-type limit on how many jobs of that type may run at once, and the tight ones carry ledger-sync traffic: makeFetchPack 1, ledgerRequest 3, ledgerData 3, updatePaths 1, fetchTxnData 5. A type at its ceiling cannot start more work no matter how many workers are idle, so this is a different kind of limit from CPU or disk.*\n\n###### How it's computed:\n*Each jobq_<jobtype>_running gauge divided by that type's own limit from JobTypes.h, so every line shares one 0-to-1 axis. 1.0 means running equals the limit. Multiply a reading by the limit shown in its legend to recover the raw job count. JobQueue::collect snapshots all three per-type counters under the queue's own lock and publishes them after releasing it, on the 10-second export cycle.*\n\n###### Reading it:\n*Read the distance to 1.0, not the absolute height. Below 1.0 the type has spare slots and its queue wait is not the limit's fault. Touching 1.0 briefly is normal work. Sitting at 1.0 means the type is pinned at its ceiling and every further job of that type is being deferred rather than started, which is what turns into queue wait downstream. Because the limits differ, a raw count of 3 is saturation for ledgerRequest but only 60 percent for fetchTxnData; normalizing is what makes the lines comparable.*\n\n###### Healthy range:\n*Below 1.0, with brief touches under load.*\n\n###### Watch for:\n*A line flat at 1.0: that type is the binding constraint. ledgerRequest pinned means the 3 slots shared by RcvGetLedger and RcvGetObjByHash are full, so peer ledger and object requests are queueing behind each other; the Ledger Data and Sync dashboard splits that wait by handler and shows the matching deferred depth. ledgerData or fetchTxnData pinned means inbound ledger data cannot be absorbed and validated ledger age will grow. makeFetchPack or updatePaths pinned at their limit of 1 means a single long job is blocking the whole type. These are sampled gauges, so a line that never reaches 1.0 is not proof the type was never momentarily saturated.*\n\n###### Keywords:\n- **Job queue / job type** *(per node)* — xrpld's worker-thread pool; every unit of background work is enqueued under a named job type.\n- **Concurrency limit** *(per node)* — the cap on how many jobs of one type may run at once; a type at its cap cannot start more work.\n- **Deferred job** *(per node)* — a job held back because its type is already at its concurrency limit; the leading indicator of queue backpressure.\n\n###### Computation boundary:\n*Result: Per node — each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[core/JobQueue.cpp](https://github.com/XRPLF/rippled/blob/develop/src/libxrpl/core/detail/JobQueue.cpp)\n\n###### Function:\n`JobQueue::getNextJob (limit enforcement) / JobQueue::collect (publication)`\n\n###### References:\n[Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#concurrency-limit)", |
Comment on lines
+514
to
+515
| if (disconnectReason_ == std::string_view{telemetry::lval::disconnect::unknown}) | ||
| disconnectReason_ = reason; |
| { | ||
| "title": "Job Queue Backlog and Deferred by Type", | ||
| "description": "###### What this is:\n*Per-job-type queue depth, two series per type. Waiting is the whole backlog: every job enqueued for that type that has not started yet. Deferred is the subset of that backlog that is blocked specifically because the type is already running at its concurrency limit. Deferred is the leading indicator of backpressure, because JobQueue::addJob never rejects a job for queue pressure -- it returns success and defers instead, so a capped type under pressure produces no error and no dropped work. Without these the only evidence is latency, which appears after the harm is already done.*\n\n###### How it's computed:\n*Two targets, each the top 10 gauges by current value: jobq_<jobtype>_waiting and jobq_<jobtype>_deferred. JobQueue::collect snapshots both counters under the one lock that guards them, so the pair is read at the same instant and is directly comparable, then publishes them on the 1-second export cycle. Gauges exist only for non-special job types, so the 11 special types -- the ones declared with a limit of 0, which bypass the limit logic entirely and therefore never defer -- do not appear on either series.*\n\n###### Reading it:\n*Read the two together; the ratio is the diagnostic, not either value alone. Deferred is always a subset of waiting, because addRefCountedJob increments waiting for every job and deferred only for the ones that arrive while the type is at its limit. Waiting high with deferred at zero means the type has spare slots and the backlog is just arrival burstiness -- it will drain without intervention. Waiting high with deferred also high means the concurrency limit is the binding constraint, not the work. Both near zero is the normal state. These are depths, not rates: the value is how many jobs are queued right now. finishJob drains deferred one per completion, so a deferred line that stays elevated means arrivals are outpacing completions rather than one isolated burst. Only the 10 highest series per state are drawn, which on an idle node is arbitrary among the zeros and under load is exactly the types under pressure.*\n\n###### Healthy range:\n*Deferred zero on all types. Waiting near zero, with brief spikes during ledger close.*\n\n###### Watch for:\n*ledgerrequest deferred above zero: the 3-slot ledgerRequest queue is full, so TMGetLedger service to syncing peers is being delayed. Use LedgerReq Wait by Handler next to see which of its two producers is responsible. ledgerdata or fetchtxndata deferred: inbound ledger data cannot be absorbed fast enough, which is what makes validated ledger age grow. A waiting line that climbs steadily while deferred stays flat points at the worker pool or at slow jobs rather than at the limit. Note both are sampled once per export cycle, so a sub-second spike can be missed; a reading of zero is not proof that nothing was ever queued or deferred.*\n\n###### Keywords:\n- **Deferred job** *(per node)* — a job held back because its type is already at its concurrency limit; the leading indicator of queue backpressure.\n- **Concurrency limit** *(per node)* — the cap on how many jobs of one type may run at once; a type at its cap cannot start more work.\n- **Job queue / job type** *(per node)* — xrpld's worker-thread pool; every unit of background work is enqueued under a named job type.\n\n###### Computation boundary:\n*Result: Per node — each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[core/JobQueue.cpp](https://github.com/XRPLF/rippled/blob/develop/src/libxrpl/core/detail/JobQueue.cpp)\n\n###### Function:\n`JobQueue::addRefCountedJob / JobQueue::collect`\n\n###### References:\n[Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#deferred-job)", | ||
| "description": "###### What this is:\n*Per-job-type queue depth, two series per type. Waiting is the whole backlog: every job enqueued for that type that has not started yet. Deferred is the subset of that backlog that is blocked specifically because the type is already running at its concurrency limit. Deferred is the leading indicator of backpressure, because JobQueue::addJob never rejects a job for queue pressure -- it returns success and defers instead, so a capped type under pressure produces no error and no dropped work. Without these the only evidence is latency, which appears after the harm is already done.*\n\n###### How it's computed:\n*Two targets, each the top 10 gauges by current value: jobq_<jobtype>_waiting and jobq_<jobtype>_deferred. JobQueue::collect snapshots both counters under the one lock that guards them, so the pair is read at the same instant and is directly comparable, then publishes them on the 10-second export cycle. Gauges exist only for non-special job types, so the 11 special types -- the ones declared with a limit of 0, which bypass the limit logic entirely and therefore never defer -- do not appear on either series.*\n\n###### Reading it:\n*Read the two together; the ratio is the diagnostic, not either value alone. Deferred is always a subset of waiting, because addRefCountedJob increments waiting for every job and deferred only for the ones that arrive while the type is at its limit. Waiting high with deferred at zero means the type has spare slots and the backlog is just arrival burstiness -- it will drain without intervention. Waiting high with deferred also high means the concurrency limit is the binding constraint, not the work. Both near zero is the normal state. These are depths, not rates: the value is how many jobs are queued right now. finishJob drains deferred one per completion, so a deferred line that stays elevated means arrivals are outpacing completions rather than one isolated burst. Only the 10 highest series per state are drawn, which on an idle node is arbitrary among the zeros and under load is exactly the types under pressure.*\n\n###### Healthy range:\n*Deferred zero on all types. Waiting near zero, with brief spikes during ledger close.*\n\n###### Watch for:\n*ledgerrequest deferred above zero: the 3-slot ledgerRequest queue is full, so TMGetLedger service to syncing peers is being delayed. Use LedgerReq Wait by Handler next to see which of its two producers is responsible. ledgerdata or fetchtxndata deferred: inbound ledger data cannot be absorbed fast enough, which is what makes validated ledger age grow. A waiting line that climbs steadily while deferred stays flat points at the worker pool or at slow jobs rather than at the limit. Note both are sampled once per export cycle, so a sub-second spike can be missed; a reading of zero is not proof that nothing was ever queued or deferred.*\n\n###### Keywords:\n- **Deferred job** *(per node)* — a job held back because its type is already at its concurrency limit; the leading indicator of queue backpressure.\n- **Concurrency limit** *(per node)* — the cap on how many jobs of one type may run at once; a type at its cap cannot start more work.\n- **Job queue / job type** *(per node)* — xrpld's worker-thread pool; every unit of background work is enqueued under a named job type.\n\n###### Computation boundary:\n*Result: Per node — each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[core/JobQueue.cpp](https://github.com/XRPLF/rippled/blob/develop/src/libxrpl/core/detail/JobQueue.cpp)\n\n###### Function:\n`JobQueue::addRefCountedJob / JobQueue::collect`\n\n###### References:\n[Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#deferred-job)", |
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
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.
Consolidated PR: #7770
High Level Overview of Change
Makes fresh-node ledger-sync problems diagnosable. A node that takes a long time to reach
full, or never gets there, previously left an operator with the coarse picture only: the mode it was in, how stale its validated ledger was, and how much peer traffic was flowing. None of that answers the three questions actually asked during an incident — how far behind am I, why is this particular ledger not arriving, and is this a peer problem, a disk problem, or a bootstrap misconfiguration.Adds 41 native metrics and 6 spans across the whole fresh-start path, a "Ledger Sync Health" dashboard, and a runbook that branches on the observed symptom rather than listing signals.
Context of Change
The starting point was an audit of the real startup path, traced from the XRPL server-state, peer-protocol and ledger-history docs plus the protobuf definitions, then reconciled against the code. It produced 18 stages from process launch to
full.The result was that existing telemetry covers the post-peering pipeline reasonably well and is blind to the entire pre-quorum bootstrap chain — which is where a fresh node, as opposed to a restarting one, most often gets stuck. Six files on that path carried no instrumentation at all: DNS resolution, the outbound dial, protocol and network-id negotiation, the validator-list fetch, the trusted-key/quorum bootstrap, and clock skew. The single worst gap: an unreachable validator-list site or a bad
validators.txtyields zero trusted keys, so quorum can never form and the node sits insyncingindefinitely, with nothing in telemetry to say why.Two patterns recurred and are fixed once rather than per signal:
ledger.acquireonly recorded one on the normal completion path, so an acquire that stalled and was later swept ended with no outcome and a duration stretched to the sweep interval. The one case these signals exist to catch was the one case that could not be traced.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)libxrplgains read-only accessors so xrpld telemetry can observe library state without libxrpl depending on telemetry: worker-pool occupancy onJobQueue, a store-duration total and accessor on the node store, missing-node and stash depth on the inbound ledger,getBad()/getDuplicate()onSHAMapAddNode, and a copy-forward total on the rotating database. No behaviour change to any of them.What Changed
Bootstrap, previously uninstrumented — DNS resolve outcome and latency; outbound dial outcome and latency; negotiation failures by reason, so a wrong
network_idis distinguishable from unreachable peers; validator-list fetch outcome per site, and trusted key count against the required quorum; the network close-time offset, whichserver_infohides below 60 seconds.Sync state machine — mode transitions labelled
from/to, so a healthy climb is distinguishable from flapping; time to firstfull; the network-ledger gate; server stall seconds; how many ledgers behind the network.Ledger acquire and SHAMap fetch — outstanding missing tree nodes per acquire, which is what separates "slow" from "will never finish"; tree-node cache hit rate; a no-progress counter; add-node good/duplicate/invalid; whether a ledger came from local storage or the network.
Job queue — whole-pool saturation, complementing the per-type occupancy from the base branch.
Quorum and publish — the trusted-validation tally against quorum at the pre-accept gate, and the gap between validated and published sequences.
Back-fill and persistence — node-store read and write service time, where the write side had no signal at all; fetch-pack no-peer; replay fallback and outcome; the per-sweep heap-trim cost and the
online_deleterotation write amplification, both named in the 3.3.0 slowdown investigation.Peer supply and terminal blockers — how many peers can actually serve the wanted sequence; disconnect reasons and connect outcomes, previously one undifferentiated number; the PeerFinder slot census; serve refusals; the amendment-block countdown; wrong-chain ledger jumps.
Spans —
txset.acquire(transaction-set acquisition had none), the acquire split into header/account-state/transaction-tree phases, the outbound dial, and serving a peer's request. Every span finalises exactly once on every exit, including the destructor.One trace per ledger — the acquire, validation-accept, validate and store spans now derive their trace id from the ledger's own hash, so one Tempo search shows whether a slow ledger was slow to fetch, to be accepted, or to persist. They are siblings rather than a chain because the accept gate is entered from three different threads.
Metric naming — every instrument name, label key and bounded label value is a compile-time constant, and the naming check gained three rules enforcing that plus the duration and counter suffix conventions, which nothing enforced before.
Dashboard and docs —
ledger-sync-health.json, 39 panels in 9 rows ordered the way a node actually progresses; a runbook decision tree branching on symptom; reference and glossary entries for every signal.Before / After
Test Plan
service_instance_idTen signals are documented rather than asserted, each with a note saying why: a healthy five-node localhost cluster cannot produce a peer disconnect, a serve refusal, a wrong-chain jump, a replay fallback, an amendment block, or an
online_deleterotation. Asserting them would fail CI on a healthy run.Future Tasks
[ledger_replay].FullBelowCachereports a hit rate of zero becauseKeyCacheupdates different counters thangetHitRate()reads. Pre-existing in libxrpl; the panel is deliberately not shipped rather than shipping a permanently empty series.