From 258adf491e9acb16b789e7310535ef23b844b6a8 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 20:45:24 +0100 Subject: [PATCH] refactor(telemetry): split consensus span labels into their own header 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. --- include/xrpl/consensus/Consensus.h | 83 ++++++++++++------ include/xrpl/consensus/ConsensusSpanLabels.h | 85 +++++++++++++++++++ include/xrpl/consensus/ConsensusSpanNames.h | 66 +------------- src/libxrpl/consensus/Consensus.cpp | 1 + .../libxrpl/telemetry/ConsensusSpanNames.cpp | 27 +++--- .../libxrpl/telemetry/SpanGuardFactory.cpp | 7 +- 6 files changed, 158 insertions(+), 111 deletions(-) create mode 100644 include/xrpl/consensus/ConsensusSpanLabels.h diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 577b154cbe..f299ce2b9a 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -34,9 +35,9 @@ namespace xrpl { /** * Determines why the current ledger should close at this time. * - * Holds the close decision that shouldCloseLedger() reduces to a bool. Call - * this one when the deciding branch matters. Both log identically, so call - * one or the other, never both. Parameters match shouldCloseLedger(). + * Holds the close decision. shouldCloseLedger() delegates here and adds only + * a comparison, so the logging happens once either way; call whichever suits. + * Parameters match shouldCloseLedger(). * * @return The deciding branch, or KeepOpen if no close condition is met. */ @@ -692,6 +693,24 @@ private: */ std::optional openSpan_; + /** + * Record how the open-phase span began. + * + * @param reason Which startRoundInternal() entry path created it. + * @param prevLedger The prior ledger, read before previousLedger_ is set. + */ + void + annotateOpenStart(StartRoundReason reason, Ledger_t const& prevLedger); + + /** + * Record what ended the open phase. + * + * @param closeReason The deciding whyCloseLedger() branch. + * @param proposersValidated Trusted peers already past the prior ledger. + */ + void + annotateOpenClose(LedgerCloseReason closeReason, std::size_t proposersValidated); + /** * Create the establish-phase span if not yet active. * Called on each phaseEstablish() invocation; no-op while span is live. @@ -811,18 +830,7 @@ Consensus::startRoundInternal( openSpan_.emplace( telemetry::SpanGuard::childSpan( telemetry::consensus::span::phaseOpen, adaptor_.roundSpanContext())); - if (*openSpan_) - { - namespace cs = telemetry::consensus::span; - // A recovery emplaces a SECOND phase.open span under the same round, - // so this is what tells the two apart. - openSpan_->setAttribute( - cs::attr::startReason, - reason == StartRoundReason::Recovered ? std::string_view{cs::val::startRecovered} - : std::string_view{cs::val::startInitial}); - // From the parameter: previousLedger_ is not assigned until below. - openSpan_->setAttribute(cs::attr::previousCloseAgree, prevLedger.closeAgree()); - } + annotateOpenStart(reason, prevLedger); // On the Recovered path, fire phase.open here because startRoundTracing // (which fires it for the Initial path) is not called on re-entry. On // the Initial path this is a no-op because the round span hasn't been @@ -1376,16 +1384,7 @@ Consensus::phaseOpen(std::unique_ptr const& clog) clog); if (closeReason != LedgerCloseReason::KeepOpen) { - // Annotate before closeLedger() ends the span. Set once: the phase - // moves to Establish, so phaseOpen() is not entered again this round. - // Absent on the simulate() path, which bypasses this decision. - if (openSpan_ && *openSpan_) - { - namespace cs = telemetry::consensus::span; - openSpan_->setAttribute(cs::attr::closeReason, cs::closeReasonLabel(closeReason)); - openSpan_->setAttribute( - cs::attr::proposersValidated, static_cast(proposersValidated)); - } + annotateOpenClose(closeReason, proposersValidated); CLOG(clog) << "closing ledger. "; closeLedger(clog); } @@ -2160,6 +2159,36 @@ Consensus::asCloseTime(NetClock::time_point raw) const return roundCloseTime(raw, closeResolution_); } +template +void +Consensus::annotateOpenStart(StartRoundReason const reason, Ledger_t const& prevLedger) +{ + if (!openSpan_ || !*openSpan_) + return; + namespace cs = telemetry::consensus::span; + openSpan_->setAttribute( + cs::attr::startReason, + reason == StartRoundReason::Recovered ? std::string_view{cs::val::startRecovered} + : std::string_view{cs::val::startInitial}); + // From the parameter: previousLedger_ is not assigned until later. + openSpan_->setAttribute(cs::attr::previousCloseAgree, prevLedger.closeAgree()); +} + +template +void +Consensus::annotateOpenClose( + LedgerCloseReason const closeReason, + std::size_t const proposersValidated) +{ + // Called before closeLedger() ends the span, and only on the closing tick, + // so each attribute is written once per round. + if (!openSpan_ || !*openSpan_) + return; + namespace cs = telemetry::consensus::span; + openSpan_->setAttribute(cs::attr::closeReason, cs::closeReasonLabel(closeReason)); + openSpan_->setAttribute(cs::attr::proposersValidated, static_cast(proposersValidated)); +} + template void Consensus::startEstablishTracing() @@ -2207,9 +2236,9 @@ Consensus::endEstablishTracing() // Terminal convergence regime, recorded once before the span ends. if (establishSpan_ && *establishSpan_) { + namespace cs = telemetry::consensus::span; establishSpan_->setAttribute( - telemetry::consensus::span::attr::closeTimeAvalancheState, - telemetry::consensus::span::avalancheStateLabel(closeTimeAvalancheState_)); + cs::attr::closeTimeAvalancheState, cs::avalancheStateLabel(closeTimeAvalancheState_)); } establishSpan_.reset(); establishSpanContext_ = telemetry::SpanContext{}; diff --git a/include/xrpl/consensus/ConsensusSpanLabels.h b/include/xrpl/consensus/ConsensusSpanLabels.h new file mode 100644 index 0000000000..cc66639c41 --- /dev/null +++ b/include/xrpl/consensus/ConsensusSpanLabels.h @@ -0,0 +1,85 @@ +#pragma once + +/** + * Enum-to-label mappings for consensus span attribute values. + * + * Split from ConsensusSpanNames.h so that header stays dependency-free like + * its siblings: the span-name and attribute-key constants are included by + * overlay and app translation units that have no use for the consensus + * enums, while these mappings are needed only by Consensus.h. + * + * ConsensusSpanNames.h (constants only, no domain deps) + * ^ + * | includes + * ConsensusSpanLabels.h --includes--> ConsensusParms.h, ConsensusTypes.h + * ^ + * | includes + * Consensus.h + */ + +#include +#include +#include + +#include + +namespace xrpl::telemetry::consensus::span { + +/** + * Map a close-time avalanche state to its `avalanche_state` label. + * + * The regime escalates Init -> Mid -> Late -> Stuck, raising the close-time + * agreement threshold at each step. + * + * @param state The state held by Consensus::closeTimeAvalancheState_. + * @return The wire label; one of val::avalanche*. + * + * @note No default arm, so a new enumerator is a -Wswitch warning; the + * fall-through returns "unknown" rather than a plausible-looking regime. + */ +[[nodiscard]] constexpr std::string_view +avalancheStateLabel(ConsensusParms::AvalancheState const state) +{ + switch (state) + { + case ConsensusParms::AvalancheState::Init: + return val::avalancheInit; + case ConsensusParms::AvalancheState::Mid: + return val::avalancheMid; + case ConsensusParms::AvalancheState::Late: + return val::avalancheLate; + case ConsensusParms::AvalancheState::Stuck: + return val::avalancheStuck; + } + return val::unknown; +} + +/** + * Map a ledger-close decision to its `close_reason` label. + * + * @param reason The value returned by whyCloseLedger(). + * @return The wire label; one of val::close*. + * + * @note No default arm, so a new enumerator is a -Wswitch warning; the + * fall-through returns "unknown". `keep_open` is mapped but never emitted. + */ +[[nodiscard]] constexpr std::string_view +closeReasonLabel(LedgerCloseReason const reason) +{ + switch (reason) + { + case LedgerCloseReason::KeepOpen: + return val::closeKeepOpen; + case LedgerCloseReason::Anomaly: + return val::closeAnomaly; + case LedgerCloseReason::OthersClosed: + return val::closeOthersClosed; + case LedgerCloseReason::Idle: + return val::closeIdle; + case LedgerCloseReason::Normal: + return val::closeNormal; + } + return val::unknown; +} + +} // namespace xrpl::telemetry::consensus::span diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index d6715f2732..4945db30d0 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -86,8 +86,6 @@ * +~~~ follows-from link (separate sub-tree, causal link) */ -#include -#include #include #include @@ -194,11 +192,8 @@ inline constexpr auto earlyCloseTriggered = makeStr("early_close_triggered"); /** * Open-phase end metadata (set on consensus.phase.open before reset). * - * A low `tx_sets_acquired` next to a high peer_positions_at_close suggests - * tx-set fetches did not land; the reverse skew is also possible, because - * handleWrongLedger clears currPeerPositions_ but not acquired_. - * `close_reason` plus `proposers_validated` separate "the network moved on - * without us" from "the network was quiet". + * `proposers_validated` counts validators of the previous ledger, unlike + * `proposers_finished` below, which counts those already past it. */ inline constexpr auto openDurationMs = makeStr("open_duration_ms"); inline constexpr auto peerPositionsAtClose = makeStr("peer_positions_at_close"); @@ -349,61 +344,4 @@ inline constexpr auto closeIdle = makeStr("idle"); inline constexpr auto closeNormal = makeStr("normal"); } // namespace val -/** - * Map a close-time avalanche state to its `avalanche_state` label. - * - * The regime escalates Init -> Mid -> Late -> Stuck, raising the close-time - * agreement threshold at each step. - * - * @param state The state held by Consensus::closeTimeAvalancheState_. - * @return The wire label; one of val::avalanche*. - * - * @note No default arm, so a new enumerator is a -Wswitch warning; the - * fall-through returns "unknown" rather than a plausible-looking regime. - */ -[[nodiscard]] constexpr std::string_view -avalancheStateLabel(ConsensusParms::AvalancheState const state) -{ - switch (state) - { - case ConsensusParms::AvalancheState::Init: - return val::avalancheInit; - case ConsensusParms::AvalancheState::Mid: - return val::avalancheMid; - case ConsensusParms::AvalancheState::Late: - return val::avalancheLate; - case ConsensusParms::AvalancheState::Stuck: - return val::avalancheStuck; - } - return val::unknown; -} - -/** - * Map a ledger-close decision to its `close_reason` label. - * - * @param reason The value returned by whyCloseLedger(). - * @return The wire label; one of val::close*. - * - * @note No default arm, so a new enumerator is a -Wswitch warning; the - * fall-through returns "unknown". `keep_open` is mapped but never emitted. - */ -[[nodiscard]] constexpr std::string_view -closeReasonLabel(LedgerCloseReason const reason) -{ - switch (reason) - { - case LedgerCloseReason::KeepOpen: - return val::closeKeepOpen; - case LedgerCloseReason::Anomaly: - return val::closeAnomaly; - case LedgerCloseReason::OthersClosed: - return val::closeOthersClosed; - case LedgerCloseReason::Idle: - return val::closeIdle; - case LedgerCloseReason::Normal: - return val::closeNormal; - } - return val::unknown; -} - } // namespace xrpl::telemetry::consensus::span diff --git a/src/libxrpl/consensus/Consensus.cpp b/src/libxrpl/consensus/Consensus.cpp index 50d9a82d5f..6f3e36fe92 100644 --- a/src/libxrpl/consensus/Consensus.cpp +++ b/src/libxrpl/consensus/Consensus.cpp @@ -27,6 +27,7 @@ whyCloseLedger( beast::Journal j, std::unique_ptr const& clog) { + // Log text keeps the shouldCloseLedger name: consumers match on it. CLOG(clog) << "shouldCloseLedger params anyTransactions: " << anyTransactions << ", prevProposers: " << prevProposers << ", proposersClosed: " << proposersClosed << ", proposersValidated: " << proposersValidated diff --git a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp index d4cd8ebcb5..708a94f05f 100644 --- a/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp +++ b/src/tests/libxrpl/telemetry/ConsensusSpanNames.cpp @@ -1,6 +1,8 @@ #include #include +#include +#include #include @@ -68,14 +70,6 @@ TEST(ConsensusSpanNames, close_reason_label_maps_every_enum_state) EXPECT_EQ(closeReasonLabel(xrpl::LedgerCloseReason::Normal), "normal"); } -TEST(ConsensusSpanNames, close_reason_label_is_usable_at_compile_time) -{ - static_assert( - closeReasonLabel(xrpl::LedgerCloseReason::Idle) == "idle", - "closeReasonLabel must be constexpr-evaluable"); - SUCCEED(); -} - TEST(ConsensusSpanNames, establish_attribute_keys) { // Qualified: DisputedTx tracks a second, per-transaction avalanche. @@ -113,12 +107,11 @@ TEST(ConsensusSpanNames, avalanche_state_label_maps_every_enum_state) EXPECT_EQ(avalancheStateLabel(AvalancheState::Stuck), "stuck"); } -TEST(ConsensusSpanNames, avalanche_state_label_is_usable_at_compile_time) -{ - // The mapping is consteval-safe so the label costs nothing at the call - // site in endEstablishTracing(). - static_assert( - avalancheStateLabel(xrpl::ConsensusParms::AvalancheState::Stuck) == "stuck", - "avalancheStateLabel must be constexpr-evaluable"); - SUCCEED(); -} +// Both mappings are constexpr, so the labels cost nothing at their call sites. +// These fire at compile time and need no test body. +static_assert( + closeReasonLabel(xrpl::LedgerCloseReason::Idle) == "idle", + "closeReasonLabel must be constexpr-evaluable"); +static_assert( + avalancheStateLabel(xrpl::ConsensusParms::AvalancheState::Stuck) == "stuck", + "avalancheStateLabel must be constexpr-evaluable"); diff --git a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp index 75e5804bbb..36ab40b1b6 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp @@ -31,9 +31,10 @@ TEST(SpanGuardFactory, category_span_returns_null_when_disabled) EXPECT_FALSE(span); // Attribute keys use the underscore convention for span attributes (the - // dotted xrpl.. form is reserved for resource attributes). The - // canonical constants live in the xrpld-level *SpanNames.h headers, which a - // libxrpl test cannot include, so the keys are written as literals here. + // dotted xrpl.. form is reserved for resource attributes). These + // rpc_* constants live in an xrpld-level header, so they are literals here; + // libxrpl-level headers such as ConsensusSpanNames.h can be included + // directly, as ConsensusSpanNames.cpp does. span.setAttribute("command", "test"); span.setAttribute("rpc_status", "success"); }