diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index b37aa81c6d..6fb60bbac2 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -178,8 +178,14 @@ #include #include #include -#include #include +#include + +#ifdef XRPL_ENABLE_TELEMETRY +// 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 namespace xrpl::telemetry { @@ -922,7 +928,17 @@ 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 it costs nothing once + * inlined. + */ + ~ScopedActivation() // NOLINT(modernize-use-equals-default) + { + } ScopedActivation(ScopedActivation&&) = delete; ScopedActivation& operator=(ScopedActivation&&) = delete; @@ -935,7 +951,15 @@ 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. The empty body costs + * nothing once inlined. + */ + ~SpanGuard() // NOLINT(modernize-use-equals-default) + { + } SpanGuard(SpanGuard&&) noexcept = default; SpanGuard& operator=(SpanGuard&&) noexcept = default; @@ -1081,7 +1105,15 @@ 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. The empty body costs + * nothing once inlined. + */ + ~ScopedSpanGuard() // NOLINT(modernize-use-equals-default) + { + } ScopedSpanGuard(ScopedSpanGuard&&) = delete; ScopedSpanGuard& @@ -1197,4 +1229,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 replaced with `= 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 diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index d064a9c892..960361595e 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -95,7 +95,6 @@ #include #include #include -#include #ifdef XRPL_ENABLE_TELEMETRY #include @@ -103,6 +102,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 diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 2ac8ed9515..9d9c1e290c 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 when + // telemetry is disabled at runtime or the pathfind category is off. + 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"); 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; diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index df8c795c3f..e3400b746f 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -223,6 +223,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 @@ -256,6 +264,8 @@ resolveCommandSpanName(JsonContext const& context) : std::string_view{rpc_span::val::unknownCommand}; } +#endif // XRPL_ENABLE_TELEMETRY + } // namespace Status @@ -264,6 +274,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. @@ -279,6 +294,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; 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_)) diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index 6da4f2cdca..44830619e4 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 when telemetry is disabled at runtime or the category is off. + 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..49b90a6e4c 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 when telemetry is disabled at runtime or the category is off. + 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);