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