Brings the telemetry startup-ordering fix forward. Two conflicts, both in
the metrics registry.
initSyncInstruments(): this branch had already replaced the registry-owned
state_changes_total with a call-site macro carrying {from,to} labels, and
the arriving branch moves the jq_trans_overflow_total observable out of
this function into registerJqTransOverflowCounter() so it is armed with
the other pull-model instruments. Kept both: the explanatory comment for
state_changes_total stays, the inline overflow block goes. The arriving
stateChangesCounter_ creation is dropped rather than merged -- this branch
removed that member, so keeping the line would not compile.
Test file: kept this branch's fuller header documentation, which records
what the disabled build can and cannot assert for the sync-diagnostics
gauges, and folded in the arriving branch's one new fact, that the
lifecycle is now two-phase. Include lists unioned.
Resolves the telemetry-startup conflict between the two branches. Both
sides move the telemetry start earlier in setup(); they disagree only on
how far the pipeline had been split at that point.
phase-1b (arriving) moved nodeIdentity_, setServiceInstanceId() and the
telemetry start up to just after the wallet DB is proven usable. phase-9
had split the metrics pipeline in two and left its copy of that block at
the old, later position.
Kept both intentions: the block stays at phase-1b's early position, and
metricsRegistry_ construction moves up with it so it precedes
startTelemetry() -- the metrics half is guarded on the registry existing,
so leaving the construction behind would have started tracing while
silently skipping metrics. phase-9's later copy is dropped as the stale
duplicate. The two-phase split is preserved: startTelemetryGauges() still
runs after overlay_ is constructed, because the observable callbacks read
it and getOverlay() asserts.
Net effect is that the metrics provider now starts earlier than on either
branch, and still before beginConsensus() emits the first spans and the
only operating-mode transition.
base_fee_xrp was observed from LoadFeeTrack::getLocalFee(), which is the
local load-scaled fee escalation, not the ledger's base fee. The panel
built on it therefore tracked this node's load state rather than the
network's cost of a reference transaction, and read as a flat line
whenever the node was unloaded.
Read it from the validated ledger's fee settings instead, alongside the
reserve values already taken from there. The observation now only
reports when a validated ledger is available, which is correct: before
that there is no network fee to report.
MetricsRegistry::start() fused three steps with different prerequisites:
the exporter/provider and the synchronous instruments need only config
strings, while the observable gauges read live Application services. The
whole pipeline therefore waited on the latest prerequisite and ran near
the end of setup() -- after beginConsensus() had already recorded the
process's only operating-mode transition. state_changes_total was
emitted into a pipeline that did not exist yet, so the series never
appeared at all.
Split the two halves. start() keeps the provider and the synchronous
instruments and now runs as soon as the registry is constructed; the new
startAsyncGauges() registers the observable callbacks and runs once
overlay_ exists, still before the first consensus round. Application
gains a matching startTelemetryGauges() so each phase has its own call
site and its own precondition.
Move the jq_trans_overflow_total observable out of initSyncInstruments()
into the gauge phase. Its callback reads getOverlay(), which asserts
overlay_ is non-null, so creating it in the early phase armed the reader
thread against a half-built application -- an assert is not caught by
the callback's catch-all. The instrument is an observable counter rather
than a gauge, which is how it was mistaken for a push-only instrument.
Both start log lines are kept, one per phase, because that timeline is
what made the original ordering bug diagnosable.
Comments and preconditions are corrected to state the rule rather than
the current arrangement: start() may only create instruments whose
values are pushed, and any observable whose callback reads a service
belongs in the gauge phase. The gauge precondition now lists the
services the callbacks actually read.
telemetry_->start() ran at the end of ApplicationImp::start(), after
overlay_->start(). Spans are emitted well before that, during setup():
beginConsensus() runs the first consensus round there. SpanGuard drops a
span whenever the global Telemetry instance is not yet live, so that
round's spans were never recorded.
Move the start into setup(), behind a new startTelemetry() seam, right
after the node identity is known. getNodeIdentity() needs only the
cmdline, the config, or the wallet DB, and initRelationalDatabase()
has already created the latter -- the adjacent peerReservations_ load
proves it is usable -- so the identity block moves up with it.
The new position is bounded on both sides: after initRelationalDatabase()
because the identity needs the wallet DB and a DB failure aborts setup(),
and before beginConsensus() because that emits the first spans.
Telemetry must read state, never change it. Two defects here did change it,
plus three smaller correctness and privacy fixes.
doPathFind and doRipplePathFind read source_account / destination_account off
context.params to hash them into span attributes. context.params is non-const,
so those reads selected json::Value's non-const operator[], which inserts a
null for a missing key. The same object is later validated by
PathRequest::parseJson, whose first checks are isMember(source_account) and
isMember(destination_account) — so a request that omitted either field looked
present and the client received Malformed instead of Missing. Reads now go
through std::as_const, whose overload returns kNull without inserting.
PathRequest::doUpdate emitted pathfind_dest_currency as
to_string(saDstAmount_.asset()). For a non-XRP asset that renders as
"<issuer>/<currency>" with the issuer as a plaintext Base58 address, so a
plain account address reached the span pipeline even though every other
account here is hashed first. The issuer is now redacted and the currency
kept; an MPT asset renders as its issuance ID and carries no address.
PathRequestManager::updateAll created pathfind.update_all with an unscoped
SpanGuard. An unscoped guard takes the ambient span as its own parent but does
not itself become the ambient parent, so the pathfind.compute spans that
doUpdate creates never nested under it, contradicting the documented hierarchy.
It is now a scoped guard, held in std::optional because ScopedSpanGuard is
deliberately non-movable and so cannot be produced by a ternary. The skip when
there are no active subscriptions is preserved. updateAll is dispatched via
addJob and doUpdate runs synchronously, so the guard is constructed and
destroyed under the same context store, as ScopedSpanGuard requires.
The WebSocket entry point emitted the client-supplied command string directly.
That value becomes a Prometheus label, so arbitrary request input could drive
unbounded label cardinality. It is now resolved against the handler registry,
collapsing anything unrecognized to "unknown", matching what the HTTP path
already does.
Also: the pathfind.discover comment claimed future child spans could be
parented off it, which its unscoped guard cannot do — corrected to say what
would be required instead. Config-reference and task-list docs named the
parser setupTelemetry(); the API is makeTelemetrySetup().
Conflict in processSession(): this branch added the request-payload-size
attribute on the same lines where the incoming change reverted
processRequest() from bool back to void. Kept both — the payload-size
attribute stays, and the call no longer captures a return value. The
trailing status block takes the incoming side, which leaves the
rpc.http_request span's status unset.
Six related defects in the RPC/gRPC span surface, all cases where a failure
was recorded as success or an attribute was missing on an error path.
GRPCServer: the non-exception branch set the span Ok unconditionally, then
sent a possibly-failed grpc::Status. The handler can return a non-OK status
without throwing, so every failed call traced as successful. Status now
follows result.second, with the error message as the span description.
ServerHandler: eight per-item error branches appended an error reply without
recording that the request failed. Batch responses and ripplerpc < 3.0 always
carry HTTP 200, so those failures were invisible and an entirely failed batch
ended its span as successful. Added an appendItemError() helper next to the
existing httpReplyError() lambda and routed all eight sites through it, so the
flag cannot be forgotten at a new call site.
ServerHandler: the early-return validation paths set the span error but not the
rpc_status attribute. Added it to httpReplyError() so every such path gets it.
RPCHandler: the fillHandler error path set only command and rpc_status, while
callMethod sets command, version and rpc_role. Error spans were therefore not
filterable by API version or role. The error path now mirrors that set.
RPCHandler: resolveCommandSpanName() checked only that command/method were
present, not that they agreed, while fillHandler rejects a mismatch as
rpcUNKNOWN_COMMAND. A request supplying both with different values was labelled
with one of the two names, misattributing the error to a command that never
dispatched. It now mirrors fillHandler's rule and collapses to "unknown".
ServerHandler: processRequest returned bool solely so the caller could set its
span status. Telemetry should read state, not shape the signature of the code it
observes, so the signature returns to void and rpc.process sets its own status
from spanHadError. The enclosing rpc.http_request span now leaves status unset:
the OTel spec has instrumentation leave status unset unless the operation itself
errored, and reserves Ok for an operator asserting verified success.
Conflict in src/libxrpl/telemetry/SpanGuard.cpp: both sides added a different
include in the same alphabetical slot — phase-4 added <initializer_list>, the
incoming std::format change added <format>. Both are required, so both are
kept in sorted order.
SpanGuard::span() and freshRoot() joined the prefix and suffix by hand with
reserve() + three append() calls. std::format expresses the same join in one
line; libstdc++ has shipped <format> since GCC 13 and the project floor is
GCC 15.2 (BUILD.md), so it is available on every supported compiler.
Both callers are noexcept and std::format can throw (std::bad_alloc, or
std::format_error on a malformed spec), so an escaping exception would
terminate the process. The call is wrapped in a joinSpanName() helper that
catches and returns std::nullopt; the caller then returns a null guard, which
is the same degrade-to-no-op path already taken when telemetry is disabled.
Telemetry must never bring the node down.
One helper rather than a try/catch at each site keeps both call sites a single
line and puts the exception-safety requirement in one documented place.
Addresses a review comment on PR #6437.
Resolves src/tests/libxrpl/CMakeLists.txt test_modules by keeping both
sides: develop's beast/nodestore/protocol additions (nodestore moved into
alphabetical position) and this branch's ledger module.
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.
The lane-attribution change added acquire_ledger_deferrals and
acquire_ledger_timeouts to the gauge but left this test expecting seven
labels, so it failed on every platform of both PRs: nine emitted against
seven expected. The exporters and the counters went in as separate
commits, which is how the test escaped the update.
The two new labels are asserted with values that differ from the all-lane
totals they are a subset of, so a counter wired to the wrong lane, or one
that ignored the flag and counted every lane, lands on the totals instead
and fails.
The round built each thread's batch inside the thread, before arriving at
the latch, so a throw there left the remaining threads waiting on an
arrival that never came -- the test hung instead of failing. A spawn loop
that ended early did the same.
Batches are now built before any thread starts, so nothing between spawn
and arrival can throw, and a guard counts down the shortfall for threads
that were never spawned before joining the ones that were.
The depth accounting having moved to insert entry, depthSamples is now
the denominator of the mean depth, so it gets its own assertions: equal
to insertCount once every thread has been joined, and moving with the
duplicate-key round. The overlap assertions are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The counters that attribute deferrals and timeouts to ledger acquisition
compared a string literal in TimeoutCounter against a separate literal in
InboundLedger. Renaming the job would have silently sent both metrics to
zero: no build error, no failing test, and documentation that quietly
became wrong. Both now use one shared constant.
