Resolutions:
- MetricsRegistry.cpp: keep both <exception> and <limits>; drop
incrementStateChanges(), which this branch removed on purpose (the
labelled state_changes_total call site in NetworkOPsImp::setMode
replaces it, and a compile-time test guards that).
- tests/MetricsRegistry.cpp: constructor-built pipeline wording from
phase-10, this branch's test list and gauge paragraphs kept; the two
lifecycle tests now call startAsyncGauges() and pass kTestOptions.
- tests/MetricMacros.cpp: comments name the recording() gate.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three conflicts, all from both branches editing the same passage:
- Main.cpp: kept phase-9's wording. The metrics registry only exists here, so
"unwinding destroys little: the metrics registry, whose destructor joins its
export thread" is the true statement on this branch.
- TESTING.md: kept both paragraphs. They document different things (the
private [network_id], and the log path plus log_level).
- 05-configuration-reference.md: composed both. The identity is now resolved
before construction and never empty, so every producer stamps the node key
from the start; the only divergence left is a wallet that already holds a
different key, which corrects the tracer alone. Rewrote the earlier
"three producers" blockquote too: its "no fallback", "first boot ... left
off" and "Known issue" claims are what this change removes.
One silent break the merge could not flag: makeMetricsRegistryOptions() took
the std::optional<std::string> node key that used to be a constructor
parameter, and that parameter is now the resolved keypair. It takes the base58
string directly, and the constructor derives it from nodeIdentity_, which is
declared before both telemetry_ and metricsRegistry_.
resolveNodePublicKey() returned std::nullopt in three real cases: a first boot
with no wallet database, a standalone run (its wallet is a private temporary
database), and --newnodeid. Telemetry's resources are built during
ApplicationImp's member-init list and are immutable, so on those runs the node
reported an empty service.instance.id and no xrpl.node.id for the whole run,
while setup() minted a key moments later and patched only the tracer.
Replace it with resolveNodeIdentity(), which always returns a keypair: derived
from a configured seed, else read from an existing wallet database, else
minted. Main.cpp passes that pair to makeApplication(), ApplicationImp stores
it in nodeIdentity_ -- now declared before telemetry_ and no longer an optional,
because it is always set -- and builds the telemetry resource from it.
setup() calls getNodeIdentity(), which now persists rather than mints: it
stores the resolved pair when the wallet holds no identity, adopts the stored
one when it does, and clears first for --newnodeid. The write stays in setup()
because that is where the database exists; a standalone run has no persistent
wallet to write to, which is why the pair has to be decided before
construction rather than read back afterwards. Wallet gains storeNodeIdentity()
for that write, and getNodeIdentity(session) now uses it instead of repeating
the insert.
The three-argument makeApplication() mints a keypair, so jtx::Env and any other
test Application behave as a standalone run always did.
Also fold the three hand-rolled "meter from a NoopMeterProvider" copies into
telemetry::noopMeter(): the base-pointer call and the kMeterVersion argument are
both easy to get wrong alone, and the meter identity has to match the one the
histogram views select on.
The new gtest covers the wallet half: store-then-read, store not replacing an
existing identity, clear-then-store, and that the mint path persists. It adds
the tests.libxrpl > xrpl.rdb levelization edge, regenerated here.
Review feedback asked for a Histogram rather than a span attribute at these two
places. Both, not either: the attribute answers how big one sampled request was,
which an aggregate cannot, and the histogram answers the distribution across all
requests, which an unsampled trace never reveals. Both attributes stay.
The metrics land here rather than with the attributes because neither
HistogramBuckets.h nor the metric macro exists on the branch that added them.
Both use kObjectCountBuckets. The argument is the floor, not the ceiling: the SDK
default edges start 0,5,10,25, so an ordinary batch of one to five falls in a
single bucket and every quantile becomes an interpolation on one edge. The object
ladder puts five edges over the mass of both distributions. Path count is bounded
at 352 by kMaxPaths times kMaxAutoSrcCur and cannot saturate. Batch size can, at
roughly 333k, but no measured traffic goes near it, so the ladder is not widened
for a range nothing occupies; the runbook records the overflow query and a test
asserts it stays readable.
Brings in the phase-10 revert of the nodestore read-latency histogram plus the
nudb_bytes -> stored_object_bytes rename.
Conflicts in MetricsRegistry.{h,cpp} resolved keeping both intents:
- MetricsRegistry.cpp: dropped everything that existed only to serve the
reverted nodestore_read_us histogram -- the addSubMillisecondHistogramView()
helper, its call site, the kSubMillisecondBoundaries array and the
NodeStoreMetricNames.h include. Kept every view this branch registers
(consensus round duration, sweep_malloc_trim_us, dns_resolve_latency_ms,
overlay_dial_latency_ms) and the shared addHistogramView() base helper.
Took the rename at the storage_detail observe() call site.
- MetricsRegistry.h: took phase-10's move of the four nodestore_state observe
helpers and their ObserveFn sink from private to public, while keeping this
branch's enriched Doxygen on observeNodeStoreTotals().
Also corrected the registered-view count in the 09 reference doc: neither side's
arithmetic survives the merge, since this branch adds four views phase-10 never
saw and the revert removes one. Ten views are registered now, not six or seven.
The 22 metric label values on the nodestore_state gauge were asserted
nowhere, and the depthSum assertions could not tell the real
fetch_add(depth) from a fetch_add(1) that would silently zero every
derived queueing time.
Make the four observe* helpers and their ObserveFn sink public rather
than private, so the seam the header already claimed is actually
reachable. All four are static and read only their arguments, so this
exposes no object state; a friend declaration would have granted access
to every private member instead. The assertions live in the existing
Beast nodestore suite because src/test compiles into xrpld, which
contains MetricsRegistry.cpp, while xrpl_tests deliberately does not
when telemetry is enabled.
Each helper now has its exact emitted label set asserted, so a typo in
any literal fails instead of silently producing a disjoint series, and
each derived mean is asserted ABSENT on a fresh store -- a refactor to
value_or(0) would draw a believable flat zero on a latency axis and
otherwise pass everything.
Replace the concurrent write-stats test with one that forces genuine
overlap through a latch. NuDB holds one global mutex for the whole
insert and doInsert reads the depth before entering it, so a blocked
thread records a depth of at least 2; asserting depthSum strictly
exceeds insertCount therefore cannot be satisfied by a constant 1. The
old bounds admitted that bug at their floor.
Also: cover the std::nullopt branch on the two backends that exist in
every build, bound the store-duration accumulator by the wall clock,
drop four assertions that cannot fail, and correct two comments that
claimed coverage the tests do not have -- the duplicate-key test is not
the throwing path, because nudb reports key_exists without throwing,
and no test drives NodeStoreScheduler::onFetch.
Two conflicts, both additive-vs-additive; each resolution keeps both sides.
check_otel_naming.py -- phase-10 taught the L6 label extractor to match the
label MAP first and to resolve a key hoisted into a `k...Label` constant,
scanning headers as well as sources. Our side had added the two-regex
first/subsequent literal scan and the `metric_constants(root)[1]` union that
covers the `namespace label` header style.
Kept phase-10's mechanism whole: METRIC_LABEL_MAP + the `(?:^|\{)` key regex
already subsumes what METRIC_LABEL_NEXT did, since matching inside the map body
makes every pair after the first open with a single `{`. So METRIC_LABEL_NEXT is
dropped as genuinely redundant rather than kept as a duplicate scan, and the
reason it existed is folded into METRIC_LABEL's comment. Re-added our
`metric_constants(root)[1]` union on top: LABEL_CONST_DEF only matches
`k`-prefixed identifiers, so it cannot see MetricNames.h's `label::jobType`
style, and without that union Rule D would reject dashboards querying labels
Rule I forced into constants. The two derivations are complementary and both
are now documented as such.
MetricsRegistry.cpp -- both sides added a new sibling view-registration helper
next to addMicrosecondHistogramView, and both added a registration call in
initExporterAndProvider(). Kept all four helpers
(addHistogramView/Microsecond/RoundDuration/SubMillisecond) and every
registration: phase-10's addSubMillisecondHistogramView + kNodeStoreReadUs
alongside our addRoundDurationHistogramView, sweepMallocTrimUs and the two
millisecond dial/resolve ladders.
phase-10's nodestore_read_us histogram does not duplicate our work. The
nodestore_latency gauge that would have overlapped it was retired in c4e434d520
before this merge, and the surviving nodestore_state gauge is complementary
rather than duplicative: both read the same fetch measurement, but the gauge
publishes only a since-boot mean via scaledMean() and cannot yield a
percentile -- the consequence observeNodeStoreTotals' own docs state plainly --
while the histogram buckets each fetch and can. The histogram also splits by
fetch_type and found, which the gauge cannot. phase-10 registered its
explicit-bucket View, so it does not inherit the SDK default ladder.
Each file keeps its own existing naming style: phase-10's k-prefixed constants
are left as-is, ours stay namespaced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- JobQueue_test: drop the unused <functional>, and make the two
read-only GaugeFixture instances const. The other two stay mutable
because they submit jobs through fixture.queue.
- DatabaseConfig_test: drop the unused SystemParameters.h and include
ByteUtilities.h for megabytes(), which was reached only transitively.
Regenerate ordering.txt for the resulting edges: test.unit_test arrives
with SuiteJournal.h, and xrpl.protocol leaves with SystemParameters.h.
Phase-10 brought in the upstream nodestore/peerfinder/consensus reorganisation
along with its own write-path telemetry, which collided with the sync-diagnostic
signals on this branch. Twelve files conflicted; every resolution keeps both
intents rather than picking a side.
The nodestore write timing was implemented twice, independently. Both sides
added getStoreDurationUs()/getFetchDurationUs() to Database and both timed the
backend call in each concrete store(). Keeping both would have added twice to
storeDurationUs_ per store while storeStats() still counted one, so the mean
write latency would have read double on every dashboard -- silently, since no
test on either side asserts an exact microsecond figure. Resolved to one
accumulator API: recordStoreDuration(), which takes a duration, clamps a
sub-microsecond sample to zero and uses a relaxed atomic add. Phase-10's
storeDurationStats() is gone and its two call sites now use the survivor, so
all three store paths -- both store() overrides and importInternal() -- add
exactly once.
SlotCensus and its pure virtual moved from src/xrpld/peerfinder/ to
include/xrpl/peerfinder/PeerfinderManager.h, following the Manager interface
upstream relocated. The xrpld header is now phase-10's makeConfig shim, and
Overlay.h, MetricMacros.cpp and the getSlotCensus() override chain point at the
new location. ConsensusSpanNames.h and peerfinder Slot.h/Config.h include paths
followed their headers into libxrpl the same way.
InboundLedger gained phase-10's AcquireStats counters next to this branch's
span activations in both the destructor abort path and done(); neither
displaces the other. nodestore_state keeps the constant-based name this branch
requires of it and phase-10's fuller description.
Upstream #7292 deleted src/test/nodestore/Database_test.cpp, which held this
branch's testDurationAccessors. Phase-10 restored the per-store half of that
coverage in DatabaseConfig_test, but nothing covered importInternal -- it writes
through storeBatch() and never through store(), so it is a third store path that
has to time itself. That half is ported to a GTest in
src/tests/libxrpl/nodestore/Database.cpp, keeping the exact zero-before and
accumulate-after assertions and the per-instance negative check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A saturated ledgerData lane makes TimeoutCounter re-arm its timer
without running the timer body, so timeouts_ never advances and the
six-timeout give-up can never fire. Acquisitions then neither finish
nor fail until the one-minute sweep destroys their partial maps, and
the work restarts. Every step of that chain was debug-log-only, so a
node at warning level could not be diagnosed after the fact.
The counters are separate on purpose: deferrals rising while timeouts
stay flat is the signature, and no single counter shows it.
Completions are recorded in done() rather than at the "Done: complete"
log line, because that line also fires for failures and misses the
checkLocal and receiveNode paths; done() is the one funnel every
outcome passes through and its signaled_ guard makes it idempotent.
AcquireStats is only forward-declared in ServiceRegistry so libxrpl
still includes nothing from xrpld. The src/ include path for the test
binary moves out of the telemetry guard, since a header-only type
under src/xrpld/ is testable in every build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The consensus headers moved into the isolated xrpl/consensus module and
took a dependency on xrpl/telemetry for the tracing span constants, but
two things were left behind:
- Four includes still pointed at the old src/xrpld/consensus/ location,
which no longer exists, so the build failed with
"fatal error: 'xrpld/consensus/ConsensusParms.h' file not found".
- xrpl.libxrpl.consensus never linked xrpl.libxrpl.telemetry. add_module
isolates each module's headers, so xrpl/telemetry/SpanNames.h was not
on the include path even once the include was repointed.
Repoint the stale includes at xrpl/consensus/, and declare the telemetry
module before consensus so consensus can link it. Regenerate ordering.txt
for the resulting edge.
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>
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>
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>
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>
Brings coroutine-aware context storage + tx/consensus worker-body activation.
Resolved: Telemetry.cpp keeps both meterProvider_ (phase-7) and contextStorage_
(coro-aware); doc-09 keeps phase-7 structure and applies the pathfind.request →
rpc.command.<name> correction to phase-7's own PathFind section.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings coroutine-aware context storage, scoped rpc.command, coro-store-swap
tests, and the scoped pathfind.request forward.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings coroutine-aware OTel context storage forward: CoroAwareContextStorage,
its install in Telemetry, ScopedSpanGuard same-store assertion, and the
non-owning ScopedActivation helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Backs the OTel active-context stack with xrpl::LocalValue so the ambient
context follows a JobQueue::Coro across yield/resume. Not yet installed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Convert leading /// blocks to house-style /** */ across the telemetry
files carried on this branch (using the updated fix_doxy.py). Also
regenerate levelization results. Comment-only: code is byte-identical.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bring the 5 telemetry files introduced on this branch to the enforced
house Doxygen style (/** alone, ' * ' continuation prefix, no single-line
blocks) so the check-doxygen-style hook passes under CI's --all-files run.
Comment-only: code is byte-identical after comment stripping (verified).
Also regenerate levelization results: the committed ordering.txt carried
stale 'xrpl.telemetry > xrpld.consensus/rpc' edges that the current include
graph no longer produces.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
