mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-22 06:40:53 +00:00
This is the change that actually lifts the 5 s ceiling. Until now the
millisecond ladder and the Unit type existed but nothing consumed them.
Telemetry.cpp registered ONE histogram view: instrument name pattern "*",
unit exactly "ms", boundaries {1, 5, ..., 1000, 5000}. Verified against the
installed SDK, "*" matches every name and "ms" matches exactly, so that view
governed every beast::insight Event -- all 54 of them, whatever they measure.
Measured on devnet: 24.9% of rpc_size samples and 100% of jobq_updatepaths
samples fell above 5000. A quantile landing in the `+Inf` bucket reads back
as the second-highest edge, so those p95s reported a flat 5000 rather than a
measurement, and the 1 s to 5 s span was a single four-second-wide bucket
that any quantile inside it had to interpolate across.
Replaces it with one view per unit, keyed on the unit an instrument declares:
- `ms` gets kMillisecondBuckets: every representable edge of the collector's
spanmetrics ladder, plus 60 s and 120 s. The extensions are deliberate --
jobq_updatepaths was measured averaging 59,956 ms, which no span
approaches, so parity alone would still censor it.
- `By` gets kByteBuckets, placed from the measured response distribution
(mean 2131 B, half under 1 kB, tail mean bounded at 7538 B).
OTelEventImpl now derives its declared unit AND its description from unit()
instead of hardcoding "Duration in ms"/"ms", so rpc_size exports as
rpc_size_bytes on the byte ladder. rpc-pathfinding's "RPC Response Size"
panel follows the rename; its unit was already decbytes and is now truthful.
Also corrects Phase7_taskList.md, which still specified the 5000 ladder as
"matching SpanMetrics". That was true when written and became false when the
collector ladder was extended on its own -- implementing the plan as written
reproduced the bug, so the spec is where the defect had come to live. The
edges now have exactly one owner and the plan points at it.
175 lines
5.9 KiB
C++
175 lines
5.9 KiB
C++
/**
|
|
* 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");
|
|
}
|
|
|
|
// The description is what an operator reads in the metric catalogue, so a
|
|
// byte-valued instrument must not describe itself as a duration.
|
|
TEST(InsightUnit, descriptionMatchesWhatTheUnitActuallyMeasures)
|
|
{
|
|
EXPECT_STREQ(otelUnitDescription(Unit::Millis), "Duration in ms");
|
|
EXPECT_STREQ(otelUnitDescription(Unit::Bytes), "Size in bytes");
|
|
}
|
|
|
|
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
|