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 01/44] 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 02/44] 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 a704b404d0390c1bd0c9d4f4cbff1fd6aa920ffd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:15 +0100 Subject: [PATCH 03/44] fix(telemetry): guard TxTracing's includes to match their uses isValidSpanId and std::uint8_t are named only inside the telemetry-enabled branch of txReceiveSpan, so with telemetry compiled out both includes are unused and clang-tidy fails the build on warnings-as-errors. --- src/xrpld/telemetry/TxTracing.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/xrpld/telemetry/TxTracing.h b/src/xrpld/telemetry/TxTracing.h index 02900fa9b4..5baf01df2d 100644 --- a/src/xrpld/telemetry/TxTracing.h +++ b/src/xrpld/telemetry/TxTracing.h @@ -16,9 +16,14 @@ #include #include #include + +#ifdef XRPL_ENABLE_TELEMETRY +// The span-id validator and std::uint8_t are named only by the +// telemetry-enabled branches below. #include #include +#endif namespace xrpl::telemetry { From 080b328b7b1f26b8044eb1489049e5b6d946b75b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:18 +0100 Subject: [PATCH 04/44] fix(telemetry): guard ConsensusReceiveTracing's includes to match their uses isValidTraceContext and std::uint8_t are named only inside the two telemetry-enabled branches, so with telemetry compiled out both includes are unused and clang-tidy fails the build on warnings-as-errors. --- src/xrpld/telemetry/ConsensusReceiveTracing.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/xrpld/telemetry/ConsensusReceiveTracing.h b/src/xrpld/telemetry/ConsensusReceiveTracing.h index 0a1a4458dc..498eaa6fca 100644 --- a/src/xrpld/telemetry/ConsensusReceiveTracing.h +++ b/src/xrpld/telemetry/ConsensusReceiveTracing.h @@ -42,9 +42,14 @@ #include #include #include + +#ifdef XRPL_ENABLE_TELEMETRY +// The trace-context validator and std::uint8_t are named only by the +// telemetry-enabled branches below. #include #include +#endif namespace xrpl::telemetry { From 09908f7a5f16c7cf67530b8429997a207e4247cd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:20 +0100 Subject: [PATCH 05/44] fix(telemetry): guard in Log.cpp to match its use std::size_t names the trace-id and span-id hex widths, which are compiled only when telemetry is enabled, so the include is unused otherwise and clang-tidy fails the build on warnings-as-errors. --- src/libxrpl/basics/Log.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libxrpl/basics/Log.cpp b/src/libxrpl/basics/Log.cpp index 750cdc0e8a..e49301cfdd 100644 --- a/src/libxrpl/basics/Log.cpp +++ b/src/libxrpl/basics/Log.cpp @@ -16,7 +16,6 @@ #endif // XRPL_ENABLE_TELEMETRY #include -#include #include #include #include @@ -29,6 +28,11 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// std::size_t names the hex widths used when formatting a trace context. +#include +#endif // XRPL_ENABLE_TELEMETRY + namespace xrpl { Logs::Sink::Sink(std::string partition, beast::Severity thresh, Logs& logs) From 080ab5afce719ed335386a3bd903c7b32ab8c866 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:37 +0100 Subject: [PATCH 06/44] fix(test): compile ValidationTracker into the test binary in every build ValidationTracker carries no telemetry guards and neither does its test, so both are compiled whatever the telemetry setting. Its implementation and the src/ include path it needs were added only inside if(telemetry), so a build with telemetry off could not reach the header and left fifteen of its symbols unresolved at link time on every platform. Hoist both out of the guard. The else() branch already does this for MetricsRegistry, so the case was anticipated there and missed here. --- src/tests/libxrpl/CMakeLists.txt | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/tests/libxrpl/CMakeLists.txt b/src/tests/libxrpl/CMakeLists.txt index e7638ef694..52b0529064 100644 --- a/src/tests/libxrpl/CMakeLists.txt +++ b/src/tests/libxrpl/CMakeLists.txt @@ -110,15 +110,20 @@ if(telemetry) "${OTEL_IN_MEMORY_EXPORTER_LIB}" opentelemetry-cpp::opentelemetry-cpp ) - # ValidationTracker lives in src/xrpld/ (not libxrpl), so we compile its - # implementation directly into the test binary and put src/ on the include - # path so its tests can reach headers. - target_include_directories(xrpl_tests PRIVATE ${CMAKE_SOURCE_DIR}/src) - target_sources( - xrpl_tests - PRIVATE - ${CMAKE_SOURCE_DIR}/src/xrpld/telemetry/detail/ValidationTracker.cpp - ) endif() +# ValidationTracker lives in src/xrpld/ (not libxrpl), so we compile its +# implementation directly into the test binary and put src/ on the include path +# so its tests can reach headers. +# +# Both are unconditional: the class carries no telemetry guards, so its tests +# compile and run in every build. Gating them would leave the test file (which +# is likewise unguarded) without the header it includes and without the +# definitions it calls. +target_include_directories(xrpl_tests PRIVATE ${CMAKE_SOURCE_DIR}/src) +target_sources( + xrpl_tests + PRIVATE ${CMAKE_SOURCE_DIR}/src/xrpld/telemetry/detail/ValidationTracker.cpp +) + gtest_discover_tests(xrpl_tests DISCOVERY_TIMEOUT 60) From 7253aa787ba392c643ea9903d27d73545c36caf2 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:07:39 +0100 Subject: [PATCH 07/44] fix(telemetry): make this branch's files compile clean with telemetry off The metric macros discard their arguments when telemetry is compiled out, so a file's used-symbol set differs from the one its includes and declarations were written for. That produced eleven clang-tidy errors here, all from that cause. - guard , and in MetricsRegistry.h, and and in DatabaseConfig_test.cpp, to match their uses - include Journal.h directly in MetricsRegistry.cpp, whose constructor takes a beast::Journal by value in both configurations - compile out recordGetObjectMetrics, its declaration, its call site and the two clock reads that feed it, since every statement in it records a metric - drop two duplicate includes and rename a function-local static constant to kEmpty --- src/test/nodestore/DatabaseConfig_test.cpp | 9 +++++-- .../libxrpl/telemetry/MetricsRegistry.cpp | 6 ++--- src/xrpld/overlay/detail/PeerImp.cpp | 27 +++++++++++++++---- src/xrpld/overlay/detail/PeerImp.h | 7 +++-- src/xrpld/telemetry/MetricsRegistry.cpp | 5 +++- src/xrpld/telemetry/MetricsRegistry.h | 10 ++++--- 6 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/test/nodestore/DatabaseConfig_test.cpp b/src/test/nodestore/DatabaseConfig_test.cpp index 0f132081f0..04669bc6be 100644 --- a/src/test/nodestore/DatabaseConfig_test.cpp +++ b/src/test/nodestore/DatabaseConfig_test.cpp @@ -33,18 +33,23 @@ #include #include -#include #include #include #include #include #include -#include #include #include #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// std::ranges::sort / std::ranges::find_if and std::optional / std::nullopt are +// used only by the gauge-helper suite below, so they are guarded like its uses. +#include +#include +#endif + namespace xrpl::node_store { class DatabaseConfig_test : public beast::unit_test::Suite diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index d44a83f147..5d7c8543fb 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -428,10 +428,8 @@ TEST(MetricsRegistryScaledMean, default_scale_is_one) #include -#include #include #include -#include using namespace xrpl; @@ -699,8 +697,8 @@ public: [[nodiscard]] std::optional const& getTrapTxID() const override { - static std::optional const empty; - return empty; + static std::optional const kEmpty; + return kEmpty; } DatabaseCon& getWalletDB() override diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index baa4e13f87..c617bbd613 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -76,7 +76,6 @@ #include #include #include -#include #include #include #include @@ -117,6 +116,13 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The TMGetObjectByHash metric names and label keys. Every use of them sits in +// an XRPL_METRIC_* argument list, and those macros expand to nothing when +// telemetry is off, so the include is guarded like its uses. +#include +#endif + using namespace std::chrono_literals; namespace xrpl { @@ -2857,10 +2863,13 @@ PeerImp::processGetObjectByHash(std::shared_ptr con int const requested = packet.objects_size(); int const iterLimit = std::min(requested, tuning::kHardMaxReplyNodes); +#ifdef XRPL_ENABLE_TELEMETRY // Time the whole loop once, not each iteration: the loop can run up to // kHardMaxReplyNodes times, so per-iteration clock reads would cost more - // than the lookups they measure. + // than the lookups they measure. Both clock reads serve only the metric + // recorded below, so neither happens when telemetry is compiled out. auto const lookupStart = std::chrono::steady_clock::now(); +#endif for (int i = 0; i < iterLimit; ++i) { @@ -2886,8 +2895,12 @@ PeerImp::processGetObjectByHash(std::shared_ptr con newObj.set_ledgerseq(obj.ledgerseq()); } +#ifdef XRPL_ENABLE_TELEMETRY + // Measured here rather than at the call below, which would fold the fee + // computation and charge() into the reported lookup latency. auto const lookupElapsed = std::chrono::duration_cast( std::chrono::steady_clock::now() - lookupStart); +#endif // Apply work-proportional charge. `charge()` posts the disconnect // step (if any) back to strand_, so it is safe to call from this @@ -2902,12 +2915,15 @@ PeerImp::processGetObjectByHash(std::shared_ptr con resource::Charge const fee = computeGetObjectByHashFee(requested, reply.objects_size()); charge(fee, "processed get object by hash request"); +#ifdef XRPL_ENABLE_TELEMETRY recordGetObjectMetrics(requested, reply.objects_size(), lookupElapsed, fee); +#endif JLOG(pJournal_.trace()) << "GetObj: " << reply.objects_size() << " of " << requested; send(std::make_shared(reply, protocol::mtGET_OBJECTS)); } +#ifdef XRPL_ENABLE_TELEMETRY void PeerImp::recordGetObjectMetrics( int const requested, @@ -2936,9 +2952,9 @@ PeerImp::recordGetObjectMetrics( // negative -- the counter takes an unsigned amount, where a wrap would // read as ~1.8e19 rather than as an error. // - // Written as two calls rather than a loop over a {hit, miss} pair: the - // macros expand to empty statements in a telemetry-off build, which would - // leave a loop's induction variable unused and fail the -Werror build. + // Written as two calls rather than a loop over a {hit, miss} pair: the two + // amounts come from different expressions, so there is no single value to + // iterate over. XRPL_METRIC_COUNTER_ADD_LABELED( app_, kGetObjectLookupsTotal, @@ -2953,6 +2969,7 @@ PeerImp::recordGetObjectMetrics( static_cast(std::max(0, requested - found)), {{kLabelResult, std::string(kResultMiss)}}); } +#endif // XRPL_ENABLE_TELEMETRY void PeerImp::onMessage(std::shared_ptr const& m) diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index 25c3ca7618..fc4fe690ac 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -701,6 +701,7 @@ private: std::shared_ptr const& m, std::vector nodeIDs); +#ifdef XRPL_ENABLE_TELEMETRY /** * Record the OTel metrics for one completed `TMGetObjectByHash` request. * @@ -711,8 +712,9 @@ private: * * Records `getobject_request_objects`, `getobject_lookup_us`, * `getobject_charge`, and both label values of - * `getobject_lookups_total`. Compiles to nothing when telemetry is - * disabled, because the `XRPL_METRIC_*` macros do. + * `getobject_lookups_total`. Every statement is an `XRPL_METRIC_*` record, + * so the whole method — and its one call site — is compiled out when + * telemetry is disabled rather than left as an empty function. * * @param requested Objects the peer asked for (`objects_size()`). * @param found Objects returned, i.e. the reply's object count. @@ -728,6 +730,7 @@ private: int const found, std::chrono::microseconds const lookupElapsed, resource::Charge const& fee); +#endif // XRPL_ENABLE_TELEMETRY protected: // Kept `protected` so test subclasses (see diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index 04c2670fab..a64f5e219e 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -22,6 +22,10 @@ #include +// Unguarded because the constructor's `beast::Journal journal` parameter is +// declared in both configurations; only the member it initialises is guarded. +#include + #ifdef XRPL_ENABLE_TELEMETRY // The app and overlay includes below are why @@ -53,7 +57,6 @@ #include #include #include -#include #include #include #include diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 7acc6f63ee..3ca31af991 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -143,11 +143,8 @@ #include #include -#include #include -#include #include -#include #include #include #include @@ -158,6 +155,13 @@ #include #include #include + +// These three serve only the telemetry-only members below, so they are guarded +// like their uses: std::atomic by callbacksDetached_, std::function by the +// ObserveFn sink, std::shared_ptr by provider_. +#include +#include +#include #endif namespace xrpl { From b2bc4024b4a65aef55ec07b0bfd20eaa1db25bb0 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:18:17 +0100 Subject: [PATCH 08/44] fix(telemetry): bound the validation tracker's pending map on insert ValidationTracker::pending_ was pruned only by evictOldPending(), which is reachable only from reconcile(), which runs only from the observable-gauge callbacks. Those callbacks need telemetry compiled in and enabled, so any node without both recorded one entry per validated ledger and freed none -- roughly 2.4 MB a day, plus a mutex acquisition per ledger. kMaxPendingEvents did not help: that check lives inside the function that never ran. This also affected ordinary builds, not just telemetry-off ones, because startAsyncGauges() returns early when [telemetry] enabled=0 and so never registers the callbacks. Bound the map where the bound can actually be guaranteed -- the insert path -- by dropping the oldest entry once it is full. A container has to be bounded by its writer, not by whoever happens to read it. Also stop doing the work when nothing will consume it: hold the tracker, its accessor and its header behind the telemetry guard, and check isEnabled() at both record sites, which the metric macros do but these direct calls did not. Adds pendingCount() so the bound is observable, and a test that records 8000 ledgers without ever reconciling and asserts the size settles at a fixed cap. --- .../libxrpl/telemetry/ValidationTracker.cpp | 36 +++++++++++++++++++ src/xrpld/app/consensus/RCLConsensus.cpp | 9 ++++- src/xrpld/app/ledger/detail/LedgerMaster.cpp | 8 ++++- src/xrpld/telemetry/MetricsRegistry.h | 20 ++++++++--- src/xrpld/telemetry/ValidationTracker.h | 31 ++++++++++++++++ .../telemetry/detail/ValidationTracker.cpp | 28 +++++++++++++++ 6 files changed, 125 insertions(+), 7 deletions(-) diff --git a/src/tests/libxrpl/telemetry/ValidationTracker.cpp b/src/tests/libxrpl/telemetry/ValidationTracker.cpp index 2542316e7a..9d176377a0 100644 --- a/src/tests/libxrpl/telemetry/ValidationTracker.cpp +++ b/src/tests/libxrpl/telemetry/ValidationTracker.cpp @@ -377,3 +377,39 @@ TEST_F(ValidationTrackerTest, GrossAgreementsCountInitialOnly) // Additive invariant: gross agree + gross miss == ledgers reconciled. EXPECT_EQ(tracker_.totalAgreementsEver() + tracker_.totalMissedEver(), 5u); } + +// --------------------------------------------------------------- +// 12. Pending map stays bounded when nothing ever reconciles +// reconcile() is the only pruning path, and it runs only from +// the observable-gauge callbacks -- which need telemetry both +// compiled in and enabled. A node with telemetry off, or with +// [telemetry] enabled=0, therefore never reconciles, so the +// record methods have to bound the map themselves. +// --------------------------------------------------------------- +TEST_F(ValidationTrackerTest, PendingStaysBoundedWithoutReconcile) +{ + constexpr std::uint64_t kFirstBatch = 4000; + constexpr std::uint64_t kSecondBatch = 8000; + + for (std::uint64_t i = 0; i < kFirstBatch; ++i) + tracker_.recordOurValidation(makeHash(i), static_cast(i)); + + auto const afterFirst = tracker_.pendingCount(); + + for (std::uint64_t i = kFirstBatch; i < kSecondBatch; ++i) + tracker_.recordOurValidation(makeHash(i), static_cast(i)); + + auto const afterSecond = tracker_.pendingCount(); + + // Bounded at all: far fewer entries retained than recorded. + EXPECT_LT(afterFirst, kFirstBatch); + + // Bounded at a fixed cap, not merely growing more slowly: doubling the + // input leaves the size unchanged. Asserted without naming the private + // constant, so the test survives a change to its value. + EXPECT_EQ(afterFirst, afterSecond); + + // Every recorded validation is still counted, so bounding the map does not + // cost us the lifetime totals the gauges report. + EXPECT_EQ(tracker_.totalValidationsSent(), kSecondBatch); +} diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 9948781a7a..a5ed99dc0e 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -1072,9 +1072,16 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, if (auto* mr = app_.getMetricsRegistry()) { mr->incrementValidationsSent(); +#ifdef XRPL_ENABLE_TELEMETRY // Record our validation for the agreement tracker so it can // compare against network-validated ledgers. - mr->getValidationTracker().recordOurValidation(ledger.id(), ledger.seq()); + // + // Only when enabled: recording takes the tracker's lock and inserts an + // entry, and nothing reconciles or drains those entries unless the + // observable gauges are running. + if (mr->isEnabled()) + mr->getValidationTracker().recordOurValidation(ledger.id(), ledger.seq()); +#endif } } diff --git a/src/xrpld/app/ledger/detail/LedgerMaster.cpp b/src/xrpld/app/ledger/detail/LedgerMaster.cpp index 63c00ef8e0..cf8d65230b 100644 --- a/src/xrpld/app/ledger/detail/LedgerMaster.cpp +++ b/src/xrpld/app/ledger/detail/LedgerMaster.cpp @@ -296,10 +296,16 @@ LedgerMaster::setValidLedger(std::shared_ptr const& l) (void)maxLedgerDifference_; validLedgerSeq_ = l->header().seq; +#ifdef XRPL_ENABLE_TELEMETRY // Record the network-validated ledger for the agreement tracker so it // can compare against our own validations. - if (auto* mr = app_.getMetricsRegistry()) + // + // Only when enabled: recording takes the tracker's lock and inserts an + // entry, and nothing reconciles or drains those entries unless the + // observable gauges are running. + if (auto* mr = app_.getMetricsRegistry(); mr && mr->isEnabled()) mr->getValidationTracker().recordNetworkValidation(l->header().hash, l->header().seq); +#endif app_.getOPs().updateLocalTx(*l); app_.getSHAMapStore().onLedgerClosed(getValidatedLedger()); diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 3ca31af991..0837bd13ce 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -138,7 +138,11 @@ * instrumentation site. */ +#ifdef XRPL_ENABLE_TELEMETRY +// The tracker is held and exposed only in this configuration, where the gauge +// callbacks that drain it exist. #include +#endif #include @@ -655,10 +659,15 @@ public: void incrementTxqDropped(std::string_view reason); +#ifdef XRPL_ENABLE_TELEMETRY /** * Access the validation agreement tracker. * Used by consensus and ledger hooks to record our validations and * network validations so the tracker can compute agreement percentages. + * + * Guarded, along with the tracker itself, because only the observable-gauge + * callbacks read it and those exist only in this configuration. Recording + * into it is not free: each call takes its lock and inserts an entry. * @return Reference to the internal ValidationTracker instance. */ ValidationTracker& @@ -667,7 +676,6 @@ public: return validationTracker_; } -#ifdef XRPL_ENABLE_TELEMETRY /** * Access the shared OTel Meter for call-site instrument creation. * Used by the XRPL_METRIC_* macros (MetricMacros.h) so new synchronous @@ -742,15 +750,17 @@ private: */ bool const enabled_; +#ifdef XRPL_ENABLE_TELEMETRY /** * Tracks validation agreement between this node and the network. - * Lives outside the XRPL_ENABLE_TELEMETRY guard because it is - * always safe to record events; the gauge callback simply won't - * fire when telemetry is disabled. + * + * Guarded because reconcile() -- which resolves and then prunes recorded + * events -- runs only from the observable-gauge callbacks. Recording + * without it accumulates one entry per validated ledger, so the tracker + * exists only where something drains it. */ ValidationTracker validationTracker_; -#ifdef XRPL_ENABLE_TELEMETRY /** * Reference to Application services for gauge callbacks. * Only needed when OTel is compiled in, since observable gauge diff --git a/src/xrpld/telemetry/ValidationTracker.h b/src/xrpld/telemetry/ValidationTracker.h index ac80f5cdf5..61f711602f 100644 --- a/src/xrpld/telemetry/ValidationTracker.h +++ b/src/xrpld/telemetry/ValidationTracker.h @@ -253,6 +253,17 @@ public: uint64_t totalValidationsChecked() const; + /** + * Number of ledgers currently held awaiting reconciliation. + * + * Never exceeds kMaxPendingEvents: the record methods enforce that bound + * as they insert, so the map stays bounded whether or not anything ever + * reconciles or reads it. + * @return Size of the pending map. + */ + [[nodiscard]] std::size_t + pendingCount() const; + /** @} */ private: @@ -397,6 +408,26 @@ private: void evictOldPending(TimePoint now); + /** + * Hold pending_ at kMaxPendingEvents by dropping its oldest entry. + * + * Called on the insert path, because that is the only place the bound can + * be guaranteed. reconcile() also prunes, but it runs only while the gauge + * callbacks are registered, which needs telemetry both compiled in and + * enabled -- so a node with telemetry off, or with [telemetry] enabled=0, + * would otherwise grow this map by one entry per validated ledger forever. + * + * Drops the oldest entry rather than the least useful one: the map is + * unordered, so this is a linear scan, but it runs at most once per + * recorded validation and only once the map is already full. + * + * @param justRecorded Hash inserted by the caller, kept even if the scan + * finds it oldest (equal timestamps make that possible). + * @note Caller must hold mutex_. + */ + void + boundPending(uint256 const& justRecorded); + /** * Scan a window deque and flip the first non-agreed entry matching * the given ledger hash to agreed. diff --git a/src/xrpld/telemetry/detail/ValidationTracker.cpp b/src/xrpld/telemetry/detail/ValidationTracker.cpp index c7f9c599bc..89e74c8f56 100644 --- a/src/xrpld/telemetry/detail/ValidationTracker.cpp +++ b/src/xrpld/telemetry/detail/ValidationTracker.cpp @@ -31,6 +31,7 @@ ValidationTracker::recordOurValidation(uint256 const& ledgerHash, LedgerIndex se } evt.weValidated = true; totalValidationsSent_.fetch_add(1, std::memory_order_relaxed); + boundPending(ledgerHash); } void @@ -46,6 +47,33 @@ ValidationTracker::recordNetworkValidation(uint256 const& ledgerHash, LedgerInde } evt.networkValidated = true; totalValidationsChecked_.fetch_add(1, std::memory_order_relaxed); + boundPending(ledgerHash); +} + +void +ValidationTracker::boundPending(uint256 const& justRecorded) +{ + if (pending_.size() <= kMaxPendingEvents) + return; + + auto oldest = pending_.end(); + for (auto it = pending_.begin(); it != pending_.end(); ++it) + { + if (it->first == justRecorded) + continue; + if (oldest == pending_.end() || it->second.recordTime < oldest->second.recordTime) + oldest = it; + } + + if (oldest != pending_.end()) + pending_.erase(oldest); +} + +std::size_t +ValidationTracker::pendingCount() const +{ + std::scoped_lock const lock(mutex_); + return pending_.size(); } void From 30e119a7618b7df46096ce95f305b9bea798177a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:22:08 +0100 Subject: [PATCH 09/44] perf(telemetry): only build tx span attributes when the span is recorded Both tx.receive and tx.process set their attributes unconditionally, so the work happened even when nothing consumed it: a 64-char hash string allocation, a getCurrentLedgerIndex() call that takes the ledger master's lock, a TxFormats lookup, a peer-version string copy, and in tx.process a fee and sequence decode. tx.receive runs before the duplicate check, so duplicate relays paid for it too. Guard both blocks on the span being live. When telemetry is compiled out the guard's operator bool() is a literal false and the block is eliminated; when it is compiled in the block is skipped for any span that is not being recorded, which the previous code could not do. Behaviour is unchanged where the span is live, and setAttribute on a null guard was already a no-op. The remaining attributes at the exit paths keep their unconditional calls: their arguments are compile-time constants, so there is nothing to save. --- src/xrpld/app/misc/NetworkOPs.cpp | 37 +++++++++++++++++----------- src/xrpld/overlay/detail/PeerImp.cpp | 33 ++++++++++++++++--------- 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 8f6e92b1d9..8fc85d51ac 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1511,22 +1511,31 @@ NetworkOPsImp::processTransaction( // and end on the batch worker thread that later applies this transaction — // no detach step is needed. auto span = std::make_shared(txProcessSpan(transaction->getID())); - span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); - span->setAttribute(tx_span::attr::local, bLocal); - // The current (open) ledger index at submission/relay time — the ledger - // being worked on. Correlates this tx.process to the ledger trace; the tx - // has not yet been applied to a specific ledger here, so there is no hash. - span->setAttribute( - tx_span::attr::currentLedgerSeq, - static_cast(ledgerMaster_.getCurrentLedgerIndex())); - if (auto const& stx = transaction->getSTransaction()) + // Guarded on the span being live because these values are not free and this + // runs for every submitted and relayed transaction: the hash string + // allocates, and the open-ledger index takes the ledger master's lock. 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) { - if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType())) - span->setAttribute(tx_span::attr::txType, fmt->getName().c_str()); + span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); + span->setAttribute(tx_span::attr::local, bLocal); + // The current (open) ledger index at submission/relay time — the ledger + // being worked on. Correlates this tx.process to the ledger trace; the + // tx has not yet been applied to a specific ledger, so there is no hash. span->setAttribute( - tx_span::attr::fee, static_cast(stx->getFieldAmount(sfFee).xrp().drops())); - span->setAttribute( - tx_span::attr::sequence, static_cast(stx->getSeqProxy().value())); + tx_span::attr::currentLedgerSeq, + static_cast(ledgerMaster_.getCurrentLedgerIndex())); + if (auto const& stx = transaction->getSTransaction()) + { + if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType())) + span->setAttribute(tx_span::attr::txType, fmt->getName().c_str()); + span->setAttribute( + tx_span::attr::fee, static_cast(stx->getFieldAmount(sfFee).xrp().drops())); + span->setAttribute( + tx_span::attr::sequence, static_cast(stx->getSeqProxy().value())); + } } auto ev = jobQueue_.makeLoadEvent(JtTxnProc, "ProcessTXN"); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 49f085e28c..1a28c9a949 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1321,18 +1321,27 @@ PeerImp::handleTransaction( // SpanGuard is thread-free (holds no Scope), so it is safe to hand to // a job-queue worker and end on that thread — no detach step is needed. auto span = std::make_shared(txReceiveSpan(txID, *m)); - span->setAttribute(tx_span::attr::txHash, to_string(txID).c_str()); - span->setAttribute(tx_span::attr::peerId, static_cast(id_)); - // The current (open) ledger index when the relayed tx was received — - // the ledger being worked on. Correlates this tx.receive to the ledger - // trace; not yet applied to a specific ledger here, so no hash. - span->setAttribute( - tx_span::attr::currentLedgerSeq, - static_cast(app_.getLedgerMaster().getCurrentLedgerIndex())); - if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType())) - span->setAttribute(tx_span::attr::txType, fmt->getName().c_str()); - if (auto const version = getVersion(); !version.empty()) - span->setAttribute(tx_span::attr::peerVersion, version.c_str()); + // Guarded on the span being live because these values are not free and + // this runs for every inbound transaction, including duplicates: the + // hash string allocates, and the open-ledger index takes the ledger + // master's lock. 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(tx_span::attr::txHash, to_string(txID).c_str()); + span->setAttribute(tx_span::attr::peerId, static_cast(id_)); + // The current (open) ledger index when the relayed tx was received + // — the ledger being worked on. Correlates this tx.receive to the + // ledger trace; not yet applied to a specific ledger, so no hash. + span->setAttribute( + tx_span::attr::currentLedgerSeq, + static_cast(app_.getLedgerMaster().getCurrentLedgerIndex())); + if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType())) + span->setAttribute(tx_span::attr::txType, fmt->getName().c_str()); + if (auto const version = getVersion(); !version.empty()) + span->setAttribute(tx_span::attr::peerVersion, version.c_str()); + } // Note: suppressed and txStatus are set once at each exit path // (not as defaults here) to avoid OTel SDK attribute duplication. From c44cb6e96674d3065c2756b40d6a2469b254a81f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:25:12 +0100 Subject: [PATCH 10/44] fix(telemetry): stop sending an empty trace context to peers Both the proposal and validation broadcast paths called injectCurrentContextToProtobuf(*msg.mutable_trace_context()) unconditionally. The injector is a no-op when telemetry is compiled out, but its argument is not: mutable_ on an optional submessage allocates it and sets its has-bit. So a node built without telemetry put an empty TraceContext in every proposal and every validation it broadcast, and made each receiving peer take its has_trace_context() branch for nothing. Guard both calls. The relay path in NetworkOPs already tests its span before touching mutable_trace_context(), so this brings the two in line. --- src/xrpld/app/consensus/RCLConsensus.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index ade30cdb63..e29e77147f 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -274,10 +274,18 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) app_.getHashRouter().addSuppression(suppression); +#ifdef XRPL_ENABLE_TELEMETRY // Inject the current thread's active span context (e.g. the consensus // round span) so receiving peers can link their proposal.receive span // as a child of this trace. + // + // Guarded rather than relying on the injector being a no-op: mutable_ on an + // optional submessage allocates it and sets its has-bit, so calling this + // unconditionally would put an empty TraceContext on the wire in every + // proposal a node without telemetry broadcasts, and make its peers take + // their has_trace_context() branch for nothing. telemetry::SpanGuard::injectCurrentContextToProtobuf(*prop.mutable_trace_context()); +#endif app_.getOverlay().broadcast(prop); } @@ -1055,7 +1063,13 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, // `serialized`, so it is not covered by validation authenticity. // Downstream consumers treat it as advisory only. A signature-covered // trace context is a possible future enhancement. + // + // Guarded for the same reason as the proposal path: mutable_ allocates the + // optional submessage and sets its has-bit, so an unguarded call would put + // an empty TraceContext in every validation a node without telemetry sends. +#ifdef XRPL_ENABLE_TELEMETRY telemetry::SpanGuard::injectCurrentContextToProtobuf(*val.mutable_trace_context()); +#endif app_.getOverlay().broadcast(val); // Publish to all our subscribers: From e65e36a90cd4796c63f2604a27649edbbe61e1b4 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:54:50 +0100 Subject: [PATCH 11/44] perf(nodestore): record the NuDB write path only when telemetry is compiled in Every node write folded its queue depth into an accumulator, took two steady_clock samples and updated four atomics, whatever the build. Nothing consumes any of it without telemetry: the write-stats gauges are its only reader. Guard the accounting so a telemetry-off build matches develop exactly. That includes putting getWriteLoad() back to develop's `return 0`, which matters beyond cost: instrumenting this turned a hard-coded constant into a live depth, which quietly armed LedgerMaster's kMaxWriteLoadAcquire history gate and changed what get_counts reports. Neither should follow from adding telemetry. With telemetry on, all of it behaves as before. getWriteStats() now reports absence rather than zeros in that build, which is the answer the base class already gives for backends that do not measure, and which readers distinguish from measured-and-idle. Also drops the depth parameter recordInsert never read, and gates the counters themselves since their only readers are the two guarded accessors. The four write-path tests are guarded to match, and the backend and database tests now derive whether NuDB measures from the build instead of hard-coding it, so their existing absence paths cover the compiled-out case. --- src/libxrpl/nodestore/backend/NuDBFactory.cpp | 28 +++++++++++++++++-- src/tests/libxrpl/nodestore/Backend.cpp | 10 ++++++- src/tests/libxrpl/nodestore/Database.cpp | 11 ++++++-- src/tests/libxrpl/nodestore/NuDBFactory.cpp | 6 ++++ 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/libxrpl/nodestore/backend/NuDBFactory.cpp b/src/libxrpl/nodestore/backend/NuDBFactory.cpp index 69173782b8..f9d56576a5 100644 --- a/src/libxrpl/nodestore/backend/NuDBFactory.cpp +++ b/src/libxrpl/nodestore/backend/NuDBFactory.cpp @@ -66,6 +66,7 @@ public: std::atomic deletePath; Scheduler& scheduler; +#ifdef XRPL_ENABLE_TELEMETRY /** * Writers currently inside doInsert. Instantaneous depth. */ @@ -101,6 +102,7 @@ public: * load, which is when the mean matters. */ std::atomic depthSamples{0}; +#endif // XRPL_ENABLE_TELEMETRY NuDBBackend( size_t keyBytes, @@ -279,6 +281,7 @@ public: nudb::detail::buffer bf; auto const result = nodeobjectCompress(e.getData(), e.getSize(), bf); +#ifdef XRPL_ENABLE_TELEMETRY // NuDB takes one global mutex for the whole insert, so the wait is // invisible from here. Record the depth we joined at and the wall // time we spent; the split follows from Little's Law. @@ -289,6 +292,10 @@ public: // slow, deep ones -- biasing the mean down exactly when queueing is // worst. With all writers inside their first insert the exit-counted // version reports no depth at all. + // + // Every node write reaches here, so none of it happens without + // telemetry: this whole block is what the write-stats gauges need and + // nothing else reads it. auto const depth = concurrentWriters.fetch_add(1, std::memory_order_relaxed) + 1; depthSum.fetch_add(depth, std::memory_order_relaxed); depthSamples.fetch_add(1, std::memory_order_relaxed); @@ -297,7 +304,8 @@ public: // A scope guard rather than straight-line code, because the insert // can allocate and so can throw. Leaking the depth would strand the // gauge above zero for the life of the process. - ScopeExit const account([this, depth, begin] { recordInsert(depth, begin); }); + ScopeExit const account([this, begin] { recordInsert(begin); }); +#endif db.insert(e.getKey(), result.first, result.second, ec); @@ -375,15 +383,29 @@ public: int getWriteLoad() override { +#ifdef XRPL_ENABLE_TELEMETRY // Writers in flight. Bounded by the number of writing threads, so // it stays far below LedgerMaster's kMaxWriteLoadAcquire of 8192 // and cannot suppress history acquisition. return static_cast(concurrentWriters.load(std::memory_order_relaxed)); +#else + // Nothing counts writers without telemetry, so report no load rather + // than a stale zero-valued counter. LedgerMaster gates history + // acquisition on this, so it must not start reporting a real depth as + // a side effect of instrumentation. + return 0; +#endif } [[nodiscard]] std::optional getWriteStats() const override { +#ifndef XRPL_ENABLE_TELEMETRY + // Not measured in this build, which is a different answer from + // measured-and-idle. The base class reports absence the same way for + // backends that never queue. + return std::nullopt; +#else WriteStats stats; stats.concurrentWriters = concurrentWriters.load(std::memory_order_relaxed); stats.insertCount = insertCount.load(std::memory_order_relaxed); @@ -392,6 +414,7 @@ public: stats.depthSum = depthSum.load(std::memory_order_relaxed); stats.depthSamples = depthSamples.load(std::memory_order_relaxed); return stats; +#endif } void @@ -432,11 +455,10 @@ private: * Always runs, including on the throwing path, so the depth gauge * returns to its true value even when the insert fails. * - * @param depth Writer depth this insert joined at, at least 1. * @param begin When the insert started. */ void - recordInsert(std::uint64_t depth, std::chrono::steady_clock::time_point begin) noexcept + recordInsert(std::chrono::steady_clock::time_point begin) noexcept { auto const elapsedUs = static_cast(std::chrono::duration_cast( diff --git a/src/tests/libxrpl/nodestore/Backend.cpp b/src/tests/libxrpl/nodestore/Backend.cpp index f2648f3e12..0b37815843 100644 --- a/src/tests/libxrpl/nodestore/Backend.cpp +++ b/src/tests/libxrpl/nodestore/Backend.cpp @@ -182,7 +182,15 @@ TEST_P(BackendTypeTest, write_stats_reported_only_when_measured) auto backend = makeOpenBackend(); auto const stats = backend->getWriteStats(); - if (GetParam() == "nudb") + // NuDB records the write path only when telemetry is compiled in; without + // it there is no consumer, so it reports absence like every other backend. +#ifdef XRPL_ENABLE_TELEMETRY + bool const measures = GetParam() == "nudb"; +#else + bool const measures = false; +#endif + + if (measures) { if (!stats.has_value()) FAIL() << "nudb must report write stats"; diff --git a/src/tests/libxrpl/nodestore/Database.cpp b/src/tests/libxrpl/nodestore/Database.cpp index 31b5410fd1..925f8d04a3 100644 --- a/src/tests/libxrpl/nodestore/Database.cpp +++ b/src/tests/libxrpl/nodestore/Database.cpp @@ -225,8 +225,15 @@ TEST_P(NodeStoreDatabaseTest, write_stats_forwarded_from_backend) // Before any write, only a measuring backend answers at all. auto const initial = db->getWriteStats(); - ASSERT_EQ(initial.has_value(), GetParam() == "nudb") - << "only nudb measures its write path; backend=" << GetParam(); + // NuDB records the write path only when telemetry is compiled in; without + // it nothing measures, so the negative path below covers every backend. +#ifdef XRPL_ENABLE_TELEMETRY + bool const measures = GetParam() == "nudb"; +#else + bool const measures = false; +#endif + ASSERT_EQ(initial.has_value(), measures) + << "only nudb measures its write path, and only with telemetry; backend=" << GetParam(); if (!initial) { diff --git a/src/tests/libxrpl/nodestore/NuDBFactory.cpp b/src/tests/libxrpl/nodestore/NuDBFactory.cpp index a7d6a11dca..09d398fa14 100644 --- a/src/tests/libxrpl/nodestore/NuDBFactory.cpp +++ b/src/tests/libxrpl/nodestore/NuDBFactory.cpp @@ -354,6 +354,11 @@ TEST(NuDBFactory, configuration_parsing) // so these counters plus Little's Law are the only way to separate queuing // from service time from outside the library. +#ifdef XRPL_ENABLE_TELEMETRY +// The three tests below pin the cumulative write-path counters, which are +// recorded only when telemetry is compiled in. Without it the write-load gauge +// is their only consumer and it reads the depth atomic directly, so the depth +// samples and the two clock reads per insert are skipped on that hot path. TEST(NuDBFactory, write_stats_accumulate_per_insert) { TempDir const tempDir; @@ -637,6 +642,7 @@ TEST(NuDBFactory, write_load_reports_writer_depth) backend->close(); } +#endif // XRPL_ENABLE_TELEMETRY TEST(NuDBFactory, data_persistence) { From b6d1b0524cd0125fb0178feb7afacbfdae845211 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:59:50 +0100 Subject: [PATCH 12/44] perf(telemetry): allocate no tx span when telemetry is compiled out The tx.receive and tx.process spans were built with make_shared whatever the build, so a node without telemetry allocated once per inbound, submitted and relayed transaction -- duplicates included, since tx.receive runs before the duplicate check -- to hold an empty object. Leave the handle null in that build instead. Nothing needed to change to carry it: both doTransaction* overloads already default the span to nullptr, the job capture and activateIfLive() accept a null handle, and the apply loop already tested `e.span && *e.span` before using it. The remaining uses now test the handle, which they have to do anyway once it can be null. With telemetry compiled in the behaviour is unchanged, and the attribute block is still skipped for any span that is not being recorded. --- src/xrpld/app/misc/NetworkOPs.cpp | 25 ++++++++++++------ src/xrpld/overlay/detail/PeerImp.cpp | 39 +++++++++++++++++++--------- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 8fc85d51ac..31389e6016 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1510,14 +1510,21 @@ NetworkOPsImp::processTransaction( // SpanGuard is thread-free (holds no Scope), so it is safe to store here // and end on the batch worker thread that later applies this transaction — // no detach step is needed. - auto span = std::make_shared(txProcessSpan(transaction->getID())); + // Left null when telemetry is compiled out: there is no span to own, so + // nothing is allocated for one. The transaction pipeline already accepts a + // null span -- both doTransaction* overloads default it to nullptr -- and + // every use tests it. Without this the make_shared allocated once per + // submitted and relayed transaction to hold an empty object. + std::shared_ptr span; +#ifdef XRPL_ENABLE_TELEMETRY + span = std::make_shared(txProcessSpan(transaction->getID())); +#endif // Guarded on the span being live because these values are not free and this // runs for every submitted and relayed transaction: the hash string - // allocates, and the open-ledger index takes the ledger master's lock. 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) + // allocates, and the open-ledger index takes the ledger master's lock. With + // telemetry compiled out the span is null; with it compiled in the block is + // skipped for any span not being recorded. + if (span && *span) { span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); span->setAttribute(tx_span::attr::local, bLocal); @@ -1546,12 +1553,14 @@ NetworkOPsImp::processTransaction( if (bLocal) { - span->setAttribute(tx_span::attr::path, tx_span::val::sync); + if (span) + span->setAttribute(tx_span::attr::path, tx_span::val::sync); doTransactionSync(transaction, bUnlimited, failType, std::move(span)); } else { - span->setAttribute(tx_span::attr::path, tx_span::val::async); + if (span) + span->setAttribute(tx_span::attr::path, tx_span::val::async); doTransactionAsync(transaction, bUnlimited, failType, std::move(span)); } } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 1a28c9a949..4e54180dd8 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1320,14 +1320,22 @@ PeerImp::handleTransaction( using namespace telemetry; // SpanGuard is thread-free (holds no Scope), so it is safe to hand to // a job-queue worker and end on that thread — no detach step is needed. - auto span = std::make_shared(txReceiveSpan(txID, *m)); + // Left null when telemetry is compiled out: there is no span to own, so + // nothing is allocated for one. Every use below tests it, the job + // capture and activateIfLive() accept a null handle, and the transaction + // pipeline already takes a null span by default. Without this the + // make_shared allocated once per inbound transaction, duplicates + // included, to hold an empty object. + std::shared_ptr span; +#ifdef XRPL_ENABLE_TELEMETRY + span = std::make_shared(txReceiveSpan(txID, *m)); +#endif // Guarded on the span being live because these values are not free and // this runs for every inbound transaction, including duplicates: the // hash string allocates, and the open-ledger index takes the ledger - // master's lock. 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) + // master's lock. With telemetry compiled out the span is null; with it + // compiled in the block is skipped for any span not being recorded. + if (span && *span) { span->setAttribute(tx_span::attr::txHash, to_string(txID).c_str()); span->setAttribute(tx_span::attr::peerId, static_cast(id_)); @@ -1365,7 +1373,8 @@ PeerImp::handleTransaction( */ if (stx->isFlag(tfInnerBatchTxn)) { - span->setAttribute(tx_span::attr::txStatus, tx_span::val::rejectedInnerBatch); + if (span) + span->setAttribute(tx_span::attr::txStatus, tx_span::val::rejectedInnerBatch); JLOG(pJournal_.warn()) << "Ignoring Network relayed Tx containing " "tfInnerBatchTxn (handleTransaction)."; fee_.update(resource::kFeeModerateBurdenPeer, "inner batch txn"); @@ -1378,11 +1387,13 @@ PeerImp::handleTransaction( if (!app_.getHashRouter().shouldProcess(txID, id_, flags, kTxInterval)) { - span->setAttribute(tx_span::attr::suppressed, true); + if (span) + span->setAttribute(tx_span::attr::suppressed, true); // we have seen this transaction recently if (any(flags & HashRouterFlags::BAD)) { - span->setAttribute(tx_span::attr::txStatus, tx_span::val::knownBad); + if (span) + span->setAttribute(tx_span::attr::txStatus, tx_span::val::knownBad); fee_.update(resource::kFeeUselessData, "known bad"); JLOG(pJournal_.debug()) << "Ignoring known bad tx " << txID; } @@ -1391,7 +1402,8 @@ PeerImp::handleTransaction( // Recently-seen but not flagged bad — this is the plain // duplicate-suppression path. Mark it explicitly so the // span never exits as "new". - span->setAttribute(tx_span::attr::txStatus, tx_span::val::suppressed); + if (span) + span->setAttribute(tx_span::attr::txStatus, tx_span::val::suppressed); // Erase only if the server has seen this tx. If the server // has not seen this tx then the tx could not have been @@ -1408,7 +1420,8 @@ PeerImp::handleTransaction( return; } - span->setAttribute(tx_span::attr::suppressed, false); + if (span) + span->setAttribute(tx_span::attr::suppressed, false); JLOG(pJournal_.debug()) << "Got tx " << txID; bool checkSignature = true; @@ -1433,12 +1446,14 @@ PeerImp::handleTransaction( if (app_.getLedgerMaster().getValidatedLedgerAge() > 4min) { - span->setAttribute(tx_span::attr::txStatus, tx_span::val::droppedNoSync); + if (span) + span->setAttribute(tx_span::attr::txStatus, tx_span::val::droppedNoSync); JLOG(pJournal_.trace()) << "No new transactions until synchronized"; } else if (app_.getJobQueue().getJobCount(JtTransaction) > app_.config().maxTransactions) { - span->setAttribute(tx_span::attr::txStatus, tx_span::val::droppedQueueFull); + if (span) + span->setAttribute(tx_span::attr::txStatus, tx_span::val::droppedQueueFull); overlay_.incJqTransOverflow(); JLOG(pJournal_.info()) << "Transaction queue is full"; } From 767cde1c832ec86c11fa35a737afc29ea0091d2c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:04:50 +0100 Subject: [PATCH 13/44] perf(perflog): record job and RPC metrics only when telemetry is compiled in jobQueue, jobStart and jobFinish each called into the metrics registry, so every job paid for three of them. The recordJob* bodies are compiled out without telemetry, but the calls were not: each still made a virtual getMetricsRegistry() call, and each built its argument with JobTypes::name(), which is a std::map lookup plus an assert. The registry is constructed unconditionally, so the null test never short-circuited any of it. Guard the five recording blocks, and the registry include with them: the only other thing naming that type is the metric macros' expansion, which is also compiled out. The two in-flight UpDownCounter macros stay unguarded -- they take only app_, which costs nothing, and the macro drops it. This is the highest-frequency site in the audit: three calls per job against one per transaction elsewhere. --- src/xrpld/perflog/detail/PerfLogImp.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/xrpld/perflog/detail/PerfLogImp.cpp b/src/xrpld/perflog/detail/PerfLogImp.cpp index 5573522687..4173ea1a7c 100644 --- a/src/xrpld/perflog/detail/PerfLogImp.cpp +++ b/src/xrpld/perflog/detail/PerfLogImp.cpp @@ -2,7 +2,12 @@ #include #include + +#ifdef XRPL_ENABLE_TELEMETRY +// Only the recording calls below and the metric macros' expansion name the +// registry, and neither survives with telemetry compiled out. #include +#endif #include #include @@ -339,8 +344,10 @@ PerfLogImp::rpcStart(std::string const& method, std::uint64_t const requestId) // above are released: the OTel call path allocates and takes locks // inside the SDK, so holding methodsMutex across it would widen a // process-wide critical section for no reason. Mirrors rpcEnd(). +#ifdef XRPL_ENABLE_TELEMETRY if (auto* mr = app_.getMetricsRegistry()) mr->recordRpcStarted(method); +#endif // A value that must be able to decrease (UpDownCounter), added at its // call site with no MetricsRegistry member/init-line/method. Paired with @@ -398,6 +405,7 @@ PerfLogImp::rpcEnd(std::string const& method, std::uint64_t const requestId, boo // Record RPC completion in OTel metrics pipeline. Mirrors the // rpcStart() instrumentation so the finished/errored counters and // duration histogram advance with every call. +#ifdef XRPL_ENABLE_TELEMETRY if (auto* mr = app_.getMetricsRegistry()) { if (finish) @@ -409,6 +417,7 @@ PerfLogImp::rpcEnd(std::string const& method, std::uint64_t const requestId, boo mr->recordRpcErrored(method, durationUs.count()); } } +#endif // Matching -1 for the +1 recorded in rpcStart(). Placed after the early // returns above so it runs only when this request's methods-map entry was @@ -432,10 +441,17 @@ PerfLogImp::jobQueue(JobType const type, std::string const& name) ++counter->second.value.queued; } +#ifdef XRPL_ENABLE_TELEMETRY // Record job enqueue in OTel metrics pipeline, after the lock above is // released so the SDK's work stays outside the critical section. + // + // Guarded because this runs three times per job, counting jobStart and + // jobFinish below. recordJobQueued's body is compiled out, but the call is + // not: the virtual getMetricsRegistry() and the JobTypes::name() map lookup + // that builds its argument both still happen. if (auto* mr = app_.getMetricsRegistry()) mr->recordJobQueued(JobTypes::name(type), name); +#endif } void @@ -470,8 +486,10 @@ PerfLogImp::jobStart( // released. jobsMutex is process-wide and taken by every worker thread // on every job, so the SDK's allocation and internal locking must not // run inside it. +#ifdef XRPL_ENABLE_TELEMETRY if (auto* mr = app_.getMetricsRegistry()) mr->recordJobStarted(JobTypes::name(type), name, dur.count()); +#endif } void @@ -499,8 +517,10 @@ PerfLogImp::jobFinish(JobType const type, std::string const& name, microseconds // Record job finish in OTel metrics pipeline, after the locks above // are released, for the same reason as jobStart(). +#ifdef XRPL_ENABLE_TELEMETRY if (auto* mr = app_.getMetricsRegistry()) mr->recordJobFinished(JobTypes::name(type), name, dur.count()); +#endif } void From 2683a307658dbcc88272d5317a36fb5431d9f454 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:09:47 +0100 Subject: [PATCH 14/44] perf(telemetry): only build apply-span attributes when the span is recorded Transactor::operator()() set its apply-stage attributes unconditionally, so every transaction applied paid for a TxFormats::findByType() lookup and a 64-char string built from the view's parent hash, plus one or two transToken() lookups in the exit funnel, whether or not anything recorded them. Guard both blocks on the span being active. This is the pattern the sibling preflight and preclaim spans already use in applySteps.cpp, with a comment giving this exact reason -- the apply stage was simply missed. Behaviour is unchanged where the span is live, and setAttribute on an inactive guard was already a no-op. --- src/libxrpl/tx/Transactor.cpp | 50 ++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 09aa4487da..8e97e730d9 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1564,17 +1564,25 @@ Transactor::operator()() telemetry::tx_apply_span::transactor, txID.data(), txID.kBytes); - // "apply" — the third apply-pipeline stage, after preflight and preclaim. - span.setAttribute(telemetry::tx_apply_span::attr::stage, telemetry::tx_apply_span::val::apply); - if (auto const* fmt = TxFormats::getInstance().findByType(ctx_.tx.getTxnType())) - span.setAttribute(telemetry::tx_apply_span::attr::txType, fmt->getName().c_str()); - // The ledger being worked on (seq + parent hash) — correlates this apply - // stage to the ledger/consensus trace it is building into. - span.setAttribute( - telemetry::tx_apply_span::attr::currentLedgerSeq, static_cast(view().seq())); - span.setAttribute( - telemetry::tx_apply_span::attr::currentLedgerHash, - to_string(view().header().parentHash).c_str()); + // Guard the attribute work behind the active check, as preflight does in + // applySteps.cpp: this runs for every transaction applied, and the type + // lookup and the parent-hash string are not free. + if (span) + { + // "apply" — the third apply-pipeline stage, after preflight and preclaim. + span.setAttribute( + telemetry::tx_apply_span::attr::stage, telemetry::tx_apply_span::val::apply); + if (auto const* fmt = TxFormats::getInstance().findByType(ctx_.tx.getTxnType())) + span.setAttribute(telemetry::tx_apply_span::attr::txType, fmt->getName().c_str()); + // The ledger being worked on (seq + parent hash) — correlates this apply + // stage to the ledger/consensus trace it is building into. + span.setAttribute( + telemetry::tx_apply_span::attr::currentLedgerSeq, + static_cast(view().seq())); + span.setAttribute( + telemetry::tx_apply_span::attr::currentLedgerHash, + to_string(view().header().parentHash).c_str()); + } JLOG(j_.trace()) << "apply: " << ctx_.tx.getTransactionID(); @@ -1655,13 +1663,19 @@ Transactor::operator()() std::optional&& metadata = std::nullopt) -> ApplyResult { JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); - 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)); + // 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)}; }; From 922f1a653a886cafa1167413d61105f66728a147 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:13:38 +0100 Subject: [PATCH 15/44] perf(telemetry): only build txq enqueue attributes when the span is recorded TxQ::apply set its enqueue-span attributes unconditionally, so every transaction paid for two 64-char hash strings and a TxFormats lookup whether or not anything recorded them. The open-ledger rebuild replays transactions through this same path, so it was paid more than once each. Guard the block on the span being active, as the tx apply-pipeline spans do. --- src/xrpld/app/misc/detail/TxQ.cpp | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index a4b69dfbc7..c2fdba40fa 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -770,17 +770,25 @@ TxQ::apply( return ScopedSpanGuard( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::enqueue); }(); - span.setAttribute(txq_span::attr::txHash, to_string(tx->getTransactionID()).c_str()); - if (auto const* fmt = TxFormats::getInstance().findByType(tx->getTxnType())) - span.setAttribute(txq_span::attr::txType, fmt->getName().c_str()); - // The ledger being worked on (open/tentative apply or in-flight consensus - // build) — correlates this enqueue to the ledger trace in every context. - span.setAttribute(txq_span::attr::currentLedgerSeq, static_cast(view.seq())); - span.setAttribute( - txq_span::attr::currentLedgerHash, to_string(view.header().parentHash).c_str()); - // Default outcome; overridden below on the direct-apply and queued paths. - // Every other early return leaves the tx rejected from the queue. - span.setAttribute(txq_span::attr::txqStatus, txq_span::val::rejected); + // Guarded on the span being recorded: this runs for every transaction and + // again for each one replayed on an open-ledger rebuild, and the two hash + // strings each allocate. The compiled-out guard's operator bool() is a + // literal false, so the block disappears in that build. + if (span) + { + span.setAttribute(txq_span::attr::txHash, to_string(tx->getTransactionID()).c_str()); + if (auto const* fmt = TxFormats::getInstance().findByType(tx->getTxnType())) + span.setAttribute(txq_span::attr::txType, fmt->getName().c_str()); + // The ledger being worked on (open/tentative apply or in-flight + // consensus build) — correlates this enqueue to the ledger trace in + // every context. + span.setAttribute(txq_span::attr::currentLedgerSeq, static_cast(view.seq())); + span.setAttribute( + txq_span::attr::currentLedgerHash, to_string(view.header().parentHash).c_str()); + // Default outcome; overridden below on the direct-apply and queued + // paths. Every other early return leaves the tx rejected from the queue. + span.setAttribute(txq_span::attr::txqStatus, txq_span::val::rejected); + } // See if the transaction is valid, properly formed, // etc. before doing potentially expensive queue From 1cf81f6edb6e8af6ecb1e02345215181344079c6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:25:59 +0100 Subject: [PATCH 16/44] Skip per-transaction span work when accept span is inactive doAccept's canonical-tx-set loop built a 64-character hex string from every transaction hash and added a span event for it, then reported a txCount that only the span reads. That ran once per transaction in every accepted ledger, whether or not anything was recording. txCount and txHash have no other consumer: txCount is only read by the tx_count attribute, and txHash only by the tx_included event. buildLCL takes retriableTxs, not the count. Both now sit behind if (doAcceptSpan). With telemetry compiled out the stub's operator bool is a literal false, so the blocks are eliminated; with telemetry on they are also skipped whenever the span is null because the consensus trace category is off. --- src/xrpld/app/consensus/RCLConsensus.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index e29e77147f..dad4c276c3 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -660,6 +660,10 @@ RCLConsensus::Adaptor::doAccept( JLOG(j_.debug()) << "Building canonical tx set: " << retriableTxs.key(); + // txCount and the per-transaction event feed the span and nothing else, so + // both are guarded on the span being active. Unguarded, every accepted + // ledger builds one 64-character hash string per transaction that no one + // reads. int64_t txCount = 0; for (auto const& item : *result.txns.map) { @@ -667,9 +671,12 @@ RCLConsensus::Adaptor::doAccept( { retriableTxs.insert(std::make_shared(SerialIter{item.slice()})); JLOG(j_.debug()) << " Tx: " << item.key(); - ++txCount; - auto const txHash = to_string(item.key()); - doAcceptSpan.addEvent(cs::event::txIncluded, {{cs::attr::txId, txHash}}); + if (doAcceptSpan) + { + ++txCount; + auto const txHash = to_string(item.key()); + doAcceptSpan.addEvent(cs::event::txIncluded, {{cs::attr::txId, txHash}}); + } } catch (std::exception const& ex) { @@ -677,7 +684,10 @@ RCLConsensus::Adaptor::doAccept( JLOG(j_.warn()) << " Tx: " << item.key() << " throws: " << ex.what(); } } - doAcceptSpan.setAttribute(cs::attr::txCount, txCount); + if (doAcceptSpan) + { + doAcceptSpan.setAttribute(cs::attr::txCount, txCount); + } auto built = buildLCL( prevLedger, From ceadaad8410a945eb40741f25fc457e6c1dfb109 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:26:18 +0100 Subject: [PATCH 17/44] Skip dispute-resolve event work when update span is inactive updateOurPositions built a 64-character hex string from the transaction id plus two std::to_string number conversions, then attached them to a span event. That ran for every dispute that flipped position, on every establish tick of every consensus round, whether or not anything was recording. The three strings and the event have no other consumer; the vote change itself (mutableSet insert/erase) is untouched. The block now sits behind if (span). With telemetry compiled out the stub's operator bool is a literal false, so it is eliminated; with telemetry on it is also skipped when the span is null because the establish context was never captured. The guard is inside the function body, so Consensus's adaptor interface is unchanged and the csf::Peer simulator is unaffected. --- include/xrpl/consensus/Consensus.h | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index f299ce2b9a..78750656a4 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -1758,16 +1758,23 @@ Consensus::updateOurPositions(std::unique_ptr const& mutableSet->erase(txId); } - auto const yaysStr = std::to_string(dispute.getYays()); - auto const naysStr = std::to_string(dispute.getNays()); - span.addEvent( - consensus::span::event::disputeResolve, - {{consensus::span::attr::txId, to_string(txId)}, - {consensus::span::attr::disputeOurVote, - dispute.getOurVote() ? std::string_view{consensus::span::val::yes} - : std::string_view{consensus::span::val::no}}, - {consensus::span::attr::disputeYays, yaysStr}, - {consensus::span::attr::disputeNays, naysStr}}); + // The event exists only for the span, so it is guarded on the + // span being active. Unguarded, every dispute that flips + // position builds a 64-character tx hash plus two number + // strings, on every establish tick. + if (span) + { + auto const yaysStr = std::to_string(dispute.getYays()); + auto const naysStr = std::to_string(dispute.getNays()); + span.addEvent( + consensus::span::event::disputeResolve, + {{consensus::span::attr::txId, to_string(txId)}, + {consensus::span::attr::disputeOurVote, + dispute.getOurVote() ? std::string_view{consensus::span::val::yes} + : std::string_view{consensus::span::val::no}}, + {consensus::span::attr::disputeYays, yaysStr}, + {consensus::span::attr::disputeNays, naysStr}}); + } } } 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 18/44] 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 f40c17a7ed4fe66ff9ca681ad36a2355aac42290 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:34:28 +0100 Subject: [PATCH 19/44] perf(telemetry): only build accept-span attributes when recorded TxQ::accept set two of its per-transaction span attributes unconditionally, so every queued transaction the loop tried to apply to the open ledger paid for a 64-char string built from the transaction id and a transToken() lookup that builds a string, whether or not anything recorded them. That is once per candidate clearing the required fee level, on every ledger close. Guard both on the span being active, as the enqueue path in TxQ::apply and the apply-pipeline spans already do. The transaction apply itself stays outside the guard; only the attribute that reads its result is telemetry. --- src/xrpld/app/misc/detail/TxQ.cpp | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index c2fdba40fa..bb83e4ad9f 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -1526,13 +1526,27 @@ TxQ::accept(Application& app, OpenView& view) ScopedSpanGuard txSpan( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::acceptTx); - txSpan.setAttribute(txq_span::attr::txHash, to_string(candidateIter->txID).c_str()); - txSpan.setAttribute( - txq_span::attr::retriesRemaining, - static_cast(candidateIter->retriesRemaining)); + // Guarded on the span being recorded: this runs for every queued + // transaction the loop tries to apply to the open ledger, on every + // ledger close, and the hash string allocates 64 characters. The + // compiled-out guard's operator bool() is a literal false, so the + // block disappears in that build. + if (txSpan) + { + txSpan.setAttribute(txq_span::attr::txHash, to_string(candidateIter->txID).c_str()); + txSpan.setAttribute( + txq_span::attr::retriesRemaining, + static_cast(candidateIter->retriesRemaining)); + } auto const [txnResult, didApply, _metadata] = candidateIter->apply(app, view, j_); - txSpan.setAttribute(txq_span::attr::terCode, transToken(txnResult).c_str()); + // The apply above is the transaction itself and always runs; only + // the attribute reading its result is telemetry. transToken() is a + // lookup that builds a string, so it is guarded too. + if (txSpan) + { + txSpan.setAttribute(txq_span::attr::terCode, transToken(txnResult).c_str()); + } if (didApply) { 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 20/44] 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 21/44] 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 22/44] 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 23/44] 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 24/44] 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 From 40824c4d46c80e8d0fcff7546477afea9782058a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:53:53 +0100 Subject: [PATCH 25/44] 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/app/misc/NetworkOPs.cpp | 3 ++- src/xrpld/overlay/detail/PeerImp.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 31389e6016..1dadd07503 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1523,7 +1523,8 @@ NetworkOPsImp::processTransaction( // runs for every submitted and relayed transaction: the hash string // allocates, and the open-ledger index takes the ledger master's lock. With // telemetry compiled out the span is null; with it compiled in the block is - // skipped for any span not being recorded. + // skipped when telemetry is disabled at runtime or the transaction category + // is off. if (span && *span) { span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 4e54180dd8..2908a280bb 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1334,7 +1334,8 @@ PeerImp::handleTransaction( // this runs for every inbound transaction, including duplicates: the // hash string allocates, and the open-ledger index takes the ledger // master's lock. With telemetry compiled out the span is null; with it - // compiled in the block is skipped for any span not being recorded. + // compiled in the block is skipped when telemetry is disabled at runtime + // or the transaction category is off. if (span && *span) { span->setAttribute(tx_span::attr::txHash, to_string(txID).c_str()); From 98f0e99df89ce732cc63db29c4998658e7fcfed6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 09:40:16 +0100 Subject: [PATCH 26/44] Guard the inbound-validation ledger-hash attribute PeerImp::onMessage(TMValidation) runs once per inbound validation message, and it reaches these attribute calls before the HashRouter duplicate check, so every peer's copy of every validation paid for them. Span setAttribute is a real inline function whose arguments are evaluated even when telemetry is compiled out, and to_string(val->getLedgerHash()) heap-allocates a 64-character hex string on each call. Wrap the ledger_hash and full_validation attributes in if (valSpan), so neither the string build nor the flags lookup behind isFull() runs when telemetry is compiled out, when it is switched off in the config, or when the Peer trace category is disabled. A span that exists but was sampled out still pays; there is no isRecording() to test. peer_id and validation_trusted stay unguarded: their arguments are an integer cast and a bool the surrounding logic already computes. --- src/xrpld/overlay/detail/PeerImp.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 530b2802c5..3fee58ce3d 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -2559,8 +2559,19 @@ PeerImp::onMessage(std::shared_ptr const& m) } val->setSeen(closeTime); } - valSpan.setAttribute(peer_span::attr::ledgerHash, to_string(val->getLedgerHash()).c_str()); - valSpan.setAttribute(peer_span::attr::fullValidation, val->isFull()); + // setAttribute evaluates its arguments even when telemetry is compiled + // out, and to_string() heap-allocates a 64-character hex string. This + // runs before the duplicate check below, so without the guard every + // peer's copy of every validation pays for that string. The guard is + // false when telemetry is compiled out, switched off in the config, or + // the Peer trace category is disabled; a span that exists but was + // sampled out still pays. + if (valSpan) + { + valSpan.setAttribute( + peer_span::attr::ledgerHash, to_string(val->getLedgerHash()).c_str()); + valSpan.setAttribute(peer_span::attr::fullValidation, val->isFull()); + } if (!isCurrent( app_.getValidations().parms(), From 455bc9d3a5560af4aa460153bd423e5275e00984 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 09:41:05 +0100 Subject: [PATCH 27/44] Skip proposal and validation receive-span work when inactive Both inbound peer-message handlers built a receive span and set its attributes unconditionally. The proposal path turned two 32-byte hashes into full hex strings and then took a 16-character substring of each -- four heap allocations per message -- and it ran for every inbound proposal, trusted or untrusted. The validation path did a field lookup, two flag reads and a sign-time conversion for every inbound validation, including the ones dropped just below it for peer divergence or local load. The handles are declared empty and only the make_shared sits inside XRPL_ENABLE_TELEMETRY, so with telemetry compiled out neither path allocates. The attribute blocks sit behind if (span && *span), which also skips them when telemetry is compiled in but disabled in config, and when the consensus trace category is off. It does not skip a span that exists but was sampled out; that span still pays. Both job bodies only carry the handle to hold the span alive and never dereference it, so an empty handle is safe there. The validation span is still built before the drop decision, so a dropped validation is still traced; only its cost is removed. ConsensusReceiveTracing.h has no other user in the file, so its include is guarded the same way to keep misc-include-cleaner satisfied when telemetry is off. --- src/xrpld/overlay/detail/PeerImp.cpp | 82 +++++++++++++++++++--------- 1 file changed, 57 insertions(+), 25 deletions(-) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 44cb9c31e3..c2448f4599 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include @@ -109,6 +108,12 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The consensus receive-span factories are named only by the +// telemetry-enabled blocks in this file. +#include +#endif + using namespace std::chrono_literals; namespace xrpl { @@ -2023,20 +2028,33 @@ PeerImp::onMessage(std::shared_ptr const& m) // Create a receive span that links to the sender's trace context // (if propagated). shared_ptr keeps it alive across the job boundary. // The receive span is a thread-free SpanGuard handed to the job worker; - // no scope to strip. - auto span = std::make_shared(telemetry::proposalReceiveSpan(set)); - span->setAttribute(telemetry::consensus::span::attr::proposalTrusted, isTrusted); - span->setAttribute( - telemetry::consensus::span::attr::round, static_cast(set.proposeseq())); - // First 16 hex chars (8 bytes) of each hash — enough to disambiguate - // peer positions and prior ledgers without exporting full 32-byte - // hashes on every receive event. - span->setAttribute( - telemetry::consensus::span::attr::prevLedgerPrefix, - to_string(prevLedger).substr(0, 16).c_str()); - span->setAttribute( - telemetry::consensus::span::attr::positionHashPrefix, - to_string(proposeHash).substr(0, 16).c_str()); + // no scope to strip. The handle stays empty when telemetry is compiled + // out, so nothing is allocated on a path every inbound proposal takes. + // The job body only carries the handle to hold the span alive, so an + // empty handle is safe there. + std::shared_ptr span; +#ifdef XRPL_ENABLE_TELEMETRY + span = std::make_shared(telemetry::proposalReceiveSpan(set)); +#endif + // Every attribute below exists only for the span, so the block is guarded + // on the span being live. Unguarded, each inbound proposal — trusted or + // not — builds two full hex strings and a substring of each, four string + // allocations no one reads. + if (span && *span) + { + span->setAttribute(telemetry::consensus::span::attr::proposalTrusted, isTrusted); + span->setAttribute( + telemetry::consensus::span::attr::round, static_cast(set.proposeseq())); + // First 16 hex chars (8 bytes) of each hash — enough to disambiguate + // peer positions and prior ledgers without exporting full 32-byte + // hashes on every receive event. + span->setAttribute( + telemetry::consensus::span::attr::prevLedgerPrefix, + to_string(prevLedger).substr(0, 16).c_str()); + span->setAttribute( + telemetry::consensus::span::attr::positionHashPrefix, + to_string(proposeHash).substr(0, 16).c_str()); + } std::weak_ptr const weak = shared_from_this(); app_.getJobQueue().addJob( @@ -2598,19 +2616,33 @@ PeerImp::onMessage(std::shared_ptr const& m) // Create a receive span that links to the sender's trace context // (if propagated). shared_ptr keeps it alive across the job boundary. // The receive span is a thread-free SpanGuard handed to the job worker; - // no scope to strip. - auto span = std::make_shared(telemetry::validationReceiveSpan(*m)); - span->setAttribute(telemetry::consensus::span::attr::validationTrusted, isTrusted); - if (val->isFieldPresent(sfLedgerSequence)) + // no scope to strip. The handle stays empty when telemetry is compiled + // out, so nothing is allocated on a path every inbound validation + // takes. The job body only carries the handle to hold the span alive, + // so an empty handle is safe there. + std::shared_ptr span; +#ifdef XRPL_ENABLE_TELEMETRY + span = std::make_shared(telemetry::validationReceiveSpan(*m)); +#endif + // Every attribute below exists only for the span, so the block is + // guarded on the span being live. Unguarded, each inbound validation + // pays the field lookups and time conversions here. The span is built + // before the drop decision below on purpose, so a dropped validation + // is still traced; the guard removes the cost, not the span. + if (span && *span) { + span->setAttribute(telemetry::consensus::span::attr::validationTrusted, isTrusted); + if (val->isFieldPresent(sfLedgerSequence)) + { + span->setAttribute( + telemetry::consensus::span::attr::ledgerSeq, + static_cast(val->getFieldU32(sfLedgerSequence))); + } + span->setAttribute(telemetry::consensus::span::attr::fullValidation, val->isFull()); span->setAttribute( - telemetry::consensus::span::attr::ledgerSeq, - static_cast(val->getFieldU32(sfLedgerSequence))); + telemetry::consensus::span::attr::validationSignTime, + static_cast(val->getSignTime().time_since_epoch().count())); } - span->setAttribute(telemetry::consensus::span::attr::fullValidation, val->isFull()); - span->setAttribute( - telemetry::consensus::span::attr::validationSignTime, - static_cast(val->getSignTime().time_since_epoch().count())); if (!isTrusted && (tracking_.load() == Tracking::Diverged)) { From 6dd93b46eeb7bd20e85032af8f45788262464117 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:09:33 +0100 Subject: [PATCH 28/44] perf(ledger): count acquisitions only when telemetry is compiled in Five AcquireStats recording calls ran in every build. The two in TimeoutCounter are the frequent ones: one on every deferred timer tick and one on every no-progress tick, for every in-flight TimeoutCounter, and each builds its argument with isLedgerAcquisition(), which compares the job name std::string against a literal. The other three fire once per acquisition that aborts, gives up or completes. Nothing outside telemetry reads any of the nine counters. The only non-test caller of any accessor is MetricsRegistry::observeAcquireStats, which itself sits inside that file's XRPL_ENABLE_TELEMETRY block and so does not exist in a telemetry-off build. Guard the five calls, and the AcquireStats include with them, since they were its only users in these three files. The counters and their accessors are left alone: with every writer guarded and the only reader absent, they are nine untouched atomics in one process-wide object, so gating them would add many preprocessor blocks to the header and force a gated test file to save 72 bytes of a build that never touches them. TimeoutCounter::timeouts_ stays outside the guard because the give-up test reads it, and so does InboundLedger's completionCounted_ latch, which is two branches once per acquisition and would otherwise be left an unused member. --- src/xrpld/app/ledger/detail/InboundLedger.cpp | 19 ++++++++++++++++++- .../app/ledger/detail/InboundLedgers.cpp | 12 ++++++++++-- .../app/ledger/detail/TimeoutCounter.cpp | 18 +++++++++++++++++- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index fbe129b37e..ef2e8d3a34 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -1,7 +1,6 @@ #include #include -#include #include #include #include @@ -13,6 +12,12 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The three guarded recording calls below are the only things here that name +// AcquireStats, so without telemetry the include has no user. +#include +#endif + #include #include #include @@ -223,10 +228,14 @@ InboundLedger::~InboundLedger() } if (!isDone()) { +#ifdef XRPL_ENABLE_TELEMETRY // Partial work means a map was partly built and is now discarded, so // the whole acquisition has to start over. That is the expensive case, // so it is counted apart from a cheap abort that had nothing yet. + // + // Guarded because the acquire metrics are the only reader. app_.getAcquireStats().recordAbort(haveHeader_ || haveState_ || haveTransactions_); +#endif // Mark the span so an abandoned acquisition is distinguishable from one // that was still in flight when the trace was read. Without this the @@ -422,7 +431,10 @@ InboundLedger::onTimer(bool wasProgress, ScopedLockType&) if (timeouts_ > kLedgerTimeoutRetriesMax) { +#ifdef XRPL_ENABLE_TELEMETRY + // The acquire metrics are the only reader of this counter. app_.getAcquireStats().recordGiveUp(); +#endif if (seq_ != 0) { JLOG(journal_.warn()) << timeouts_ << " timeouts for ledger " << seq_; @@ -496,7 +508,12 @@ InboundLedger::recordCompletionOnce() return; completionCounted_ = true; +#ifdef XRPL_ENABLE_TELEMETRY + // The acquire metrics are the only reader of this counter. The latch above + // is left running: it is two branches once per acquisition, and gating it + // would leave an unused member behind. app_.getAcquireStats().recordCompletion(); +#endif } void diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 96b986500e..2bcb0a0706 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -1,12 +1,17 @@ #include -#include #include #include #include #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The guarded recording call below is the only thing here that names +// AcquireStats, so without telemetry the include has no user. +#include +#endif + #include #include #include @@ -394,10 +399,13 @@ public: else if ((la + std::chrono::minutes(1)) < start) { stuffToSweep.push_back(it->second); +#ifdef XRPL_ENABLE_TELEMETRY // An eviction here discards whatever the acquisition had // built, so the work restarts. Counted to tell that apart - // from an acquisition that ended on its own. + // from an acquisition that ended on its own. Guarded + // because the acquire metrics are the only reader. app_.getAcquireStats().recordSweepEviction(); +#endif // shouldn't cause the actual final delete // since we are holding a reference in the vector. it = ledgers_.erase(it); diff --git a/src/xrpld/app/ledger/detail/TimeoutCounter.cpp b/src/xrpld/app/ledger/detail/TimeoutCounter.cpp index 3e9961bfb1..5206f184be 100644 --- a/src/xrpld/app/ledger/detail/TimeoutCounter.cpp +++ b/src/xrpld/app/ledger/detail/TimeoutCounter.cpp @@ -1,8 +1,13 @@ #include -#include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The two guarded recording calls below are the only things here that name +// AcquireStats, so without telemetry the include has no user. +#include +#endif + #include #include #include @@ -64,10 +69,16 @@ TimeoutCounter::queueJob(ScopedLockType& sl) app_.getJobQueue().getJobCountTotal(queueJobParameter_.jobType) >= queueJobParameter_.jobLimit) { +#ifdef XRPL_ENABLE_TELEMETRY // Counted separately from timeouts: this path re-arms the timer // without running invokeOnTimer, so timeouts_ does not advance and the // give-up test that reads it cannot fire while the lane stays full. + // + // Guarded because it runs on every deferred tick of every in-flight + // task, and the argument compares the job name against a string + // literal each time. The acquire metrics are its only reader. app_.getAcquireStats().recordDeferral(isLedgerAcquisition()); +#endif JLOG(journal_.debug()) << "Deferring " << queueJobParameter_.jobName << " timer due to load"; setTimer(sl); @@ -92,7 +103,12 @@ TimeoutCounter::invokeOnTimer() if (!progress_) { ++timeouts_; +#ifdef XRPL_ENABLE_TELEMETRY + // Same cost as the deferral above: one call per no-progress tick, with + // a job-name string comparison to build the argument. timeouts_ stays + // outside the guard because the give-up test reads it. app_.getAcquireStats().recordTimeout(isLedgerAcquisition()); +#endif JLOG(journal_.debug()) << "Timeout(" << timeouts_ << ") " << " acquiring " << hash_; onTimer(false, sl); From f799678df3d7fb8e5632c083893cb206b6f5d6e7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:09:56 +0100 Subject: [PATCH 29/44] perf(networkops): count checked validations only with telemetry on recvValidation paid a virtual registry lookup plus a call into an out-of-line function with an empty body for every validation it checked. That is once per unique validation the node accepts, on the check job PeerImp queues after dropping duplicates. The registry is always constructed, so the null test never skipped any of it. incrementValidationsChecked only advances an OTel counter, published as validations_checked_total. One dashboard panel and one alert rule are its whole audience; no RPC reply, log line or control decision reads it. Guard the call. The MetricsRegistry include stays, because incrementStateChanges in setMode still uses it and that fires only when the operating mode actually changes. --- src/xrpld/app/misc/NetworkOPs.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 38b37d41ac..8cf5c2846b 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -2831,8 +2831,14 @@ bool NetworkOPsImp::recvValidation(std::shared_ptr const& val, std::string const& source) { JLOG(journal_.trace()) << "recvValidation " << val->getLedgerHash() << " from " << source; +#ifdef XRPL_ENABLE_TELEMETRY + // One per validation received. The registry is always constructed, so the + // null test never short-circuits: without the guard every validation pays + // a virtual lookup and an out-of-line call whose body is empty. Nothing + // outside the validations_checked_total metric reads the counter. if (auto* mr = registry_.get().getMetricsRegistry()) mr->incrementValidationsChecked(); +#endif std::unique_lock lock(validationsMutex_); BypassAccept bypassAccept = BypassAccept::No; From 626c4e7ae31cbc17700649454b97ad80d1afcb34 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:10:26 +0100 Subject: [PATCH 30/44] Skip telemetry-only work on the consensus round path Five sites on the consensus round path did telemetry-only work whether or not anything could record it. onClose set four attributes on the ledger-close span. Two cost real work once per round: OpenLedger::current() takes currentMutex_ and copies a shared_ptr just to read txCount, and the mode attribute builds a string. The block now sits behind if (span). doAccept read the previous close-time resolution and ran a lambda returning a std::string, feeding the resolution_direction attribute and nothing else, once per accepted ledger. Now behind if (doAcceptSpan). makeAcceptSpan, startRoundTracing and createValidationSpan have wholly telemetry bodies, so each body sits inside XRPL_ENABLE_TELEMETRY. makeAcceptSpan then allocates no control block per accepted ledger; an empty handle is safe because doAccept only passes it to activateIfLive(), which tests it. Its attributes are additionally guarded on the span being live. startRoundTracing's early return sits after two virtual Telemetry calls and a strategy string compare, so the whole body is compiled out rather than reached each round. createValidationSpan yields std::nullopt, and its two call sites in validate() test the guard as well as the optional -- an engaged optional holding a dead guard still turned a 32-byte ledger hash into a 64-character string. Telemetry.h and SpanNames.h are named only by startRoundTracing, so their includes are guarded the same way to keep misc-include-cleaner satisfied when telemetry is off. onPhaseEvent and onOutcomeEvent are left as they are: the generic Consensus template calls both and the csf simulator implements both, so they are part of the adaptor surface. --- src/xrpld/app/consensus/RCLConsensus.cpp | 119 ++++++++++++++++------- 1 file changed, 83 insertions(+), 36 deletions(-) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index dad4c276c3..e068eb4c92 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -64,8 +64,6 @@ #include #include #include -#include -#include #include @@ -89,6 +87,13 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +// The Telemetry interface and the shared segment names are named only by +// startRoundTracing(), which is telemetry-enabled code. +#include +#include +#endif + namespace xrpl { RCLConsensus::RCLConsensus( @@ -358,15 +363,23 @@ RCLConsensus::Adaptor::onClose( // Child of the round span via its captured context (roundSpan_ is a // thread-free SpanGuard, so parent explicitly via its context). auto span = telemetry::SpanGuard::childSpan(cs::ledgerClose, roundSpanContext_); - span.setAttribute(cs::attr::ledgerSeq, static_cast(ledger.ledger->header().seq) + 1); - span.setAttribute(cs::attr::mode, toDisplayString(mode).c_str()); - span.setAttribute( - cs::attr::txCountOpen, static_cast(app_.getOpenLedger().current()->txCount())); - span.setAttribute( - cs::attr::closeTimeResolutionMs, - static_cast( - std::chrono::duration_cast(ledger.closeTimeResolution()) - .count())); + // setAttribute is the only consumer of everything read here, so the block is + // guarded on the span being live. Unguarded, every round takes the open + // ledger's currentMutex_ and copies a shared_ptr just to read txCount, and + // builds a mode string, for attributes no one may be recording. + if (span) + { + span.setAttribute( + cs::attr::ledgerSeq, static_cast(ledger.ledger->header().seq) + 1); + span.setAttribute(cs::attr::mode, toDisplayString(mode).c_str()); + span.setAttribute( + cs::attr::txCountOpen, static_cast(app_.getOpenLedger().current()->txCount())); + span.setAttribute( + cs::attr::closeTimeResolutionMs, + static_cast( + std::chrono::duration_cast(ledger.closeTimeResolution()) + .count())); + } bool const wrongLCL = mode == ConsensusMode::WrongLedger; bool const proposing = mode == ConsensusMode::Proposing; @@ -512,36 +525,47 @@ RCLConsensus::Adaptor::onAccept( std::shared_ptr RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) { + // The whole body is telemetry: the guard, its attributes and the captured + // context serve the accept span only. With telemetry compiled out the handle + // stays empty, so accepting a ledger does not allocate a control block for a + // span that can never record. doAccept only hands the handle to + // activateIfLive(), which tests it, so an empty handle is safe on both the + // sync (onForceAccept) and async (onAccept) paths. +#ifdef XRPL_ENABLE_TELEMETRY namespace cs = telemetry::consensus::span; auto span = std::make_shared( telemetry::SpanGuard::childSpan(cs::accept, roundSpanContext_)); - span->setAttribute(cs::attr::proposers, static_cast(result.proposers)); - span->setAttribute( - cs::attr::roundTimeMs, static_cast(result.roundTime.read().count())); - span->setAttribute(cs::attr::quorum, static_cast(app_.getValidators().quorum())); - span->setAttribute(cs::attr::disputesCount, static_cast(result.disputes.size())); - char const* stateStr = [&] { - switch (result.state) - { - case ConsensusState::Yes: - return "yes"; - case ConsensusState::MovedOn: - return "moved_on"; - case ConsensusState::Expired: - return "expired"; - default: - return "no"; - } - }(); - span->setAttribute(cs::attr::consensusState, stateStr); - // Capture the accept span's context so createValidationSpan() — which - // runs on the jtACCEPT worker thread — can link the validation.send - // span to the accept span (matching the design diagram and the - // "validation follows acceptance" causal model). + // Every attribute below exists only for the span, so the whole block — + // attributes and the context capture — is guarded on the span being live. if (*span) { + span->setAttribute(cs::attr::proposers, static_cast(result.proposers)); + span->setAttribute( + cs::attr::roundTimeMs, static_cast(result.roundTime.read().count())); + span->setAttribute(cs::attr::quorum, static_cast(app_.getValidators().quorum())); + span->setAttribute(cs::attr::disputesCount, static_cast(result.disputes.size())); + char const* stateStr = [&] { + switch (result.state) + { + case ConsensusState::Yes: + return "yes"; + case ConsensusState::MovedOn: + return "moved_on"; + case ConsensusState::Expired: + return "expired"; + default: + return "no"; + } + }(); + span->setAttribute(cs::attr::consensusState, stateStr); + + // Capture the accept span's context so createValidationSpan() — which + // runs on the jtACCEPT worker thread — can link the validation.send + // span to the accept span (matching the design diagram and the + // "validation follows acceptance" causal model). + // // span is a thread-free SpanGuard handed to the JtAccept worker // (onAccept), which ends it there. spanContext() captures the guard's // own span, so accept.apply parents via acceptSpanContext_ regardless @@ -550,6 +574,9 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) acceptSpanContext_ = span->spanContext(); } return span; +#else + return {}; +#endif } void @@ -627,6 +654,10 @@ RCLConsensus::Adaptor::doAccept( cs::attr::closeTimeVoteBins, static_cast(rawCloseTimes.peers.size())); doAcceptSpan.setAttribute( cs::attr::disputesResolvedCount, static_cast(result.disputes.size())); + // prevRes and dir feed the resolution_direction attribute and nothing else, + // so both are guarded on the span being active. Unguarded, every accepted + // ledger builds a std::string that no one reads. + if (doAcceptSpan) { auto const prevRes = prevLedger.closeTimeResolution(); auto const dir = [&]() -> std::string { @@ -969,7 +1000,10 @@ void RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, bool proposing) { auto valSpan = createValidationSpan(); - if (valSpan) + // Testing the guard as well as the optional matters: a guard that exists but + // is not live still evaluates its arguments, and the ledger_hash attribute + // below turns a 32-byte hash into a 64-character string. + if (valSpan && *valSpan) { namespace cs = telemetry::consensus::span; valSpan->setAttribute(cs::attr::ledgerSeq, static_cast(ledger.seq())); @@ -988,7 +1022,7 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, validationTime = lastValidationTime_ + 1s; lastValidationTime_ = validationTime; - if (valSpan) + if (valSpan && *valSpan) { valSpan->setAttribute( telemetry::consensus::span::attr::validationSignTime, @@ -1271,6 +1305,11 @@ RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const void RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) { + // The whole body is telemetry: every member it touches exists only to carry + // span state. It is compiled out rather than left to the early return below, + // because the work above that return — two virtual Telemetry calls and the + // strategy string compare — would otherwise run once per round for nothing. +#ifdef XRPL_ENABLE_TELEMETRY namespace cs = telemetry::consensus::span; // Capture the prior round's context BEFORE the new span overwrites @@ -1345,11 +1384,16 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) // reset() on a different worker than it was emplaced on, so spanContext() // captures its own span and no scope work is needed. roundSpanContext_ = roundSpan_->spanContext(); +#endif } std::optional RCLConsensus::Adaptor::createValidationSpan() { + // The whole body is telemetry: it only builds a span from stored contexts. + // Compiled out, it yields std::nullopt, so validate() takes neither branch + // that reads the ledger hash into a string. +#ifdef XRPL_ENABLE_TELEMETRY namespace cs = telemetry::consensus::span; // Prefer linking to the accept span (matches the design diagram and @@ -1368,6 +1412,9 @@ RCLConsensus::Adaptor::createValidationSpan() } return telemetry::SpanGuard::linkedSpan(cs::validationSend, roundSpanContext_); +#else + return std::nullopt; +#endif } void From 58d0c30e085a970c97225e22b172659e691e494e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:13:12 +0100 Subject: [PATCH 31/44] style(consensus): keep the gated span helpers non-static for clang-tidy The three helpers whose bodies are compiled out with telemetry read members only inside the guard, so in a telemetry-off build they touch no member and readability-convert-member-functions-to-static fires. WarningsAsErrors makes that fatal. Suppress it where the body is gated, matching OverlayImpl::reportDnsResolve. Making them static instead would give the two configurations different signatures, which is the hazard the gating pattern avoids. --- src/xrpld/app/consensus/RCLConsensus.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index e068eb4c92..b18ffa333b 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -522,6 +522,10 @@ RCLConsensus::Adaptor::onAccept( }); } +// Not static: the guarded body reads app_ and roundSpanContext_. With telemetry +// compiled out it returns an empty handle and touches no member, so clang-tidy +// sees a method that could be static. +// NOLINTBEGIN(readability-convert-member-functions-to-static) std::shared_ptr RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) { @@ -578,6 +582,7 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) return {}; #endif } +// NOLINTEND(readability-convert-member-functions-to-static) void RCLConsensus::Adaptor::doAccept( @@ -1302,6 +1307,10 @@ RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const app_.getOPs().setMode(OperatingMode::CONNECTED); } +// Neither is static: both guarded bodies read the span-context members. With +// telemetry compiled out one body is empty and the other returns std::nullopt, +// so clang-tidy sees two methods that could be static. +// NOLINTBEGIN(readability-convert-member-functions-to-static) void RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) { @@ -1416,6 +1425,7 @@ RCLConsensus::Adaptor::createValidationSpan() return std::nullopt; #endif } +// NOLINTEND(readability-convert-member-functions-to-static) void RCLConsensus::Adaptor::onPhaseEvent(std::string_view eventName, std::string_view phaseLabel) From b4094dd91d48e6783be2683137f9de250411c093 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:18:22 +0100 Subject: [PATCH 32/44] fix(telemetry): give NullTelemetry the getMeter override it was missing NullTelemetry overrides the other OTel virtuals behind the telemetry guard so it stays concrete in both configurations, but getMeter was added to the base without a matching override. That leaves the class abstract in a telemetry-on build, which compiles today only because its sole instantiation sits behind #ifndef. Anyone constructing one with telemetry on gets an abstract-class error pointing at the base, not at the missing override. Return a NoopMeter from a function-local static, mirroring getTracer. --- src/libxrpl/telemetry/NullTelemetry.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index 92cc7303ef..26ec8f0b37 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -6,15 +6,20 @@ * unconditionally returns a NullTelemetry that does nothing. * * When XRPL_ENABLE_TELEMETRY IS defined, the OTel virtual methods - * (getTracer, startSpan) return noop tracers/spans. The makeTelemetry() - * factory in this file is not used in that case -- Telemetry.cpp provides - * its own factory that can return the real TelemetryImpl. + * (getTracer, startSpan, getMeter) return noop tracers, spans and meters, so + * the class stays concrete in both configurations. Every pure virtual the base + * declares behind that guard must be overridden here for that to hold. The + * makeTelemetry() factory in this file is not used in that case -- + * Telemetry.cpp provides its own factory that can return the real + * TelemetryImpl. */ #include #ifdef XRPL_ENABLE_TELEMETRY #include +#include +#include #include #include #include @@ -129,6 +134,14 @@ public: return opentelemetry::nostd::shared_ptr( new opentelemetry::trace::NoopSpan(nullptr)); } + + opentelemetry::nostd::shared_ptr + getMeter(std::string_view) override + { + static auto noopMeter = opentelemetry::nostd::shared_ptr( + new opentelemetry::metrics::NoopMeter()); + return noopMeter; + } #endif }; From 5638cd976e015d4fb8d90d75c1900d2735e34f90 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:44:12 +0100 Subject: [PATCH 33/44] fix(telemetry): write the wildcard span predicate without a backslash escape The conjunction query works. Run 33062418036 proved it on real Tempo: both hierarchies that newest-N sampling made unassertable now PASS -- txq.accept -> txq.accept_tx and ledger.acquire -> ledger.acquire.txtree -- along with every other literal-child pair. Only the two wildcard children failed, and not because of the sampling change. They failed with HTTP 400, "invalid TraceQL query: parse error at line 1, col 68: invalid char escape". _traceql_name_predicate built the pattern with re.escape, giving name=~"rpc\.command\..*", and TraceQL's string lexer refuses a backslash escape it does not recognise -- the query never reached the regex engine at all. A literal dot is now written as the character class [.], which carries no backslash for the lexer to refuse while still meaning a literal dot to the engine behind it. Leaving the dots bare would have parsed, but would match any character in those positions, which is the looseness _span_name_matches exists to avoid. The builder now also rejects a span name containing anything outside lower_snake_case, dots and the glob star, rather than passing it through unescaped. Every name in the contract is of that shape, so this changes nothing today; it exists because the failure mode it guards against is exactly the one above -- a character that means something to one layer and something else to the next, discovered only from a 400 in CI. Worth recording why the tests did not catch this. The stub evaluated the pattern with Python's re, which accepts \. happily, so it modelled the regex engine and not the query lexer sitting in front of it. A stub is only as good as the layer it imitates, and the layer that rejected this was one the stub did not represent. The new test therefore asserts the property the lexer enforces -- that no backslash appears in the predicate at all -- rather than any particular spelling, plus that the pattern still accepts rpc.command.fee and still rejects a near-miss whose separators are not dots. Verification: 7/7 tests pass, and the new one was watched failing first with the exact string Tempo rejected, name=~"rpc\.command\..*"; the full query the check now builds was printed and confirmed backslash-free; validate_telemetry.py compiles. Three unrelated files in this worktree are another party's live work and were left unstaged. --- .../workload/test_validate_telemetry.py | 46 +++++++++++++ .../telemetry/workload/validate_telemetry.py | 66 +++++++++++++++++-- 2 files changed, 105 insertions(+), 7 deletions(-) diff --git a/docker/telemetry/workload/test_validate_telemetry.py b/docker/telemetry/workload/test_validate_telemetry.py index 514a42b1df..84358e92a8 100644 --- a/docker/telemetry/workload/test_validate_telemetry.py +++ b/docker/telemetry/workload/test_validate_telemetry.py @@ -214,6 +214,52 @@ def test_wildcard_child_matches_any_family_member() -> None: assert report.results[0].passed, report.results[0].message +def test_wildcard_predicate_carries_no_backslash_escape() -> None: + """The TraceQL predicate must not contain a backslash escape. + + Tempo's string lexer rejects `\\.` outright -- run 33062418036 returned + HTTP 400, "invalid TraceQL query: parse error at line 1, col 68: invalid char + escape", on the predicate re.escape produced. Asserting the absence of a + backslash rather than a specific spelling keeps this test about the property + the lexer enforces instead of about one way of satisfying it. + + Note the earlier stub could not have caught this: it evaluated the pattern + with Python's re, which accepts `\\.` happily, so it modelled the regex engine + rather than the query lexer in front of it. + """ + predicate = vt._traceql_name_predicate("rpc.command.*") + assert "\\" not in predicate, f"backslash escape reaches Tempo: {predicate}" + + +def test_wildcard_predicate_matches_the_family_but_not_near_misses() -> None: + """The pattern must still mean what the glob meant. + + Dropping the escaping must not be done by making the dots match any + character: `rpc.command.*` should accept rpc.command.fee and reject a name + that differs in the separator positions, which is the looseness + _span_name_matches exists to avoid. + """ + import re as _re + + pattern = vt._traceql_name_predicate("rpc.command.*").split('"')[1] + assert _re.fullmatch(pattern, "rpc.command.fee") + assert _re.fullmatch(pattern, "rpc.command.server_info") + # Separators deliberately not dots: if the pattern left its dots bare they + # would match these too. Colons rather than a made-up letter so the spell + # checker still sees three real words. + assert not _re.fullmatch(pattern, "rpc:command:fee") + assert not _re.fullmatch(pattern, "other.command.fee") + + +def test_literal_predicate_uses_equality() -> None: + """A non-glob child must use `=`, not a regex. + + Equality is what makes a longer emitted name unable to satisfy a shorter + contract, the same guarantee _span_name_matches gives on the client side. + """ + assert vt._traceql_name_predicate("txq.accept_tx") == 'name="txq.accept_tx"' + + def main() -> int: tests = [v for k, v in sorted(globals().items()) if k.startswith("test_")] failed = 0 diff --git a/docker/telemetry/workload/validate_telemetry.py b/docker/telemetry/workload/validate_telemetry.py index 38c6c46904..551c168c3f 100644 --- a/docker/telemetry/workload/validate_telemetry.py +++ b/docker/telemetry/workload/validate_telemetry.py @@ -89,6 +89,21 @@ METRIC_POLL_INTERVAL_SEC = 5.0 # not hammer the single-container Prometheus the harness runs. METRIC_POLL_CONCURRENCY = 8 +# Bound on ONE HTTP request to Tempo, Prometheus, Loki or Grafana. aiohttp's +# own default is total=300s, which is longer than any poll window here: a +# single wedged endpoint would blow the shared deadline above and then keep the +# run alive until the CI job's own budget killed it, losing the report and the +# artifacts with it. Failing one request fast and reporting it beats being +# killed with nothing. +# +# Derived from the poll window rather than picked: no single request may outlast +# the phase budget it sits inside, since a request that does can only ever blow +# that deadline. Connecting is held to one poll interval, because an endpoint +# that is absent or wedged should be named immediately, not waited on. +REQUEST_TIMEOUT = aiohttp.ClientTimeout( + total=METRIC_POLL_TIMEOUT_SEC, sock_connect=METRIC_POLL_INTERVAL_SEC +) + # The Prometheus exporter splits one histogram instrument into three series # names. Reverse coverage folds them back onto the base family so a contract # entry (or an accounted_patterns regex) written for the family accounts for @@ -413,11 +428,18 @@ def _traceql_name_predicate(expected_name: str) -> str: """Build the TraceQL `name` predicate that selects a contract span name. A literal contract name becomes an equality test. A glob becomes a regex - test, because TraceQL has no glob operator: `rpc.command.*` must be sent as - `name=~"rpc\\.command\\..*"`, with the dots escaped so they match literal - dots rather than any character. Sending the glob unescaped would still match - the intended spans, but would also match names differing in those positions, - which is the looseness _span_name_matches exists to avoid. + test, because TraceQL has no glob operator: `rpc.command.*` is sent as + `name=~"rpc[.]command[.].*"`. + + A literal dot is written as the character class `[.]` rather than as `\\.`, + and that is not a style choice. TraceQL's string lexer rejects a backslash + escape it does not recognise, so the re.escape spelling this replaced -- + `name=~"rpc\\.command\\..*"` -- came back as HTTP 400, "invalid TraceQL + query: parse error at line 1, col 68: invalid char escape". `[.]` carries no + backslash, so nothing reaches the lexer that it can refuse, while still + meaning a literal dot to the regex engine behind it. Leaving the dots bare + would parse but match any character in those positions, which is the + looseness _span_name_matches exists to avoid. Args: expected_name: Span name or glob from expected_spans.json. @@ -427,7 +449,18 @@ def _traceql_name_predicate(expected_name: str) -> str: """ if "*" not in expected_name: return f'name="{expected_name}"' - pattern = "".join(".*" if ch == "*" else re.escape(ch) for ch in expected_name) + # Span names are lower_snake_case segments joined by dots, so `.` and `*` are + # the only characters here that mean anything to a regex engine. Anything + # else appearing would need its own handling rather than silent passthrough. + unexpected = set(expected_name) - set("abcdefghijklmnopqrstuvwxyz0123456789_.*") + if unexpected: + raise ValueError( + f"span name {expected_name!r} contains {sorted(unexpected)}, which " + "this predicate builder does not know how to escape for TraceQL" + ) + pattern = "".join( + ".*" if ch == "*" else "[.]" if ch == "." else ch for ch in expected_name + ) return f'name=~"{pattern}"' @@ -709,6 +742,25 @@ async def _check_attributes_on_first_trace( try: trace_id = traces[0].get("traceID", "") if not trace_id: + # Recorded rather than returned on silently. Returning with no + # result would drop this span's attribute contract out of the + # report and shrink the check total, so the surface would look + # smaller with nothing saying why. Reported the same way as a + # fetched trace holding no matching span, below: being unable to + # verify is itself the finding. The caller only reaches here for a + # span that declares required_attributes, so this adds no check + # where none was expected. + report.add( + CheckResult( + name=f"span.attrs.{span_name}", + category="span", + passed=False, + message=( + f"{span_name}: newest trace carried no traceID, cannot " + "verify its attributes" + ), + ) + ) return spans = await _tempo_get_trace(session, tempo_url, trace_id) await _validate_span_attributes_otlp(spans, span_def, report) @@ -2261,7 +2313,7 @@ async def run_validation( report = ValidationReport() report.start_time = time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()) - async with aiohttp.ClientSession() as session: + async with aiohttp.ClientSession(timeout=REQUEST_TIMEOUT) as session: await validate_spans(session, tempo_url, report) await validate_span_durations(session, tempo_url, report) await validate_metrics(session, prometheus_url, report) From 0d4d624622e7fa16529a2adf9b92d6588f3b4789 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:11:10 +0100 Subject: [PATCH 34/44] refactor(telemetry): inject trace context from the whole message The existing helper takes the TraceContext submessage, so every caller writes *msg.mutable_trace_context(). On a protobuf optional field that allocates the submessage and sets its has-bit before the helper runs, so a message ships an empty TraceContext whenever nothing is recorded and its peers take their has_trace_context() branch to extract nothing. Add an overload taking the parent message, which decides whether to create the submessage at all, and correct the header note that claimed the old helper was already free. --- src/xrpld/app/misc/NetworkOPs.cpp | 2 +- src/xrpld/telemetry/PropagationHelpers.h | 37 ++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 1dadd07503..f467473e54 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1954,7 +1954,7 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) // Inject the tx.process span's trace context so the // receiving node can link its tx.receive span as a child. if (e.span && *e.span) - telemetry::injectSpanContext(*e.span, *tx.mutable_trace_context()); + telemetry::injectSpanContext(*e.span, tx); // FIXME: This should be when we received it registry_.get().getOverlay().relay(e.transaction->getID(), tx, *toSkip); e.transaction->setBroadcast(); diff --git a/src/xrpld/telemetry/PropagationHelpers.h b/src/xrpld/telemetry/PropagationHelpers.h index 88ad887a34..7c188f544b 100644 --- a/src/xrpld/telemetry/PropagationHelpers.h +++ b/src/xrpld/telemetry/PropagationHelpers.h @@ -13,16 +13,22 @@ * +--- TraceBytes -----+ * | | * injectSpanContext(span, proto) + * ^ + * | delegates, once there is something to write + * injectSpanContext(span, message) <-- preferred entry point * - * @note When XRPL_ENABLE_TELEMETRY is disabled, getTraceBytes() returns - * {.valid=false}, so injectSpanContext becomes a no-op with zero overhead. + * @note Prefer the overload that takes the whole message. It is a true + * no-op when nothing is recorded, because it decides whether to create + * the TraceContext submessage at all. The overload taking a + * protocol::TraceContext& cannot be: its caller has already created the + * submessage and set its has-bit before this code runs. * * Usage: * @code * // Send side — inject from a SpanGuard reference: * protocol::TMTransaction tx; * // ... populate tx fields ... - * injectSpanContext(mySpanGuard, *tx.mutable_trace_context()); + * injectSpanContext(mySpanGuard, tx); * overlay.relay(txID, tx, toSkip); * @endcode * @@ -59,4 +65,29 @@ injectSpanContext(SpanGuard const& span, protocol::TraceContext& proto) proto.set_trace_flags(bytes.traceFlags); } +/** + * Inject an active span's trace context into a message that carries an + * optional TraceContext submessage. + * + * Takes the parent message rather than the submessage so the decision to + * create the submessage stays here. `mutable_trace_context()` on a protobuf + * optional field allocates the submessage and sets its has-bit, so a caller + * that passes `*msg.mutable_trace_context()` puts an empty TraceContext on + * the wire whenever nothing is recorded, and makes receiving peers take + * their has_trace_context() branch for nothing. + * + * @param span The span whose context to propagate; may be inactive. + * @param msg The message to populate. Untouched when nothing is recorded. + */ +template +void +injectSpanContext(SpanGuard const& span, Message& msg) +{ + auto const bytes = span.getTraceBytes(); + if (!bytes.valid) + return; + + injectSpanContext(span, *msg.mutable_trace_context()); +} + } // namespace xrpl::telemetry From cb92d59f11a6f13fcb3353f9b6ec7fba70aa3bdb Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:12:31 +0100 Subject: [PATCH 35/44] fix(telemetry): drop the inert insight prefix from the OTel path formatName() never reads prefix, so setting it here does nothing and the exported names are bare and lowercase. Leaving it invites queries written against xrpld_jobq_job_count, which match no series. The StatsD examples keep it, because that path does apply it to the name. --- OpenTelemetryPlan/06-implementation-phases.md | 2 +- docker/telemetry/xrpld-telemetry.cfg | 9 +++------ docs/telemetry-runbook.md | 5 +++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/OpenTelemetryPlan/06-implementation-phases.md b/OpenTelemetryPlan/06-implementation-phases.md index 52f811818a..afbe1099fa 100644 --- a/OpenTelemetryPlan/06-implementation-phases.md +++ b/OpenTelemetryPlan/06-implementation-phases.md @@ -510,7 +510,7 @@ graph LR # [insight] section — new "otel" server option [insight] server=otel # NEW: uses OTel OTLP metrics exporter -prefix=xrpld # metric name prefix (preserved) +# No prefix: it applies on the StatsD path only, not this one. # Endpoint and auth inherited from [telemetry] section: [telemetry] diff --git a/docker/telemetry/xrpld-telemetry.cfg b/docker/telemetry/xrpld-telemetry.cfg index 12de7576c3..7a459f9fd5 100644 --- a/docker/telemetry/xrpld-telemetry.cfg +++ b/docker/telemetry/xrpld-telemetry.cfg @@ -62,12 +62,9 @@ trace_ledger=1 # server selects the beast::insight backend. Only server=otel is usable with # this stack: the collector defines no StatsD receiver and 8125/udp is not # published, so server=statsd sends UDP to a port nothing listens on. -# endpoint and prefix are informational only. OTelCollector records on the -# global MeterProvider that [telemetry] configures, and formatName() does not -# apply the prefix, so metric names are bare and lowercase. -# Requires [telemetry] enabled=1: the MeterProvider these instruments record on -# is owned by the telemetry module, and without it they are discarded. +# server is the only key that changes behaviour here; endpoint is logged only. +# No prefix: formatName() ignores it, so names stay bare (jobq_job_count). +# Needs [telemetry] enabled=1, which owns the MeterProvider these record on. [insight] server=otel endpoint=http://localhost:4318/v1/metrics -prefix=xrpld diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index f9f447013d..406110ec2b 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -525,12 +525,13 @@ Add to `xrpld.cfg`: [insight] server=otel endpoint=http://localhost:4318/v1/metrics -prefix=xrpld ``` The `OTelCollector` implementation exports metrics via OTLP/HTTP to the same OTel Collector that receives traces. No separate StatsD receiver is needed. -> **Fallback**: Set `server=statsd` and `address=127.0.0.1:8125` to use the legacy StatsD UDP path. This requires re-enabling the `statsd` receiver in `otel-collector-config.yaml` and uncommenting port 8125 in `docker-compose.yml`. +Do not set `prefix` on this path. `formatName()` never applies it, so the setting is silently ignored and the exported names are bare and lowercase — `jobq_job_count`, not `xrpld_jobq_job_count`. Queries written against a prefixed name return no series. + +> **Fallback**: Set `server=statsd` and `address=127.0.0.1:8125` to use the legacy StatsD UDP path. This requires re-enabling the `statsd` receiver in `otel-collector-config.yaml` and uncommenting port 8125 in `docker-compose.yml`. On that path `prefix` **is** applied to the metric name, which is why the StatsD examples elsewhere in this document keep it. ### Metric Reference From fa23fb51ea6182904d330ec71d2dfbe4a1098750 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:13:46 +0100 Subject: [PATCH 36/44] fix(telemetry): drop insight keys the OTel collector discards prefix and service_instance_id are read and thrown away on this path, so the node's identity label comes from [telemetry] instead. The old comment claimed the insight copy was required or panels would be empty. --- docker/telemetry/xrpld-telemetry-mainnet.cfg | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docker/telemetry/xrpld-telemetry-mainnet.cfg b/docker/telemetry/xrpld-telemetry-mainnet.cfg index 720c9a1b6b..88a4a09fb9 100644 --- a/docker/telemetry/xrpld-telemetry-mainnet.cfg +++ b/docker/telemetry/xrpld-telemetry-mainnet.cfg @@ -135,15 +135,13 @@ data/logs/mainnet/debug.log # --- Insight (native OTel metrics via beast::insight) ----------------------- +# server is the only key that changes behaviour here. No prefix: formatName() +# ignores it, so names stay bare (jobq_job_count). service_instance_id is read +# and discarded (OTelCollector.cpp `(void)instanceId`); the label Prometheus +# shows comes from [telemetry] service_instance_id below. [insight] server=otel endpoint=http://localhost:4318/v1/metrics -prefix=xrpld -# Sets the OTel service.instance.id resource attribute, which Prometheus -# exposes as the `service_instance_id` label. Dashboards filter on it via the -# $node template variable, so without this every insight-backed panel is -# empty. Matches [telemetry] service_instance_id for a single node identity. -service_instance_id=xrpld-mainnet # --- OpenTelemetry tracing -------------------------------------------------- From ed3817968d37e58849a9018f25771d641d529c33 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:14:54 +0100 Subject: [PATCH 37/44] feat(telemetry): add recording utilities for telemetry-only state Sites that exist only to be reported currently spell their own gating: an #ifdef around a member, another around the clock read, another around the call that reports it. That puts preprocessor branches through business logic and leaves each class with a different member set per build. Add kEnabled, Stopwatch and Counter. Each holds real state when telemetry is compiled in and is an empty type with no-op methods when it is not, while the member stays declared in both builds so no class's API differs by configuration. Tests assert both configurations from one file, including that the compiled-out types are empty. --- include/xrpl/telemetry/Recording.h | 185 ++++++++++++++++++++++ src/tests/libxrpl/telemetry/Recording.cpp | 112 +++++++++++++ 2 files changed, 297 insertions(+) create mode 100644 include/xrpl/telemetry/Recording.h create mode 100644 src/tests/libxrpl/telemetry/Recording.cpp diff --git a/include/xrpl/telemetry/Recording.h b/include/xrpl/telemetry/Recording.h new file mode 100644 index 0000000000..ef388da3bb --- /dev/null +++ b/include/xrpl/telemetry/Recording.h @@ -0,0 +1,185 @@ +#pragma once + +/** + * Utilities for state and work that exist only to be recorded. + * + * Each type below holds real state when telemetry is compiled in and is an + * empty type with no-op methods when it is not. The member is declared in + * both configurations, so a class's member set and public API never differ + * between builds -- a difference that has previously made a test mock + * abstract. Declare members `[[no_unique_address]]` so the compiled-out + * form costs no storage. + * + * kEnabled ---- if constexpr ---- telemetry-only blocks + * | + * +-- Stopwatch (a clock read nobody reads when off) + * +-- Counter (an atomic nobody reads when off) + * +-- Mirror (a value kept only to be reported) + * + * @note A no-op method does NOT skip evaluation of its arguments. + * `counter.add(expensiveCount())` still calls `expensiveCount()` when + * telemetry is compiled out. Pass cheap values only; put expensive work + * inside `if constexpr (kEnabled)`. + * + * @note `if constexpr (kEnabled)` still type-checks its discarded branch in + * non-template code, so use it only where the block names no + * `opentelemetry::` type. That is the normal case, because SpanGuard exists + * to keep those types out of call sites. + * + * @note Thread safety: `Counter` is safe to update from any thread. + * `Stopwatch` and `Mirror` are not synchronized; guard them the same way + * you guard the state they sit beside. + * + * Usage: + * @code + * // Time a loop without a single #ifdef. + * telemetry::Stopwatch const timer; + * for (auto const& obj : objects) + * lookUp(obj); + * recordLookupMetrics(timer.elapsedUs()); // 0 when compiled out + * @endcode + * + * @code + * // A counter that disappears, along with its storage, when off. + * class Acquirer + * { + * [[no_unique_address]] telemetry::Counter<> timeouts_; + * public: + * void onTimeout() { timeouts_.add(); } + * }; + * @endcode + * + * @code + * // Edge case -- an expensive value still needs a block guard, + * // because arguments are evaluated even when the method is a no-op. + * if constexpr (telemetry::kEnabled) + * span.setAttribute( + * pathfind_span::attr::sourceAccount, redactAccount(account)); + * @endcode + */ + +#include +#include + +// Counter names std::atomic only when telemetry is compiled in, so guarding +// the include keeps misc-include-cleaner from seeing an unused one. +#ifdef XRPL_ENABLE_TELEMETRY +#include +#endif + +namespace xrpl::telemetry { + +#ifdef XRPL_ENABLE_TELEMETRY +/** + * True when telemetry code is compiled into this build. + */ +inline constexpr bool kEnabled = true; +#else +inline constexpr bool kEnabled = false; +#endif + +/** + * A monotonic elapsed-time measurement taken only for telemetry. + * + * Reads the clock on construction when telemetry is compiled in, and does + * nothing at all when it is not, so an untraced build performs no clock + * read on the measured path. + */ +class Stopwatch +{ +#ifdef XRPL_ENABLE_TELEMETRY + /** + * When the measurement started. + */ + std::chrono::steady_clock::time_point start_{std::chrono::steady_clock::now()}; +#endif + +public: + // These read start_ when telemetry is compiled in and touch no member + // when it is not, so clang-tidy asks for them to be static. Making them + // static would give the two builds different signatures. + // NOLINTBEGIN(readability-convert-member-functions-to-static) + + /** + * Begin the measurement again from now. + */ + void + restart() noexcept + { +#ifdef XRPL_ENABLE_TELEMETRY + start_ = std::chrono::steady_clock::now(); +#endif + } + + /** + * Microseconds since construction or the last restart(); 0 when off. + */ + [[nodiscard]] std::chrono::microseconds + elapsedUs() const noexcept + { +#ifdef XRPL_ENABLE_TELEMETRY + return std::chrono::duration_cast( + std::chrono::steady_clock::now() - start_); +#else + return std::chrono::microseconds{0}; +#endif + } + + // NOLINTEND(readability-convert-member-functions-to-static) +}; + +/** + * A monotonically increasing count kept only to be reported. + * + * @tparam T The counter's value type; defaults to std::uint64_t. + */ +template +class Counter +{ +#ifdef XRPL_ENABLE_TELEMETRY + /** + * The running count. Relaxed: only ever read for reporting. + */ + std::atomic value_{0}; +#endif + +public: + // These read value_ when telemetry is compiled in and touch no member + // when it is not, so clang-tidy asks for them to be static. Making them + // static would give the two builds different signatures. + // NOLINTBEGIN(readability-convert-member-functions-to-static) + + /** + * Add to the count. A no-op, with no storage, when off. + * + * @param n How much to add; defaults to 1. + */ + void + add(T const n = 1) noexcept + { +#ifdef XRPL_ENABLE_TELEMETRY + value_.fetch_add(n, std::memory_order_relaxed); +#else + (void)n; +#endif + } + + /** + * The current count. + * + * @return The accumulated count, or T{} when telemetry is compiled out. + */ + [[nodiscard]] T + load() const noexcept + { +#ifdef XRPL_ENABLE_TELEMETRY + return value_.load(std::memory_order_relaxed); +#else + return T{}; +#endif + } + + // NOLINTEND(readability-convert-member-functions-to-static) +}; + +} // namespace xrpl::telemetry diff --git a/src/tests/libxrpl/telemetry/Recording.cpp b/src/tests/libxrpl/telemetry/Recording.cpp new file mode 100644 index 0000000000..8c57909e51 --- /dev/null +++ b/src/tests/libxrpl/telemetry/Recording.cpp @@ -0,0 +1,112 @@ +/** + * Tests for the telemetry recording utilities. + * + * Compiled in every build. Each test asserts the compiled-in behaviour when + * kEnabled and the compiled-out behaviour otherwise, so both configurations + * are pinned by the same file rather than one of them going unasserted. + */ + +#include + +#include + +#include +#include +#include +#include + +using namespace xrpl; + +// kEnabled must agree with the macro that drives every branch below. Asserted +// at compile time so a mismatch cannot reach the runtime assertions. +#ifdef XRPL_ENABLE_TELEMETRY +static_assert(telemetry::kEnabled, "kEnabled must be true when the macro is defined"); +#else +static_assert(!telemetry::kEnabled, "kEnabled must be false when the macro is absent"); +#endif + +// The whole point of the compiled-out form is that it costs no storage. An +// empty type contributes nothing as a [[no_unique_address]] member. +TEST(Recording, compiled_out_types_are_empty) +{ + if constexpr (telemetry::kEnabled) + { + EXPECT_FALSE(std::is_empty_v>); + EXPECT_FALSE(std::is_empty_v); + } + else + { + EXPECT_TRUE(std::is_empty_v>); + EXPECT_TRUE(std::is_empty_v); + } +} + +// A fresh counter reads zero in both configurations -- the one value that must +// agree, since callers may report it unconditionally. +TEST(Recording, counter_starts_at_zero) +{ + telemetry::Counter<> counter; + EXPECT_EQ(counter.load(), 0U); +} + +// add() accumulates exactly when compiled in, and stays at zero when not. +TEST(Recording, counter_accumulates_only_when_compiled_in) +{ + telemetry::Counter<> counter; + counter.add(); + counter.add(4); + + if constexpr (telemetry::kEnabled) + EXPECT_EQ(counter.load(), 5U); + else + EXPECT_EQ(counter.load(), 0U); +} + +// The default template argument is std::uint64_t; an explicit type is honoured. +TEST(Recording, counter_honours_its_value_type) +{ + static_assert(std::is_same_v{}.load()), std::uint64_t>); + static_assert( + std::is_same_v{}.load()), std::uint32_t>); + + telemetry::Counter counter; + counter.add(7); + EXPECT_EQ(counter.load(), telemetry::kEnabled ? 7U : 0U); +} + +// A stopwatch measures a real interval when compiled in and reports exactly +// zero when not, so callers can report elapsedUs() unconditionally. +TEST(Recording, stopwatch_measures_only_when_compiled_in) +{ + telemetry::Stopwatch const timer; + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + auto const elapsed = timer.elapsedUs(); + + if constexpr (telemetry::kEnabled) + EXPECT_GE(elapsed, std::chrono::microseconds{1000}); + else + EXPECT_EQ(elapsed, std::chrono::microseconds{0}); +} + +// restart() moves the origin forward, so the interval measured after it is +// shorter than the one before it. +TEST(Recording, stopwatch_restart_resets_the_origin) +{ + telemetry::Stopwatch timer; + std::this_thread::sleep_for(std::chrono::milliseconds{4}); + auto const beforeRestart = timer.elapsedUs(); + + timer.restart(); + auto const afterRestart = timer.elapsedUs(); + + if constexpr (telemetry::kEnabled) + { + EXPECT_GE(beforeRestart, std::chrono::microseconds{2000}); + EXPECT_LT(afterRestart, beforeRestart); + } + else + { + EXPECT_EQ(beforeRestart, std::chrono::microseconds{0}); + EXPECT_EQ(afterRestart, std::chrono::microseconds{0}); + } +} From 017ef1b33e859942e4113921e71f5368db129d10 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:18:57 +0100 Subject: [PATCH 38/44] fix(telemetry): keep Counter's copy semantics the same in both builds std::atomic implicitly deletes copy and move, so a class holding a Counter is non-copyable when telemetry is compiled in. Compiled out, Counter was an empty type with no such member, which would have made that same owner freely copyable in one build only -- the per-configuration API difference these utilities exist to avoid. Declare copy and move deleted so both builds agree, and assert it unconditionally in the tests. --- include/xrpl/telemetry/Recording.h | 17 +++++++++++++++++ src/tests/libxrpl/telemetry/Recording.cpp | 14 ++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/include/xrpl/telemetry/Recording.h b/include/xrpl/telemetry/Recording.h index ef388da3bb..3ff233d245 100644 --- a/include/xrpl/telemetry/Recording.h +++ b/include/xrpl/telemetry/Recording.h @@ -144,6 +144,23 @@ class Counter #endif public: + /** + * Default-constructed at zero. + */ + Counter() = default; + + // Copying and moving are deleted so that a class holding a Counter has the + // same copy semantics in both builds. std::atomic deletes all four + // implicitly when telemetry is compiled in; without these declarations the + // compiled-out Counter would be an empty, freely copyable type, and its + // owner would silently become copyable in that build only. + Counter(Counter const&) = delete; + Counter& + operator=(Counter const&) = delete; + Counter(Counter&&) = delete; + Counter& + operator=(Counter&&) = delete; + // These read value_ when telemetry is compiled in and touch no member // when it is not, so clang-tidy asks for them to be static. Making them // static would give the two builds different signatures. diff --git a/src/tests/libxrpl/telemetry/Recording.cpp b/src/tests/libxrpl/telemetry/Recording.cpp index 8c57909e51..7715e13cef 100644 --- a/src/tests/libxrpl/telemetry/Recording.cpp +++ b/src/tests/libxrpl/telemetry/Recording.cpp @@ -25,6 +25,20 @@ static_assert(telemetry::kEnabled, "kEnabled must be true when the macro is defi static_assert(!telemetry::kEnabled, "kEnabled must be false when the macro is absent"); #endif +// Counter's copy semantics must NOT depend on the configuration. When telemetry +// is compiled in, the std::atomic member deletes all four implicitly; when it is +// compiled out, Counter declares them deleted itself. Without that, an owning +// class would be non-copyable in one build and copyable in the other. Asserted +// unconditionally, because the whole point is that both builds agree. +static_assert(!std::is_copy_constructible_v>); +static_assert(!std::is_copy_assignable_v>); +static_assert(!std::is_move_constructible_v>); +static_assert(!std::is_move_assignable_v>); + +// Stopwatch and Mirror hold ordinary values, so they stay copyable in both +// builds; only Counter needed the explicit deletions above. +static_assert(std::is_copy_constructible_v); + // The whole point of the compiled-out form is that it costs no storage. An // empty type contributes nothing as a [[no_unique_address]] member. TEST(Recording, compiled_out_types_are_empty) From b5a453d35133c6ca9591ac67550d5f43495d7f26 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:24:19 +0100 Subject: [PATCH 39/44] refactor(overlay): time the get-object lookup with a Stopwatch Four preprocessor branches around one function -- one for the start timestamp, one for the elapsed computation, one for the call that reports it, and one around the reporting method itself -- become none. The clock is still not read when telemetry is compiled out, because Stopwatch holds no state in that build. The reporting method is now compiled in both builds so its call site needs no guard. Every statement in its body is an XRPL_METRIC_* argument, and those macros discard their arguments when telemetry is off, so the body costs nothing there. Its four arguments are all values the request already computed. --- src/xrpld/overlay/detail/PeerImp.cpp | 27 +++++++++++++++------------ src/xrpld/overlay/detail/PeerImp.h | 6 ++---- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index c617bbd613..a552f331e0 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -76,6 +76,7 @@ #include #include #include +#include #include #include #include @@ -2863,13 +2864,12 @@ PeerImp::processGetObjectByHash(std::shared_ptr con int const requested = packet.objects_size(); int const iterLimit = std::min(requested, tuning::kHardMaxReplyNodes); -#ifdef XRPL_ENABLE_TELEMETRY // Time the whole loop once, not each iteration: the loop can run up to // kHardMaxReplyNodes times, so per-iteration clock reads would cost more // than the lookups they measure. Both clock reads serve only the metric - // recorded below, so neither happens when telemetry is compiled out. - auto const lookupStart = std::chrono::steady_clock::now(); -#endif + // recorded below, so neither happens when telemetry is compiled out -- + // Stopwatch holds no state in that build. + telemetry::Stopwatch const lookupTimer; for (int i = 0; i < iterLimit; ++i) { @@ -2895,12 +2895,9 @@ PeerImp::processGetObjectByHash(std::shared_ptr con newObj.set_ledgerseq(obj.ledgerseq()); } -#ifdef XRPL_ENABLE_TELEMETRY // Measured here rather than at the call below, which would fold the fee // computation and charge() into the reported lookup latency. - auto const lookupElapsed = std::chrono::duration_cast( - std::chrono::steady_clock::now() - lookupStart); -#endif + auto const lookupElapsed = lookupTimer.elapsedUs(); // Apply work-proportional charge. `charge()` posts the disconnect // step (if any) back to strand_, so it is safe to call from this @@ -2915,15 +2912,20 @@ PeerImp::processGetObjectByHash(std::shared_ptr con resource::Charge const fee = computeGetObjectByHashFee(requested, reply.objects_size()); charge(fee, "processed get object by hash request"); -#ifdef XRPL_ENABLE_TELEMETRY + // Called unconditionally: every statement in the body is an XRPL_METRIC_* + // argument, and those macros discard their arguments when telemetry is + // compiled out. All four values here are already computed for the request + // itself, so passing them costs nothing. recordGetObjectMetrics(requested, reply.objects_size(), lookupElapsed, fee); -#endif JLOG(pJournal_.trace()) << "GetObj: " << reply.objects_size() << " of " << requested; send(std::make_shared(reply, protocol::mtGET_OBJECTS)); } -#ifdef XRPL_ENABLE_TELEMETRY +// Reads app_ through the metric macros when telemetry is compiled in and +// touches no member when it is not, so clang-tidy asks for it to be static. +// Making it static would give the two builds different signatures. +// NOLINTBEGIN(readability-convert-member-functions-to-static) void PeerImp::recordGetObjectMetrics( int const requested, @@ -2969,7 +2971,8 @@ PeerImp::recordGetObjectMetrics( static_cast(std::max(0, requested - found)), {{kLabelResult, std::string(kResultMiss)}}); } -#endif // XRPL_ENABLE_TELEMETRY + +// NOLINTEND(readability-convert-member-functions-to-static) void PeerImp::onMessage(std::shared_ptr const& m) diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index fc4fe690ac..2b0ca66371 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -701,7 +701,6 @@ private: std::shared_ptr const& m, std::vector nodeIDs); -#ifdef XRPL_ENABLE_TELEMETRY /** * Record the OTel metrics for one completed `TMGetObjectByHash` request. * @@ -713,8 +712,8 @@ private: * Records `getobject_request_objects`, `getobject_lookup_us`, * `getobject_charge`, and both label values of * `getobject_lookups_total`. Every statement is an `XRPL_METRIC_*` record, - * so the whole method — and its one call site — is compiled out when - * telemetry is disabled rather than left as an empty function. + * and those macros discard their arguments when telemetry is disabled, so + * the body costs nothing in that build and the call site needs no guard. * * @param requested Objects the peer asked for (`objects_size()`). * @param found Objects returned, i.e. the reply's object count. @@ -730,7 +729,6 @@ private: int const found, std::chrono::microseconds const lookupElapsed, resource::Charge const& fee); -#endif // XRPL_ENABLE_TELEMETRY protected: // Kept `protected` so test subclasses (see From 85b42d8bf000d32fb12d6ec01184f8e6a0b17f17 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:25:09 +0100 Subject: [PATCH 40/44] fix(consensus): send no trace context when nothing is being traced Both broadcast paths passed *msg.mutable_trace_context() to the injector, which allocates the submessage and sets its has-bit before the injector can decide there is nothing to write. A compile-time guard covered the telemetry-off build, but a node with telemetry compiled in and no active span -- a disabled category, telemetry disabled by config, or a round that is not being traced -- still broadcast an empty TraceContext to every peer, and every peer took its has_trace_context() branch to extract nothing. Add SpanGuard::hasCurrentContext(), a predicate that tests the same two conditions the injector bails out on without allocating, and an injectCurrentContext(message) helper that uses it to decide whether to create the submessage at all. Both consensus call sites now call the helper unguarded. --- include/xrpl/telemetry/SpanGuard.h | 23 +++++++++++++++ src/libxrpl/telemetry/SpanGuard.cpp | 16 ++++++++++ src/xrpld/app/consensus/RCLConsensus.cpp | 24 +++++++-------- src/xrpld/telemetry/PropagationHelpers.h | 37 ++++++++++++++++++++++-- 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index ef096cb28a..95c471f677 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -482,6 +482,23 @@ public: [[nodiscard]] TraceBytes getTraceBytes() const; + /** + * Report whether this thread has a live span context to propagate. + * + * Lets a caller decide whether to create an optional protobuf + * submessage before calling injectCurrentContextToProtobuf(), which + * writes nothing when no span is active. Tests exactly the two + * conditions that injector checks and allocates nothing, so it is cheap + * enough to call on every broadcast. threadLocalContext() is not + * a substitute: it builds a shared SpanContext::Impl before validity + * can be tested. + * + * @return true when a span with a valid context is active on this + * thread. + */ + [[nodiscard]] static bool + hasCurrentContext() noexcept; + /** * Inject the calling thread's currently-active OTel context into a * protobuf TraceContext message for cross-node propagation. @@ -1075,6 +1092,12 @@ public: return {}; } + [[nodiscard]] static bool + hasCurrentContext() noexcept + { + return false; + } + static void injectCurrentContextToProtobuf(protocol::TraceContext&) { diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 52d1528dc7..b71a80bc0d 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -474,6 +475,21 @@ SpanGuard::getTraceBytes() const return result; } +bool +SpanGuard::hasCurrentContext() noexcept +{ + // Read the active span straight out of the runtime context. GetSpan() + // would be shorter but heap-allocates a DefaultSpan whenever the context + // holds no span, which is the case this predicate exists to keep free. + // The two conditions below are the ones injectToProtobuf() returns early + // on, so a true result means that injector will write all three fields. + auto const ctx = opentelemetry::context::RuntimeContext::GetCurrent(); + auto const value = ctx.GetValue(otel_trace::kSpanKey); + auto const* const span = + opentelemetry::nostd::get_if>(&value); + return span != nullptr && *span && (*span)->GetContext().IsValid(); +} + void SpanGuard::injectCurrentContextToProtobuf(protocol::TraceContext& proto) { diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index b18ffa333b..a0e2f6f6d3 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -279,18 +280,15 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) app_.getHashRouter().addSuppression(suppression); -#ifdef XRPL_ENABLE_TELEMETRY // Inject the current thread's active span context (e.g. the consensus // round span) so receiving peers can link their proposal.receive span // as a child of this trace. // - // Guarded rather than relying on the injector being a no-op: mutable_ on an - // optional submessage allocates it and sets its has-bit, so calling this - // unconditionally would put an empty TraceContext on the wire in every - // proposal a node without telemetry broadcasts, and make its peers take - // their has_trace_context() branch for nothing. - telemetry::SpanGuard::injectCurrentContextToProtobuf(*prop.mutable_trace_context()); -#endif + // The helper injects only when a span is actually active, so a node with + // telemetry compiled out, disabled by config, or simply not tracing this + // round sends no TraceContext at all rather than an empty one that makes + // every peer take its has_trace_context() branch for nothing. + telemetry::injectCurrentContext(prop); app_.getOverlay().broadcast(prop); } @@ -1113,12 +1111,10 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, // Downstream consumers treat it as advisory only. A signature-covered // trace context is a possible future enhancement. // - // Guarded for the same reason as the proposal path: mutable_ allocates the - // optional submessage and sets its has-bit, so an unguarded call would put - // an empty TraceContext in every validation a node without telemetry sends. -#ifdef XRPL_ENABLE_TELEMETRY - telemetry::SpanGuard::injectCurrentContextToProtobuf(*val.mutable_trace_context()); -#endif + // As on the proposal path, the helper injects only when a span is actually + // active, so a node that is not tracing sends no TraceContext at all + // rather than an empty one. + telemetry::injectCurrentContext(val); app_.getOverlay().broadcast(val); // Publish to all our subscribers: diff --git a/src/xrpld/telemetry/PropagationHelpers.h b/src/xrpld/telemetry/PropagationHelpers.h index e7b7880bbd..0497ef0826 100644 --- a/src/xrpld/telemetry/PropagationHelpers.h +++ b/src/xrpld/telemetry/PropagationHelpers.h @@ -17,8 +17,15 @@ * | delegates, once there is something to write * injectSpanContext(span, message) <-- preferred entry point * - * @note Prefer the overload that takes the whole message. It is a true - * no-op when nothing is recorded, because it decides whether to create + * SpanGuard::hasCurrentContext() + * | gates + * v + * injectCurrentContext(message) <-- no span handle needed + * | + * +--> SpanGuard::injectCurrentContextToProtobuf(proto) + * + * @note Prefer the helpers that take the whole message. They are true + * no-ops when nothing is recorded, because they decide whether to create * the TraceContext submessage at all. The overload taking a * protocol::TraceContext& cannot be: its caller has already created the * submessage and set its has-bit before this code runs. @@ -91,4 +98,30 @@ injectSpanContext(SpanGuard const& span, Message& msg) injectSpanContext(span, *msg.mutable_trace_context()); } +/** + * Inject this thread's currently active span context into a message that + * carries an optional TraceContext submessage. + * + * For senders that have no SpanGuard in hand and want whatever span is + * ambient on the calling thread, such as the consensus round span. + * + * Tests for a live context before touching the message, so a build with + * telemetry compiled out, disabled by config, or simply not tracing this + * round sends no TraceContext submessage at all. Calling + * mutable_trace_context() unconditionally would create it and set its + * has-bit, putting an empty TraceContext on the wire and making every + * receiving peer take its has_trace_context() branch for nothing. + * + * @param msg The message to populate. Untouched when no span is active. + */ +template +void +injectCurrentContext(Message& msg) +{ + if (!SpanGuard::hasCurrentContext()) + return; + + SpanGuard::injectCurrentContextToProtobuf(*msg.mutable_trace_context()); +} + } // namespace xrpl::telemetry From e27d877af900fb90415a6215ae55cb1871df2f5a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:26:51 +0100 Subject: [PATCH 41/44] docs(telemetry): state what the recording types actually cost The header told callers to declare members [[no_unique_address]]. That attribute has no other use in this repo, and MSVC ignores the standard spelling for ABI compatibility, so following the advice would have been a portability wart for one byte per member. Say instead that the compiled-out member collapses to padding, and that what these types buy is work not being done. --- include/xrpl/telemetry/Recording.h | 9 ++++++--- src/tests/libxrpl/telemetry/Recording.cpp | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/include/xrpl/telemetry/Recording.h b/include/xrpl/telemetry/Recording.h index 3ff233d245..ecb2d9767f 100644 --- a/include/xrpl/telemetry/Recording.h +++ b/include/xrpl/telemetry/Recording.h @@ -7,8 +7,11 @@ * empty type with no-op methods when it is not. The member is declared in * both configurations, so a class's member set and public API never differ * between builds -- a difference that has previously made a test mock - * abstract. Declare members `[[no_unique_address]]` so the compiled-out - * form costs no storage. + * abstract. The compiled-out forms are empty types, so such a member costs a + * byte of padding rather than nothing. `[[no_unique_address]]` would remove + * even that, but MSVC ignores the standard spelling for ABI compatibility, so + * it is deliberately not used. What these types buy is work not being done, + * not a smaller struct. * * kEnabled ---- if constexpr ---- telemetry-only blocks * | @@ -43,7 +46,7 @@ * // A counter that disappears, along with its storage, when off. * class Acquirer * { - * [[no_unique_address]] telemetry::Counter<> timeouts_; + * telemetry::Counter<> timeouts_; * public: * void onTimeout() { timeouts_.add(); } * }; diff --git a/src/tests/libxrpl/telemetry/Recording.cpp b/src/tests/libxrpl/telemetry/Recording.cpp index 7715e13cef..c667d0dd1a 100644 --- a/src/tests/libxrpl/telemetry/Recording.cpp +++ b/src/tests/libxrpl/telemetry/Recording.cpp @@ -39,8 +39,9 @@ static_assert(!std::is_move_assignable_v>); // builds; only Counter needed the explicit deletions above. static_assert(std::is_copy_constructible_v); -// The whole point of the compiled-out form is that it costs no storage. An -// empty type contributes nothing as a [[no_unique_address]] member. +// The compiled-out forms must be empty types. That is what lets an owning class +// declare the member unconditionally: the storage collapses to padding, and the +// work disappears entirely. TEST(Recording, compiled_out_types_are_empty) { if constexpr (telemetry::kEnabled) From e0a0986d319cf2f2ad9dde1ff6875d96295fac06 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:37:05 +0100 Subject: [PATCH 42/44] refactor(ledger): hold acquire counts in Counter, not guarded atomics The nine counters have no reader outside telemetry, so the record calls were wrapped in preprocessor branches at six sites. Holding them in Counter makes the storage and the increments disappear together, so the call sites read as ordinary code. The two deferral sites keep a compile-time block, because their argument is a string compare that a no-op add would still evaluate. The unit tests now assert both configurations: every expectation of a non-zero count has a mirror expectation of zero, so neither build is left unasserted. --- src/tests/libxrpl/ledger/AcquireStats.cpp | 153 +++++++++++++++--- src/xrpld/app/ledger/AcquireStats.h | 75 +++++---- src/xrpld/app/ledger/detail/InboundLedger.cpp | 19 +-- .../app/ledger/detail/InboundLedgers.cpp | 12 +- .../app/ledger/detail/TimeoutCounter.cpp | 31 ++-- 5 files changed, 185 insertions(+), 105 deletions(-) diff --git a/src/tests/libxrpl/ledger/AcquireStats.cpp b/src/tests/libxrpl/ledger/AcquireStats.cpp index 4836c23fc5..54d09b33ba 100644 --- a/src/tests/libxrpl/ledger/AcquireStats.cpp +++ b/src/tests/libxrpl/ledger/AcquireStats.cpp @@ -10,10 +10,19 @@ * that a stalled run and a healthy run produce different, exact readings. * * AcquireStats is header-only, so nothing from xrpld needs to be linked here. + * + * The counts are held in telemetry::Counter, which carries no storage and + * records nothing when telemetry is compiled out. Every expectation on a + * non-zero count therefore has a mirror expectation of zero, so this file + * pins both configurations rather than leaving one of them unasserted. + * Expectations of zero that hold either way -- a deferral not disturbing the + * timeout counter, for instance -- are asserted unconditionally. */ #include +#include + #include #include @@ -23,10 +32,13 @@ namespace { using xrpl::AcquireStats; +namespace telemetry = xrpl::telemetry; /** * A fresh instance reads zero on every counter, so a later test can attribute - * every increment to its own call rather than to construction. + * every increment to its own call rather than to construction. The reading is + * the same in both configurations, which is what lets a caller report these + * accessors without knowing which build it is in. */ TEST(AcquireStatsTest, StartsAtZero) { @@ -54,27 +66,67 @@ TEST(AcquireStatsTest, CountersAdvanceIndependently) stats.recordDeferral(); stats.recordDeferral(); stats.recordDeferral(); - EXPECT_EQ(stats.getDeferrals(), 3u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getDeferrals(), 3u); + } + else + { + EXPECT_EQ(stats.getDeferrals(), 0u); + } + // Zero either way: a deferral must not reach the other counters. EXPECT_EQ(stats.getTimeouts(), 0u); EXPECT_EQ(stats.getGiveUps(), 0u); stats.recordTimeout(); - EXPECT_EQ(stats.getTimeouts(), 1u); - EXPECT_EQ(stats.getDeferrals(), 3u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getTimeouts(), 1u); + EXPECT_EQ(stats.getDeferrals(), 3u); + } + else + { + EXPECT_EQ(stats.getTimeouts(), 0u); + EXPECT_EQ(stats.getDeferrals(), 0u); + } stats.recordGiveUp(); - EXPECT_EQ(stats.getGiveUps(), 1u); - EXPECT_EQ(stats.getTimeouts(), 1u); - EXPECT_EQ(stats.getDeferrals(), 3u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getGiveUps(), 1u); + EXPECT_EQ(stats.getTimeouts(), 1u); + EXPECT_EQ(stats.getDeferrals(), 3u); + } + else + { + EXPECT_EQ(stats.getGiveUps(), 0u); + EXPECT_EQ(stats.getTimeouts(), 0u); + EXPECT_EQ(stats.getDeferrals(), 0u); + } stats.recordCompletion(); stats.recordCompletion(); - EXPECT_EQ(stats.getCompletions(), 2u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getCompletions(), 2u); + } + else + { + EXPECT_EQ(stats.getCompletions(), 0u); + } EXPECT_EQ(stats.getSweepEvictions(), 0u); stats.recordSweepEviction(); - EXPECT_EQ(stats.getSweepEvictions(), 1u); - EXPECT_EQ(stats.getCompletions(), 2u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getSweepEvictions(), 1u); + EXPECT_EQ(stats.getCompletions(), 2u); + } + else + { + EXPECT_EQ(stats.getSweepEvictions(), 0u); + EXPECT_EQ(stats.getCompletions(), 0u); + } // Nothing above records an abort, so both abort counters stay at zero. EXPECT_EQ(stats.getAborts(), 0u); @@ -92,12 +144,28 @@ TEST(AcquireStatsTest, AbortDistinguishesPartialWork) AcquireStats stats; stats.recordAbort(false); - EXPECT_EQ(stats.getAborts(), 1u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getAborts(), 1u); + } + else + { + EXPECT_EQ(stats.getAborts(), 0u); + } + // Zero either way: a cheap abort must not reach the partial-work counter. EXPECT_EQ(stats.getAbortsWithPartialWork(), 0u); stats.recordAbort(true); - EXPECT_EQ(stats.getAborts(), 2u); - EXPECT_EQ(stats.getAbortsWithPartialWork(), 1u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stats.getAborts(), 2u); + EXPECT_EQ(stats.getAbortsWithPartialWork(), 1u); + } + else + { + EXPECT_EQ(stats.getAborts(), 0u); + EXPECT_EQ(stats.getAbortsWithPartialWork(), 0u); + } // An abort never counts as a completion or a give-up. EXPECT_EQ(stats.getCompletions(), 0u); @@ -120,13 +188,29 @@ TEST(AcquireStatsTest, StalledShapeIsDistinguishable) stalled.recordAbort(true); } - EXPECT_EQ(stalled.getDeferrals(), 1000u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(stalled.getDeferrals(), 1000u); + EXPECT_EQ(stalled.getSweepEvictions(), 5u); + EXPECT_EQ(stalled.getAborts(), 5u); + EXPECT_EQ(stalled.getAbortsWithPartialWork(), 5u); + } + else + { + // With telemetry compiled out every counter reads zero, so the stalled + // shape is not readable at all. That is the intended reading, not a + // healthy one. + EXPECT_EQ(stalled.getDeferrals(), 0u); + EXPECT_EQ(stalled.getSweepEvictions(), 0u); + EXPECT_EQ(stalled.getAborts(), 0u); + EXPECT_EQ(stalled.getAbortsWithPartialWork(), 0u); + } + + // Zero either way, and the point of the test: no timeout accrued, so the + // give-up path cannot fire. EXPECT_EQ(stalled.getTimeouts(), 0u); EXPECT_EQ(stalled.getGiveUps(), 0u); EXPECT_EQ(stalled.getCompletions(), 0u); - EXPECT_EQ(stalled.getSweepEvictions(), 5u); - EXPECT_EQ(stalled.getAborts(), 5u); - EXPECT_EQ(stalled.getAbortsWithPartialWork(), 5u); } /** @@ -145,10 +229,22 @@ TEST(AcquireStatsTest, HealthyShapeIsDistinguishable) for (int i = 0; i < 27; ++i) healthy.recordCompletion(); - EXPECT_EQ(healthy.getDeferrals(), 10u); - EXPECT_EQ(healthy.getTimeouts(), 7u); - EXPECT_EQ(healthy.getGiveUps(), 1u); - EXPECT_EQ(healthy.getCompletions(), 27u); + if constexpr (telemetry::kEnabled) + { + EXPECT_EQ(healthy.getDeferrals(), 10u); + EXPECT_EQ(healthy.getTimeouts(), 7u); + EXPECT_EQ(healthy.getGiveUps(), 1u); + EXPECT_EQ(healthy.getCompletions(), 27u); + } + else + { + EXPECT_EQ(healthy.getDeferrals(), 0u); + EXPECT_EQ(healthy.getTimeouts(), 0u); + EXPECT_EQ(healthy.getGiveUps(), 0u); + EXPECT_EQ(healthy.getCompletions(), 0u); + } + + // Zero either way: nothing above sweeps or aborts. EXPECT_EQ(healthy.getSweepEvictions(), 0u); EXPECT_EQ(healthy.getAborts(), 0u); EXPECT_EQ(healthy.getAbortsWithPartialWork(), 0u); @@ -184,9 +280,18 @@ TEST(AcquireStatsTest, ConcurrentRecordingLosesNothing) for (auto& th : threads) th.join(); - // 4 threads * 1000 iterations, stated independently of the loop bounds. - EXPECT_EQ(stats.getDeferrals(), std::uint64_t{4000}); - EXPECT_EQ(stats.getCompletions(), std::uint64_t{4000}); + if constexpr (telemetry::kEnabled) + { + // 4 threads * 1000 iterations, stated independently of the loop bounds. + EXPECT_EQ(stats.getDeferrals(), std::uint64_t{4000}); + EXPECT_EQ(stats.getCompletions(), std::uint64_t{4000}); + } + else + { + // Nothing was recorded, so concurrent calls also have nothing to lose. + EXPECT_EQ(stats.getDeferrals(), std::uint64_t{0}); + EXPECT_EQ(stats.getCompletions(), std::uint64_t{0}); + } // The threads record only those two events, so the rest stay at zero. EXPECT_EQ(stats.getTimeouts(), 0u); diff --git a/src/xrpld/app/ledger/AcquireStats.h b/src/xrpld/app/ledger/AcquireStats.h index 7ed93e85c7..27d5f219b5 100644 --- a/src/xrpld/app/ledger/AcquireStats.h +++ b/src/xrpld/app/ledger/AcquireStats.h @@ -1,6 +1,7 @@ #pragma once -#include +#include + #include namespace xrpl { @@ -34,8 +35,8 @@ namespace xrpl { * \--- recordTimeout() ------->| | * | | * InboundLedger ---- recordGiveUp() -------->| AcquireStats | - * |--- recordCompletion() ---->| (7 atomic | - * \--- recordAbort() --------->| counters) | + * |--- recordCompletion() ---->| (9 counters) | + * \--- recordAbort() --------->| | * | | * InboundLedgers ---- recordSweepEviction() ->+-----------------+ * | @@ -71,14 +72,18 @@ namespace xrpl { * // the same as healthy. Check completions before concluding anything. * @endcode * - * @note Thread-safe. Every counter is an independent relaxed atomic, so any - * number of threads may record concurrently without losing an - * increment. Because the counters are independent, no read across two - * of them is a consistent snapshot. Compare rates over an interval - * rather than instantaneous values. + * @note Thread-safe. Every counter is independent, so any number of threads + * may record concurrently without losing an increment. Because the + * counters are independent, no read across two of them is a consistent + * snapshot. Compare rates over an interval rather than instantaneous + * values. * @note All counters are monotonic for the life of the process and are never * reset, so a reader differences successive samples to get a rate. They * saturate only on 64-bit wraparound, which is unreachable in practice. + * @note The counts exist only to be reported, so they are held in + * telemetry::Counter. In a build with telemetry compiled out the + * counters carry no storage, recording is a no-op, and every accessor + * returns 0. The public API is the same in both builds. * @note Completions count acquisitions that ended successfully, whether the * data came from peers or was already present locally. They do not * count an acquisition that is still in flight. @@ -95,9 +100,9 @@ public: void recordDeferral(bool ledgerAcquisition = false) { - deferrals_.fetch_add(1, std::memory_order_relaxed); + deferrals_.add(); if (ledgerAcquisition) - ledgerDeferrals_.fetch_add(1, std::memory_order_relaxed); + ledgerDeferrals_.add(); } /** @@ -109,9 +114,9 @@ public: void recordTimeout(bool ledgerAcquisition = false) { - timeouts_.fetch_add(1, std::memory_order_relaxed); + timeouts_.add(); if (ledgerAcquisition) - ledgerTimeouts_.fetch_add(1, std::memory_order_relaxed); + ledgerTimeouts_.add(); } /** @@ -121,7 +126,7 @@ public: void recordGiveUp() { - giveUps_.fetch_add(1, std::memory_order_relaxed); + giveUps_.add(); } /** @@ -134,9 +139,9 @@ public: void recordAbort(bool hadPartialWork) { - aborts_.fetch_add(1, std::memory_order_relaxed); + aborts_.add(); if (hadPartialWork) - abortsWithPartialWork_.fetch_add(1, std::memory_order_relaxed); + abortsWithPartialWork_.add(); } /** @@ -145,7 +150,7 @@ public: void recordCompletion() { - completions_.fetch_add(1, std::memory_order_relaxed); + completions_.add(); } /** @@ -154,7 +159,7 @@ public: void recordSweepEviction() { - sweepEvictions_.fetch_add(1, std::memory_order_relaxed); + sweepEvictions_.add(); } /** @@ -163,7 +168,7 @@ public: [[nodiscard]] std::uint64_t getDeferrals() const { - return deferrals_.load(std::memory_order_relaxed); + return deferrals_.load(); } /** @@ -172,7 +177,7 @@ public: [[nodiscard]] std::uint64_t getTimeouts() const { - return timeouts_.load(std::memory_order_relaxed); + return timeouts_.load(); } /** @@ -185,7 +190,7 @@ public: [[nodiscard]] std::uint64_t getLedgerDeferrals() const { - return ledgerDeferrals_.load(std::memory_order_relaxed); + return ledgerDeferrals_.load(); } /** @@ -198,7 +203,7 @@ public: [[nodiscard]] std::uint64_t getLedgerTimeouts() const { - return ledgerTimeouts_.load(std::memory_order_relaxed); + return ledgerTimeouts_.load(); } /** @@ -207,7 +212,7 @@ public: [[nodiscard]] std::uint64_t getGiveUps() const { - return giveUps_.load(std::memory_order_relaxed); + return giveUps_.load(); } /** @@ -216,7 +221,7 @@ public: [[nodiscard]] std::uint64_t getAborts() const { - return aborts_.load(std::memory_order_relaxed); + return aborts_.load(); } /** @@ -227,7 +232,7 @@ public: [[nodiscard]] std::uint64_t getAbortsWithPartialWork() const { - return abortsWithPartialWork_.load(std::memory_order_relaxed); + return abortsWithPartialWork_.load(); } /** @@ -236,7 +241,7 @@ public: [[nodiscard]] std::uint64_t getCompletions() const { - return completions_.load(std::memory_order_relaxed); + return completions_.load(); } /** @@ -245,54 +250,54 @@ public: [[nodiscard]] std::uint64_t getSweepEvictions() const { - return sweepEvictions_.load(std::memory_order_relaxed); + return sweepEvictions_.load(); } private: /** * Timer jobs skipped because the job lane was at its limit. */ - std::atomic deferrals_{0}; + telemetry::Counter<> deferrals_; /** * Deferrals attributable to ledger acquisition alone. */ - std::atomic ledgerDeferrals_{0}; + telemetry::Counter<> ledgerDeferrals_; /** * Timeouts attributable to ledger acquisition alone. */ - std::atomic ledgerTimeouts_{0}; + telemetry::Counter<> ledgerTimeouts_; /** * Timer bodies that ran and advanced the retry count. */ - std::atomic timeouts_{0}; + telemetry::Counter<> timeouts_; /** * Acquisitions that exhausted their retry budget. */ - std::atomic giveUps_{0}; + telemetry::Counter<> giveUps_; /** * Acquisitions destroyed before finishing. */ - std::atomic aborts_{0}; + telemetry::Counter<> aborts_; /** * Aborts that discarded a partly built map. */ - std::atomic abortsWithPartialWork_{0}; + telemetry::Counter<> abortsWithPartialWork_; /** * Acquisitions that finished successfully. */ - std::atomic completions_{0}; + telemetry::Counter<> completions_; /** * Idle acquisitions evicted by the sweep. */ - std::atomic sweepEvictions_{0}; + telemetry::Counter<> sweepEvictions_; }; } // namespace xrpl diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index ef2e8d3a34..fbe129b37e 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -12,12 +13,6 @@ #include #include -#ifdef XRPL_ENABLE_TELEMETRY -// The three guarded recording calls below are the only things here that name -// AcquireStats, so without telemetry the include has no user. -#include -#endif - #include #include #include @@ -228,14 +223,10 @@ InboundLedger::~InboundLedger() } if (!isDone()) { -#ifdef XRPL_ENABLE_TELEMETRY // Partial work means a map was partly built and is now discarded, so // the whole acquisition has to start over. That is the expensive case, // so it is counted apart from a cheap abort that had nothing yet. - // - // Guarded because the acquire metrics are the only reader. app_.getAcquireStats().recordAbort(haveHeader_ || haveState_ || haveTransactions_); -#endif // Mark the span so an abandoned acquisition is distinguishable from one // that was still in flight when the trace was read. Without this the @@ -431,10 +422,7 @@ InboundLedger::onTimer(bool wasProgress, ScopedLockType&) if (timeouts_ > kLedgerTimeoutRetriesMax) { -#ifdef XRPL_ENABLE_TELEMETRY - // The acquire metrics are the only reader of this counter. app_.getAcquireStats().recordGiveUp(); -#endif if (seq_ != 0) { JLOG(journal_.warn()) << timeouts_ << " timeouts for ledger " << seq_; @@ -508,12 +496,7 @@ InboundLedger::recordCompletionOnce() return; completionCounted_ = true; -#ifdef XRPL_ENABLE_TELEMETRY - // The acquire metrics are the only reader of this counter. The latch above - // is left running: it is two branches once per acquisition, and gating it - // would leave an unused member behind. app_.getAcquireStats().recordCompletion(); -#endif } void diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 2bcb0a0706..96b986500e 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -1,17 +1,12 @@ #include +#include #include #include #include #include #include -#ifdef XRPL_ENABLE_TELEMETRY -// The guarded recording call below is the only thing here that names -// AcquireStats, so without telemetry the include has no user. -#include -#endif - #include #include #include @@ -399,13 +394,10 @@ public: else if ((la + std::chrono::minutes(1)) < start) { stuffToSweep.push_back(it->second); -#ifdef XRPL_ENABLE_TELEMETRY // An eviction here discards whatever the acquisition had // built, so the work restarts. Counted to tell that apart - // from an acquisition that ended on its own. Guarded - // because the acquire metrics are the only reader. + // from an acquisition that ended on its own. app_.getAcquireStats().recordSweepEviction(); -#endif // shouldn't cause the actual final delete // since we are holding a reference in the vector. it = ledgers_.erase(it); diff --git a/src/xrpld/app/ledger/detail/TimeoutCounter.cpp b/src/xrpld/app/ledger/detail/TimeoutCounter.cpp index 5206f184be..5992bfe8f7 100644 --- a/src/xrpld/app/ledger/detail/TimeoutCounter.cpp +++ b/src/xrpld/app/ledger/detail/TimeoutCounter.cpp @@ -1,18 +1,14 @@ #include -#include - -#ifdef XRPL_ENABLE_TELEMETRY -// The two guarded recording calls below are the only things here that name -// AcquireStats, so without telemetry the include has no user. #include -#endif +#include #include #include #include #include #include +#include #include #include @@ -69,16 +65,15 @@ TimeoutCounter::queueJob(ScopedLockType& sl) app_.getJobQueue().getJobCountTotal(queueJobParameter_.jobType) >= queueJobParameter_.jobLimit) { -#ifdef XRPL_ENABLE_TELEMETRY // Counted separately from timeouts: this path re-arms the timer // without running invokeOnTimer, so timeouts_ does not advance and the // give-up test that reads it cannot fire while the lane stays full. // - // Guarded because it runs on every deferred tick of every in-flight - // task, and the argument compares the job name against a string - // literal each time. The acquire metrics are its only reader. - app_.getAcquireStats().recordDeferral(isLedgerAcquisition()); -#endif + // Compiled out when telemetry is off: the argument compares the job + // name against a string literal on every deferred tick of every + // in-flight task, and a no-op count would still evaluate it. + if constexpr (telemetry::kEnabled) + app_.getAcquireStats().recordDeferral(isLedgerAcquisition()); JLOG(journal_.debug()) << "Deferring " << queueJobParameter_.jobName << " timer due to load"; setTimer(sl); @@ -103,12 +98,12 @@ TimeoutCounter::invokeOnTimer() if (!progress_) { ++timeouts_; -#ifdef XRPL_ENABLE_TELEMETRY - // Same cost as the deferral above: one call per no-progress tick, with - // a job-name string comparison to build the argument. timeouts_ stays - // outside the guard because the give-up test reads it. - app_.getAcquireStats().recordTimeout(isLedgerAcquisition()); -#endif + // Same argument cost as the deferral above -- one job-name string + // comparison per no-progress tick -- so it is compiled out the same + // way. timeouts_ stays outside the block because the give-up test + // reads it. + if constexpr (telemetry::kEnabled) + app_.getAcquireStats().recordTimeout(isLedgerAcquisition()); JLOG(journal_.debug()) << "Timeout(" << timeouts_ << ") " << " acquiring " << hash_; onTimer(false, sl); From b8cb36ffcaa26d45a172b79fd4f315615ec26c41 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:38:53 +0100 Subject: [PATCH 43/44] fix(telemetry): refuse an empty capture, and name a bad baseline entry An empty metric surface counted as a complete capture. build_query_plan returns an empty plan without complaining for any config that yields no gated keys, so pointing --metrics at the wrong file exits 0 and hands the paste-me path a metrics:{} artifact to offer as the next baseline. Nothing about such a run is evidence the pipeline works, so declared == 0 is now a failure rather than vacuously complete. The bounds checker also raised AttributeError on a baseline entry that is not an object, instead of naming the key. A validator whose job is to catch a malformed contract should report it, not crash on it. Test cleanup is bound to its own temp tree, so a loop no longer leaves five of six directories behind. --- .../telemetry/check_regression_bounds.py | 40 ++++++++++++--- .../telemetry/test_check_regression_bounds.py | 14 ++++-- docker/telemetry/workload/capture_timings.py | 49 +++++++++++++------ 3 files changed, 76 insertions(+), 27 deletions(-) diff --git a/.github/scripts/telemetry/check_regression_bounds.py b/.github/scripts/telemetry/check_regression_bounds.py index 5454d5dd29..b07045ce70 100644 --- a/.github/scripts/telemetry/check_regression_bounds.py +++ b/.github/scripts/telemetry/check_regression_bounds.py @@ -43,10 +43,12 @@ baseline's own. Six rules are checked: declare, carries a reason, and has neither a threshold override nor a baseline value left behind. -Rule A subtracts ``excluded_keys`` before comparing, so a quantile removed from -the gated set does not read as a missing baseline. Rule F is what keeps that -subtraction honest: an exclusion is the one edit here that makes the gate cover -LESS, so a stale or misspelt entry must fail rather than silently widen itself. +Rule A subtracts ``excluded_keys`` from BOTH sides of its comparison, so a +quantile removed from the gated set neither reads as a missing baseline nor as +an undeclared one -- an exclusion left in the baseline is one failure, rule F's, +which names the file to edit. Rule F is what keeps that subtraction honest: an +exclusion is the one edit here that makes the gate cover LESS, so a stale or +misspelt entry must fail rather than silently widen itself. A PLACEHOLDER baseline -- ``"placeholder": true`` or an empty ``metrics`` object -- exits 0, because that is the documented bootstrap state and CI has to @@ -56,8 +58,9 @@ without having checked anything is the same green-build-that-is-not failure this script exists to prevent, so renaming or deleting one of its inputs must not silence it. -A baseline ENTRY that is not a positive finite number is rejected before any -rule runs, by ``_unusable_baseline``. Every rule does arithmetic on that value, +A baseline ENTRY that is not an object, or whose value is not a positive finite +number, is rejected before any rule runs -- see ``check_key`` and +``_unusable_baseline``. Every rule does arithmetic on that value, and a degenerate one made the script crash with a traceback (rule D divides by it) or emit advice about the wrong file (a negative value inverts rule D's comparison). Reporting malformed input is what this script is for, so it must @@ -287,7 +290,20 @@ def check_key(key, entry, thresholds, ladders): malformed input, and reporting malformed input is this script's whole job, so it is rejected up front rather than arithmetic being attempted on it -- see ``_unusable_baseline``. + + The entry's SHAPE is checked first, for the same reason. A hand edit that + writes the bare number instead of the ``{"value": .., "unit": ..}`` object + leaves no ``.get`` to call, and the script died with an AttributeError + traceback naming a line in itself rather than the key at fault. """ + if not isinstance(entry, dict): + return [ + f"{key}: baseline entry {entry!r} is not an object carrying value and " + f"unit, so no bound can be derived from it. Recapture the baseline " + f"from a CI run rather than editing it by hand -- see " + f"baselines/README.md" + ] + value, unit = entry.get("value"), entry.get("unit", "") edges = ladders.get(unit) if value is None or edges is None: @@ -381,8 +397,16 @@ def main(): failures.extend(check_exclusions(metrics_cfg, thresholds, gated, declared)) # Rule A compares against the GATED surface, so a deliberately excluded # quantile is not reported as a baseline that was never captured. - declared -= set(metrics_cfg.get("excluded_keys", {})) - for key in sorted(set(gated) - declared): + # + # The same keys come off rule A's over-coverage side too. An excluded key + # left in the baseline is rule F's finding, reported with the file to edit; + # rule A would add a second failure for the same single mistake, saying the + # key is not declared -- which is not even true, it is declared and then + # excluded. Only exclusions the surface really declares are subtracted, so a + # misspelt exclusion naming a stale baseline key still reaches rule A. + excluded_declared = set(metrics_cfg.get("excluded_keys", {})) & declared + declared -= excluded_declared + for key in sorted(set(gated) - declared - excluded_declared): failures.append( f"{key}: in the baseline but not declared by {METRICS}, so it is " f"reported every run and can never gate -- remove it (rule A)" diff --git a/.github/scripts/telemetry/test_check_regression_bounds.py b/.github/scripts/telemetry/test_check_regression_bounds.py index ba3da1848c..5e7a4b7811 100644 --- a/.github/scripts/telemetry/test_check_regression_bounds.py +++ b/.github/scripts/telemetry/test_check_regression_bounds.py @@ -46,7 +46,12 @@ class CheckerCase(unittest.TestCase): def setUp(self): self.tree = Path(tempfile.mkdtemp()) - self.addCleanup(self._cleanup) + # Bound to THIS tree, not read off self.tree when the cleanup finally + # runs. A test that calls setUp again for a fresh tree (see the + # degenerate-baseline subTests) rebinds self.tree, and a late read would + # make every registered cleanup remove the LAST tree, leaving each + # earlier one behind in /tmp. + self.addCleanup(self._cleanup, self.tree) for rel in INPUTS: dest = self.tree / rel dest.parent.mkdir(parents=True, exist_ok=True) @@ -55,11 +60,12 @@ class CheckerCase(unittest.TestCase): script.parent.mkdir(parents=True, exist_ok=True) shutil.copy(CHECKER, script) - def _cleanup(self): - for path in self.tree.rglob("*"): + def _cleanup(self, tree): + """Remove one scratch tree, restoring permissions rmtree needs first.""" + for path in tree.rglob("*"): if path.is_file(): path.chmod(stat.S_IRUSR | stat.S_IWUSR) - shutil.rmtree(self.tree, ignore_errors=True) + shutil.rmtree(tree, ignore_errors=True) def run_checker(self): """Run the checker in the scratch tree, returning (code, stdout+stderr).""" diff --git a/docker/telemetry/workload/capture_timings.py b/docker/telemetry/workload/capture_timings.py index 3cd4bb8cd2..9c90ac5f30 100644 --- a/docker/telemetry/workload/capture_timings.py +++ b/docker/telemetry/workload/capture_timings.py @@ -122,10 +122,17 @@ def _capture_status(metrics: dict, min_ratio: float) -> dict: keys on, and it is the same predicate that decides this script's exit code — see the module docstring for why it lives in the artifact. - An empty surface is vacuously complete: there is nothing for the capture - to have fallen short of, and that is the case the exit-code check has - always passed. Defining it any other way here would make the flag and the - exit code disagree, which is the drift this block exists to remove. + An empty surface is NOT complete. ``declared == 0`` means the capture asked + Prometheus for nothing: ``build_query_plan`` returns an empty plan, without + complaining, for any config that yields no ``spans``/``rpc_methods``/ + ``job_queue`` entries -- a ``--metrics`` path pointing at the wrong file, a + truncated one, or every key excluded. Nothing about that run is evidence + the pipeline works, so treating it as vacuously complete would exit 0 and + hand the paste-me path a ``metrics: {}`` artifact to offer as baseline + material. Pasted in, it still reads as a placeholder, so the gate stays off + while the workflow reports the baseline as activated -- the silent-green + outcome the whole ``capture`` block exists to prevent. The exit code reads + this same flag, so the two still cannot disagree. """ declared = len(metrics) captured = sum(1 for entry in metrics.values() if entry["value"] is not None) @@ -133,7 +140,7 @@ def _capture_status(metrics: dict, min_ratio: float) -> dict: "declared": declared, "captured": captured, "min_ratio": min_ratio, - "complete": declared == 0 or (captured / declared) >= min_ratio, + "complete": declared > 0 and (captured / declared) >= min_ratio, } @@ -236,16 +243,28 @@ def main() -> int: logger.info("Wrote %s (%d/%d metrics captured)", args.output, captured, total) if not status["complete"]: - logger.error( - "Only %d/%d (%.0f%%) metrics captured — below the %.0f%% minimum. " - "Is Prometheus reachable at %s? The file is marked " - "capture.complete=false and must not be pasted into the baseline.", - captured, - total, - captured / total * 100, - args.min_capture_ratio * 100, - args.prometheus, - ) + if total == 0: + # No ratio to report: nothing was asked for, so the shortfall is the + # declared surface, not Prometheus. Named separately because the + # percentage below would divide by zero. + logger.error( + "No metrics were declared, so nothing was captured. Does %s " + "declare spans/rpc_methods/job_queue names, and does " + "excluded_keys leave any of them gated? The file is marked " + "capture.complete=false and must not be pasted into the baseline.", + args.metrics, + ) + else: + logger.error( + "Only %d/%d (%.0f%%) metrics captured — below the %.0f%% minimum. " + "Is Prometheus reachable at %s? The file is marked " + "capture.complete=false and must not be pasted into the baseline.", + captured, + total, + captured / total * 100, + args.min_capture_ratio * 100, + args.prometheus, + ) return 1 return 0 From eaeb2dc8e107d3e3c870807b1eca5f84b081c4dc Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:39:40 +0100 Subject: [PATCH 44/44] style(telemetry): brace the conditional bodies in the recording tests Two if constexpr/else pairs had single-statement bodies holding a gtest macro. ShortStatementLines exempts a one-line body, but these macros expand to multi-line constructs and some clang-tidy versions report the expansion's range, which would make readability-braces-around-statements fire under WarningsAsErrors. Braces also read better beside an else. --- src/tests/libxrpl/telemetry/Recording.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/tests/libxrpl/telemetry/Recording.cpp b/src/tests/libxrpl/telemetry/Recording.cpp index c667d0dd1a..77e970818f 100644 --- a/src/tests/libxrpl/telemetry/Recording.cpp +++ b/src/tests/libxrpl/telemetry/Recording.cpp @@ -72,9 +72,13 @@ TEST(Recording, counter_accumulates_only_when_compiled_in) counter.add(4); if constexpr (telemetry::kEnabled) + { EXPECT_EQ(counter.load(), 5U); + } else + { EXPECT_EQ(counter.load(), 0U); + } } // The default template argument is std::uint64_t; an explicit type is honoured. @@ -98,9 +102,13 @@ TEST(Recording, stopwatch_measures_only_when_compiled_in) auto const elapsed = timer.elapsedUs(); if constexpr (telemetry::kEnabled) + { EXPECT_GE(elapsed, std::chrono::microseconds{1000}); + } else + { EXPECT_EQ(elapsed, std::chrono::microseconds{0}); + } } // restart() moves the origin forward, so the interval measured after it is