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