diff --git a/OpenTelemetryPlan/Phase7_taskList.md b/OpenTelemetryPlan/Phase7_taskList.md index be90294ff8..a9e9abef99 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 80d92f7bb8..8feac10762 100644 --- a/docker/telemetry/grafana/dashboards/rpc-pathfinding.json +++ b/docker/telemetry/grafana/dashboards/rpc-pathfinding.json @@ -154,7 +154,7 @@ }, { "title": "RPC Response Size", - "description": "\u26a0 Instrument mismatch \u2014 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 this is:\n*The 95th-percentile size of RPC response payloads in bytes.*\n\n###### How it's computed:\n*95th-percentile of response payload sizes over the dashboard rate interval, per node.*\n\n###### Reading it:\n*Smaller is cheaper; large responses cost bandwidth and memory.*\n\n###### Healthy range:\n*Workload-dependent; small for status queries, large for bulk data queries.*\n\n###### Watch for:\n*Growth in large responses, consistent with expensive queries or API misuse.*\n\n###### Keywords:\n- **RPC command / method** *(per node)* \u2014 a named API request served by the node (e.g. account_info, ledger, submit), the unit RPC panels break down by.\n\n###### Computation boundary:\n*Result: Per node \u2014 each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[ServerHandler.cpp](https://github.com/XRPLF/rippled/blob/develop/src/xrpld/rpc/detail/ServerHandler.cpp)\n\n###### Function:\n`ServerHandler ctor`\n\n###### References:\n[RPC command / method](https://xrpl.org/docs/references/http-websocket-apis/public-api-methods) \u00b7 [Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#rpc-command-method)", + "description": "\u26a0 Instrument mismatch \u2014 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 this is:\n*The 95th-percentile size of RPC response payloads in bytes.*\n\n###### How it's computed:\n*95th-percentile of response payload sizes over the dashboard rate interval, per node.*\n\n###### Reading it:\n*Smaller is cheaper; large responses cost bandwidth and memory.*\n\n###### Healthy range:\n*Workload-dependent; small for status queries, large for bulk data queries.*\n\n###### Watch for:\n*Growth in large responses, consistent with expensive queries or API misuse.*\n\n###### Keywords:\n- **RPC command / method** *(per node)* \u2014 a named API request served by the node (e.g. account_info, ledger, submit), the unit RPC panels break down by.\n\n###### Computation boundary:\n*Result: Per node \u2014 each series is one server's own value.*\n*Recorded in xrpld code as a native metric (beast::insight); the collector only forwards it; the Grafana query selects and aggregates it.*\n\n###### Source:\n[ServerHandler.cpp](https://github.com/XRPLF/rippled/blob/develop/src/xrpld/rpc/detail/ServerHandler.cpp)\n\n###### Function:\n`ServerHandler ctor`\n\n###### References:\n[RPC command / method](https://xrpl.org/docs/references/http-websocket-apis/public-api-methods) \u00b7 [Telemetry glossary](https://github.com/XRPLF/rippled/blob/develop/docs/telemetry-glossary.md#rpc-command-method)", "type": "timeseries", "gridPos": { "h": 10, @@ -175,7 +175,7 @@ "type": "prometheus", "uid": "${DS_PROMETHEUS}" }, - "expr": "label_replace(label_join(label_replace(histogram_quantile(0.95, sum by (le, service_instance_id, xrpl_branch, xrpl_node_role, xrpl_work_item) (rate(rpc_size_milliseconds_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\", xrpl_work_item=~\"$xrpl_work_item\", xrpl_branch=~\"$xrpl_branch\", xrpl_node_role=~\"$xrpl_node_role\"}[5m]))), \"series\", \"P95 Response Size\", \"\", \"\"), \"xrpl_ident\", \", \", \"service_instance_id\", \"xrpl_branch\", \"xrpl_work_item\"), \"xrpl_ident\", \"[$1]\", \"xrpl_ident\", \"(?:, )*(.*[^, ])(?:, )*\")" + "expr": "label_replace(label_join(label_replace(histogram_quantile(0.95, sum by (le, service_instance_id, xrpl_branch, xrpl_node_role, xrpl_work_item) (rate(rpc_size_bytes_bucket{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\", xrpl_work_item=~\"$xrpl_work_item\", xrpl_branch=~\"$xrpl_branch\", xrpl_node_role=~\"$xrpl_node_role\"}[5m]))), \"series\", \"P95 Response Size\", \"\", \"\"), \"xrpl_ident\", \", \", \"service_instance_id\", \"xrpl_branch\", \"xrpl_work_item\"), \"xrpl_ident\", \"[$1]\", \"xrpl_ident\", \"(?:, )*(.*[^, ])(?:, )*\")" } ], "fieldConfig": { 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 db7bd74ecc..1d12dd0422 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include @@ -168,10 +169,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. */ @@ -183,10 +191,14 @@ public: * formatName() by the collector: lowercase, with `.` and * ` ` mapped to `_` (e.g. "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; @@ -474,6 +486,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; @@ -652,8 +667,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))) { } @@ -842,7 +859,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 74faead1fa..30c9a22b50 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 @@ -553,31 +555,46 @@ public: std::make_unique(), resourceAttrs); meterProvider_->AddMetricReader(std::move(reader)); - // Histogram view: SpanMetrics-compatible bucket boundaries (ms) so - // histogram instruments align with the collector's SpanMetrics. The - // view is created with an EMPTY name so it applies the buckets WITHOUT - // renaming instruments — a non-empty view name would collapse every - // matching histogram (ios_latency, rpc_size, rpc_time, pathfind_*) - // into a single series under that one name. - auto histogramSelector = metrics_sdk::InstrumentSelectorFactory::Create( - metrics_sdk::InstrumentType::kHistogram, "*", "ms"); - // Meter selector MUST match the meter name used by getMeter() and the - // beast OTelCollector (kMeterName = "xrpld"); otherwise this histogram - // view never applies and duration histograms fall back to the SDK - // default boundaries instead of these SpanMetrics-aligned buckets. - 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}; - auto histogramView = metrics_sdk::ViewFactory::Create( - "", // empty name: keep each instrument's own name, only set buckets - "SpanMetrics-compatible histogram buckets", - metrics_sdk::AggregationType::kHistogram, - histogramConfig); + // 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. + // + // Each view keeps the "*" name pattern and an EMPTY view name: a + // non-empty view name would rename every matching histogram to it and + // collapse them (ios_latency, rpc_size, rpc_time, pathfind_*, and all + // the jobq_* pairs) into a single series. + // + // The meter selector MUST match the meter name used by getMeter() and + // the beast OTelCollector (kMeterName = "xrpld"); otherwise a view + // never applies and instruments fall back to the SDK default ladder, + // whose ceiling is 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( + "", // empty name: keep each instrument's own name, only set buckets + description, + metrics_sdk::AggregationType::kHistogram, + std::move(config)); + meterProvider_->AddView(std::move(selector), std::move(meterSelector), std::move(view)); + }; - 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