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/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/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/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 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';