From cbfbea67f2d2e6438e64cea23d0306191139f264 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:51:22 +0100 Subject: [PATCH 1/2] feat(telemetry): own every histogram ladder in one tested header The bucket edges for the OTel histograms lived as file-local `namespace {}` constants, unreachable from any test, and they drifted from the collector's spanmetrics ladder they were specified to match. The millisecond ladder stayed capped at 5 s after the collector side was extended to 30 s, so any quantile above 5 s read back as a flat 5000 -- Prometheus returns the second-highest edge for a quantile in the `+Inf` bucket, which looks like a measurement rather than an error. Adds include/xrpl/telemetry/HistogramBuckets.h as the single owner of the ladders, with a constexpr validator plus static_asserts so a descending or duplicated edge cannot compile, and gtest coverage that pins the floor and ceiling against the measured distributions: - kMillisecondBuckets carries every representable collector edge and extends to 120 s, because the updatepaths job type averages ~60 s and a 30 s ceiling would censor it exactly as 5 s does today. Sub-millisecond collector edges are omitted: beast::insight::Event rounds durations up to whole milliseconds, so they would collect nothing. - kByteBuckets is new, for Events whose samples are sizes rather than durations. Edges follow the measured RPC response distribution (mean 2131 B, half under 1 kB, tail mean bounded at 7538 B) rather than a guess, so the resolution sits between 512 B and 64 kB. No behaviour change yet -- nothing consumes the header until the views are rewired. --- include/xrpl/telemetry/HistogramBuckets.h | 179 +++++++++++++++++ .../libxrpl/telemetry/HistogramBuckets.cpp | 190 ++++++++++++++++++ 2 files changed, 369 insertions(+) create mode 100644 include/xrpl/telemetry/HistogramBuckets.h create mode 100644 src/tests/libxrpl/telemetry/HistogramBuckets.cpp diff --git a/include/xrpl/telemetry/HistogramBuckets.h b/include/xrpl/telemetry/HistogramBuckets.h new file mode 100644 index 0000000000..381362034e --- /dev/null +++ b/include/xrpl/telemetry/HistogramBuckets.h @@ -0,0 +1,179 @@ +#pragma once + +#include +#include +#include +#include + +namespace xrpl::telemetry::buckets { + +/** + * @file HistogramBuckets.h + * @brief Explicit histogram bucket edges for xrpld's OTel instruments. + * + * One header owns every ladder so a reviewer sees all of them at once and a + * test can assert their invariants. Before this existed the edges lived as + * file-local `namespace {}` constants, unreachable from any test, and they + * drifted apart. + * + * Why a ladder is worth this much care: when a quantile falls in the `+Inf` + * bucket, Prometheus returns the *second-highest* edge, not `+Inf`. A + * saturated histogram therefore reports a believable constant instead of an + * obvious error. The same trap exists at the bottom -- if nearly every + * sample lands in bucket 0, `histogram_quantile` interpolates inside it and + * invents a value. A ladder is correct only when its floor sits below the + * mass of the distribution and its ceiling above the tail. + * + * sample --> [ SDK lower_bound over edges ] --> per-bucket counter + * | | + * edges come from v + * THIS header OTLP export + * | + * v + * histogram_quantile() in Grafana + * + * Ladders are `std::array` so they are constant-initialised and + * usable in a `static_assert`. The OTel SDK wants `std::vector` in + * its aggregation config, so call toVector() at the registration site + * rather than storing vectors here. + * + * Example -- register a view with the millisecond ladder: + * @code + * auto config = std::make_shared(); + * config->boundaries_ = buckets::toVector(buckets::kMillisecondBuckets); + * @endcode + * + * Example -- the edge case that motivated a second ladder. An Event whose + * samples are sizes rather than durations must not borrow a latency ladder, + * or a quarter of its samples land in `+Inf` and every quantile reads back + * as the top edge: + * @code + * config->boundaries_ = buckets::toVector(buckets::kByteBuckets); + * @endcode + * + * @note Thread safety: every member is `constexpr` and immutable, so + * reading them from any thread is safe. toVector() allocates and is + * meant for start-up registration paths, never for a record path. + * @note Limitation: changing a ladder changes the exported series count and + * ends bucket comparability across the change -- existing series keep + * their old `le` values, so panels show a break at restart. Grafana + * Cloud bills per series, so re-measure the series count after any + * edit here. + */ + +/** + * Bucket edges, in milliseconds, for whole-millisecond `beast::insight` + * Events: job queue wait and run times, io latency, RPC time, pathfinding. + * + * **This list must contain every representable edge of the collector's + * spanmetrics ladder, and may extend above it.** Agreement over the shared + * range is deliberate: it lets a span-derived latency panel and a native + * histogram panel be read on the same scale. It was specified that way + * originally, then silently broken when the collector ladder alone was + * extended, which left this side capped at 5 s while spans reached 30 s and + * censored every quantile above 5 s. `check_bucket_parity.py` now enforces + * the containment -- add a collector edge, add it here too. + * + * The sub-millisecond edges the collector carries (0.01 to 0.5 ms) are + * deliberately absent. `beast::insight::Event` rounds every duration up to + * a whole millisecond before it reaches the histogram, so those edges would + * collect nothing. Metrics that genuinely need finer resolution belong on + * the microsecond ladder, on the OTel-native path. + * + * The 60 s and 120 s edges exceed the collector's 30 s top on purpose, + * because jobs outlive spans: the updatepaths job type was measured + * averaging about 60 s, so a 30 s ceiling would censor its quantiles just + * as 5 s censors them today. All these Events share one ladder, so its + * ceiling has to cover the slowest member rather than the typical one. + * + * The 2, 3 and 4 s edges resolve second-scale work that previously had to + * interpolate across a single four-second-wide bucket. + */ +inline constexpr std::array kMillisecondBuckets{ + 1.0, + 5.0, + 10.0, + 25.0, + 50.0, + 100.0, + 250.0, + 500.0, + 1'000.0, + 2'000.0, + 3'000.0, + 4'000.0, + 5'000.0, + 10'000.0, + 30'000.0, + 60'000.0, + 120'000.0}; + +/** + * Bucket edges, in bytes, for `beast::insight` Events whose samples are + * sizes rather than durations. Currently only the RPC response size. + * + * Placed from the measured distribution rather than from a guess about how + * large a response could theoretically be. Measured over 24 h: mean 2131 B, + * half of all responses under 1 kB, three quarters under 5 kB. The tail + * above 5 kB has a mean of at most 7538 B, which bounds p99 near 80 kB and + * p99.75 below 256 kB. + * + * So the resolution belongs between 512 B and 64 kB, where the + * distribution actually turns, and two further edges are ample headroom. + * Spending edges at the megabyte scale would cost cardinality on a range + * nothing measured occupies. If a genuinely multi-megabyte response ever + * shows up in the top bucket, extend this -- but extend it on evidence. + */ +inline constexpr std::array kByteBuckets{ + 512.0, + 1'024.0, + 2'048.0, + 4'096.0, + 8'192.0, + 16'384.0, + 32'768.0, + 65'536.0, + 262'144.0, + 1'048'576.0}; + +/** + * @brief Check that a ladder is strictly ascending and non-negative. + * + * The SDK places a sample with `std::lower_bound` over the edges, which + * silently misbuckets when edges repeat or descend. Checking at compile + * time makes that class of typo impossible to ship. + * + * @param ladder Bucket upper bounds to check. + * @return true when the ladder is non-empty, starts at or above zero, and + * every later edge is strictly greater than its predecessor. + */ +constexpr bool +isAscendingNonNegative(std::span ladder) noexcept +{ + if (ladder.empty() || ladder.front() < 0.0) + return false; + + for (std::size_t i = 1; i < ladder.size(); ++i) + { + if (!(ladder[i] > ladder[i - 1])) + return false; + } + return true; +} + +static_assert(isAscendingNonNegative(kMillisecondBuckets)); +static_assert(isAscendingNonNegative(kByteBuckets)); + +/** + * @brief Copy a ladder into the `std::vector` the OTel SDK wants. + * + * @param ladder Bucket upper bounds. + * @return A vector holding the same edges in the same order. + */ +inline std::vector +toVector(std::span ladder) +{ + return std::vector(ladder.begin(), ladder.end()); +} + +} // namespace xrpl::telemetry::buckets diff --git a/src/tests/libxrpl/telemetry/HistogramBuckets.cpp b/src/tests/libxrpl/telemetry/HistogramBuckets.cpp new file mode 100644 index 0000000000..1eb013784e --- /dev/null +++ b/src/tests/libxrpl/telemetry/HistogramBuckets.cpp @@ -0,0 +1,190 @@ +/** + * GTest unit tests for the histogram bucket ladders. + * + * These ladders decide whether a Grafana percentile panel reports a + * measurement or an artefact, and neither failure mode is visible in the + * panel itself: a quantile that falls in the `+Inf` bucket reads back as the + * second-highest edge, and one that falls inside bucket 0 is interpolated. + * Both look like plausible numbers. So the invariants are asserted here + * rather than left to review. + * + * The ladders are `constexpr`, so most of this could be `static_assert`. + * They are runtime tests as well so that a failure names which edge is + * wrong instead of only failing the compile. + */ + +#include + +#include + +#include +#include +#include +#include +#include +#include + +namespace xrpl::telemetry::buckets { + +// Every ladder must be strictly ascending and non-negative. The SDK places a +// sample with std::lower_bound over the edges, so a duplicated or +// out-of-order edge silently sends samples to the wrong bucket. +class HistogramBucketsTest : public ::testing::TestWithParam> +{ +}; + +TEST_P(HistogramBucketsTest, isStrictlyAscending) +{ + auto const ladder = GetParam(); + ASSERT_FALSE(ladder.empty()); + for (std::size_t i = 1; i < ladder.size(); ++i) + EXPECT_LT(ladder[i - 1], ladder[i]) << "edge index " << i << " does not ascend"; +} + +TEST_P(HistogramBucketsTest, isNonNegativeAndFinite) +{ + for (double const edge : GetParam()) + { + EXPECT_GE(edge, 0.0); + EXPECT_TRUE(std::isfinite(edge)) << "edge " << edge << " is not finite"; + } +} + +TEST_P(HistogramBucketsTest, passesTheCompileTimeValidator) +{ + EXPECT_TRUE(isAscendingNonNegative(GetParam())); +} + +INSTANTIATE_TEST_SUITE_P( + AllLadders, + HistogramBucketsTest, + ::testing::Values( + std::span{kMillisecondBuckets}, + std::span{kByteBuckets})); + +// The validator must also REJECT. A predicate that only ever returns true +// would let every ladder above pass while proving nothing. +TEST(HistogramBucketsValidator, rejectsEmptyDescendingDuplicateAndNegative) +{ + EXPECT_FALSE(isAscendingNonNegative(std::span{})); + + constexpr std::array descending{5.0, 1.0}; + EXPECT_FALSE(isAscendingNonNegative(descending)); + + constexpr std::array duplicated{1.0, 1.0, 2.0}; + EXPECT_FALSE(isAscendingNonNegative(duplicated)); + + constexpr std::array negative{-1.0, 1.0}; + EXPECT_FALSE(isAscendingNonNegative(negative)); +} + +TEST(HistogramBucketsValidator, acceptsASingleEdgeAndALeadingZero) +{ + constexpr std::array single{1.0}; + EXPECT_TRUE(isAscendingNonNegative(single)); + + // A leading zero is legal: the GetObject charge ladder starts at 0 to + // separate the free tier from everything else. + constexpr std::array leadingZero{0.0, 100.0}; + EXPECT_TRUE(isAscendingNonNegative(leadingZero)); +} + +TEST(HistogramBucketsRange, millisecondFloorIsOneAndCeilingCoversTheSlowestJob) +{ + // beast::insight::Event rounds durations up to whole milliseconds, so 1 + // is the smallest edge that can ever collect a sample. + EXPECT_EQ(kMillisecondBuckets.front(), 1.0); + + // The updatepaths job type was measured averaging 59,956 ms. A 30 s + // ceiling -- the collector's top edge -- would censor it just as the old + // 5 s ceiling does, so this ladder has to reach further. + EXPECT_GE(kMillisecondBuckets.back(), 120'000.0); +} + +TEST(HistogramBucketsRange, millisecondLadderClearsTheMeasuredCensoringPoint) +{ + // rpc_size had 24.9% of samples above the old 5000 ceiling and + // jobq_updatepaths had 100%. A ceiling at or below 5000 reintroduces the + // exact defect this ladder exists to fix. + EXPECT_GT(kMillisecondBuckets.back(), 5'000.0); +} + +TEST(HistogramBucketsRange, millisecondLadderContainsEveryRepresentableCollectorEdge) +{ + // Agreement with the collector's spanmetrics ladder over the shared + // range is the invariant; edges above its 30 s top are allowed because + // jobs outlive spans. Sub-millisecond collector edges are excluded + // because Event cannot represent them. check_bucket_parity.py enforces + // this against the YAML; this test pins it for the C++ side alone so a + // local edit fails fast. + constexpr std::array collectorEdges{ + 1.0, + 5.0, + 10.0, + 25.0, + 50.0, + 100.0, + 250.0, + 500.0, + 1'000.0, + 2'000.0, + 3'000.0, + 4'000.0, + 5'000.0, + 10'000.0, + 30'000.0}; + + for (double const edge : collectorEdges) + { + EXPECT_NE(std::ranges::find(kMillisecondBuckets, edge), kMillisecondBuckets.end()) + << edge << " ms is a collector spanmetrics edge and must be present"; + } +} + +TEST(HistogramBucketsRange, millisecondLadderResolvesTheOneToFiveSecondBand) +{ + // Without these the 1 s to 5 s span was one four-second-wide bucket, so + // any quantile landing inside it was interpolated across four seconds. + for (double const edge : {2'000.0, 3'000.0, 4'000.0}) + { + EXPECT_NE(std::ranges::find(kMillisecondBuckets, edge), kMillisecondBuckets.end()) + << edge << " ms edge missing"; + } +} + +TEST(HistogramBucketsRange, byteLadderBracketsTheMeasuredResponseDistribution) +{ + // Measured: mean 2131 B, half under 1 kB, three quarters under 5 kB, and + // the tail above 5 kB has a mean of at most 7538 B -- which puts p99 + // near 80 kB. The floor must sit at or below the measured median region + // and the ceiling well past the p99 bound. + EXPECT_LE(kByteBuckets.front(), 512.0); + EXPECT_GE(kByteBuckets.back(), 1'048'576.0); + + // Most of the resolution belongs where the distribution actually turns. + auto const withinWorkingRange = + std::ranges::count_if(kByteBuckets, [](double e) { return e >= 512.0 && e <= 65'536.0; }); + EXPECT_GE(withinWorkingRange, 6) << "too little resolution between 512 B and 64 kB"; +} + +TEST(HistogramBucketsRange, byteAndMillisecondLaddersAreDistinct) +{ + // A single shared ladder is what put a byte count on a latency scale and + // censored a quarter of its samples. + EXPECT_NE(kByteBuckets.size(), kMillisecondBuckets.size()); + EXPECT_GT(kByteBuckets.back(), kMillisecondBuckets.back()); +} + +TEST(HistogramBucketsConvert, toVectorPreservesOrderAndSize) +{ + auto const converted = toVector(kByteBuckets); + ASSERT_EQ(converted.size(), kByteBuckets.size()); + EXPECT_TRUE(std::ranges::equal(converted, kByteBuckets)); +} + +TEST(HistogramBucketsConvert, toVectorHandlesAnEmptyLadder) +{ + EXPECT_TRUE(toVector(std::span{}).empty()); +} + +} // namespace xrpl::telemetry::buckets From 76c90512030f1044688b18268ced2c660592ed00 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:11:32 +0100 Subject: [PATCH 2/2] feat(insight): let an Event declare what it measures beast::insight::Event documents itself as carrying "a millisecond time, or other integral value", but both backends assumed the first case: the OTel bridge declared every instrument with unit `ms` and StatsD tagged every sample `|ms`. One Event does not measure time -- ServerHandler's "size" records the serialized RPC response length -- so it exported as rpc_size_milliseconds and inherited the millisecond bucket ladder. A quarter of its samples landed above that ladder's top edge, and since Prometheus returns the second-highest edge for a quantile in the `+Inf` bucket, its p95 panel showed a flat 5.00 kB rather than a measurement. Adds beast::insight::Unit (Millis, Bytes) plus otelUnitCode(), carried on EventImpl and selectable at makeEvent(). Naming the unit at creation is what lets a backend pick the export unit and, through it, the bucket ladder. - Collector gains a virtual makeEvent(name, Unit) whose default delegates to the millisecond overload, so a collector that cannot act on a unit keeps working unchanged. NullCollector and the Groups wrapper override it. - The Groups override matters most: call sites reach a collector through a Group, so forwarding only the prefixed name would silently drop the unit. A test covers that hop specifically. - Event gains notify(std::uint64_t) for non-duration samples, replacing ServerHandler's `Event::value_type{response.size()}` -- wrapping a byte count in a std::chrono::milliseconds compiles but reads as a duration to everything downstream. - EventImpl::value_type stays std::chrono::milliseconds. Widening it would change the wire value of every existing StatsD timer, and metrics needing finer resolution use the OTel-native microsecond instruments. The StatsD collector deliberately keeps emitting `|ms`: that path is retired here (its UDP port is commented out of the compose file and the integration test fails if anything listens on 8125), so changing its wire format would alter a legacy contract with no consumer and no way to verify it. The exported name does not change yet -- OTelEventImpl still hardcodes its unit. That follows with the unit-keyed histogram views. --- include/xrpl/beast/insight/Collector.h | 27 ++++ include/xrpl/beast/insight/Event.h | 18 +++ include/xrpl/beast/insight/EventImpl.h | 39 +++++ include/xrpl/beast/insight/Unit.h | 69 ++++++++ src/libxrpl/beast/insight/Groups.cpp | 14 ++ src/libxrpl/beast/insight/NullCollector.cpp | 19 ++- src/tests/libxrpl/beast/insight/Unit.cpp | 166 ++++++++++++++++++++ src/xrpld/rpc/detail/ServerHandler.cpp | 9 +- 8 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 include/xrpl/beast/insight/Unit.h create mode 100644 src/tests/libxrpl/beast/insight/Unit.cpp diff --git a/include/xrpl/beast/insight/Collector.h b/include/xrpl/beast/insight/Collector.h index d3e7f4d5e5..c4ba20e3f4 100644 --- a/include/xrpl/beast/insight/Collector.h +++ b/include/xrpl/beast/insight/Collector.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include @@ -103,6 +104,24 @@ public: virtual Event makeEvent(std::string const& name) = 0; + /** + * Create an event whose samples measure `unit` rather than milliseconds. + * + * The default delegates to the millisecond overload, so a collector that + * cannot act on a unit keeps working unchanged -- the StatsD collector + * relies on this. Collectors that map a unit onto an export format, such + * as the OTel collector, override it. + * + * @param name Metric name, already prefixed if it came through a Group. + * @param unit What the samples measure. + */ + virtual Event + makeEvent(std::string const& name, Unit unit) + { + (void)unit; + return makeEvent(name); + } + Event makeEvent(std::string const& prefix, std::string const& name) { @@ -110,6 +129,14 @@ public: return makeEvent(name); return makeEvent(prefix + "." + name); } + + Event + makeEvent(std::string const& prefix, std::string const& name, Unit unit) + { + if (prefix.empty()) + return makeEvent(name, unit); + return makeEvent(prefix + "." + name, unit); + } /** @} */ /** diff --git a/include/xrpl/beast/insight/Event.h b/include/xrpl/beast/insight/Event.h index c3ff1a8877..9640e2c1b4 100644 --- a/include/xrpl/beast/insight/Event.h +++ b/include/xrpl/beast/insight/Event.h @@ -51,6 +51,24 @@ public: impl_->notify(ceil(value)); } + /** + * Push a raw integral sample. + * + * For Events whose unit is not a duration, such as a byte count. The + * value is stored in the same integral field the duration overload uses + * and is interpreted per the Event's unit by the backend. + * + * Prefer this over constructing an `Event::value_type` at the call site: + * wrapping a byte count in a `std::chrono::milliseconds` compiles, but + * reads as a duration to everything downstream. + */ + void + notify(std::uint64_t value) const + { + if (impl_) + impl_->notify(value_type{value}); + } + [[nodiscard]] std::shared_ptr const& impl() const { diff --git a/include/xrpl/beast/insight/EventImpl.h b/include/xrpl/beast/insight/EventImpl.h index ede649d195..aa2298150f 100644 --- a/include/xrpl/beast/insight/EventImpl.h +++ b/include/xrpl/beast/insight/EventImpl.h @@ -1,5 +1,7 @@ #pragma once +#include + #include #include @@ -10,11 +12,48 @@ class Event; class EventImpl : public std::enable_shared_from_this { public: + /** + * The integral type every sample is stored as. + * + * Named for the common case -- durations -- and deliberately left as a + * duration type. Widening it would change the wire value of every + * existing StatsD timer, and metrics that need finer resolution than a + * whole millisecond use the OTel-native microsecond instruments instead. + * A sample whose unit() is not a duration is carried in the same integral + * field and interpreted per unit() by the backend. + */ using value_type = std::chrono::milliseconds; virtual ~EventImpl() = 0; virtual void notify(value_type const& value) = 0; + + /** + * @brief What this Event's samples measure. Fixed at construction. + * + * The OTel backend reads this to choose the instrument's declared unit + * and, through that, its bucket ladder. The StatsD backend ignores it. + */ + [[nodiscard]] Unit + unit() const noexcept + { + return unit_; + } + +protected: + /** + * @param unit What the samples measure. Defaults to milliseconds so + * existing implementations keep their behaviour unchanged. + */ + explicit EventImpl(Unit unit = Unit::Millis) : unit_(unit) + { + } + +private: + /** + * What the samples measure; selects the export unit and bucket ladder. + */ + Unit unit_; }; } // namespace beast::insight diff --git a/include/xrpl/beast/insight/Unit.h b/include/xrpl/beast/insight/Unit.h new file mode 100644 index 0000000000..4ddfaa51d5 --- /dev/null +++ b/include/xrpl/beast/insight/Unit.h @@ -0,0 +1,69 @@ +#pragma once + +#include + +namespace beast::insight { + +/** + * @brief What an Event's samples measure. + * + * `Event` documents itself as carrying "a millisecond time, or other integral + * value", but both backends used to assume the first case: the OTel bridge + * declared every instrument with unit `ms`, and StatsD tagged every sample + * `|ms`. A size metric therefore exported under a `_milliseconds` name and + * inherited a latency bucket ladder, which censored a quarter of its samples + * and pinned its p95 to a constant. + * + * Naming the unit at creation time is what lets the OTel bridge pick both the + * instrument unit and the matching bucket ladder: + * + * makeEvent("time", Unit::Millis) --> OTel unit "ms" --> millisecond ladder + * makeEvent("size", Unit::Bytes) --> OTel unit "By" --> byte ladder + * + * The StatsD backend deliberately ignores this and keeps emitting `|ms` for + * every Event. That path is retired here -- its UDP port is commented out of + * the compose file and the integration test fails if anything is listening on + * 8125 -- so changing its wire format would alter a legacy contract for no + * local benefit and with no way to verify it. + * + * @note Adding a member requires extending otelUnitCode(), which switches + * exhaustively so a new member is a compile error rather than a silent + * fallthrough to milliseconds. + */ +enum class Unit : std::uint8_t { + /** + * Whole milliseconds. The default, and what every duration Event uses. + */ + Millis, + + /** + * A byte count, such as a serialized response size. + */ + Bytes +}; + +/** + * @brief The OTel (UCUM) unit code for a Unit. + * + * The collector's Prometheus exporter derives the exported metric-name suffix + * from this code, so `ms` yields `_milliseconds` and `By` yields `_bytes`. It + * is also the key the histogram views match on, which is how each unit gets + * its own bucket ladder. + * + * @param unit The unit to translate. + * @return A static, null-terminated UCUM code. + */ +constexpr char const* +otelUnitCode(Unit unit) noexcept +{ + switch (unit) + { + case Unit::Bytes: + return "By"; + case Unit::Millis: + break; + } + return "ms"; +} + +} // namespace beast::insight diff --git a/src/libxrpl/beast/insight/Groups.cpp b/src/libxrpl/beast/insight/Groups.cpp index 6a60c75aa2..a3126d1d75 100644 --- a/src/libxrpl/beast/insight/Groups.cpp +++ b/src/libxrpl/beast/insight/Groups.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -56,12 +57,25 @@ public: return collector_->makeCounter(makeName(name)); } + using Collector::makeEvent; + Event makeEvent(std::string const& name) override { return collector_->makeEvent(makeName(name)); } + // Forwards the unit as well as the prefixed name. Without this override + // the base-class default would delegate to the single-argument overload + // above and silently drop the unit, which is how a byte-valued Event ends + // up declared as milliseconds -- call sites reach a collector through a + // Group, so this is the hop that actually matters. + Event + makeEvent(std::string const& name, Unit unit) override + { + return collector_->makeEvent(makeName(name), unit); + } + Gauge makeGauge(std::string const& name) override { diff --git a/src/libxrpl/beast/insight/NullCollector.cpp b/src/libxrpl/beast/insight/NullCollector.cpp index 03a12ee498..f5b444b3d3 100644 --- a/src/libxrpl/beast/insight/NullCollector.cpp +++ b/src/libxrpl/beast/insight/NullCollector.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -49,7 +50,15 @@ public: class NullEventImpl : public EventImpl { public: - explicit NullEventImpl() = default; + /** + * @param unit What the samples would measure. Recorded even though + * nothing is collected, so a caller can still read back the + * unit it asked for -- which is what makes the null collector + * usable for testing the unit plumbing. + */ + explicit NullEventImpl(Unit unit = Unit::Millis) : EventImpl(unit) + { + } void notify(value_type const&) override @@ -119,12 +128,20 @@ public: return Counter(std::make_shared()); } + using Collector::makeEvent; + Event makeEvent(std::string const&) override { return Event(std::make_shared()); } + Event + makeEvent(std::string const&, Unit unit) override + { + return Event(std::make_shared(unit)); + } + Gauge makeGauge(std::string const&) override { diff --git a/src/tests/libxrpl/beast/insight/Unit.cpp b/src/tests/libxrpl/beast/insight/Unit.cpp new file mode 100644 index 0000000000..dee9e45e50 --- /dev/null +++ b/src/tests/libxrpl/beast/insight/Unit.cpp @@ -0,0 +1,166 @@ +/** + * GTest unit tests for beast::insight::Unit and its plumbing. + * + * A metric's unit decides two things that are invisible at the call site: the + * name suffix the exporter appends, and which bucket ladder the histogram + * view applies. Getting it wrong is silent -- a byte count declared as + * milliseconds still records, still exports, still draws a graph, and the + * graph is wrong. So each hop the unit has to survive is asserted here + * rather than left to inspection. + * + * The hop that matters most is the group wrapper. Call sites reach a + * collector through Groups, so a unit that reaches OTelCollector correctly + * but is dropped by the group prefixing layer would pass a naive test while + * failing in production. + */ + +#include + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace beast::insight { + +namespace { + +/** + * An EventImpl that records what it was notified with. + * + * Needed because every shipped implementation either discards the sample + * (NullCollector) or sends it somewhere external. Asserting the recorded + * value proves the raw-integral path preserves it, rather than only proving + * that notify() can be called without crashing. + */ +class RecordingEventImpl : public EventImpl +{ +public: + explicit RecordingEventImpl(Unit unit) : EventImpl(unit) + { + } + + void + notify(value_type const& value) override + { + samples.push_back(value); + } + + /** + * Every value passed to notify(), in call order. + */ + std::vector samples; +}; + +} // namespace + +// The unit code is a contract with the collector's Prometheus exporter: it +// derives the exported name suffix from this string. Assert the exact codes, +// not merely that they differ. +TEST(InsightUnit, otelCodeIsTheUcumCodeForEachUnit) +{ + EXPECT_STREQ(otelUnitCode(Unit::Millis), "ms"); + EXPECT_STREQ(otelUnitCode(Unit::Bytes), "By"); +} + +TEST(InsightUnit, defaultEventUnitIsMillisForBackwardCompatibility) +{ + // Every pre-existing makeEvent(name) call site records a duration, so the + // one-argument overload must keep meaning milliseconds. + auto const collector = NullCollector::make(); + auto const event = collector->makeEvent("legacy"); + ASSERT_NE(event.impl(), nullptr); + EXPECT_EQ(event.impl()->unit(), Unit::Millis); +} + +TEST(InsightUnit, makeEventCarriesTheRequestedUnitToTheImpl) +{ + auto const collector = NullCollector::make(); + auto const event = collector->makeEvent("size", Unit::Bytes); + ASSERT_NE(event.impl(), nullptr); + EXPECT_EQ(event.impl()->unit(), Unit::Bytes); +} + +TEST(InsightUnit, prefixedMakeEventCarriesTheUnit) +{ + auto const collector = NullCollector::make(); + auto const event = collector->makeEvent("rpc", "size", Unit::Bytes); + ASSERT_NE(event.impl(), nullptr); + EXPECT_EQ(event.impl()->unit(), Unit::Bytes); +} + +TEST(InsightUnit, groupWrapperForwardsTheUnitAlongWithThePrefix) +{ + // ServerHandler creates its events through a Group, not through the + // collector directly. If the group's makeEvent override forwards only the + // name, the unit silently reverts to milliseconds and the byte histogram + // inherits the latency ladder again. + auto const collector = NullCollector::make(); + auto const groups = makeGroups(collector); + auto const event = groups->get("rpc")->makeEvent("size", Unit::Bytes); + ASSERT_NE(event.impl(), nullptr); + EXPECT_EQ(event.impl()->unit(), Unit::Bytes); +} + +TEST(InsightUnit, groupWrapperStillDefaultsToMillis) +{ + auto const collector = NullCollector::make(); + auto const groups = makeGroups(collector); + auto const event = groups->get("rpc")->makeEvent("time"); + ASSERT_NE(event.impl(), nullptr); + EXPECT_EQ(event.impl()->unit(), Unit::Millis); +} + +TEST(InsightUnit, rawIntegralNotifyPreservesTheValueExactly) +{ + // The byte path must not be rounded or scaled on its way through the + // duration-typed storage field. + auto const impl = std::make_shared(Unit::Bytes); + Event const event(impl); + + event.notify(std::uint64_t{4096}); + event.notify(std::uint64_t{0}); + event.notify(std::uint64_t{1'048'577}); + + ASSERT_EQ(impl->samples.size(), 3U); + EXPECT_EQ(impl->samples[0].count(), 4096); + EXPECT_EQ(impl->samples[1].count(), 0); + EXPECT_EQ(impl->samples[2].count(), 1'048'577); +} + +TEST(InsightUnit, durationNotifyStillRoundsUpToWholeMilliseconds) +{ + // Pre-existing behaviour, asserted so the new overload cannot quietly + // change it: Event applies ceil to whole milliseconds, which is why + // sub-millisecond resolution is impossible on this path. + auto const impl = std::make_shared(Unit::Millis); + Event const event(impl); + + event.notify(std::chrono::microseconds{40}); + event.notify(std::chrono::microseconds{1'000}); + event.notify(std::chrono::milliseconds{7}); + + ASSERT_EQ(impl->samples.size(), 3U); + EXPECT_EQ(impl->samples[0].count(), 1) << "40us must round up to 1ms, not down to 0"; + EXPECT_EQ(impl->samples[1].count(), 1); + EXPECT_EQ(impl->samples[2].count(), 7); +} + +TEST(InsightUnit, notifyOnANullEventIsSafeForBothOverloads) +{ + // A default-constructed Event has no impl. Both overloads must be no-ops + // rather than dereferencing null. + Event const none; + ASSERT_EQ(none.impl(), nullptr); + EXPECT_NO_THROW(none.notify(std::uint64_t{4096})); + EXPECT_NO_THROW(none.notify(std::chrono::milliseconds{5})); +} + +} // namespace beast::insight diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 0376345611..ebf318b247 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -183,7 +184,11 @@ ServerHandler::ServerHandler( { auto const& group(cm.group("rpc")); rpcRequests_ = group->makeCounter("requests"); - rpcSize_ = group->makeEvent("size"); + // "size" measures the serialized response in bytes, not a duration. It + // has to say so: the unit picks both the exported name suffix and the + // histogram bucket ladder, and borrowing the millisecond ladder censored + // a quarter of these samples. + rpcSize_ = group->makeEvent("size", beast::insight::Unit::Bytes); rpcTime_ = group->makeEvent("time"); } @@ -1125,7 +1130,7 @@ ServerHandler::processRequest( std::chrono::duration_cast( std::chrono::high_resolution_clock::now() - start)); ++rpcRequests_; - rpcSize_.notify(beast::insight::Event::value_type{response.size()}); + rpcSize_.notify(static_cast(response.size())); response += '\n';