From 7d875c0e763d54f8d5d7b59d5cc53d34863bc113 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:30:13 +0100 Subject: [PATCH 1/8] refactor(telemetry): name the unit and epoch in the close-time span attrs 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. --- include/xrpl/consensus/ConsensusSpanNames.h | 24 ++++++++++++++----- include/xrpl/telemetry/SpanNames.h | 13 +++++++++- .../libxrpl/telemetry/SpanGuardFactory.cpp | 2 +- src/xrpld/app/consensus/RCLConsensus.cpp | 7 +++--- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index fc56adcd44..3db180c463 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -59,10 +59,10 @@ * | | * | +-- consensus.accept.apply [jtACCEPT thread, child of accept] * | Created: Adaptor::doAccept() - * | Attrs: ledger_seq, close_time, close_time_correct, + * | Attrs: ledger_seq, close_time_ripple_epoch_s, close_time_correct, * | close_resolution_ms, consensus_state, proposing, round_time_ms, - * | parent_close_time, close_time_self, close_time_vote_bins, - * | resolution_direction, tx_count + * | parent_close_time_ripple_epoch_s, close_time_self_ripple_epoch_s, + * | close_time_vote_bins, resolution_direction, tx_count * | Events: tx.included (per tx, attrs: tx_id) * | * +~~~ consensus.validation.send [jtACCEPT thread, linked] @@ -140,8 +140,8 @@ namespace attr { * concept, same key, distinguished by span name (not an emitter prefix). */ using ::xrpl::telemetry::attr::closeResolutionMs; -using ::xrpl::telemetry::attr::closeTime; using ::xrpl::telemetry::attr::closeTimeCorrect; +using ::xrpl::telemetry::attr::closeTimeRippleEpochS; using ::xrpl::telemetry::attr::fullValidation; using ::xrpl::telemetry::attr::ledgerHash; using ::xrpl::telemetry::attr::ledgerSeq; @@ -232,8 +232,20 @@ inline constexpr auto positionHashPrefix = makeStr("position_hash_prefix"); * "consensus_state" — domain-qualified (collides with other domains' state). */ inline constexpr auto consensusState = makeStr("consensus_state"); -inline constexpr auto parentCloseTime = makeStr("parent_close_time"); -inline constexpr auto closeTimeSelf = makeStr("close_time_self"); +/** + * Close-time instants, both NetClock readings in whole seconds since the XRP + * Ledger epoch (2000-01-01T00:00:00Z) — see `closeTimeRippleEpochS` in + * SpanNames.h for why the epoch is spelled into the key. + * + * `parentCloseTimeRippleEpochS` is the previous ledger's close time; + * `closeTimeSelfRippleEpochS` is this node's own close-time vote for the round, + * so the pair shows how far the node's position sat from the ledger it built on. + * + * `closeTimeVoteBins` is not a time: it holds the number of distinct close-time + * positions seen from peers this round. + */ +inline constexpr auto parentCloseTimeRippleEpochS = makeStr("parent_close_time_ripple_epoch_s"); +inline constexpr auto closeTimeSelfRippleEpochS = makeStr("close_time_self_ripple_epoch_s"); inline constexpr auto closeTimeVoteBins = makeStr("close_time_vote_bins"); inline constexpr auto resolutionDirection = makeStr("resolution_direction"); inline constexpr auto convergePercent = makeStr("converge_percent"); diff --git a/include/xrpl/telemetry/SpanNames.h b/include/xrpl/telemetry/SpanNames.h index b848f96c03..c24f0b254b 100644 --- a/include/xrpl/telemetry/SpanNames.h +++ b/include/xrpl/telemetry/SpanNames.h @@ -133,8 +133,19 @@ inline constexpr auto ledgerSeq = makeStr("ledger_seq"); /** * Shared close-time attrs — bare names, reused by consensus and ledger. + * + * `closeTimeRippleEpochS` carries a NetClock reading: whole seconds since the + * XRP Ledger epoch (2000-01-01T00:00:00Z), never the Unix epoch. The key names + * both the unit and the epoch because neither is recoverable from the value. + * A consumer rendering it as wall-clock time must first add kEpochOffset + * (946684800 seconds, see basics/chrono.h); read as a Unix timestamp instead, + * it lands roughly 30 years early. + * + * `closeResolutionMs` is a duration, not an instant — the granularity the + * close time is rounded to. NetClock resolution is whole seconds, so this + * value is always a multiple of 1000. */ -inline constexpr auto closeTime = makeStr("close_time"); +inline constexpr auto closeTimeRippleEpochS = makeStr("close_time_ripple_epoch_s"); inline constexpr auto closeTimeCorrect = makeStr("close_time_correct"); inline constexpr auto closeResolutionMs = makeStr("close_resolution_ms"); /** diff --git a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp index 36ab40b1b6..6cec7a5c86 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp @@ -99,7 +99,7 @@ TEST(SpanGuardFactory, consensus_close_time_attributes) auto span = telemetry::SpanGuard::span( telemetry::TraceCategory::Consensus, telemetry::seg::consensus, "accept.apply"); span.setAttribute("ledger_seq", static_cast(42)); - span.setAttribute("close_time", static_cast(780000000)); + span.setAttribute("close_time_ripple_epoch_s", static_cast(780000000)); span.setAttribute("close_time_correct", true); span.setAttribute("close_resolution_ms", static_cast(30000)); span.setAttribute("consensus_state", std::string("finished")); diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index a0e2f6f6d3..460a447631 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -635,7 +635,8 @@ RCLConsensus::Adaptor::doAccept( : telemetry::SpanGuard::childSpan(cs::acceptApply, roundSpanContext_); doAcceptSpan.setAttribute(cs::attr::ledgerSeq, static_cast(prevLedger.seq()) + 1); doAcceptSpan.setAttribute( - cs::attr::closeTime, static_cast(consensusCloseTime.time_since_epoch().count())); + cs::attr::closeTimeRippleEpochS, + static_cast(consensusCloseTime.time_since_epoch().count())); doAcceptSpan.setAttribute(cs::attr::closeTimeCorrect, closeTimeCorrect); doAcceptSpan.setAttribute( cs::attr::closeResolutionMs, @@ -648,10 +649,10 @@ RCLConsensus::Adaptor::doAccept( doAcceptSpan.setAttribute( cs::attr::roundTimeMs, static_cast(result.roundTime.read().count())); doAcceptSpan.setAttribute( - cs::attr::parentCloseTime, + cs::attr::parentCloseTimeRippleEpochS, static_cast(prevLedger.closeTime().time_since_epoch().count())); doAcceptSpan.setAttribute( - cs::attr::closeTimeSelf, + cs::attr::closeTimeSelfRippleEpochS, static_cast(rawCloseTimes.self.time_since_epoch().count())); doAcceptSpan.setAttribute( cs::attr::closeTimeVoteBins, static_cast(rawCloseTimes.peers.size())); From e0924231c1443707eb1dbb49e62cfd0e4f6b536d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:17 +0100 Subject: [PATCH 2/8] docs(telemetry): state the pathfind.discover span trade-off in the present tense The comment said per-asset timing 'is no longer split into individual spans'. This change introduces the span, so per-asset spans never existed for it to be split out of, and the comparison resolves against nothing once squash-merged. Comments only, no behaviour change. --- src/xrpld/rpc/detail/PathRequest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 827393b071..1154109fc9 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -596,8 +596,8 @@ PathRequest::findPaths( // One `pathfind.discover` span wraps the entire per-source-asset loop so // that a single RPC call produces one discover span instead of N (one per // candidate source asset). Trade-off: per-asset discovery/ranking timing - // is no longer split into individual spans — span count and Tempo storage - // are bounded per RPC at the cost of per-asset visibility. + // is not measured separately — span count and Tempo storage are bounded + // per RPC at the cost of per-asset visibility. // // This is an unscoped guard: it takes the ambient span as its own parent, // but does not itself become the ambient parent. Adding per-asset child From bbfde30eb1889916b44b07bcec7c9cd67f93a8a4 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:27 +0100 Subject: [PATCH 3/8] docs(consensus): drop pre-squash comparisons from the tracing comments Both comments pointed at a state the squash merge does not publish. - Consensus.h: 'yields a null guard, same as before' had no antecedent in the round or the function. Say instead that a null guard makes the setAttribute calls below no-ops, which is what SpanGuard's impl_ guard does. - ConsensusSpanLabels.h: 'Split from ConsensusSpanNames.h' describes a split performed entirely within this change; both headers first appear here. The dependency rationale and the diagram are unchanged. Comments only, no behaviour change. --- include/xrpl/consensus/Consensus.h | 5 +++-- include/xrpl/consensus/ConsensusSpanLabels.h | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 78750656a4..aced7555ea 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -1689,8 +1689,9 @@ Consensus::updateOurPositions(std::unique_ptr const& // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above using namespace telemetry; // Child of the establish span via its captured context (establishSpan_ is - // a thread-free SpanGuard, so parent explicitly via its context). Null - // context (establish not started) yields a null guard, same as before. + // a thread-free SpanGuard, so parent explicitly via its context). A null + // context — the establish phase has not started — yields a null guard, so + // the setAttribute calls below are no-ops. auto span = SpanGuard::childSpan(consensus::span::updatePositions, establishSpanContext_); span.setAttribute( consensus::span::attr::convergePercent, static_cast(convergePercent_)); diff --git a/include/xrpl/consensus/ConsensusSpanLabels.h b/include/xrpl/consensus/ConsensusSpanLabels.h index cc66639c41..21b867d128 100644 --- a/include/xrpl/consensus/ConsensusSpanLabels.h +++ b/include/xrpl/consensus/ConsensusSpanLabels.h @@ -3,10 +3,11 @@ /** * 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. + * These mappings live in their own header so ConsensusSpanNames.h 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) * ^ From 488617cc0b540ee4a7ceb77a1d791e1a97794ec3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:51:47 +0100 Subject: [PATCH 4/8] refactor(telemetry): mark the unused setter parameter [[maybe_unused]] The default setServiceInstanceId() body ignores its argument. Use the attribute rather than a (void) cast: the codebase already uses it 104 times and the build is C++23. --- include/xrpl/telemetry/Telemetry.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 624fcf15dc..3c487638f2 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -260,10 +260,9 @@ public: * @param id The node's base58-encoded public key or custom identifier. */ virtual void - setServiceInstanceId(std::string const& id) + setServiceInstanceId([[maybe_unused]] std::string const& id) { // Default no-op for NullTelemetry implementations. - (void)id; } /** From 255a4134e73a629f4a4e81998bab93dc5c8c5977 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:51:20 +0100 Subject: [PATCH 5/8] fix(telemetry): record preclaim errors and apply-stage exceptions The three apply-pipeline stages disagreed on span status. preflight set Error on any non-success TER, preclaim never set it at all, and the apply stage recorded nothing when a transaction threw. - preclaim now sets Error for any non-success result, matching preflight. Routine retryable results (terPRE_SEQ, telINSUF_FEE_P) are included, so stage=preclaim error rates will rise and track normal queueing. - The apply span no longer keys Error on canApply. A dry run reports tesSUCCESS with canApply false, which was recorded as an error. Every other path with canApply false already has a non-success result, so !isTesSuccess covers them. - Exceptions escaping the apply stage set ter_result=tefEXCEPTION and record an exception event, then rethrow unchanged, mirroring what invokePreflight and invokePreclaim already do. The caller still maps the exception to tefEXCEPTION, so behaviour is unchanged. - Two comments claimed every exit funnels through the logger lambda. A throw does not, so they now say each return path. --- src/libxrpl/tx/Transactor.cpp | 247 ++++++++++++++++++---------------- src/libxrpl/tx/applySteps.cpp | 4 + 2 files changed, 135 insertions(+), 116 deletions(-) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 4eb43d1596..4e6dbf33b2 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -49,6 +49,7 @@ #include #include #include +#include #include #include #include @@ -1624,126 +1625,140 @@ Transactor::operator()() trapTransaction(*trap); } - auto result = ctx_.preclaimResult; - if (isTesSuccess(result)) - result = apply(); - - // No transaction can return temUNKNOWN from apply, - // and it can't be passed in from a preclaim. - XRPL_ASSERT(result != temUNKNOWN, "xrpl::Transactor::operator() : result is not temUNKNOWN"); - - if (auto stream = j_.trace()) - stream << "preclaim result: " << transToken(result); - - auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); - bool const canApply = std::invoke([&result, &fee, this] { - bool canApplyTmp = isTesSuccess(result); - - if (ctx_.size() > kOversizeMetaDataCap) - result = tecOVERSIZE; - - if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) - { - // If the TapFailHard flag is set, a tec result - // must not do anything - ctx_.discard(); - canApplyTmp = false; - } - else if ( - (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || - (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) - { - // This is and must remain the only place where `canApplyTmp` can change from false to - // true. Changing from true to false is no problem. - std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); - } - return canApplyTmp; - }); - - // Every exit from this function funnels through here, so this is also where - // the apply span records its outcome: each return path reports the engine - // result and whether the transaction was applied. - auto const logger = [this, &span]( - TER result, - bool canApply, - std::optional&& metadata = std::nullopt) -> ApplyResult { - JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); - - // Also guarded: transToken() is a lookup returning a string, and this - // funnel runs on every exit path. - if (span) - { - span.setAttribute( - telemetry::tx_apply_span::attr::terResult, transToken(result).c_str()); - span.setAttribute(telemetry::tx_apply_span::attr::applied, canApply); - // Mark the span as errored when the transaction was not applied or - // the engine result is not a success, so failed applies surface in - // span-status error counts alongside preflight and preclaim. - if (!canApply || !isTesSuccess(result)) - span.setError(transToken(result)); - } - - return {result, canApply, std::move(metadata)}; - }; - - if (!canApply) - return logger(result, canApply); - - // First invariant pass: both protocol and transaction-specific - // checks run against the transaction's tentative outcome. If it - // does not return tecINVARIANT_FAILED, we can proceed to apply the - // tx. - result = checkInvariants(result, fee, InvariantScope::Full); - if (result == tecINVARIANT_FAILED) + try { - // Fee-claim reset: roll the transaction's effects back so that - // only the fee deduction remains. This is the reset referenced - // by InvariantScope::ProtocolOnly. - auto const resetResult = reset(fee); - if (!isTesSuccess(resetResult.first)) - result = resetResult.first; + auto result = ctx_.preclaimResult; + if (isTesSuccess(result)) + result = apply(); - fee = resetResult.second; + // No transaction can return temUNKNOWN from apply, + // and it can't be passed in from a preclaim. + XRPL_ASSERT( + result != temUNKNOWN, "xrpl::Transactor::operator() : result is not temUNKNOWN"); - // Re-check invariants against the post-reset (fee-claim only) - // state. The transaction's effects are gone, so the - // transaction-specific invariants no longer apply and only the - // protocol invariants are re-run. A failure here escalates to - // tefINVARIANT_FAILED and excludes the tx from the ledger. - if (isTesSuccess(result) || isTecClaim(result)) - result = checkInvariants(result, fee, InvariantScope::ProtocolOnly); + if (auto stream = j_.trace()) + stream << "preclaim result: " << transToken(result); + + auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); + bool const canApply = std::invoke([&result, &fee, this] { + bool canApplyTmp = isTesSuccess(result); + + if (ctx_.size() > kOversizeMetaDataCap) + result = tecOVERSIZE; + + if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) + { + // If the TapFailHard flag is set, a tec result + // must not do anything + ctx_.discard(); + canApplyTmp = false; + } + else if ( + (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || + (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) + { + // This is and must remain the only place where `canApplyTmp` can change from false + // to true. Changing from true to false is no problem. + std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); + } + return canApplyTmp; + }); + + // Each return path funnels through here, so this is also where the apply + // span records its outcome: the engine result and whether the transaction + // was applied. A throw bypasses it and ends the span with no outcome. + auto const logger = [this, &span]( + TER result, + bool canApply, + std::optional&& metadata = std::nullopt) -> ApplyResult { + JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); + + // Also guarded: transToken() is a lookup returning a string, and this + // funnel runs on every return path. + if (span) + { + span.setAttribute( + telemetry::tx_apply_span::attr::terResult, transToken(result).c_str()); + span.setAttribute(telemetry::tx_apply_span::attr::applied, canApply); + // Mark the span as errored when the engine result is not a success, + // so failed applies surface alongside preflight and preclaim. Not + // keyed on `canApply`: a dry run reports tesSUCCESS with canApply + // false, and that is not a failure. + if (!isTesSuccess(result)) + span.setError(transToken(result)); + } + + return {result, canApply, std::move(metadata)}; + }; + + if (!canApply) + return logger(result, canApply); + + // First invariant pass: both protocol and transaction-specific + // checks run against the transaction's tentative outcome. If it + // does not return tecINVARIANT_FAILED, we can proceed to apply the + // tx. + result = checkInvariants(result, fee, InvariantScope::Full); + if (result == tecINVARIANT_FAILED) + { + // Fee-claim reset: roll the transaction's effects back so that + // only the fee deduction remains. This is the reset referenced + // by InvariantScope::ProtocolOnly. + auto const resetResult = reset(fee); + if (!isTesSuccess(resetResult.first)) + result = resetResult.first; + + fee = resetResult.second; + + // Re-check invariants against the post-reset (fee-claim only) + // state. The transaction's effects are gone, so the + // transaction-specific invariants no longer apply and only the + // protocol invariants are re-run. A failure here escalates to + // tefINVARIANT_FAILED and excludes the tx from the ledger. + if (isTesSuccess(result) || isTecClaim(result)) + result = checkInvariants(result, fee, InvariantScope::ProtocolOnly); + } + + // We ran through the invariant checker, which can, in some cases, + // return a tef error code. Don't apply the transaction in that case. + if (!isTecClaim(result) && !isTesSuccess(result)) + return logger(result, false); + + std::optional metadata; + + // Transaction succeeded fully or (retries are not allowed and the + // transaction could claim a fee) + + // The transactor and invariant checkers guarantee that this will + // *never* trigger but if it, somehow, happens, don't allow a tx + // that charges a negative fee. + if (fee < beast::kZero) + Throw("fee charged is negative!"); + + // Charge whatever fee they specified. The fee has already been + // deducted from the balance of the account that issued the + // transaction. We just need to account for it in the ledger + // header. + if (!view().open() && fee != beast::kZero) + ctx_.destroyXRP(fee); + + // Once we call apply, we will no longer be able to look at view() + metadata = ctx_.apply(result); + + if ((ctx_.flags() & TapDryRun) != 0u) + return logger(result, false, std::move(metadata)); + + return logger(result, canApply, std::move(metadata)); + } + catch (std::exception const& e) + { + // The caller's doApply() maps this to tefEXCEPTION. Record it on the + // span before unwinding so per-stage error counts include exceptions. + span.setAttribute( + telemetry::tx_apply_span::attr::terResult, transToken(tefEXCEPTION).c_str()); + span.recordException(e); + throw; } - - // We ran through the invariant checker, which can, in some cases, - // return a tef error code. Don't apply the transaction in that case. - if (!isTecClaim(result) && !isTesSuccess(result)) - return logger(result, false); - - std::optional metadata; - - // Transaction succeeded fully or (retries are not allowed and the - // transaction could claim a fee) - - // The transactor and invariant checkers guarantee that this will - // *never* trigger but if it, somehow, happens, don't allow a tx - // that charges a negative fee. - if (fee < beast::kZero) - Throw("fee charged is negative!"); - - // Charge whatever fee they specified. The fee has already been - // deducted from the balance of the account that issued the - // transaction. We just need to account for it in the ledger - // header. - if (!view().open() && fee != beast::kZero) - ctx_.destroyXRP(fee); - - // Once we call apply, we will no longer be able to look at view() - metadata = ctx_.apply(result); - - if ((ctx_.flags() & TapDryRun) != 0u) - return logger(result, false, std::move(metadata)); - - return logger(result, canApply, std::move(metadata)); } } // namespace xrpl diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 45758bf616..3ec6c25aa8 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -314,6 +314,10 @@ invokePreclaim(PreclaimContext const& ctx) { span.setAttribute( telemetry::tx_apply_span::attr::terResult, transToken(preclaimTer).c_str()); + // Mark the span as errored when preclaim rejects the transaction so + // failed stages surface in span-status error counts. + if (!isTesSuccess(preclaimTer)) + span.setError(transToken(preclaimTer)); } return preclaimTer; } From ef10ef98139d8e3e6716cee9363707434d877d9f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:51:56 +0100 Subject: [PATCH 6/8] fix(telemetry): separate the validation.receive latency populations consensus.validation.receive is created before the drop decision, so one span name covered three exits with unrelated cost profiles: two drop paths that end in microseconds, and a queued path whose handle is moved into the job and so covers job wait plus checkValidation. No existing attribute separated them. validation_trusted=true implies the queued path, but validation_trusted=false spans both drop paths and the queued path, so any quantile over the span mixed the populations. Both drop paths are live in the default config, which sets relayUntrustedValidations. Adds validation_status, set once on each exit rather than as a default, following the tx_status precedent in the same file: dropped_diverged, queued, dropped_load. On the queued path it is set before the handle is moved into the job. --- include/xrpl/consensus/ConsensusSpanNames.h | 12 +++++++++++ src/xrpld/overlay/detail/PeerImp.cpp | 23 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index 3db180c463..f71b332457 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -287,6 +287,14 @@ inline constexpr auto disputesCount = makeStr("disputes_count"); */ inline constexpr auto proposalTrusted = makeStr("proposal_trusted"); inline constexpr auto validationTrusted = makeStr("validation_trusted"); + +/** + * "validation_status" — which exit the inbound validation took. Set once per + * exit, so a dropped validation (microseconds) is separable from a queued one + * (job wait plus checkValidation). Without it the span name reports two + * unrelated latency distributions and every quantile over it is meaningless. + */ +inline constexpr auto validationStatus = makeStr("validation_status"); } // namespace attr // ===== Event names =========================================================== @@ -352,6 +360,10 @@ inline constexpr auto closeAnomaly = makeStr("anomaly"); inline constexpr auto closeOthersClosed = makeStr("others_closed"); inline constexpr auto closeIdle = makeStr("idle"); inline constexpr auto closeNormal = makeStr("normal"); +// validation_status values, one per exit of the inbound validation path. +inline constexpr auto validationQueued = makeStr("queued"); +inline constexpr auto validationDroppedDiverged = makeStr("dropped_diverged"); +inline constexpr auto validationDroppedLoad = makeStr("dropped_load"); } // namespace val } // namespace xrpl::telemetry::consensus::span diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 9505a954be..cfc921d63e 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -2671,12 +2671,29 @@ PeerImp::onMessage(std::shared_ptr const& m) static_cast(val->getSignTime().time_since_epoch().count())); } + // validation_status is set once on each exit below, not as a default + // here, to avoid OTel SDK attribute duplication. It is what separates + // the microsecond drop paths from the queued path, which also covers + // job wait and checkValidation. if (!isTrusted && (tracking_.load() == Tracking::Diverged)) { + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationDroppedDiverged); + } JLOG(pJournal_.debug()) << "Dropping untrusted validation from diverged peer"; } else if (isTrusted || !app_.getFeeTrack().isLoadedLocal()) { + // Set before the handle is moved into the job below. + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationQueued); + } std::string const name = isTrusted ? "ChkTrust" : "ChkUntrust"; std::weak_ptr const weak = shared_from_this(); @@ -2690,6 +2707,12 @@ PeerImp::onMessage(std::shared_ptr const& m) } else { + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationDroppedLoad); + } JLOG(pJournal_.debug()) << "Dropping untrusted validation for load"; } } From 9d71dea9723c37da1c354cdaa11f4577e05e3b29 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:00:28 +0100 Subject: [PATCH 7/8] refactor(telemetry): mark the telemetry accessors nodiscard getInstance(), getTracer(), both startSpan() overloads and networkTypeFromId() return values that a caller must use. A discarded startSpan() result destroys the returned span immediately, so the span opens and closes with no content. Six methods in Telemetry.h already carried the attribute, on the base and on every override. The new attributes follow that: the overrides in Telemetry.cpp and NullTelemetry.cpp get it too, because [[nodiscard]] is not inherited and a call bound to the derived type would otherwise be unchecked. No caller anywhere in the chain discards any of these results. --- include/xrpl/telemetry/Telemetry.h | 8 ++++---- src/libxrpl/telemetry/NullTelemetry.cpp | 6 +++--- src/libxrpl/telemetry/Telemetry.cpp | 12 ++++++------ src/libxrpl/telemetry/TelemetryConfig.cpp | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 3c487638f2..a7156c0bfa 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -120,7 +120,7 @@ public: * Get the global Telemetry instance. * @return Pointer to the active instance, or nullptr if not started. */ - static Telemetry* + [[nodiscard]] static Telemetry* getInstance() { return instance.load(std::memory_order_acquire); @@ -322,7 +322,7 @@ public: * @param name Tracer name used to identify the instrumentation library. * @return A shared pointer to the Tracer. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr getTracer(std::string_view name = kTracerName) = 0; /** @@ -340,7 +340,7 @@ public: * - kConsumer: async message receive * @return A shared pointer to the new Span. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::trace::SpanKind kind = opentelemetry::trace::SpanKind::kInternal) = 0; @@ -356,7 +356,7 @@ public: * @param kind The span kind (defaults to kInternal). * @return A shared pointer to the new Span. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::context::Context const& parentContext, diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index 32244af0b5..81ad9e108a 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -104,7 +104,7 @@ public: } #ifdef XRPL_ENABLE_TELEMETRY - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view) override { static auto noopTracer = opentelemetry::nostd::shared_ptr( @@ -112,14 +112,14 @@ public: return noopTracer; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, opentelemetry::trace::SpanKind) override { return opentelemetry::nostd::shared_ptr( new opentelemetry::trace::NoopSpan(nullptr)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan( std::string_view, opentelemetry::context::Context const&, diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 8076146a9c..d07f480a58 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -215,7 +215,7 @@ public: return false; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view) override { static auto noopTracer = @@ -223,13 +223,13 @@ public: return noopTracer; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, trace_api::SpanKind) override { return opentelemetry::nostd::shared_ptr(new trace_api::NoopSpan(nullptr)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, opentelemetry::context::Context const&, trace_api::SpanKind) override { @@ -428,7 +428,7 @@ public: return setup_.traceLedger; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view name = kTracerName) override { if (!sdkProvider_) @@ -438,7 +438,7 @@ public: return sdkProvider_->GetTracer(std::string(name)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view name, trace_api::SpanKind kind) override { auto tracer = getTracer(); @@ -447,7 +447,7 @@ public: return tracer->StartSpan(std::string(name), opts); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::context::Context const& parentContext, diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index e18a8d4a97..828b303c58 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -65,7 +65,7 @@ constexpr std::uint32_t maxQueueSize = 2048u; * @param networkId The network identifier from [network_id] config. * @return "mainnet", "testnet", "devnet", or "unknown" for other values. */ -std::string +[[nodiscard]] std::string networkTypeFromId(std::uint32_t networkId) { switch (networkId) From 75ccf9a0970ed7019c710de3e23c61fcecdcaa57 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:01:08 +0100 Subject: [PATCH 8/8] refactor(telemetry): mark the span factories and trace-context checks nodiscard The trace-context checks validate bytes received from a peer, so a discarded result means untrusted input was accepted unchecked. hashSpan(), txReceiveSpan() and txProcessSpan() return an RAII guard; discarding one ends the span on the same line it began. The telemetry-disabled twins of both hashSpan overloads already carried the attribute, so the two #ifdef arms now agree. Both hashSpan overloads also gain the @return line their siblings already had, now that the result cannot be dropped. No caller in the chain discards any of these results. --- include/xrpl/telemetry/SpanGuard.h | 8 ++++++-- include/xrpl/telemetry/TraceContextPropagator.h | 2 +- include/xrpl/telemetry/TraceContextValidation.h | 6 +++--- src/xrpld/telemetry/TxTracing.h | 11 +++++++++-- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 6fb60bbac2..6a31ecd83a 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -394,8 +394,10 @@ public: * @param name Full span name (e.g. "tx.receive"). * @param hashData Pointer to at least 16 bytes of hash data. * @param hashSize Size of the hash buffer (must be >= 16). + * @return An active guard, or a null guard when the category is + * disabled or hashSize is under 16. */ - static SpanGuard + [[nodiscard]] static SpanGuard hashSpan( TraceCategory const cat, std::string_view const name, @@ -414,8 +416,10 @@ public: * @param parentSpanId Pointer to 8 bytes of parent span ID. * @param parentSpanSize Size of parent span ID buffer (must be 8). * @param traceFlags Trace flags from remote context. + * @return An active guard, or a null guard when the category is + * disabled, hashSize is under 16, or parentSpanSize is not 8. */ - static SpanGuard + [[nodiscard]] static SpanGuard hashSpan( TraceCategory const cat, std::string_view const name, diff --git a/include/xrpl/telemetry/TraceContextPropagator.h b/include/xrpl/telemetry/TraceContextPropagator.h index 6a698c1933..ca1e0676f1 100644 --- a/include/xrpl/telemetry/TraceContextPropagator.h +++ b/include/xrpl/telemetry/TraceContextPropagator.h @@ -42,7 +42,7 @@ namespace xrpl::telemetry { * @return An OTel Context with the extracted parent span, or an empty * context if the protobuf fields are missing or invalid. */ -inline opentelemetry::context::Context +[[nodiscard]] inline opentelemetry::context::Context extractFromProtobuf(protocol::TraceContext const& proto) { namespace trace = opentelemetry::trace; diff --git a/include/xrpl/telemetry/TraceContextValidation.h b/include/xrpl/telemetry/TraceContextValidation.h index e69b2d17ca..c299ba6cf9 100644 --- a/include/xrpl/telemetry/TraceContextValidation.h +++ b/include/xrpl/telemetry/TraceContextValidation.h @@ -57,7 +57,7 @@ namespace xrpl::telemetry { * @param traceId The raw trace_id bytes from a protobuf TraceContext. * @return true if usable as a trace identifier, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidTraceId(std::string const& traceId) { return traceId.size() == 16 && std::ranges::any_of(traceId, [](char c) { return c != 0; }); @@ -69,7 +69,7 @@ isValidTraceId(std::string const& traceId) * @param spanId The raw span_id bytes from a protobuf TraceContext. * @return true if usable as a span identifier, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidSpanId(std::string const& spanId) { return spanId.size() == 8 && std::ranges::any_of(spanId, [](char c) { return c != 0; }); @@ -86,7 +86,7 @@ isValidSpanId(std::string const& spanId) * @param tc The protobuf TraceContext received from a peer. * @return true if both ids are present and valid, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidTraceContext(protocol::TraceContext const& tc) { return tc.has_trace_id() && isValidTraceId(tc.trace_id()) && tc.has_span_id() && diff --git a/src/xrpld/telemetry/TxTracing.h b/src/xrpld/telemetry/TxTracing.h index 5baf01df2d..682f482f1b 100644 --- a/src/xrpld/telemetry/TxTracing.h +++ b/src/xrpld/telemetry/TxTracing.h @@ -32,8 +32,12 @@ namespace xrpl::telemetry { * trace_id is derived from txID[0:16]. If the incoming message carries * a protobuf TraceContext with a valid span_id, it is used as the * parent to preserve relay ordering. + * @param txID Transaction id; its first 16 bytes become the trace_id. + * @param msg The received message, read only for its trace context. + * @return An active guard, or a null guard when the Transactions category + * is disabled. Bind it: a discarded guard ends the span immediately. */ -inline SpanGuard +[[nodiscard]] inline SpanGuard txReceiveSpan(uint256 const& txID, [[maybe_unused]] protocol::TMTransaction const& msg) { #ifdef XRPL_ENABLE_TELEMETRY @@ -63,8 +67,11 @@ txReceiveSpan(uint256 const& txID, [[maybe_unused]] protocol::TMTransaction cons /** * Create a "tx.process" span for transaction processing in NetworkOPs. * trace_id is derived from txID[0:16]. + * @param txID Transaction id; its first 16 bytes become the trace_id. + * @return An active guard, or a null guard when the Transactions category + * is disabled. Bind it: a discarded guard ends the span immediately. */ -inline SpanGuard +[[nodiscard]] inline SpanGuard txProcessSpan(uint256 const& txID) { return SpanGuard::hashSpan(