Eight attributes added on phase 4 were missing from the attribute catalogue.
Seven are on consensus.phase.open, which had no entries at all; the eighth is
the terminal regime on consensus.establish.
Does not touch the neighbouring proposers_agreed row, which names an attribute
the code never sets -- pre-existing and outside this change.
Review of the preceding commits found a clang-tidy failure and a convention
break, both rooted in the same place: the enum-to-label helpers were put in
ConsensusSpanNames.h, which pulled two domain headers into it.
misc-include-cleaner rejected the new test: it used xrpl::LedgerCloseReason
without directly including ConsensusTypes.h, relying on the transitive
include. misc-* is enabled and this path is not in IgnoreHeaders, so it would
have failed CI.
ConsensusSpanNames.h had also become the only one of the eight *SpanNames.h
headers to include anything beyond SpanNames.h. That cost is paid by every
consumer: PeerImp.cpp, ConsensusReceiveTracing.h and RCLConsensus.cpp want
only name and key constants, but were newly compiling ConsensusTypes.h and
DisputedTx.h through it.
Move both helpers to a new ConsensusSpanLabels.h, which owns the domain
includes. ConsensusSpanNames.h is dependency-free again like its siblings, and
the labels reach their only production caller, Consensus.h, directly.
Also from the review:
- phaseOpen() had grown to 81 lines, over the 80-line limit. Extract
annotateOpenStart() and annotateOpenClose(), which also removes the repeated
span guards. phaseOpen is 72 lines; startRoundInternal drops 103 to 93,
still over the limit but it was 99 before this work began.
- Note at the CLOG why the log text keeps the shouldCloseLedger name: existing
consumers match on it.
- whyCloseLedger's doc claimed "both log identically", implying the wrapper
logs too. It delegates, so the logging happens once either way.
- Cross-reference proposers_validated and proposers_finished, which sit eight
lines apart and count different things: validators of the previous ledger
versus those already past it.
- The two static_asserts no longer sit inside TEST bodies with SUCCEED(); they
fire at compile time regardless. Also "consteval-safe" was wrong; they are
constexpr.
- SpanGuardFactory.cpp claimed a libxrpl test cannot include the consensus
span-name header. The new test in the same directory does exactly that, so
the claim is corrected to name the real constraint: the rpc_* constants it
needs live in an xrpld-level header.
Three defects found in review of the two preceding commits.
Drop disputes_count_initial. It claimed to be the dispute count carried in
from the positions held at close, but startEstablishTracing() runs a full
timer tick after closeLedger(): timerEntry() dispatches
`if (phase_ == Open) phaseOpen(); else if (phase_ == Establish)
phaseEstablish();`, and phase_ was Open on the closing tick, so the else-if
cannot run. With ledgerGRANULARITY at 1s the value absorbed up to a second of
dispute growth from peer proposals and arriving tx sets. Making it honest
needs either a member captured at close or moving span creation into
closeLedger(), so it is removed rather than shipped mislabelled.
Record close_time_avalanche_state on recovered rounds. startRoundInternal()
reset establishSpan_ inline, discarding the span before the attribute was
written, so the value was present only on rounds that reached Accepted --
survivor bias in exactly the rounds worth investigating. It now calls
endEstablishTracing(). The comment claiming this avoided "reporting a stale
regime" was wrong: closeTimeAvalancheState_ is not reset until 39 lines
later, so the value was still that span's terminal regime.
Rename avalanche_state to close_time_avalanche_state. DisputedTx carries a
second, per-transaction avalanche tracker; the bare name invited reading a
close-time-only value as the transaction one, which is the tracker that
actually escalates in a stuck round.
Also: both label helpers now fall through to "unknown" instead of a
plausible-looking regime, matching to_string(ConsensusPhase); and the header
now records that the end-of-open attributes are absent on recovered and
simulated rounds, and that tx_sets_acquired can skew either way because
handleWrongLedger clears currPeerPositions_ but not acquired_.
Tests: the minimum-open-time assertion used prevRoundTime=10s, where
openTime=1s trips the too-fast branch as well, so deleting the ledgerMinClose
check entirely left it green. Replaced with prevRoundTime=2s, which isolates
the branch. Added the others-closed boundary, which is strict and was
untested in either direction, its integer truncation for odd prevProposers,
and its precedence over the no-transactions and minimum-open branches.
The open phase ended for one of four distinct reasons, but
shouldCloseLedger() collapsed them into a bool, so a trace could say when a
phase ended and never why. "The network closed without us" and "nothing was
waiting" are the same span today.
Add whyCloseLedger(), which holds the decision and returns
LedgerCloseReason. shouldCloseLedger() keeps its exact signature and becomes
a one-line delegation, so its callers and unit tests are untouched and the
branch logic is not duplicated. phaseOpen() calls whyCloseLedger() directly;
both emit the same journal and CLOG output, so only one is called.
New attributes on consensus.phase.open, both set once on the closing tick:
close_reason anomaly | others_closed | idle | normal
proposers_validated trusted peers that had already validated the prior
ledger, reusing the value the decision was made on
Absent on the simulate() close path, which bypasses the decision rather than
having a reason invented for it.
Skipped has_open_transactions: hasOpenTransactions() is
!getOpenLedger().empty(), which is false on a quiet network for most of a
round, and close_reason=idle already implies it. The sibling
consensus.ledger_close span carries tx_count_open, which is the same fact
with a count instead of a boolean.
shouldCloseLedger() now has no production caller; it stays exported so the
public API and its tests are unchanged.
Tests pin every input vector from should_close_ledger to its literal reason,
including that the anomaly check outranks others-closed, and cover the
inclusive idle boundary either side by one millisecond.
consensus.phase.open and consensus.establish carried almost no state of
their own. Span attributes are not inherited, so the ledger context on the
parent consensus.round span does not describe either child, and the few
attributes the establish span did carry are rewritten on every iteration
and therefore only ever report the final value.
Add seven attributes that are read from state already in scope, are
written exactly once, and are not duplicates of the parent round span:
consensus.phase.open (start)
start_reason initial, or recovered on a handleWrongLedger
re-entry, which emplaces a SECOND phase.open
span under the same round
previous_close_agree feeds the sinceClose branch in phaseOpen()
peer_positions_at_open positions in hand after playbackProposals(),
the head start the round began with
early_close_triggered the round skipped the timer because enough
peers had already closed
consensus.phase.open (end)
tx_sets_acquired candidate tx sets held at close, read before
our own position is added; a low count against
a high peer_positions_at_close means tx-set
fetches did not land, not disagreement
consensus.establish (start)
disputes_count_initial disputes carried in from the positions held at
close, as opposed to disputes_count, which is
overwritten each iteration
consensus.establish (end)
avalanche_state terminal close-time convergence regime; the
derived avalanche_threshold is a weight and
cannot be inverted back to the state
The avalanche label is mapped by a new constexpr avalancheStateLabel() in
ConsensusSpanNames.h rather than an inline switch, so the four labels stay
under the naming check's L1 ownership and are unit-testable.
Deliberately not added: ledger_seq and consensus_mode, which would only
copy the parent round span's values down; tx set size and position hash,
which the TxSet concept does not expose portably across RCLTxSet and the
csf simulator; and the peer-unchanged and dead-node counters, whose
underlying state is reset mid-round and so would report a misleading value.
Behaviour is unchanged. The early-close condition is hoisted into a named
local so the annotation happens before timerEntry(), which can reach
closeLedger() and end the open-phase span.
Tests pin the wire strings for every new key and value and cover all four
enumerators of the avalanche mapping. They need no telemetry runtime: the
csf simulator returns an invalid round span context and a null Telemetry,
so consensus spans there are null guards and attribute writes are no-ops.
Bring the three documentation surfaces in line with the new parse-time check:
- The @throws clause on makeTelemetrySetup now names the third failure
condition and records that an empty path is skipped.
- cfg/xrpld-example.cfg states, under all three TLS keys, that with enabled=1
and use_tls=1 a path that does not exist or cannot be read stops startup. The
tls_ca_cert wording still says that empty selects the system CA store, since
only a path that is set is checked.
- The runbook troubleshooting entry gains a third bullet for the "cannot be
read" message, whose remedy is the path or its permissions rather than the
certificate and key pairing.
Documentation only; no behaviour change.
With telemetry enabled and use_tls=1, makeTelemetrySetup now reads each
non-empty tls_ca_cert / tls_client_cert / tls_client_key path and refuses to
start when the file is missing or cannot be read. The message names the config
key, the path and the OS error, instead of leaving the problem to surface much
later as an opaque TLS handshake failure inside the exporter.
Reading the file with getFileContents, as the gRPC server already does for its
own ssl_cert and ssl_key pair, proves the file is both present and readable; an
existence test alone would miss a permissions problem. The contents are
discarded.
Both gates are deliberate. The check is skipped when enabled is 0, so a stale
cert line still cannot stop a node from booting, and when use_tls is 0, where
the exporter never opens the files. An empty path stays valid; for tls_ca_cert
it selects the system CA store.
Six GTest cases cover the three keys that can fail, the all-readable case, and
each gate on its own.
makeTelemetrySetup() rejects a contradictory [telemetry] mutual-TLS
setup by throwing, but it is called from ApplicationImp's
member-initializer list. A try/catch in the constructor body cannot
reach a throw from there, and nothing further up the stack caught it
either, so a config mistake reached std::terminate: the default handler
printed a terminate dump and raised SIGABRT, leaving a core file
instead of a startup error.
Catch std::exception around makeApplication() in run(), report the
reason on stderr and return -1, so the failure is a clean non-zero exit
with a message an operator can act on. Only the construction is
wrapped. setup() starts subsystems whose shutdown order is delicate and
is left outside deliberately, because unwinding a half-started
Application would skip the normal stop sequence.
Gate both validation guards on enabled. A node with telemetry switched
off previously refused to start over certificate paths that nothing
would read.
Document both throws on makeTelemetrySetup(), state in
cfg/xrpld-example.cfg and the configuration reference that a partial
mutual-TLS setup is fatal and that the checks apply only when
enabled=1, and add a runbook troubleshooting entry keyed on the two
error messages.
Tests cover both guards with the message asserted so the two are told
apart, both enabled=0 paths, and the default plaintext configuration.
The integration test asserted `rippled_job_count`, which never reports any
series, so that check always failed. `JobQueue` registers the gauge as
`makeGauge("job_count")`, but `Application.cpp` passes it
`collectorManager_->group("jobq")`, so the emitted StatsD name is
`jobq.job_count` and the exported Prometheus name is
`<prefix>_jobq_job_count`.
Corrected the same name in two runbook tables that also dropped the `jobq`
segment. `09-data-collection-reference.md` already had it right, which is why
the two documents disagreed.
Routed here rather than to the phase-10 PR where it was reported: the wrong
name is present in `integration-test.sh` on every branch from phase 6
onward, and this is the branch that introduces the file.
Left alone deliberately:
- `statsd-node-health.json` still queries the old name, but that dashboard is
deleted at phase 7 in favour of `node-health.json`
- `06-implementation-phases.md` names `job_count`, which is accurate as the
code-level makeGauge argument rather than the exported metric name
All five TraceQL panels on this dashboard returned nothing, and did so
without any visible error: they filtered on
span.close_time_correct=~"$close_time_correct", but close_time_correct is
a boolean attribute (RCLConsensus.cpp:601 passes a raw bool), and Tempo
restricts the regex operator to string operands, so the spanset resolved
to false. With the variable defaulting to All the clause rendered as
=~".*", so the panels were empty out of the box and looked exactly like a
node with no consensus activity.
Note this is the opposite of PromQL, where an absent or empty label does
match ".*" — which is why the 17 Prometheus panels on this same board were
unaffected and the dashboard appeared healthy.
Dropped the clause from all six queries, matching phases 9 and 10 where it
is already gone. The $close_time_correct variable now filters the
Prometheus "Close Time Agreement" panel instead, which already grouped by
that label but never filtered on it, so the control stays useful rather
than becoming dead UI.
Two defects were masked behind the empty panels and are fixed too:
- "Close Time: Raw Proposals" and "Close Time: Effective / Quantized"
carried unit dateTimeFromNow over close_time_self/close_time, which are
NetClock seconds (Ripple epoch), while Grafana's dateTime formatters
expect a millisecond Unix epoch — every point would have rendered as
roughly 1970. They now plot as plain numbers with the axis labelled
"NetClock Seconds (Ripple Epoch)", and the descriptions give the
946684800 offset for converting to Unix time.
- "Close Time Vote Bins & Resolution" matched its unit and axis overrides
byName against "Vote Bins" and "Resolution", which are not field names;
TraceQL select() yields close_time_vote_bins and close_resolution_ms, so
neither override applied. Switched to byRegexp so the match holds
whichever scope prefix Grafana emits.
No panel was added or removed: the (type, title) multiset is unchanged at
22. resolution_direction keeps its regex filter, which is correct there —
it is set from a std::string whose values are exactly the variable's
increased/decreased/unchanged.
The integration test's span assertions never actually ran. check_span()
built a Tempo /api/search call with --data-urlencode but no -G, so curl
POSTed the params as a body; Tempo answers 200 and ignores the query, so
every span name looked present. Verified against a live Tempo 2.9.4: the
buggy form returns the store's total trace count for any name, including
"zzz.does.not.exist"; with -G a real name returns 1 and a bogus one 0.
Fixed alongside it: the RPC check asserted "rpc.request", which is never
emitted (ServerHandler.cpp builds "rpc.http_request"). These two had to
change together, since -G turns the bogus name from a silent pass into a
hard failure.
Also in the script: a consensus timeout logged two failures and counted
two, because a post-loop else re-reported what the timeout branch had
already reported; and three unguarded curl calls aborted the whole script
under set -euo pipefail, making the ACCOUNT_ZERO fallback dead code with
no cleanup. Guarded the curls and wired an EXIT trap to the existing
cleanup(). The trap deliberately fires only before the summary, so a
completed run still leaves the stack up as the header documents.
Docs corrections, all re-derived from code:
- span inventory heading 35 -> 38, attribute heading 83 -> 89 rows
(78 unique keys), and the section 6 header table now carries the
missing TxApplySpanNames.h row so its columns sum to the same figures
- two stale paths: ConsensusSpanNames.h is under include/xrpl/consensus/,
TxSpanNames.h under src/xrpld/telemetry/
- consensus_round_id is int64, not string (RCLConsensus.cpp sets
prevLgr.seq() + 1); the runbook's TraceQL examples now use a numeric
literal instead of an unparseable bare <round_id>
- state-accounting duration gauges are cumulative MICROSECONDS, not
seconds (NetworkOPs.cpp declares std::chrono::microseconds and
publishes dur.count() raw)
- sampling_ratio is not a config key; head sampling is fixed at 1.0 and
the shipped collector has no tail sampling, so the caveat was rewritten
- the plan blurb referenced Jaeger; this stack is Tempo
Run with no arguments the script iterated an empty list, found no
violations and printed "OK: 0 dashboard(s) passed" with exit 0 -- a clean
bill of health for reading no files, indistinguishable from a real pass.
A bare run now defaults to every dashboard beside the script, and a run
that still ends up with nothing to check exits 2 rather than reporting
success. Passing paths explicitly behaves as before.
Two panel descriptions pointed at a rollout phase and task number defined
only in a planning folder outside the shipped tree. The note still names
the file and the change it is waiting on, which is the part a reader can
act on.
The catalog tagged every span with a rollout phase number defined only in
a planning folder outside the shipped tree, so the column meant nothing
to a reader of the repository. The span name and source file identify
each entry.
