From 6e76fa1b9a8014aa2dc56ed70f5b7b9fb0b1b072 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:01:42 +0100 Subject: [PATCH] fix(telemetry): read single-ledger ranges in the complete_ledgers gauge xrpl::to_string(ClosedInterval) renders a one-ledger range as a bare sequence number with no dash. The gauge's parser required a dash, so it dropped that range: a node holding a single complete ledger published no series at all, and because the index counter only advanced on emitted segments, every later range's index label shifted down by one. parseLedgerRange() now treats a dashless segment as a range of one ledger. It is inline in the header because MetricsRegistry.cpp is not compiled into the unit-test binary, so an out-of-line definition could not be tested. It uses std::from_chars rather than std::stoll: stoll threw out of the whole callback, losing every remaining range that collection cycle, where from_chars costs only the segment it cannot read. Six tests cover both emitted shapes, the refused shapes, the sequence limits, and a round trip through the real producer. The same file's unused-parameter casts in the telemetry-disabled stubs move to [[maybe_unused]]; eight more cast the member enabled_, which isEnabled() reads outside every #ifdef, so they suppressed nothing and are deleted. --- .../libxrpl/telemetry/MetricsRegistry.cpp | 164 +++++++++++++++++- src/xrpld/telemetry/MetricsRegistry.cpp | 82 +++------ src/xrpld/telemetry/MetricsRegistry.h | 73 +++++++- 3 files changed, 260 insertions(+), 59 deletions(-) diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index a36c2a220d..fc06d1cc39 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -1,7 +1,7 @@ /** * GTest unit tests for MetricsRegistry. * - * Three independent groups, split by what they can link: + * Four independent groups, split by what they can link: * * 1. sanitiseHandler() — the `handler` label sanitiser. Runs in **both** * builds. sanitiseHandler() is a public static constexpr defined inline @@ -14,7 +14,13 @@ * on the nodestore_state gauge. Also a public static constexpr inline, * so it runs in both builds for the same reason. * - * 3. The no-op / telemetry-disabled path — construction, the two-phase + * 3. parseLedgerRange() — reads one segment of the complete-ledger range + * string the complete_ledgers gauge publishes. A public static inline, so + * it runs in both builds for the same reason. The last case drives the + * real producer, xrpl::to_string(RangeSet), rather than restating its + * format. + * + * 4. The no-op / telemetry-disabled path — construction, the two-phase * start() / startAsyncGauges() / stop() lifecycle, and the synchronous * record*() methods. Guarded, because when XRPL_ENABLE_TELEMETRY is * defined MetricsRegistry.cpp is not compiled into this binary (see @@ -24,6 +30,8 @@ #include +#include + #include #include @@ -33,7 +41,10 @@ #include #include #include +#include #include +#include +#include namespace { @@ -417,6 +428,155 @@ TEST(MetricsRegistryScaledMean, default_scale_is_one) EXPECT_EQ(Registry::scaledMean(360, 8), 45); } +namespace { + +/** + * Segments the producer can emit, paired with the range each denotes. + * + * Both shapes come from xrpl::to_string(ClosedInterval): `first-last`, and a + * bare number when first equals last. + */ +constexpr std::array>, 6> + kProducibleSegments{{ + {"32570-50000", {32570, 50000}}, + {"50005-75891421", {50005, 75891421}}, + {"0-1", {0, 1}}, + {"5000", {5000, 5000}}, + {"0", {0, 0}}, + {"1-1", {1, 1}}, + }}; + +/** + * Segments no producer emits and the parser must refuse. + * + * `5-6 ` and `0x10` are the two that pin the consumed-everything check: they + * start with digits from_chars can read, so only the `ptr != end` test rejects + * them. from_chars refuses the other ten on its own. Keep those two. + */ +constexpr std::array kUnreadableSegments{ + "", + "-", + "-5", + "5-", + "abc", + "5-a", + "a-5", + "5--6", + " 5-6", + "5-6 ", + "+5", + "0x10", +}; + +} // namespace + +TEST(MetricsRegistryParseLedgerRange, dashed_segment_yields_both_bounds) +{ + // The ordinary shape. Both bounds must survive, because the gauge publishes + // them as separate `start` and `end` series and a dashboard subtracts them. + EXPECT_EQ( + Registry::parseLedgerRange("32570-50000"), + (std::pair{32570, 50000})); + EXPECT_EQ( + Registry::parseLedgerRange("50005-75891421"), + (std::pair{50005, 75891421})); +} + +TEST(MetricsRegistryParseLedgerRange, single_ledger_segment_is_a_range_not_a_reject) +{ + // A node holding exactly one complete ledger renders as a bare number, so + // treating a dashless segment as malformed reports nothing at all for that + // node -- the reading an operator most needs while a node is catching up. + auto const one = Registry::parseLedgerRange("5000"); + ASSERT_TRUE(one.has_value()); + EXPECT_EQ(one->first, 5000u); + EXPECT_EQ(one->second, 5000u); + + // Cause, not just state: acceptance is specific to an all-digit segment. + // These two prove the dashless branch is not simply accepting everything, + // so the test above would still fail if the guard were removed outright. + EXPECT_FALSE(Registry::parseLedgerRange("abc").has_value()); + EXPECT_FALSE(Registry::parseLedgerRange("5-").has_value()); +} + +TEST(MetricsRegistryParseLedgerRange, every_producible_segment_parses_exactly) +{ + for (auto const& [segment, expected] : kProducibleSegments) + { + auto const parsed = Registry::parseLedgerRange(segment); + ASSERT_TRUE(parsed.has_value()) << "rejected a producible segment: " << segment; + EXPECT_EQ(*parsed, expected) << "wrong bounds for segment: " << segment; + } +} + +TEST(MetricsRegistryParseLedgerRange, unreadable_segments_are_refused) +{ + for (auto const segment : kUnreadableSegments) + { + EXPECT_FALSE(Registry::parseLedgerRange(segment).has_value()) + << "accepted an unreadable segment: [" << segment << "]"; + } +} + +TEST(MetricsRegistryParseLedgerRange, bounds_are_exact_at_the_sequence_limits) +{ + // The width comes from the function's own return type, so widening the + // sequence cannot leave this asserting against a stale boundary. + using Seq = decltype(Registry::parseLedgerRange("0"))::value_type::first_type; + constexpr auto kMaxSeq = std::numeric_limits::max(); + auto const maxText = std::to_string(kMaxSeq); + + auto const atLimit = Registry::parseLedgerRange(maxText); + ASSERT_TRUE(atLimit.has_value()) << "rejected the largest representable sequence"; + EXPECT_EQ(atLimit->first, kMaxSeq); + EXPECT_EQ(atLimit->second, kMaxSeq); + + // One past the limit does not wrap to a small, believable sequence. + auto const pastLimit = std::to_string(static_cast(kMaxSeq) + 1); + EXPECT_FALSE(Registry::parseLedgerRange(pastLimit).has_value()) + << "overflowed instead of refusing: " << pastLimit; +} + +TEST(MetricsRegistryParseLedgerRange, reads_back_what_the_real_producer_wrote) +{ + // Drives the actual producer rather than a restatement of its format, so a + // change to to_string() fails here instead of silently changing what the + // gauge reports. The middle interval is one ledger wide on purpose: that is + // the shape that renders without a dash. + xrpl::RangeSet ledgers; + ledgers.insert(xrpl::range(32570, 50000)); + ledgers.insert(xrpl::range(60000, 60000)); + ledgers.insert(xrpl::range(70000, 75891421)); + + auto const rendered = xrpl::to_string(ledgers); + + std::vector> recovered; + std::string_view rest{rendered}; + while (!rest.empty()) + { + auto const comma = rest.find(','); + auto const segment = rest.substr(0, comma); + + auto const parsed = Registry::parseLedgerRange(segment); + ASSERT_TRUE(parsed.has_value()) << "producer emitted a segment the parser refuses: [" + << segment << "] from " << rendered; + recovered.push_back(*parsed); + + rest = (comma == std::string_view::npos) ? std::string_view{} : rest.substr(comma + 1); + } + + std::vector> const expected{ + {32570, 50000}, + {60000, 60000}, + {70000, 75891421}, + }; + EXPECT_EQ(recovered, expected) << "rendered as: " << rendered; + + // Cause, not just state: every interval survived the round trip, so none + // was dropped and no later index shifted down to fill a gap. + EXPECT_EQ(recovered.size(), ledgers.iterative_size()); +} + // When telemetry is globally enabled, MetricsRegistry.cpp requires xrpld // link dependencies we cannot satisfy in a standalone GTest binary. #ifndef XRPL_ENABLE_TELEMETRY diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index a41b851239..357aad4ca4 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -199,9 +199,9 @@ MetricsRegistry::~MetricsRegistry() void MetricsRegistry::start( - std::string const& endpoint, - std::string const& instanceId, - std::string const& nodeId) + [[maybe_unused]] std::string const& endpoint, + [[maybe_unused]] std::string const& instanceId, + [[maybe_unused]] std::string const& nodeId) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_) @@ -222,11 +222,6 @@ MetricsRegistry::start( initSyncInstruments(); JLOG(journal_.info()) << "MetricsRegistry: provider and instruments ready"; -#else - (void)endpoint; - (void)instanceId; - (void)nodeId; - (void)enabled_; #endif // XRPL_ENABLE_TELEMETRY } @@ -249,8 +244,6 @@ MetricsRegistry::startAsyncGauges() registerAsyncGauges(); JLOG(journal_.info()) << "MetricsRegistry: started successfully"; -#else - (void)enabled_; #endif // XRPL_ENABLE_TELEMETRY } @@ -410,20 +403,19 @@ MetricsRegistry::stop() // ----------------------------------------------------------------- void -MetricsRegistry::recordRpcStarted(std::string_view method) +MetricsRegistry::recordRpcStarted([[maybe_unused]] std::string_view method) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !rpcStartedCounter_) return; rpcStartedCounter_->Add(1, {{"method", std::string(method)}}); -#else - (void)method; - (void)enabled_; #endif } void -MetricsRegistry::recordRpcFinished(std::string_view method, std::int64_t durationUs) +MetricsRegistry::recordRpcFinished( + [[maybe_unused]] std::string_view method, + [[maybe_unused]] std::int64_t durationUs) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !rpcFinishedCounter_) @@ -436,15 +428,13 @@ MetricsRegistry::recordRpcFinished(std::string_view method, std::int64_t duratio {{"method", std::string(method)}}, opentelemetry::context::Context{}); } -#else - (void)method; - (void)durationUs; - (void)enabled_; #endif } void -MetricsRegistry::recordRpcErrored(std::string_view method, std::int64_t durationUs) +MetricsRegistry::recordRpcErrored( + [[maybe_unused]] std::string_view method, + [[maybe_unused]] std::int64_t durationUs) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !rpcErroredCounter_) @@ -457,10 +447,6 @@ MetricsRegistry::recordRpcErrored(std::string_view method, std::int64_t duration {{"method", std::string(method)}}, opentelemetry::context::Context{}); } -#else - (void)method; - (void)durationUs; - (void)enabled_; #endif } @@ -469,7 +455,9 @@ MetricsRegistry::recordRpcErrored(std::string_view method, std::int64_t duration // ----------------------------------------------------------------- void -MetricsRegistry::recordJobQueued(std::string_view jobType, std::string_view jobName) +MetricsRegistry::recordJobQueued( + [[maybe_unused]] std::string_view jobType, + [[maybe_unused]] std::string_view jobName) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !jobQueuedCounter_) @@ -478,18 +466,14 @@ MetricsRegistry::recordJobQueued(std::string_view jobType, std::string_view jobN 1, {{kJobTypeLabel, std::string(jobType)}, {kHandlerLabel, std::string(sanitiseHandler(jobName))}}); -#else - (void)jobType; - (void)jobName; - (void)enabled_; #endif } void MetricsRegistry::recordJobStarted( - std::string_view jobType, - std::string_view jobName, - std::int64_t queuedDurUs) + [[maybe_unused]] std::string_view jobType, + [[maybe_unused]] std::string_view jobName, + [[maybe_unused]] std::int64_t queuedDurUs) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !jobStartedCounter_) @@ -509,19 +493,14 @@ MetricsRegistry::recordJobStarted( {{kJobTypeLabel, std::string(jobType)}, {kHandlerLabel, handler}}, opentelemetry::context::Context{}); } -#else - (void)jobType; - (void)jobName; - (void)queuedDurUs; - (void)enabled_; #endif } void MetricsRegistry::recordJobFinished( - std::string_view jobType, - std::string_view jobName, - std::int64_t runningDurUs) + [[maybe_unused]] std::string_view jobType, + [[maybe_unused]] std::string_view jobName, + [[maybe_unused]] std::int64_t runningDurUs) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_ || !jobFinishedCounter_) @@ -535,11 +514,6 @@ MetricsRegistry::recordJobFinished( {{kJobTypeLabel, std::string(jobType)}, {kHandlerLabel, handler}}, opentelemetry::context::Context{}); } -#else - (void)jobType; - (void)jobName; - (void)runningDurUs; - (void)enabled_; #endif } @@ -1096,32 +1070,30 @@ MetricsRegistry::registerCompleteLedgersGauge() return; // Parse comma-separated ranges like - // "32570-50000,50005-75891421". + // "32570-50000,50005-75891421". A range of one ledger arrives + // as a bare sequence number, so parseLedgerRange() decides what + // a segment is; only genuinely unreadable ones are skipped. std::size_t rangeIndex = 0; std::istringstream stream(rangeStr); std::string segment; while (std::getline(stream, segment, ',')) { - auto const dashPos = segment.find('-'); - if (dashPos == std::string::npos || dashPos == 0 || - dashPos == segment.size() - 1) + auto const range = MetricsRegistry::parseLedgerRange(segment); + if (!range) continue; - auto const startStr = segment.substr(0, dashPos); - auto const endStr = segment.substr(dashPos + 1); - auto const idxStr = std::to_string(rangeIndex); opentelemetry::nostd::get>>(result) ->Observe( - static_cast(std::stoll(startStr)), + static_cast(range->first), {{"bound", "start"}, {"index", idxStr}}); opentelemetry::nostd::get>>(result) ->Observe( - static_cast(std::stoll(endStr)), + static_cast(range->second), {{"bound", "end"}, {"index", idxStr}}); ++rangeIndex; @@ -1463,7 +1435,7 @@ MetricsRegistry::registerStateTrackingGauge() // State value: 0-4 from OperatingMode, 5=validating, 6=proposing. auto const mode = app.getOPs().getOperatingMode(); - auto stateValue = static_cast(mode); + auto stateValue = static_cast(std::to_underlying(mode)); // If FULL, refine using consensus info for validating/proposing. if (mode == OperatingMode::FULL) diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index f66f9c8fcb..b18260f6c2 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -147,11 +147,13 @@ #include #include +#include #include #include #include #include #include +#include #ifdef XRPL_ENABLE_TELEMETRY #include @@ -557,6 +559,73 @@ public: return static_cast(scaled + fraction); } + /** + * Read one comma-separated segment of a complete-ledger range string. + * + * The producer is xrpl::to_string(RangeSet), documented in + * xrpl/basics/RangeSet.h. It renders an interval as `first-last`, and an + * interval whose first equals its last as a bare sequence number. A segment + * with no dash is therefore a range of one ledger, not a malformed one. + * + * Defined inline for the same reason as sanitiseHandler(): in a + * telemetry-enabled build MetricsRegistry.cpp is not compiled into the + * unit-test binary, so an out-of-line definition would be untestable. + * + * @param segment One segment, already split on ','. Leading or trailing + * whitespace is rejected, because the producer emits none. + * @return The inclusive first and last sequence of the range. The two are + * equal for a single-ledger range. std::nullopt when @p segment is not + * something this producer can emit. + * + * @note Pure and reentrant: holds no state, performs no I/O, and is safe to + * call concurrently from any thread. + * @note Reports malformed input instead of throwing, so one unreadable + * segment costs its own range and not every range after it. + * @note A reversed range such as "9-4" is returned as given. RangeSet + * cannot emit one. + * + * Example: + * @code + * parseLedgerRange("32570-50000"); // {32570, 50000} + * parseLedgerRange("5000"); // {5000, 5000} -- one ledger + * parseLedgerRange("5-"); // nullopt + * @endcode + */ + [[nodiscard]] static std::optional> + parseLedgerRange(std::string_view segment) noexcept + { + auto const parseSeq = [](std::string_view text) -> std::optional { + std::uint32_t value = 0; + auto const* const begin = text.data(); + auto const* const end = begin + text.size(); + auto const [ptr, ec] = std::from_chars(begin, end, value); + + // from_chars stops at the first character it cannot use, so the + // whole segment counts as read only when it consumed all of it. + if (ec != std::errc{} || ptr != end) + return std::nullopt; + + return value; + }; + + auto const dash = segment.find('-'); + if (dash == std::string_view::npos) + { + auto const only = parseSeq(segment); + if (!only) + return std::nullopt; + + return std::pair{*only, *only}; + } + + auto const first = parseSeq(segment.substr(0, dash)); + auto const last = parseSeq(segment.substr(dash + 1)); + if (!first || !last) + return std::nullopt; + + return std::pair{*first, *last}; + } + /** * Record a job enqueued event. * @param jobType The job type name (e.g. "ledgerData"). @@ -670,7 +739,7 @@ public: * into it is not free: each call takes its lock and inserts an entry. * @return Reference to the internal ValidationTracker instance. */ - ValidationTracker& + [[nodiscard]] ValidationTracker& getValidationTracker() { return validationTracker_; @@ -684,7 +753,7 @@ public: * start() has run or when disabled. * @return The shared Meter, or empty if not yet started. */ - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr meter() const noexcept { return meter_;