From 24094e427b70d5f57f15814e724f5a05b3ec9081 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:30:38 +0100 Subject: [PATCH] fix(telemetry): give each histogram unit its own bucket ladder 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. --- OpenTelemetryPlan/Phase7_taskList.md | 2 +- .../grafana/dashboards/rpc-pathfinding.json | 6 +- include/xrpl/beast/insight/Unit.h | 25 ++++++++ src/libxrpl/beast/insight/OTelCollector.cpp | 39 +++++++++--- src/libxrpl/telemetry/Telemetry.cpp | 59 +++++++++++-------- src/tests/libxrpl/beast/insight/Unit.cpp | 8 +++ 6 files changed, 104 insertions(+), 35 deletions(-) diff --git a/OpenTelemetryPlan/Phase7_taskList.md b/OpenTelemetryPlan/Phase7_taskList.md index d7ce34b7b8..f0331d4c46 100644 --- a/OpenTelemetryPlan/Phase7_taskList.md +++ b/OpenTelemetryPlan/Phase7_taskList.md @@ -53,7 +53,7 @@ - **OTelCounterImpl**: Wraps `opentelemetry::metrics::Counter`. `increment(amount)` calls `counter->Add(amount)`. - **OTelGaugeImpl**: Uses `opentelemetry::metrics::ObservableGauge` with an async callback. `set(value)` stores value atomically; callback reads it during collection. - **OTelMeterImpl**: Wraps `opentelemetry::metrics::Counter`. `increment(amount)` calls `counter->Add(amount)`. Semantically identical to Counter but unsigned. - - **OTelEventImpl**: Wraps `opentelemetry::metrics::Histogram`. `notify(duration)` calls `histogram->Record(duration.count())`. Uses explicit bucket boundaries matching SpanMetrics: [1, 5, 10, 25, 50, 100, 250, 500, 1000, 5000] ms. + - **OTelEventImpl**: Wraps `opentelemetry::metrics::Histogram`. `notify()` calls `histogram->Record(value.count())`. Declares its unit from `beast::insight::Unit`, which is what selects its bucket ladder: the histogram views in `Telemetry.cpp` match on unit, so a `ms` instrument gets the millisecond ladder and a `By` instrument the byte ladder. Bucket edges live in `include/xrpl/telemetry/HistogramBuckets.h` — do not restate them here. The millisecond ladder must contain every representable edge of the collector's spanmetrics ladder and may extend above it (jobs outlive spans); `.github/scripts/telemetry/check_bucket_parity.py` enforces that. An earlier version of this line specified `[1, 5, 10, 25, 50, 100, 250, 500, 1000, 5000] ms` as "matching SpanMetrics" — true when written, then silently false once the collector ladder was extended on its own, which capped every quantile above 5s at a flat 5000. - **OTelHookImpl**: Stores handler function. Called during periodic metric collection (same 1s pattern via PeriodicMetricReader). - **OTelCollectorImp**: Main class. - Creates `MeterProvider` with `PeriodicMetricReader` (1s export interval) diff --git a/docker/telemetry/grafana/dashboards/rpc-pathfinding.json b/docker/telemetry/grafana/dashboards/rpc-pathfinding.json index eb97770086..83c7c975f6 100644 --- a/docker/telemetry/grafana/dashboards/rpc-pathfinding.json +++ b/docker/telemetry/grafana/dashboards/rpc-pathfinding.json @@ -114,7 +114,7 @@ }, { "title": "RPC Response Size", - "description": "**⚠ Instrument mismatch — values unreliable.** Response size is recorded through the millisecond-scaled event histogram (rpc_size_milliseconds_bucket), so byte values saturate at the top time bucket (5000) and the percentiles are not true byte sizes. A dedicated byte-unit histogram is needed to fix this; tracked separately. Treat this panel as indicative only until then.\n\n**What:** P95 and P50 of RPC response payload size in bytes.\n**How it's computed:** 95th and 50th percentiles over the dashboard rate interval.\n**Reading it:** Larger responses cost more bandwidth and CPU to build.\n**Healthy range:** workload-dependent.\n**Watch for:** Large P95 (result-heavy queries such as broad account_tx, or API misuse).\n**Source:** src/xrpld/rpc/detail/ServerHandler.cpp ServerHandler ctor", + "description": "**⚠ Instrument mismatch — values unreliable.** Response size is recorded through the millisecond-scaled event histogram (rpc_size_bytes_bucket), so byte values saturate at the top time bucket (5000) and the percentiles are not true byte sizes. A dedicated byte-unit histogram is needed to fix this; tracked separately. Treat this panel as indicative only until then.\n\n**What:** P95 and P50 of RPC response payload size in bytes.\n**How it's computed:** 95th and 50th percentiles over the dashboard rate interval.\n**Reading it:** Larger responses cost more bandwidth and CPU to build.\n**Healthy range:** workload-dependent.\n**Watch for:** Large P95 (result-heavy queries such as broad account_tx, or API misuse).\n**Source:** src/xrpld/rpc/detail/ServerHandler.cpp ServerHandler ctor", "type": "timeseries", "gridPos": { "h": 10, @@ -134,7 +134,7 @@ "datasource": { "type": "prometheus" }, - "expr": "histogram_quantile(0.95, sum by (le, service_instance_id) (rate(rpc_size_milliseconds_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))", + "expr": "histogram_quantile(0.95, sum by (le, service_instance_id) (rate(rpc_size_bytes_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))", "interval": "15s", "legendFormat": "P95 Response Size [{{service_instance_id}}]" }, @@ -142,7 +142,7 @@ "datasource": { "type": "prometheus" }, - "expr": "histogram_quantile(0.5, sum by (le, service_instance_id) (rate(rpc_size_milliseconds_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))", + "expr": "histogram_quantile(0.5, sum by (le, service_instance_id) (rate(rpc_size_bytes_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[5m])))", "interval": "15s", "legendFormat": "P50 Response Size [{{service_instance_id}}]" } diff --git a/include/xrpl/beast/insight/Unit.h b/include/xrpl/beast/insight/Unit.h index 4ddfaa51d5..cd9c863d1e 100644 --- a/include/xrpl/beast/insight/Unit.h +++ b/include/xrpl/beast/insight/Unit.h @@ -66,4 +66,29 @@ otelUnitCode(Unit unit) noexcept return "ms"; } +/** + * @brief Human-readable description for an instrument of this unit. + * + * Exported alongside the metric, so this is the text an operator reads in a + * metric catalogue. A byte-valued instrument that describes itself as a + * duration is exactly the confusion this whole type exists to remove, so the + * description is derived from the unit rather than written out at each + * instrument site. + * + * @param unit The unit to describe. + * @return A static, null-terminated description. + */ +constexpr char const* +otelUnitDescription(Unit unit) noexcept +{ + switch (unit) + { + case Unit::Bytes: + return "Size in bytes"; + case Unit::Millis: + break; + } + return "Duration in ms"; +} + } // namespace beast::insight diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index 1839825898..1301c72652 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -42,6 +42,7 @@ #include #include #include +#include #include #include @@ -169,10 +170,17 @@ private: /** * @brief OTel-backed implementation of beast::insight::EventImpl. * - * Wraps an OTel Histogram instrument. Each notify() call - * records the duration in milliseconds. Uses explicit bucket boundaries - * matching the SpanMetrics connector configuration: - * [1, 5, 10, 25, 50, 100, 250, 500, 1000, 5000] ms + * Wraps an OTel Histogram instrument. Each notify() call records one + * sample, interpreted per the Event's unit(). + * + * The instrument's declared unit is what selects its bucket ladder: the + * histogram views registered in Telemetry.cpp match on unit, so a `ms` + * instrument gets the millisecond ladder and a `By` instrument the byte + * ladder. The edges themselves live in xrpl/telemetry/HistogramBuckets.h -- + * do not restate them here. An earlier version of this comment listed + * `[1, 5, ..., 1000, 5000] ms` as "matching the SpanMetrics connector"; that + * was true when written and silently became false when the connector's + * ladder was extended, which is why the edges now have one owner. * * Thread safety: OTel Histogram::Record() is thread-safe by specification. */ @@ -184,10 +192,14 @@ public: * formatName() by the collector: prefix prepended and * dots replaced with underscores (e.g. "xrpld_rpc_size"). * @param meter OTel Meter used to create the histogram instrument. + * @param unit What the samples measure. Selects the instrument's + * declared unit, its description, and through the unit the + * bucket ladder a histogram view applies. */ OTelEventImpl( std::string const& name, - opentelemetry::nostd::shared_ptr const& meter); + opentelemetry::nostd::shared_ptr const& meter, + Unit unit); ~OTelEventImpl() override = default; @@ -469,6 +481,9 @@ public: Event makeEvent(std::string const& name) override; + Event + makeEvent(std::string const& name, Unit unit) override; + Gauge makeGauge(std::string const& name) override; @@ -644,8 +659,10 @@ OTelCounterImpl::increment(value_type amount) OTelEventImpl::OTelEventImpl( std::string const& name, - opentelemetry::nostd::shared_ptr const& meter) - : histogram_(meter->CreateDoubleHistogram(name, "Duration in ms", "ms")) + opentelemetry::nostd::shared_ptr const& meter, + Unit unit) + : EventImpl(unit) + , histogram_(meter->CreateDoubleHistogram(name, otelUnitDescription(unit), otelUnitCode(unit))) { } @@ -834,7 +851,13 @@ OTelCollectorImp::makeCounter(std::string const& name) Event OTelCollectorImp::makeEvent(std::string const& name) { - return Event(std::make_shared(formatName(name), otelMeter_)); + return makeEvent(name, Unit::Millis); +} + +Event +OTelCollectorImp::makeEvent(std::string const& name, Unit unit) +{ + return Event(std::make_shared(formatName(name), otelMeter_, unit)); } Gauge diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index bc0135124e..cac48144a9 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -19,10 +19,12 @@ #include #include +#include #include #include #include #include +#include #include #include @@ -405,30 +407,41 @@ class TelemetryImpl : public Telemetry std::make_unique(), makeResource()); meterProvider_->AddMetricReader(std::move(reader)); - // Histogram view: SpanMetrics-compatible bucket boundaries (ms) so - // histogram instruments align with the collector's SpanMetrics. - auto histogramSelector = metrics_sdk::InstrumentSelectorFactory::Create( - metrics_sdk::InstrumentType::kHistogram, "*", "ms"); + // One histogram view per unit. The unit is the selector, so an + // instrument gets the ladder that fits what it measures -- a byte + // count no longer inherits a latency ladder. Edges come from + // HistogramBuckets.h, which owns every ladder. + // + // Both views keep the "*" name pattern and an EMPTY view name: a + // non-empty view name would rename every matching histogram to it and + // collapse them into a single series. + // + // The meter selector MUST match the meter name used by getMeter() and + // the beast OTelCollector, or a view never applies and instruments + // fall back to the SDK default ladder (ceiling 10,000). + auto const addUnitView = [this]( + std::string const& unitCode, + std::vector boundaries, + std::string const& description) { + auto selector = metrics_sdk::InstrumentSelectorFactory::Create( + metrics_sdk::InstrumentType::kHistogram, "*", unitCode); + auto meterSelector = + metrics_sdk::MeterSelectorFactory::Create(std::string(kMeterName), "", ""); + auto config = std::make_shared(); + config->boundaries_ = std::move(boundaries); + auto view = metrics_sdk::ViewFactory::Create( + "", description, metrics_sdk::AggregationType::kHistogram, std::move(config)); + meterProvider_->AddView(std::move(selector), std::move(meterSelector), std::move(view)); + }; - // Must match the meter name used by getMeter() and the beast - // OTelCollector, or the view never applies. - auto meterSelector = - metrics_sdk::MeterSelectorFactory::Create(std::string(kMeterName), "", ""); - - auto histogramConfig = std::make_shared(); - histogramConfig->boundaries_ = - std::vector{1.0, 5.0, 10.0, 25.0, 50.0, 100.0, 250.0, 500.0, 1000.0, 5000.0}; - - // An empty view name applies the buckets without renaming. A name here - // would collapse every matching histogram into one series. - auto histogramView = metrics_sdk::ViewFactory::Create( - "", - "SpanMetrics-compatible histogram buckets", - metrics_sdk::AggregationType::kHistogram, - std::move(histogramConfig)); - - meterProvider_->AddView( - std::move(histogramSelector), std::move(meterSelector), std::move(histogramView)); + addUnitView( + beast::insight::otelUnitCode(beast::insight::Unit::Millis), + buckets::toVector(buckets::kMillisecondBuckets), + "Duration buckets, 1 ms to 120 s"); + addUnitView( + beast::insight::otelUnitCode(beast::insight::Unit::Bytes), + buckets::toVector(buckets::kByteBuckets), + "Size buckets, 512 B to 1 MiB"); // Publish as the global meter provider so developers (and the beast // OTelCollector shim) reach the same pipeline. diff --git a/src/tests/libxrpl/beast/insight/Unit.cpp b/src/tests/libxrpl/beast/insight/Unit.cpp index dee9e45e50..29ceac2b32 100644 --- a/src/tests/libxrpl/beast/insight/Unit.cpp +++ b/src/tests/libxrpl/beast/insight/Unit.cpp @@ -70,6 +70,14 @@ TEST(InsightUnit, otelCodeIsTheUcumCodeForEachUnit) 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