From 095a702fbe700407b874f271c6602eb7cde061a6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 15:46:06 +0100 Subject: [PATCH 1/8] fix(telemetry): keep the compiled-out span guards non-trivially destructible 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 inside the telemetry guard: only the telemetry-enabled types hold a unique_ptr, so the include is unused when telemetry is compiled out. --- include/xrpl/telemetry/SpanGuard.h | 54 +++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 7b317c09de..ee4ac779e5 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -176,8 +176,14 @@ #include #include -#include #include +#include + +#ifdef XRPL_ENABLE_TELEMETRY +// Only the telemetry-enabled types hold a unique_ptr; the compiled-out ones +// hold nothing, so this include would be unused there. +#include +#endif namespace xrpl::telemetry { @@ -847,7 +853,16 @@ class ScopedActivation { public: ScopedActivation() = default; - ~ScopedActivation() = default; + /** + * Written out by hand rather than defaulted, on purpose. A defaulted + * destructor on an empty class is trivial, and compilers then report every + * activation that is held only for its scope as an unused variable. Writing + * the destructor by hand matches the real ScopedActivation and keeps those + * call sites warning free. The body is empty, so no code is generated. + */ + ~ScopedActivation() // NOLINT(modernize-use-equals-default) + { + } ScopedActivation(ScopedActivation&&) = delete; ScopedActivation& operator=(ScopedActivation&&) = delete; @@ -860,7 +875,14 @@ class SpanGuard { public: SpanGuard() = default; - ~SpanGuard() = default; + /** + * Written out by hand rather than defaulted, for the same reason as + * ScopedActivation above: a trivial destructor makes a guard that is held + * only for its scope look like an unused variable. Empty body, no code. + */ + ~SpanGuard() // NOLINT(modernize-use-equals-default) + { + } SpanGuard(SpanGuard&&) noexcept = default; SpanGuard& operator=(SpanGuard&&) noexcept = default; @@ -983,7 +1005,14 @@ public: ScopedSpanGuard(TraceCategory, std::string_view, std::string_view) noexcept { } - ~ScopedSpanGuard() = default; + /** + * Written out by hand rather than defaulted, for the same reason as + * ScopedActivation above: a trivial destructor makes a guard that is held + * only for its scope look like an unused variable. Empty body, no code. + */ + ~ScopedSpanGuard() // NOLINT(modernize-use-equals-default) + { + } ScopedSpanGuard(ScopedSpanGuard&&) = delete; ScopedSpanGuard& @@ -1099,4 +1128,21 @@ activateIfLive(SpanGuardHandle const& guard) #endif // XRPL_ENABLE_TELEMETRY +// These three types are held purely for their scope: callers create one and +// never read it again. A compiler only stays quiet about such a variable if +// destroying it might do something, which means the destructor must not be +// trivial. Both the real types and the compiled-out ones therefore declare a +// destructor by hand. Asserting it here fails the build immediately if one is +// ever changed back to `= default`, instead of producing an unused-variable +// error at every call site. +static_assert( + !std::is_trivially_destructible_v, + "SpanGuard must keep a hand-written destructor; see the note above"); +static_assert( + !std::is_trivially_destructible_v, + "ScopedSpanGuard must keep a hand-written destructor; see the note above"); +static_assert( + !std::is_trivially_destructible_v, + "ScopedActivation must keep a hand-written destructor; see the note above"); + } // namespace xrpl::telemetry From 05d500d755cbe1969ee78d305fc4ae0e1a616e88 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:12 +0100 Subject: [PATCH 2/8] fix(telemetry): guard the includes only the telemetry build uses With telemetry compiled out, in Telemetry.h and 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. --- include/xrpl/telemetry/SpanGuard.h | 15 +++++++++------ include/xrpl/telemetry/Telemetry.h | 4 +++- src/libxrpl/telemetry/NullTelemetry.cpp | 6 ++++++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index ee4ac779e5..284815a900 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -180,8 +180,8 @@ #include #ifdef XRPL_ENABLE_TELEMETRY -// Only the telemetry-enabled types hold a unique_ptr; the compiled-out ones -// hold nothing, so this include would be unused there. +// The smart-pointer members all belong to the telemetry-enabled declarations; +// the compiled-out types hold nothing, so this include is unused there. #include #endif @@ -858,7 +858,8 @@ public: * destructor on an empty class is trivial, and compilers then report every * activation that is held only for its scope as an unused variable. Writing * the destructor by hand matches the real ScopedActivation and keeps those - * call sites warning free. The body is empty, so no code is generated. + * call sites warning free. The body is empty, so it costs nothing once + * inlined. */ ~ScopedActivation() // NOLINT(modernize-use-equals-default) { @@ -878,7 +879,8 @@ public: /** * Written out by hand rather than defaulted, for the same reason as * ScopedActivation above: a trivial destructor makes a guard that is held - * only for its scope look like an unused variable. Empty body, no code. + * only for its scope look like an unused variable. The empty body costs + * nothing once inlined. */ ~SpanGuard() // NOLINT(modernize-use-equals-default) { @@ -1008,7 +1010,8 @@ public: /** * Written out by hand rather than defaulted, for the same reason as * ScopedActivation above: a trivial destructor makes a guard that is held - * only for its scope look like an unused variable. Empty body, no code. + * only for its scope look like an unused variable. The empty body costs + * nothing once inlined. */ ~ScopedSpanGuard() // NOLINT(modernize-use-equals-default) { @@ -1133,7 +1136,7 @@ activateIfLive(SpanGuardHandle const& guard) // destroying it might do something, which means the destructor must not be // trivial. Both the real types and the compiled-out ones therefore declare a // destructor by hand. Asserting it here fails the build immediately if one is -// ever changed back to `= default`, instead of producing an unused-variable +// ever replaced with `= default`, instead of producing an unused-variable // error at every call site. static_assert( !std::is_trivially_destructible_v, diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index f915818d48..624fcf15dc 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -79,7 +79,6 @@ #include #include #include -#include #ifdef XRPL_ENABLE_TELEMETRY #include @@ -87,6 +86,9 @@ #include #include #include + +// std::string_view appears only in the telemetry-enabled declarations below. +#include #endif namespace xrpl::telemetry { diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index 48d48e557d..32244af0b5 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -13,6 +13,12 @@ #include +#ifndef XRPL_ENABLE_TELEMETRY +// beast::Journal is named only by the compiled-out makeTelemetry() below, so +// this include belongs to that configuration and would be unused in the other. +#include +#endif + #include #include From 12e0fddeae68dd472a51a3a60c0fe5a66c5f959c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:32:19 +0100 Subject: [PATCH 3/8] perf(rpc): skip WS command resolution when no span records it The rpc.ws_message span's command attribute was resolved for every inbound WebSocket message, whether or not anything was recording it. The resolver does two JSON member tests plus two subscripts, copies the command into a std::string, calls getAPIVersionNumber and then looks the name up in the handler multimap. Because it is a call argument to setAttribute, it ran even with telemetry compiled out, where setAttribute's body is empty. Wrapping the call in "if (span)" drops that work entirely when telemetry is off, and also when telemetry is on but this span is not being recorded. Nothing outside the attribute reads the resolved value, so no other behaviour changes; the real request validation further down computes its own api version, command string and handler role. --- src/xrpld/rpc/detail/ServerHandler.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 0376345611..16184df92e 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -478,7 +478,15 @@ ServerHandler::processSession( // else collapses to "unknown". Emitting the raw string would let request // input drive unbounded label cardinality. Mirrors the HTTP path's // resolveCommandSpanName(). - span.setAttribute(rpc_span::attr::command, resolveWsCommandSpanName(jv, app_.config())); + // + // The guard is required because the resolver is a call argument: it runs + // even when setAttribute itself is an empty no-op. Without it, every + // WebSocket message pays for the JSON member lookups, a string copy and a + // handler-registry lookup that nothing reads. + if (span) + { + span.setAttribute(rpc_span::attr::command, resolveWsCommandSpanName(jv, app_.config())); + } auto is = std::static_pointer_cast(session->appDefined); if (is->getConsumer().disconnect(journal_)) From da68beaef4358f868cb0ea32c3abe9818c0e1704 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:44:28 +0100 Subject: [PATCH 4/8] Compile out the RPC error-span name resolver with telemetry off resolveCommandSpanName() and the error span in doCommand() exist only to name and label a telemetry span. With telemetry compiled out the resolver still ran on every RPC that fillHandler() rejects: up to five json isMember lookups, up to three string copies, a virtual config() call and a handler-table lookup, all to build a name nobody records. A storm of malformed requests paid that cost once per request. A runtime `if (span)` guard cannot work here. The resolver's result is the span name itself, passed as the third argument of the ScopedSpanGuard constructor, so no span object exists yet to test. That leaves `#ifdef XRPL_ENABLE_TELEMETRY`, matching the house style used elsewhere. The guard covers the whole telemetry block at the call site and the helper definition too, so the file-static helper does not become an unreferenced function, which the build rejects because warnings are errors. injectError() and the error return stay outside the guard, so behaviour on the failure path is unchanged. No include is orphaned: ErrorCodes.h, SpanGuard.h, RpcSpanNames.h and all keep uses outside the guards. With telemetry on nothing changes: only comments and the four directives were added. --- src/xrpld/rpc/detail/RPCHandler.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index b1c213c463..e3f136c4fe 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -222,6 +222,14 @@ callMethod(JsonContext& context, Method method, std::string const& name, Object& } } +// Telemetry-only helper, so it is compiled out with telemetry off. Left +// running it would cost several json lookups, up to two string copies and a +// handler-table lookup on every failed request, for a name nobody records. +// Its result IS the span name, so `if (span)` cannot guard it: at that point +// no span exists to test. The single call site is gated the same way, which +// also keeps this file-static function referenced in both configurations. +#ifdef XRPL_ENABLE_TELEMETRY + // Resolve the span suffix / command attribute for a request that failed in // fillHandler. Returns the canonical handler name for a recognized command // (a finite, bounded set) or the literal "unknown" for a request that omits @@ -255,6 +263,8 @@ resolveCommandSpanName(JsonContext const& context) : std::string_view{rpc_span::val::unknownCommand}; } +#endif // XRPL_ENABLE_TELEMETRY + } // namespace Status @@ -263,6 +273,11 @@ doCommand(rpc::JsonContext& context, json::Value& result) Handler const* handler = nullptr; if (auto error = fillHandler(context, handler)) { + // Every statement below only feeds the error span, and the span name + // itself comes from resolveCommandSpanName(), so there is no span + // object to test with `if (span)`. With telemetry off the whole block + // is compiled out and a storm of malformed requests pays nothing. +#ifdef XRPL_ENABLE_TELEMETRY // Bound the span name and command attribute to the finite set of // registered handler names (plus "unknown") — see the helper for why // raw request input must not reach the telemetry pipeline. @@ -278,6 +293,7 @@ doCommand(rpc::JsonContext& context, json::Value& result) : std::string_view(rpc_span::val::user)); span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error); span.setError(getErrorInfo(error).token.cStr()); +#endif // XRPL_ENABLE_TELEMETRY injectError(error, result); return error; From fcdbbabedfd0ee6f41ae5f3319205fc08ddd3056 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:48:38 +0100 Subject: [PATCH 5/8] perf(rpc): hash pathfind accounts only when the span records them doPathFind and doRipplePathFind fill two span attributes from the request's source and destination accounts. Both values are call arguments, so they are built whatever the build: asString() copies the address out of the JSON and redactAccount() takes a SHA-512Half over it and formats 16 hex characters. That is two copies and two hashes on every pathfinding RPC, for pathfind_source_account and pathfind_dest_account, which nothing outside the span reads. Wrapping the block in "if (span)" drops that work when telemetry is compiled out, where the guard's operator bool() is a literal false, and also when telemetry is on but this span is not being recorded. The const-reference read of context.params moves inside the guard with the code that needs it, so a telemetry read still never inserts a null into the request. --- src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 31 +++++++++++++------ .../rpc/handlers/orderbook/RipplePathFind.cpp | 31 +++++++++++++------ 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index 6da4f2cdca..37a451142b 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -25,16 +25,27 @@ doPathFind(rpc::JsonContext& context) // thread) nest under it. doPathFind does not yield, so scoping is safe. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Addresses are hashed before emission for privacy. Read through a const - // reference: the non-const json::Value::operator[] inserts a null for a - // missing key, which would make PathRequest::parseJson's isMember() checks - // see an absent field as present and return Malformed instead of Missing. - // Reading for telemetry must not alter what the request looks like. - auto const& params = std::as_const(context.params); - if (auto const& src = params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + // Guarded on the span being live because setAttribute's arguments are + // evaluated whatever the build, and neither is free: asString() copies the + // address out of the JSON and redactAccount() takes a SHA-512Half over it. + // That is two copies and two hashes on every path_find call. The + // compiled-out guard's operator bool() is a literal false, so the block + // disappears entirely in that build; with telemetry compiled in it is + // skipped whenever this span is not being recorded. + if (span) + { + // Addresses are hashed before emission for privacy. Read through a + // const reference: the non-const json::Value::operator[] inserts a null + // for a missing key, which would make PathRequest::parseJson's + // isMember() checks see an absent field as present and return Malformed + // instead of Missing. Reading for telemetry must not alter what the + // request looks like. + auto const& params = std::as_const(context.params); + if (auto const& src = params[jss::source_account]; src.isString()) + span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); + if (auto const& dst = params[jss::destination_account]; dst.isString()) + span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + } if (context.app.config().pathSearchMax == 0) return rpcError(RpcNotSupported); diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index 1b5e881d2b..2444e090a3 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -34,16 +34,27 @@ doRipplePathFind(rpc::JsonContext& context) // span's log lines stay trace-correlated. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Addresses are hashed before emission for privacy. Read through a const - // reference: the non-const json::Value::operator[] inserts a null for a - // missing key, which would make PathRequest::parseJson's isMember() checks - // see an absent field as present and return Malformed instead of Missing. - // Reading for telemetry must not alter what the request looks like. - auto const& params = std::as_const(context.params); - if (auto const& src = params[jss::source_account]; src.isString()) - span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = params[jss::destination_account]; dst.isString()) - span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + // Guarded on the span being live because setAttribute's arguments are + // evaluated whatever the build, and neither is free: asString() copies the + // address out of the JSON and redactAccount() takes a SHA-512Half over it. + // That is two copies and two hashes on every ripple_path_find call. The + // compiled-out guard's operator bool() is a literal false, so the block + // disappears entirely in that build; with telemetry compiled in it is + // skipped whenever this span is not being recorded. + if (span) + { + // Addresses are hashed before emission for privacy. Read through a + // const reference: the non-const json::Value::operator[] inserts a null + // for a missing key, which would make PathRequest::parseJson's + // isMember() checks see an absent field as present and return Malformed + // instead of Missing. Reading for telemetry must not alter what the + // request looks like. + auto const& params = std::as_const(context.params); + if (auto const& src = params[jss::source_account]; src.isString()) + span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); + if (auto const& dst = params[jss::destination_account]; dst.isString()) + span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); + } if (context.app.config().pathSearchMax == 0) return rpcError(RpcNotSupported); From ef22e20a1ed66e433ad9f2b2f90c27c5c23e67f5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:48:59 +0100 Subject: [PATCH 6/8] perf(rpc): build pathfind update attributes only when recorded Two pieces of pathfinding telemetry ran regardless of the build. doUpdate fills pathfind_dest_currency by rendering the destination asset for the pathfind.compute span. For a non-XRP issue that is a base58 check encode of the issuer, two SHA-256 rounds, then a SHA-512Half over the result and three string allocations. It is a call argument, so it ran even where setAttribute's body is empty. doUpdate is not a cold path: besides once per pathfinding RPC, PathRequestManager::updateAll calls it once per active path_find subscription on every ledger close, so a node with N subscriptions paid N times a close. It now sits inside "if (span)" with the cheap pathfind_fast flag, so it is skipped with telemetry compiled out and also for any span that is not being recorded. findPaths keeps a totalPaths counter across its per-source-asset loop. Its only reader is the pathfind_num_paths attribute at the end of the same function, so the counter is maintained only when telemetry is compiled in. That needs an #ifdef rather than "if (span)": the attribute cannot read a variable that does not exist, and a counter kept up to date but never read is an unused variable, which fails the build. --- src/xrpld/rpc/detail/PathRequest.cpp | 54 ++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 2ac8ed9515..8ebbd29fe2 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -602,7 +602,14 @@ PathRequest::findPaths( span.setAttribute( pathfind_span::attr::numSourceAssets, static_cast(sourceAssets.size())); +#ifdef XRPL_ENABLE_TELEMETRY + // Only the numPaths attribute at the end of this function reads this, so it + // is not maintained at all when telemetry is compiled out. An #ifdef rather + // than `if (span)`, because the attribute cannot read a variable that does + // not exist, and a counter kept up to date but never read is an unused + // variable, which fails the build. std::int64_t totalPaths = 0; +#endif for (auto const& asset : sourceAssets) { if (continueCallback && !continueCallback()) @@ -622,7 +629,9 @@ PathRequest::findPaths( auto ps = pathfinder->getBestPaths( kMaxPaths, fullLiquidityPath, context_[asset], asset.getIssuer(), continueCallback); context_[asset] = ps; +#ifdef XRPL_ENABLE_TELEMETRY totalPaths += static_cast(ps.size()); +#endif auto const& sourceAccount = [&] { if (!isXRP(asset.getIssuer())) @@ -725,7 +734,9 @@ PathRequest::findPaths( } } +#ifdef XRPL_ENABLE_TELEMETRY span.setAttribute(pathfind_span::attr::numPaths, totalPaths); +#endif /* The resource fee is based on the number of source currencies used. The minimum cost is 50 and the maximum is 400. The cost increases @@ -748,21 +759,34 @@ PathRequest::doUpdate( // nests under it. doUpdate does not yield, so scoping is safe. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::compute); - span.setAttribute(pathfind_span::attr::fast, fast); - // to_string(Issue) renders a non-XRP asset as "/" with the - // issuer as a plaintext Base58 address, so it cannot be emitted as-is: every - // account reaching a span is hashed first. Redact just the issuer and keep - // the currency, which is what this attribute is for. An MPT asset renders as - // its issuance ID and carries no address, so it needs no redaction. - span.setAttribute( - pathfind_span::attr::destCurrency, - saDstAmount_.asset().visit( - [](Issue const& issue) { - return isXRP(issue.account) - ? to_string(issue.currency) - : redactAccount(toBase58(issue.account)) + "/" + to_string(issue.currency); - }, - [](MPTIssue const& mpt) { return to_string(mpt.getMptID()); })); + // Guarded on the span being live because setAttribute's arguments are + // evaluated whatever the build, and doUpdate is hot: PathRequestManager + // calls it once per active path_find subscription on every ledger close, so + // a node with N subscriptions pays this N times a close. The destCurrency + // value costs a base58check encode of the issuer (two SHA-256 rounds), a + // SHA-512Half over the result and three string allocations. The compiled-out + // guard's operator bool() is a literal false, so the block disappears + // entirely in that build; with telemetry compiled in it is skipped whenever + // this span is not being recorded. + if (span) + { + span.setAttribute(pathfind_span::attr::fast, fast); + // to_string(Issue) renders a non-XRP asset as "/" with + // the issuer as a plaintext Base58 address, so it cannot be emitted + // as-is: every account reaching a span is hashed first. Redact just the + // issuer and keep the currency, which is what this attribute is for. An + // MPT asset renders as its issuance ID and carries no address, so it + // needs no redaction. + span.setAttribute( + pathfind_span::attr::destCurrency, + saDstAmount_.asset().visit( + [](Issue const& issue) { + return isXRP(issue.account) + ? to_string(issue.currency) + : redactAccount(toBase58(issue.account)) + "/" + to_string(issue.currency); + }, + [](MPTIssue const& mpt) { return to_string(mpt.getMptID()); })); + } JLOG(journal_.debug()) << iIdentifier_ << " update " << (fast ? "fast" : "normal"); From e8bbc362653813d0032517fd6bf049b8009ca50d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:49:09 +0100 Subject: [PATCH 7/8] perf(rpc): compile out the pathfind update_all span with telemetry off updateAll's update_all span is wholly telemetry: the optional guard, the empty-requests test that decides whether to emit at all, and the two attributes have no reader outside the span. It runs on every ledger close, so with telemetry compiled out the function still constructed a stub guard and discarded pathfind_ledger_index and pathfind_num_requests once a close for nothing. Everything the rest of updateAll depends on, including the isNewPathRequest() flag reset, is outside the block and unchanged. No span object exists to test before it is created, so the guard is an #ifdef over the whole block. The three includes it was the sole user of -- PathFindSpanNames.h, SpanGuard.h and -- are gated the same way, because otherwise they would be unused includes in that build and clang-tidy's misc-include-cleaner would reject them. --- src/xrpld/rpc/detail/PathRequestManager.cpp | 29 +++++++++++++++------ 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequestManager.cpp b/src/xrpld/rpc/detail/PathRequestManager.cpp index 794b03124e..07345e325a 100644 --- a/src/xrpld/rpc/detail/PathRequestManager.cpp +++ b/src/xrpld/rpc/detail/PathRequestManager.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include @@ -16,17 +15,26 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include +// Needed only by the update_all span in updateAll(), which is compiled out when +// telemetry is off. Without the same guard here they would be unused includes in +// that build, which clang-tidy's misc-include-cleaner rejects. +#ifdef XRPL_ENABLE_TELEMETRY +#include + +#include + +#include +#endif // XRPL_ENABLE_TELEMETRY + namespace xrpl { /** @@ -75,12 +83,16 @@ PathRequestManager::updateAll(std::shared_ptr const& inLedger) cache = getAssetCache(inLedger, true); } +#ifdef XRPL_ENABLE_TELEMETRY using namespace telemetry; - // updateAll runs on every ledger close. Skip span emission when there are - // no active path subscriptions, to avoid a steady stream of empty spans at - // mainnet close cadence. All other work still runs unchanged (notably the - // isNewPathRequest() flag reset below), so behaviour matches the pre-span - // code path. + // Nothing outside telemetry reads this block, and updateAll runs on every + // ledger close, so it is compiled out entirely when telemetry is off rather + // than left to construct a stub guard and discard two attributes per close. + // No span object exists to test here, so the guard has to be an #ifdef. + // + // Skip span emission when there are no active path subscriptions, to avoid + // a steady stream of empty spans at mainnet close cadence. All other work + // still runs unchanged (notably the isNewPathRequest() flag reset below). // // Scoped, so the pathfind.compute spans that doUpdate() creates below on // this thread nest under it. std::optional because ScopedSpanGuard is @@ -93,6 +105,7 @@ PathRequestManager::updateAll(std::shared_ptr const& inLedger) span->setAttribute(pathfind_span::attr::ledgerIndex, static_cast(inLedger->seq())); span->setAttribute(pathfind_span::attr::numRequests, static_cast(requests.size())); } +#endif // XRPL_ENABLE_TELEMETRY bool newRequests = app_.getLedgerMaster().isNewPathRequest(); bool mustBreak = false; From a2a5ba778ea961f14717a6ffb4e38fe7c7f5692f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:53:50 +0100 Subject: [PATCH 8/8] docs(telemetry): correct what the span-liveness guard actually skips The comments claimed the guard skips work for a span that is "not being recorded", which reads as sampling awareness. It has none: operator bool() is impl_ != nullptr, and the span factories return an empty guard only when telemetry is absent, disabled at runtime, or the trace category is off. A span that exists but was sampled out still pays. There is no isRecording() in the telemetry API, so the guard is still the strongest available; only the justification was overstated. --- src/xrpld/rpc/detail/PathRequest.cpp | 4 ++-- src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 2 +- src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 8ebbd29fe2..9d9c1e290c 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -766,8 +766,8 @@ PathRequest::doUpdate( // value costs a base58check encode of the issuer (two SHA-256 rounds), a // SHA-512Half over the result and three string allocations. The compiled-out // guard's operator bool() is a literal false, so the block disappears - // entirely in that build; with telemetry compiled in it is skipped whenever - // this span is not being recorded. + // entirely in that build; with telemetry compiled in it is skipped when + // telemetry is disabled at runtime or the pathfind category is off. if (span) { span.setAttribute(pathfind_span::attr::fast, fast); diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index 37a451142b..44830619e4 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -31,7 +31,7 @@ doPathFind(rpc::JsonContext& context) // That is two copies and two hashes on every path_find call. The // compiled-out guard's operator bool() is a literal false, so the block // disappears entirely in that build; with telemetry compiled in it is - // skipped whenever this span is not being recorded. + // skipped when telemetry is disabled at runtime or the category is off. if (span) { // Addresses are hashed before emission for privacy. Read through a diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index 2444e090a3..49b90a6e4c 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -40,7 +40,7 @@ doRipplePathFind(rpc::JsonContext& context) // That is two copies and two hashes on every ripple_path_find call. The // compiled-out guard's operator bool() is a literal false, so the block // disappears entirely in that build; with telemetry compiled in it is - // skipped whenever this span is not being recorded. + // skipped when telemetry is disabled at runtime or the category is off. if (span) { // Addresses are hashed before emission for privacy. Read through a