Three conflicts, all composed rather than resolved by taking a side:
- TelemetryConfig.cpp: phase-6 kept networkTypeFromId file-local with
[[nodiscard]]; phase-7 had relocated it to public scope for
Application.cpp. Kept phase-7's relocation, so one definition remains.
The [[nodiscard]] survives on the declaration in Telemetry.h.
- Telemetry.cpp x2: phase-7 added getMeter overrides, phase-6 added
[[nodiscard]] to the startSpan below them. Kept both, and put
[[nodiscard]] on getMeter too.
- TESTING.md: phase-7 had the right metric name (span_calls_total, which
the spanmetrics namespace produces) but the wrong label. Its
xrpl.rpc.command appears nowhere else in the branch; the attribute is
bare `command`, which is what the dashboards query. Took phase-7's
metric with the correct label.
Both signalEndpoint call sites follow the renamed member. signalEndpoint
itself is left in place: removing it and adding metrics_endpoint is a
design change, not part of propagating a rename.
The rename arrived from phase-1b by merge, which left this test naming a
member that no longer exists. Updates both assertions to tracesEndpoint
and the section key to traces_endpoint.
The key matters as much as the member: had only the member been renamed,
the parse would have fallen back to the default and the test would have
compared the collector URL against localhost.
The three close-time span attributes hold NetClock readings, which are whole
seconds since the XRP Ledger epoch of 2000-01-01 rather than the Unix epoch.
Neither the unit nor the epoch was recoverable from the key, so a consumer
rendering one as a wall-clock time without first adding the epoch offset lands
roughly thirty years early. The sibling close_resolution_ms already named its
unit, so the header disagreed with itself.
close_time -> close_time_ripple_epoch_s
parent_close_time -> parent_close_time_ripple_epoch_s
close_time_self -> close_time_self_ripple_epoch_s
Emitted values do not change; only the keys do. The public RPC response fields
of the same name are deliberately untouched, as renaming those would break the
ledger API.
ValidationTracker carries no telemetry guards and neither does its test, so
both are compiled whatever the telemetry setting. Its implementation and the
src/ include path it needs were added only inside if(telemetry), so a build
with telemetry off could not reach the header and left fifteen of its symbols
unresolved at link time on every platform.
Hoist both out of the guard. The else() branch already does this for
MetricsRegistry, so the case was anticipated there and missed here.
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.
This is the change that actually lifts the 5 s ceiling. Until now the
millisecond ladder and the Unit type existed but nothing consumed them.
Telemetry.cpp registered ONE histogram view: instrument name pattern "*",
unit exactly "ms", boundaries {1, 5, ..., 1000, 5000}. Verified against the
installed SDK, "*" matches every name and "ms" matches exactly, so that view
governed every beast::insight Event -- all 54 of them, whatever they measure.
Measured on devnet: 24.9% of rpc_size samples and 100% of jobq_updatepaths
samples fell above 5000. A quantile landing in the `+Inf` bucket reads back
as the second-highest edge, so those p95s reported a flat 5000 rather than a
measurement, and the 1 s to 5 s span was a single four-second-wide bucket
that any quantile inside it had to interpolate across.
Replaces it with one view per unit, keyed on the unit an instrument declares:
- `ms` gets kMillisecondBuckets: every representable edge of the collector's
spanmetrics ladder, plus 60 s and 120 s. The extensions are deliberate --
jobq_updatepaths was measured averaging 59,956 ms, which no span
approaches, so parity alone would still censor it.
- `By` gets kByteBuckets, placed from the measured response distribution
(mean 2131 B, half under 1 kB, tail mean bounded at 7538 B).
OTelEventImpl now derives its declared unit AND its description from unit()
instead of hardcoding "Duration in ms"/"ms", so rpc_size exports as
rpc_size_bytes on the byte ladder. rpc-pathfinding's "RPC Response Size"
panel follows the rename; its unit was already decbytes and is now truthful.
Also corrects Phase7_taskList.md, which still specified the 5000 ladder as
"matching SpanMetrics". That was true when written and became false when the
collector ladder was extended on its own -- implementing the plan as written
reproduced the bug, so the spec is where the defect had come to live. The
edges now have exactly one owner and the plan points at it.
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.
beast::insight::Event documents itself as carrying "a millisecond time, or
other integral value", but both backends assumed the first case: the OTel
bridge declared every instrument with unit `ms` and StatsD tagged every
sample `|ms`. One Event does not measure time -- ServerHandler's "size"
records the serialized RPC response length -- so it exported as
rpc_size_milliseconds and inherited the millisecond bucket ladder. A quarter
of its samples landed above that ladder's top edge, and since Prometheus
returns the second-highest edge for a quantile in the `+Inf` bucket, its p95
panel showed a flat 5.00 kB rather than a measurement.
Adds beast::insight::Unit (Millis, Bytes) plus otelUnitCode(), carried on
EventImpl and selectable at makeEvent(). Naming the unit at creation is what
lets a backend pick the export unit and, through it, the bucket ladder.
- Collector gains a virtual makeEvent(name, Unit) whose default delegates to
the millisecond overload, so a collector that cannot act on a unit keeps
working unchanged. NullCollector and the Groups wrapper override it.
- The Groups override matters most: call sites reach a collector through a
Group, so forwarding only the prefixed name would silently drop the unit.
A test covers that hop specifically.
- Event gains notify(std::uint64_t) for non-duration samples, replacing
ServerHandler's `Event::value_type{response.size()}` -- wrapping a byte
count in a std::chrono::milliseconds compiles but reads as a duration to
everything downstream.
- EventImpl::value_type stays std::chrono::milliseconds. Widening it would
change the wire value of every existing StatsD timer, and metrics needing
finer resolution use the OTel-native microsecond instruments.
The StatsD collector deliberately keeps emitting `|ms`: that path is retired
here (its UDP port is commented out of the compose file and the integration
test fails if anything listens on 8125), so changing its wire format would
alter a legacy contract with no consumer and no way to verify it.
The exported name does not change yet -- OTelEventImpl still hardcodes its
unit. That follows with the unit-keyed histogram views.
The bucket edges for the OTel histograms lived as file-local `namespace {}`
constants, unreachable from any test, and they drifted from the collector's
spanmetrics ladder they were specified to match. The millisecond ladder
stayed capped at 5 s after the collector side was extended to 30 s, so any
quantile above 5 s read back as a flat 5000 -- Prometheus returns the
second-highest edge for a quantile in the `+Inf` bucket, which looks like a
measurement rather than an error.
Adds include/xrpl/telemetry/HistogramBuckets.h as the single owner of the
ladders, with a constexpr validator plus static_asserts so a descending or
duplicated edge cannot compile, and gtest coverage that pins the floor and
ceiling against the measured distributions:
- kMillisecondBuckets carries every representable collector edge and extends
to 120 s, because the updatepaths job type averages ~60 s and a 30 s
ceiling would censor it exactly as 5 s does today. Sub-millisecond
collector edges are omitted: beast::insight::Event rounds durations up to
whole milliseconds, so they would collect nothing.
- kByteBuckets is new, for Events whose samples are sizes rather than
durations. Edges follow the measured RPC response distribution (mean
2131 B, half under 1 kB, tail mean bounded at 7538 B) rather than a guess,
so the resolution sits between 512 B and 64 kB.
No behaviour change yet -- nothing consumes the header until the views are
rewired.
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.
* upstream/release/3.3.x: (41 commits)
chore: Bump version to 3.3.0
chore: Bump version to 3.3.0-rc7
fix: Increase manifest protocol message size cap and fix manifests relay
fix: Cap untrusted manifests per message and drop oversized ones
chore: Bump version to 3.2.1
chore: Bump version to 3.2.1-rc1
fix: Cap untrusted manifests per message and drop oversized ones
fix: Reject oversized validator manifest before decoding
fix: Reduce untrusted manifest cache cap to 100
fix: Bound untrusted manifest cache
chore: Bump version to 3.3.0-rc6
feat: Package validator-keys inside rippled
chore: Bump version to 3.3.0-rc5
fix: Switch SponsorshipSet to use a delta for sfFeeAmount
fix: Re-revert "fix: Set request size limits and differential pricing for get-object-by-hash calls"
chore: Bump version to 3.3.0-rc4
fix: Revert "fix: Set request size limits and differential pricing for get-object-by-hash calls"
chore: Bump version to 3.3.0-rc3
fix: Reduce untrusted manifest cache cap to 100
fix: Revert "fix: Reject oversized SHAMap nodes in gotStaleData and fetch-pack path"
...
