A comment that describes an earlier state of its own branch documents something
no reader can look up, because the branch is squash-merged and the state it
contrasts against never reaches the merged history. Four passages in
HistogramBuckets.h and one in Telemetry.cpp did exactly that, and the
OTelCollector and Unit.h wording implied a transition rather than a fact.
- HistogramBuckets.h: the ladders now explain the invariant they enforce,
instead of recounting where the edges used to live and how they drifted.
- Telemetry.cpp: one view per unit means a byte count is bucketed on the byte
ladder, stated directly rather than as something it stopped inheriting.
- OTelCollector.cpp: the collector is described by what it is, a thin adapter
over the shared pipeline, rather than as a shim that gave up an exporter.
- Unit.h: the StatsD path is out of service as a present fact, and the contract
its wire format would break is an external protocol one.
Comment text only. No declaration, signature or emitted value changes.
The class documented a name format it does not produce. Eleven comments said
the [insight] prefix is prepended, and gave examples like "xrpld_rpc_size" and
"xrpld_LedgerMaster_Validated_Ledger_Age" that also kept the original casing.
formatName() lowercases the raw instrument name and maps dots and spaces to
underscores, and applies no prefix. All four instrument factories go through
it, so "RPC.Size" exports as "rpc_size". prefix_ is written in the constructor
and read in one place, the startup log line, so nothing it holds can reach an
exported name. The service is identified by the service.name resource
attribute.
Comments only, in both the header and the implementation. The ASCII diagram
still lists prefix_ as a member, which it is.
Two conflicts, both in PeerImp's proposal and validation receive paths, and
both resolved by taking the incoming side: it holds the span in a handle that
stays empty when telemetry is compiled out and moves every attribute behind
if (span && *span), which supersedes the unguarded form on this side.
Taking the incoming text renamed consSpan to span in both blocks, while the
two job-lambda captures further down had merged cleanly and still named
consSpan. Renamed those captures so each names the handle its own function
declares.
This branch's own guard on the inbound-validation ledger_hash attribute is a
different span in a different function; it merged cleanly and is preserved.
Both broadcast paths passed *msg.mutable_trace_context() to the injector,
which allocates the submessage and sets its has-bit before the injector can
decide there is nothing to write. A compile-time guard covered the
telemetry-off build, but a node with telemetry compiled in and no active
span -- a disabled category, telemetry disabled by config, or a round that
is not being traced -- still broadcast an empty TraceContext to every peer,
and every peer took its has_trace_context() branch to extract nothing.
Add SpanGuard::hasCurrentContext(), a predicate that tests the same two
conditions the injector bails out on without allocating, and an
injectCurrentContext(message) helper that uses it to decide whether to
create the submessage at all. Both consensus call sites now call the helper
unguarded.
Two conflicts, both in the telemetry include blocks.
SpanGuard.h: kept the union. The incoming side moves <memory> inside the
telemetry guard and adds <type_traits>; this branch adds <initializer_list>,
<utility> and the protocol::TraceContext forward declaration. Guarding
<memory> is correct here: the only std::shared_ptr uses are SpanContext's
member and constructor, both inside the guard, and SpanGuardHandle is a
template parameter name rather than a smart-pointer typedef.
NullTelemetry.cpp: took only the incoming guarded Journal.h block. The
incoming hunk also carried <memory> and <utility>, which this branch already
includes below the guarded OpenTelemetry block; taking them as well would
have tripped readability-duplicate-include.
updateOurPositions built a 64-character hex string from the transaction
id plus two std::to_string number conversions, then attached them to a
span event. That ran for every dispute that flipped position, on every
establish tick of every consensus round, whether or not anything was
recording.
The three strings and the event have no other consumer; the vote change
itself (mutableSet insert/erase) is untouched.
The block now sits behind if (span). With telemetry compiled out the
stub's operator bool is a literal false, so it is eliminated; with
telemetry on it is also skipped when the span is null because the
establish context was never captured. The guard is inside the function
body, so Consensus<Adaptor>'s adaptor interface is unchanged and the
csf::Peer simulator is unaffected.
With telemetry compiled out, <string_view> in Telemetry.h and <memory> in
SpanGuard.h are named only by declarations that are themselves guarded, so
clang-tidy's include-cleaner reports them unused and warnings-as-errors fails
the build. NullTelemetry.cpp has the mirror problem: it names beast::Journal
only in the compiled-out makeTelemetry(), reaching the type transitively.
Guard each include to match the configuration that uses it, and correct three
comments that overstated what the empty destructors cost.
The compiled-out ScopedActivation, SpanGuard and ScopedSpanGuard each used a
defaulted destructor. A defaulted destructor on an empty class is trivial, so
compilers report any guard held only for its scope as an unused variable. With
telemetry compiled out that produced seven -Wunused-variable errors under
-Werror, across the ledger acquire, consensus, ledger master and overlay paths.
Write the three destructors by hand so destruction is not trivial, matching the
telemetry-enabled types, and assert that property so it cannot quietly regress
to `= default`. The bodies are empty, so no code is generated either way.
Also move <memory> inside the telemetry guard: only the telemetry-enabled types
hold a unique_ptr, so the include is unused when telemetry is compiled out.
Splitting the consensus span labels into their own header moved both
constexpr std::string_view helpers out of ConsensusSpanNames.h, but left
the include they needed behind. The header names string_view nowhere now,
and SpanNames.h already provides the type for the conversion operator.
clang-tidy misc-include-cleaner reports this as an error under
-warnings-as-errors, which fails the clang-tidy job for the whole chain.
Event::notify takes std::uint64_t but the header never included <cstdint>,
relying on it arriving through another include. clang-tidy's include-cleaner
flags it, which fails CI on any branch where this file lands in the
changed-file set.
Fixed here, on the branch that introduced the std::uint64_t parameter, rather
than only downstream where it happened to surface.
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.
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.
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.
