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] 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