ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-27 15:28:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(telemetry): read single-ledger ranges in the complete_ledgers gauge

Browse Source 
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.
This commit is contained in:
Pratik Mankawde
2026-09-03 15:01:42 +01:00
parent 34cc9dea06
commit 6e76fa1b9a
3 changed files with 260 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

164
src/tests/libxrpl/telemetry/MetricsRegistry.cpp
View File

@@ -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 <xrpld/telemetry/MetricsRegistry.h>
#include <xrpl/basics/RangeSet.h>
#include <gtest/gtest.h>
#include <algorithm>
@@ -33,7 +41,10 @@
#include <limits>
#include <optional>
#include <set>
#include <string>
#include <string_view>
#include <utility>
#include <vector>
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<std::pair<std::string_view, std::pair<std::uint32_t, std::uint32_t>>, 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<std::string_view, 12> 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<std::uint32_t, std::uint32_t>{32570, 50000}));
EXPECT_EQ(
Registry::parseLedgerRange("50005-75891421"),
(std::pair<std::uint32_t, std::uint32_t>{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<Seq>::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<std::uint64_t>(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<std::uint32_t> ledgers;
ledgers.insert(xrpl::range<std::uint32_t>(32570, 50000));
ledgers.insert(xrpl::range<std::uint32_t>(60000, 60000));
ledgers.insert(xrpl::range<std::uint32_t>(70000, 75891421));
auto const rendered = xrpl::to_string(ledgers);
std::vector<std::pair<std::uint32_t, std::uint32_t>> 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<std::pair<std::uint32_t, std::uint32_t>> 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

82
src/xrpld/telemetry/MetricsRegistry.cpp
View File

@@ -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<opentelemetry::nostd::shared_ptr<
opentelemetry::metrics::ObserverResultT<int64_t>>>(result)
->Observe(
static_cast<int64_t>(std::stoll(startStr)),
static_cast<int64_t>(range->first),
{{"bound", "start"}, {"index", idxStr}});
opentelemetry::nostd::get<opentelemetry::nostd::shared_ptr<
opentelemetry::metrics::ObserverResultT<int64_t>>>(result)
->Observe(
static_cast<int64_t>(std::stoll(endStr)),
static_cast<int64_t>(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<double>(mode);
auto stateValue = static_cast<double>(std::to_underlying(mode));
// If FULL, refine using consensus info for validating/proposing.
if (mode == OperatingMode::FULL)

73
src/xrpld/telemetry/MetricsRegistry.h
View File

@@ -147,11 +147,13 @@
#include <xrpl/beast/utility/Journal.h>
#include <algorithm>
#include <charconv>
#include <cstdint>
#include <limits>
#include <optional>
#include <string>
#include <string_view>
#include <utility>
#ifdef XRPL_ENABLE_TELEMETRY
#include <opentelemetry/metrics/meter.h>
@@ -557,6 +559,73 @@ public:
return static_cast<std::int64_t>(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<std::pair<std::uint32_t, std::uint32_t>>
parseLedgerRange(std::string_view segment) noexcept
{
auto const parseSeq = [](std::string_view text) -> std::optional<std::uint32_t> {
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<opentelemetry::metrics::Meter>
[[nodiscard]] opentelemetry::nostd::shared_ptr<opentelemetry::metrics::Meter>
meter() const noexcept
{
return meter_;