From 3662d420e7cb149b9b5a8f78a72d2a89eb74bb64 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:07:27 +0100 Subject: [PATCH] docs(telemetry): drop PR-internal history from the new comments A comment that describes an earlier state of its own branch documents something no reader can look up, because the branch is squash-merged and the state it contrasts against never reaches the merged history. Four passages in HistogramBuckets.h and one in Telemetry.cpp did exactly that, and the OTelCollector and Unit.h wording implied a transition rather than a fact. - HistogramBuckets.h: the ladders now explain the invariant they enforce, instead of recounting where the edges used to live and how they drifted. - Telemetry.cpp: one view per unit means a byte count is bucketed on the byte ladder, stated directly rather than as something it stopped inheriting. - OTelCollector.cpp: the collector is described by what it is, a thin adapter over the shared pipeline, rather than as a shim that gave up an exporter. - Unit.h: the StatsD path is out of service as a present fact, and the contract its wire format would break is an external protocol one. Comment text only. No declaration, signature or emitted value changes. --- include/xrpl/beast/insight/Unit.h | 10 +++++----- include/xrpl/telemetry/HistogramBuckets.h | 19 ++++++++----------- src/libxrpl/beast/insight/OTelCollector.cpp | 19 +++++++++---------- src/libxrpl/telemetry/Telemetry.cpp | 6 +++--- 4 files changed, 25 insertions(+), 29 deletions(-) diff --git a/include/xrpl/beast/insight/Unit.h b/include/xrpl/beast/insight/Unit.h index cd9c863d1e..fd3516b3ab 100644 --- a/include/xrpl/beast/insight/Unit.h +++ b/include/xrpl/beast/insight/Unit.h @@ -20,11 +20,11 @@ namespace beast::insight { * makeEvent("time", Unit::Millis) --> OTel unit "ms" --> millisecond ladder * makeEvent("size", Unit::Bytes) --> OTel unit "By" --> byte ladder * - * The StatsD backend deliberately ignores this and keeps emitting `|ms` for - * every Event. That path is retired here -- its UDP port is commented out of - * the compose file and the integration test fails if anything is listening on - * 8125 -- so changing its wire format would alter a legacy contract for no - * local benefit and with no way to verify it. + * The StatsD backend deliberately ignores this and emits `|ms` for every + * Event. That path is out of service -- its UDP port is commented out of the + * compose file and the integration test fails if anything is listening on + * 8125 -- so changing its wire format would alter an external protocol + * contract for no local benefit and with no way to verify it. * * @note Adding a member requires extending otelUnitCode(), which switches * exhaustively so a new member is a compile error rather than a silent diff --git a/include/xrpl/telemetry/HistogramBuckets.h b/include/xrpl/telemetry/HistogramBuckets.h index 381362034e..4072cf64f5 100644 --- a/include/xrpl/telemetry/HistogramBuckets.h +++ b/include/xrpl/telemetry/HistogramBuckets.h @@ -11,10 +11,8 @@ namespace xrpl::telemetry::buckets { * @file HistogramBuckets.h * @brief Explicit histogram bucket edges for xrpld's OTel instruments. * - * One header owns every ladder so a reviewer sees all of them at once and a - * test can assert their invariants. Before this existed the edges lived as - * file-local `namespace {}` constants, unreachable from any test, and they - * drifted apart. + * One header owns every ladder, so a reviewer sees all of them together and + * a test can assert their invariants. * * Why a ladder is worth this much care: when a quantile falls in the `+Inf` * bucket, Prometheus returns the *second-highest* edge, not `+Inf`. A @@ -68,11 +66,10 @@ namespace xrpl::telemetry::buckets { * **This list must contain every representable edge of the collector's * spanmetrics ladder, and may extend above it.** Agreement over the shared * range is deliberate: it lets a span-derived latency panel and a native - * histogram panel be read on the same scale. It was specified that way - * originally, then silently broken when the collector ladder alone was - * extended, which left this side capped at 5 s while spans reached 30 s and - * censored every quantile above 5 s. `check_bucket_parity.py` now enforces - * the containment -- add a collector edge, add it here too. + * histogram panel be read on the same scale. Drop an edge the collector + * carries and every quantile above it reads back as the top edge instead of + * failing. `check_bucket_parity.py` enforces the containment -- add a + * collector edge, add it here too. * * The sub-millisecond edges the collector carries (0.01 to 0.5 ms) are * deliberately absent. `beast::insight::Event` rounds every duration up to @@ -86,8 +83,8 @@ namespace xrpl::telemetry::buckets { * as 5 s censors them today. All these Events share one ladder, so its * ceiling has to cover the slowest member rather than the typical one. * - * The 2, 3 and 4 s edges resolve second-scale work that previously had to - * interpolate across a single four-second-wide bucket. + * The 2, 3 and 4 s edges resolve second-scale work, which a single + * four-second-wide bucket can only interpolate across. */ inline constexpr std::array kMillisecondBuckets{ 1.0, diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index d8c70241f1..cfa2c36850 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -4,10 +4,9 @@ * * Compiled only when XRPL_ENABLE_TELEMETRY is defined (via CMake * telemetry=ON). Maps beast::insight instruments to OTel SDK instruments - * created on the GLOBAL Meter published by the telemetry module. This class - * is a legacy shim: it no longer owns an export pipeline. The MeterProvider, - * PeriodicExportingMetricReader, OTLP exporter and histogram view all live in - * xrpl::telemetry::Telemetry. + * created on the GLOBAL Meter published by the telemetry module. It owns no + * export pipeline of its own: the MeterProvider, PeriodicExportingMetricReader, + * OTLP exporter and histogram view all live in xrpl::telemetry::Telemetry. * * When XRPL_ENABLE_TELEMETRY is not defined, OTelCollector::New() returns * a NullCollector so the build succeeds without OTel dependencies. @@ -380,7 +379,7 @@ private: //------------------------------------------------------------------------------ /** - * @brief Main OTel Collector implementation (legacy shim). + * @brief Main OTel Collector implementation. * * Obtains its Meter from the GLOBAL MeterProvider owned and published by the * telemetry module (xrpl::telemetry::Telemetry), rather than building its own @@ -389,7 +388,7 @@ private: * * The metrics pipeline (MeterProvider + PeriodicExportingMetricReader + OTLP * HTTP exporter + histogram view) lives in the telemetry module. This class is - * a thin adapter kept for beast::insight callers during deprecation. + * the thin adapter that lets beast::insight callers reach it. * * Class diagram: * @@ -445,10 +444,10 @@ public: * * @param endpoint OTLP/HTTP metrics endpoint URL. Informational only: * the global telemetry pipeline is authoritative for - * the actual export endpoint. Retained for logging and - * back-compat with the New() signature. - * @param prefix Legacy metric-name prefix. Not applied to metric - * names; used only in the startup log line. + * the actual export endpoint. Used only in the startup + * log line. + * @param prefix Metric-name prefix. Not applied to metric names; + * used only in the startup log line. * @param instanceId Value for the service.instance.id resource attribute. * When empty, the attribute is omitted. * @param serviceName Value for the service.name resource attribute. diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index cac48144a9..aada42756c 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -408,9 +408,9 @@ class TelemetryImpl : public Telemetry meterProvider_->AddMetricReader(std::move(reader)); // 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. + // instrument gets the ladder that fits what it measures: a byte count + // is bucketed on the byte ladder and never on a latency one. 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