Merge branch 'pratik/otel-phase8-log-correlation' into pratik/otel-phase9-metric-gap-fill

# Conflicts: # docker/telemetry/grafana/dashboards/rpc-pathfinding.json # src/libxrpl/telemetry/Telemetry.cpp
2026-08-21 14:20:56 +00:00 · 2026-08-21 12:33:13 +01:00
parent 63dc5cce65 8169894ada
commit 7735d725fb
6 changed files with 108 additions and 35 deletions
--- a/OpenTelemetryPlan/Phase7_taskList.md
+++ b/OpenTelemetryPlan/Phase7_taskList.md
@@ -53,7 +53,7 @@
  - **OTelCounterImpl**: Wraps `opentelemetry::metrics::Counter<int64_t>`. `increment(amount)` calls `counter->Add(amount)`.
  - **OTelGaugeImpl**: Uses `opentelemetry::metrics::ObservableGauge<uint64_t>` with an async callback. `set(value)` stores value atomically; callback reads it during collection.
  - **OTelMeterImpl**: Wraps `opentelemetry::metrics::Counter<uint64_t>`. `increment(amount)` calls `counter->Add(amount)`. Semantically identical to Counter but unsigned.
-  - **OTelEventImpl**: Wraps `opentelemetry::metrics::Histogram<double>`. `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<double>`. `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)
--- 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": {
--- 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
--- a/src/libxrpl/beast/insight/OTelCollector.cpp
+++ b/src/libxrpl/beast/insight/OTelCollector.cpp
@@ -41,6 +41,7 @@
 #include <xrpl/beast/insight/Hook.h>
 #include <xrpl/beast/insight/HookImpl.h>
 #include <xrpl/beast/insight/MeterImpl.h>
+#include <xrpl/beast/insight/Unit.h>
 #include <xrpl/beast/utility/Journal.h>

 #include <opentelemetry/metrics/async_instruments.h>
@@ -168,10 +169,17 @@ private:
 /**
 * @brief OTel-backed implementation of beast::insight::EventImpl.
 *
- * Wraps an OTel Histogram<double> 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<double> 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<metrics_api::Meter> const& meter);
+        opentelemetry::nostd::shared_ptr<metrics_api::Meter> 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<metrics_api::Meter> const& meter)
-    : histogram_(meter->CreateDoubleHistogram(name, "Duration in ms", "ms"))
+    opentelemetry::nostd::shared_ptr<metrics_api::Meter> 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<OTelEventImpl>(formatName(name), otelMeter_));
+    return makeEvent(name, Unit::Millis);
+}
+
+Event
+OTelCollectorImp::makeEvent(std::string const& name, Unit unit)
+{
+    return Event(std::make_shared<OTelEventImpl>(formatName(name), otelMeter_, unit));
 }

 Gauge
--- a/src/libxrpl/telemetry/Telemetry.cpp
+++ b/src/libxrpl/telemetry/Telemetry.cpp
@@ -19,10 +19,12 @@
 #include <xrpl/telemetry/Telemetry.h>

 #include <xrpl/basics/Log.h>
+#include <xrpl/beast/insight/Unit.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/telemetry/CoroAwareContextStorage.h>
 #include <xrpl/telemetry/DeterministicIdGenerator.h>
 #include <xrpl/telemetry/DiscardFlag.h>
+#include <xrpl/telemetry/HistogramBuckets.h>
 #include <xrpl/telemetry/SpanNames.h>

 #include <opentelemetry/context/context.h>
@@ -553,31 +555,46 @@ public:
            std::make_unique<metrics_sdk::ViewRegistry>(), 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<metrics_sdk::HistogramAggregationConfig>();
-        histogramConfig->boundaries_ =
-            std::vector<double>{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<double> 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<metrics_sdk::HistogramAggregationConfig>();
+            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.
--- 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