From d50e0ff48e84ecc94b31dba085e3b1d43ad9d57d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Apr 2026 17:58:06 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20event=20name=20constants,=20span=20timing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add cons_span::event namespace with disputeResolve and txIncluded constants; replace hardcoded strings in Consensus.h and RCLConsensus.cpp - Move proposal.receive and validation.receive spans in PeerImp into shared_ptr captured by job lambdas so they measure checkPropose and checkValidation timing, not just message parsing Co-Authored-By: Claude Opus 4.6 --- src/xrpld/app/consensus/ConsensusSpanNames.h | 9 +++++ src/xrpld/app/consensus/RCLConsensus.cpp | 4 +- src/xrpld/consensus/Consensus.h | 2 +- src/xrpld/overlay/detail/PeerImp.cpp | 40 ++++++++++---------- 4 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/xrpld/app/consensus/ConsensusSpanNames.h b/src/xrpld/app/consensus/ConsensusSpanNames.h index 40e8eb4117..9304599e30 100644 --- a/src/xrpld/app/consensus/ConsensusSpanNames.h +++ b/src/xrpld/app/consensus/ConsensusSpanNames.h @@ -223,6 +223,15 @@ inline constexpr auto disputesCount = join(xrplConsensus, makeStr("disputes_coun inline constexpr auto trusted = join(xrplConsensus, makeStr("trusted")); } // namespace attr +// ===== Event names =========================================================== + +namespace event { +/// "dispute.resolve" +inline constexpr auto disputeResolve = join(makeStr("dispute"), makeStr("resolve")); +/// "tx.included" +inline constexpr auto txIncluded = join(makeStr("tx"), makeStr("included")); +} // namespace event + // ===== Attribute values ====================================================== namespace val { diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index e23eec1ecf..034b083d04 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -597,7 +597,9 @@ RCLConsensus::Adaptor::doAccept( JLOG(j_.debug()) << " Tx: " << item.key(); ++txCount; auto const txHash = to_string(item.key()); - doAcceptSpan.addEvent("tx.included", {{telemetry::cons_span::attr::txId, txHash}}); + doAcceptSpan.addEvent( + telemetry::cons_span::event::txIncluded, + {{telemetry::cons_span::attr::txId, txHash}}); } catch (std::exception const& ex) { diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index e2d1501b9c..bbaf1d9999 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -1557,7 +1557,7 @@ Consensus::updateOurPositions(std::unique_ptr const& auto const yaysStr = std::to_string(dispute.getYays()); auto const naysStr = std::to_string(dispute.getNays()); span.addEvent( - "dispute.resolve", + cons_span::event::disputeResolve, {{cons_span::attr::txId, to_string(txId)}, {cons_span::attr::disputeOurVote, dispute.getOurVote() ? "yes" : "no"}, {cons_span::attr::disputeYays, yaysStr}, diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 151285dc3c..03c743fc9b 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1944,13 +1944,6 @@ PeerImp::onMessage(std::shared_ptr const& m) } } - { - using namespace telemetry; - auto span = SpanGuard::span( - TraceCategory::Consensus, seg::consensus, cons_span::op::proposalReceive); - span.setAttribute(cons_span::attr::trusted, isTrusted); - } - JLOG(p_journal_.trace()) << "Proposal: " << (isTrusted ? "trusted" : "untrusted"); auto proposal = RCLCxPeerPos( @@ -1965,9 +1958,17 @@ PeerImp::onMessage(std::shared_ptr const& m) app_.getTimeKeeper().closeTime(), calcNodeID(app_.getValidatorManifests().getMasterKey(publicKey))}); + auto span = std::make_shared(telemetry::SpanGuard::span( + telemetry::TraceCategory::Consensus, + telemetry::seg::consensus, + telemetry::cons_span::op::proposalReceive)); + span->setAttribute(telemetry::cons_span::attr::trusted, isTrusted); + std::weak_ptr const weak = shared_from_this(); app_.getJobQueue().addJob( - isTrusted ? jtPROPOSAL_t : jtPROPOSAL_ut, "checkPropose", [weak, isTrusted, m, proposal]() { + isTrusted ? jtPROPOSAL_t : jtPROPOSAL_ut, + "checkPropose", + [weak, isTrusted, m, proposal, sp = std::move(span)]() { if (auto peer = weak.lock()) peer->checkPropose(isTrusted, m, proposal); }); @@ -2542,17 +2543,16 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } + auto span = std::make_shared(telemetry::SpanGuard::span( + telemetry::TraceCategory::Consensus, + telemetry::seg::consensus, + telemetry::cons_span::op::validationReceive)); + span->setAttribute(telemetry::cons_span::attr::trusted, isTrusted); + if (val->isFieldPresent(sfLedgerSequence)) { - using namespace telemetry; - auto span = SpanGuard::span( - TraceCategory::Consensus, seg::consensus, cons_span::op::validationReceive); - span.setAttribute(cons_span::attr::trusted, isTrusted); - if (val->isFieldPresent(sfLedgerSequence)) - { - span.setAttribute( - cons_span::attr::ledgerSeq, - static_cast(val->getFieldU32(sfLedgerSequence))); - } + span->setAttribute( + telemetry::cons_span::attr::ledgerSeq, + static_cast(val->getFieldU32(sfLedgerSequence))); } if (!isTrusted && (tracking_.load() == Tracking::diverged)) @@ -2565,7 +2565,9 @@ PeerImp::onMessage(std::shared_ptr const& m) std::weak_ptr const weak = shared_from_this(); app_.getJobQueue().addJob( - isTrusted ? jtVALIDATION_t : jtVALIDATION_ut, name, [weak, val, m, key]() { + isTrusted ? jtVALIDATION_t : jtVALIDATION_ut, + name, + [weak, val, m, key, sp = std::move(span)]() { if (auto peer = weak.lock()) peer->checkValidation(val, key, m); });