Merge branch 'pratik/otel-phase8-log-correlation' into pratik/otel-phase9-metric-gap-fill

include/xrpl/beast/insight/Collector.h

@@ -6,6 +6,7 @@
 #include <xrpl/beast/insight/Hook.h>
 #include <xrpl/beast/insight/HookImpl.h>
 #include <xrpl/beast/insight/Meter.h>
+#include <xrpl/beast/insight/Unit.h>
 
 #include <memory>
 #include <string>
@@ -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);
+    }
     /** @} */
 
     /**

include/xrpl/beast/insight/Event.h

@@ -51,6 +51,24 @@ public:
             impl_->notify(ceil<value_type>(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<EventImpl> const&
     impl() const
     {

include/xrpl/beast/insight/EventImpl.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <xrpl/beast/insight/Unit.h>
+
 #include <chrono>
 #include <memory>
 
@@ -10,11 +12,48 @@ class Event;
 class EventImpl : public std::enable_shared_from_this<EventImpl>
 {
 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

include/xrpl/beast/insight/Unit.h

@@ -0,0 +1,69 @@
+#pragma once
+
+#include <cstdint>
+
+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

include/xrpl/telemetry/HistogramBuckets.h

@@ -0,0 +1,179 @@
+#pragma once
+
+#include <array>
+#include <cstddef>
+#include <span>
+#include <vector>
+
+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<double, N>` so they are constant-initialised and
+ * usable in a `static_assert`. The OTel SDK wants `std::vector<double>` 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<HistogramAggregationConfig>();
+ * 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<double const> 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<double>` the OTel SDK wants.
+ *
+ * @param ladder Bucket upper bounds.
+ * @return A vector holding the same edges in the same order.
+ */
+inline std::vector<double>
+toVector(std::span<double const> ladder)
+{
+    return std::vector<double>(ladder.begin(), ladder.end());
+}
+
+}  // namespace xrpl::telemetry::buckets

src/libxrpl/beast/insight/Groups.cpp

@@ -9,6 +9,7 @@
 #include <xrpl/beast/insight/Hook.h>
 #include <xrpl/beast/insight/HookImpl.h>
 #include <xrpl/beast/insight/Meter.h>
+#include <xrpl/beast/insight/Unit.h>
 
 #include <memory>
 #include <string>
@@ -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
     {

src/libxrpl/beast/insight/NullCollector.cpp

@@ -11,6 +11,7 @@
 #include <xrpl/beast/insight/HookImpl.h>
 #include <xrpl/beast/insight/Meter.h>
 #include <xrpl/beast/insight/MeterImpl.h>
+#include <xrpl/beast/insight/Unit.h>
 
 #include <memory>
 #include <string>
@@ -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<detail::NullCounterImpl>());
     }
 
+    using Collector::makeEvent;
+
     Event
     makeEvent(std::string const&) override
     {
         return Event(std::make_shared<detail::NullEventImpl>());
     }
 
+    Event
+    makeEvent(std::string const&, Unit unit) override
+    {
+        return Event(std::make_shared<detail::NullEventImpl>(unit));
+    }
+
     Gauge
     makeGauge(std::string const&) override
     {

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 <xrpl/beast/insight/Unit.h>
+
+#include <xrpl/beast/insight/Event.h>
+#include <xrpl/beast/insight/EventImpl.h>
+#include <xrpl/beast/insight/Groups.h>
+#include <xrpl/beast/insight/NullCollector.h>
+
+#include <gtest/gtest.h>
+
+#include <chrono>
+#include <cstdint>
+#include <memory>
+#include <vector>
+
+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<value_type> 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<RecordingEventImpl>(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<RecordingEventImpl>(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

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 <xrpl/telemetry/HistogramBuckets.h>
+
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <array>
+#include <cmath>
+#include <cstddef>
+#include <span>
+#include <vector>
+
+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<std::span<double const>>
+{
+};
+
+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<double const>{kMillisecondBuckets},
+        std::span<double const>{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<double const>{}));
+
+    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<double const>{}).empty());
+}
+
+}  // namespace xrpl::telemetry::buckets

src/xrpld/rpc/detail/ServerHandler.cpp

@@ -16,6 +16,7 @@
 #include <xrpl/basics/base64.h>
 #include <xrpl/basics/contract.h>
 #include <xrpl/basics/make_SSLContext.h>
+#include <xrpl/beast/insight/Unit.h>
 #include <xrpl/beast/net/IPAddress.h>
 #include <xrpl/beast/net/IPAddressConversion.h>
 #include <xrpl/beast/rfc2616.h>
@@ -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::milliseconds>(
             std::chrono::high_resolution_clock::now() - start));
     ++rpcRequests_;
-    rpcSize_.notify(beast::insight::Event::value_type{response.size()});
+    rpcSize_.notify(static_cast<std::uint64_t>(response.size()));
 
     response += '\n';