These comments dated themselves against this branch or against an earlier
revision of the same change, neither of which survives a squash merge.
- 'as of this branch' in the pricing-case doc and the pass-through static_assert
becomes a statement about what the constants currently produce.
- 'Extracted from processGetObjectByHash()' and 'Split from start()' describe
edits internal to this change; both now say why the method stands alone.
- Recording.h: the mock-abstract mismatch is a standing consequence of a member
set that differs between builds, not something that 'has previously' happened.
- MetricsRegistry.cpp: describe the loops.txt entry as recording two cycles
rather than as what ordering.txt 'previously had'.
- InboundLedger.h: the acquire span is the only signal for back-fill cost; it
did not 'previously emit' nothing.
- check_bucket_parity.py: replace the eleven-phase drift story with the reason
the check exists, and point the failure message at HistogramBuckets.h and the
collector config instead of OpenTelemetryPlan/, which does not reach develop.
Comments and one error message only, no behaviour change.
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.
The overview diagram and the thread-safety note named Mirror<T>, which this
header does not declare. Drop both mentions so the file documents what it
ships.
Four conflicts. None was a take-a-side.
xrpld-telemetry.cfg: dropped the incoming [insight] block. This branch already
has one, and duplicate ini sections do not replace each other -- parseIniFile
appends onto the same section, so keys merge last-wins and the effective config
is one that appears nowhere in the file.
src/tests/libxrpl/CMakeLists.txt: kept this branch's else() branch, which
compiles MetricsRegistry.cpp for the telemetry-off build, and dropped only the
ValidationTracker target_sources inside if(telemetry). Upstream relocated that
one out of the guard, so keeping both would have compiled it twice. The
else() branch is this branch's own: the MetricsRegistry test exists only here,
and without its implementation the off build would not link.
RCLConsensus.cpp: union. The metric macros and registry come from this branch,
PropagationHelpers from upstream; all three are used.
PeerImp.cpp: MetricMacros.h stays unguarded, because all seven XRPL_METRIC_*
uses in this file are on unconditional paths and the header is what defines
them. ConsensusReceiveTracing.h takes upstream's guarded placement, and this
branch's guarded GetObjectMetricNames.h is kept beside it.
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.
The header told callers to declare members [[no_unique_address]]. That
attribute has no other use in this repo, and MSVC ignores the standard
spelling for ABI compatibility, so following the advice would have been a
portability wart for one byte per member.
Say instead that the compiled-out member collapses to padding, and that what
these types buy is work not being done.
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.
std::atomic implicitly deletes copy and move, so a class holding a Counter is
non-copyable when telemetry is compiled in. Compiled out, Counter was an empty
type with no such member, which would have made that same owner freely
copyable in one build only -- the per-configuration API difference these
utilities exist to avoid.
Declare copy and move deleted so both builds agree, and assert it
unconditionally in the tests.
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.
Sites that exist only to be reported currently spell their own gating: an
#ifdef around a member, another around the clock read, another around the
call that reports it. That puts preprocessor branches through business logic
and leaves each class with a different member set per build.
Add kEnabled, Stopwatch and Counter. Each holds real state when telemetry is
compiled in and is an empty type with no-op methods when it is not, while the
member stays declared in both builds so no class's API differs by
configuration. Tests assert both configurations from one file, including that
the compiled-out types are empty.
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.
