diff --git a/.github/scripts/telemetry/check_bucket_parity.py b/.github/scripts/telemetry/check_bucket_parity.py index 4c723fb4c0..b0baef2f90 100755 --- a/.github/scripts/telemetry/check_bucket_parity.py +++ b/.github/scripts/telemetry/check_bucket_parity.py @@ -2,13 +2,13 @@ """Assert the C++ millisecond ladder agrees with the collector's spanmetrics ladder. The two are specified to match so a span-derived latency panel and a native -histogram panel can be read on the same scale. They *were* identical when first -shipped. Then the collector ladder alone was extended -- sub-millisecond edges -below 1ms and second-scale edges up to 30s -- and nothing checked the other -side, so the C++ ladder stayed capped at 5s. Every quantile above 5s then read -back as a flat 5000, because Prometheus returns the second-highest edge for a -quantile landing in the `+Inf` bucket. That looks like a measurement rather -than an error, which is why it survived for eleven phases. +histogram panel can be read on the same scale. Nothing else couples them, so +extending one ladder alone -- sub-millisecond edges below 1ms, second-scale +edges up to 30s -- silently leaves the other short. That failure is quiet: +Prometheus returns the second-highest edge for a quantile landing in the +`+Inf` bucket, so every quantile above a too-low ceiling reads back as a flat +number that looks like a measurement rather than an error. This check is what +makes the drift loud. The rule is containment, not equality: @@ -17,7 +17,7 @@ The rule is containment, not equality: * the C++ ladder MAY carry extra edges ABOVE the collector's highest edge, because jobs outlive spans -- the updatepaths job type was measured averaging ~60s, which no span approaches. Demanding equality would force a - ceiling that censors it, reintroducing the bug this guards against; + ceiling that censors it, recreating the failure this guards against; * collector edges below 1ms are expected to be ABSENT rather than missing: beast::insight::Event rounds every duration up to a whole millisecond before it reaches the histogram, so those edges could never collect a @@ -117,8 +117,10 @@ def main(): ) print( "\nThe two ladders must agree over their shared range. Extra C++ edges are\n" - "permitted only ABOVE the collector's highest edge. Change both sides, or\n" - "change the spec in OpenTelemetryPlan/Phase7_taskList.md.", + "permitted only ABOVE the collector's highest edge. To re-price the shared\n" + f"range, edit the ladder in {HEADER} and the\n" + f"spanmetrics 'buckets:' list in {COLLECTOR}\n" + "in the same change, so both sides stay in step.", file=sys.stderr, ) return 1 diff --git a/include/xrpl/telemetry/Recording.h b/include/xrpl/telemetry/Recording.h index dc2613588e..9b8f4b251c 100644 --- a/include/xrpl/telemetry/Recording.h +++ b/include/xrpl/telemetry/Recording.h @@ -6,12 +6,12 @@ * Each type below holds real state when telemetry is compiled in and is an * empty type with no-op methods when it is not. The member is declared in * both configurations, so a class's member set and public API never differ - * between builds -- a difference that has previously made a test mock - * abstract. The compiled-out forms are empty types, so such a member costs a - * byte of padding rather than nothing. `[[no_unique_address]]` would remove - * even that, but MSVC ignores the standard spelling for ABI compatibility, so - * it is deliberately not used. What these types buy is work not being done, - * not a smaller struct. + * between builds -- a difference that leaves a test mock complete in one + * configuration and abstract in the other, where it then fails to compile. + * The compiled-out forms are empty types, so such a member costs a byte of + * padding rather than nothing. `[[no_unique_address]]` would remove even that, but MSVC ignores + * the standard spelling for ABI compatibility, so it is deliberately not + * used. What these types buy is work not being done, not a smaller struct. * * kEnabled ---- if constexpr ---- telemetry-only blocks * | diff --git a/src/test/overlay/TMGetObjectByHash_test.cpp b/src/test/overlay/TMGetObjectByHash_test.cpp index 9c4b062001..759c78fefc 100644 --- a/src/test/overlay/TMGetObjectByHash_test.cpp +++ b/src/test/overlay/TMGetObjectByHash_test.cpp @@ -723,9 +723,9 @@ class TMGetObjectByHash_test : public beast::unit_test::Suite * One pricing case: inputs, the derived expectation, and the literal. * * Both expectations are kept. `derived` is written from the Tuning - * constants so a deliberate re-pricing needs one edit; `literal` is the - * number as of this branch so a re-pricing cannot pass unnoticed by - * being self-consistently wrong. + * constants so a deliberate re-pricing needs one edit; `literal` pins the + * number those constants currently produce, so a re-pricing cannot pass + * unnoticed by being self-consistently wrong. */ struct FeeCase { diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index 5d7c8543fb..a36c2a220d 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -204,9 +204,9 @@ allFoldToOther() static_assert(allPassThroughUnchanged()); static_assert(allFoldToOther()); -// The verified size of the pass-through set as of this branch: 43 all-letter -// job-name literals. Pinned so that adding or removing a job name without -// revisiting the label-cardinality budget fails the build here. +// The pass-through set is every all-letter job-name literal in the tree: 43 +// of them. Pinned so that adding or removing a job name without revisiting +// the label-cardinality budget fails the build here. static_assert(kPassThroughHandlers.size() == 43); /** diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index 38f318c39b..87396666b5 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -226,8 +226,8 @@ private: /** * Spans the acquire lifecycle: started in init(), finalized in done() * with the outcome (complete/failed), timeout count, and peer count. - * Gives operators visibility into back-fill / fork-recovery cost, which - * previously emitted no span or metric. + * This span is the only signal for back-fill / fork-recovery cost; no + * other span or metric covers it. * Thread-free: emplaced by the acquiring thread, reset on a JtLedgerData * worker. A SpanGuard owns no thread-local Scope, so it can be destroyed * on the worker without corrupting the origin thread's context stack. diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index 2b0ca66371..a443e7707f 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -704,10 +704,10 @@ private: /** * Record the OTel metrics for one completed `TMGetObjectByHash` request. * - * Extracted from `processGetObjectByHash()` purely to keep that method - * within the 80-line limit; it holds no logic of its own beyond deriving - * the hit/miss split from `requested` and `found`. Called once per - * request, after the fetch loop and the `charge()` call. + * Called once per request from `processGetObjectByHash()`, after the fetch + * loop and the `charge()` call. A separate method so that one stays within + * the 80-line limit; it holds no logic of its own beyond deriving the + * hit/miss split from `requested` and `found`. * * Records `getobject_request_objects`, `getobject_lookup_us`, * `getobject_charge`, and both label values of diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index a64f5e219e..a41b851239 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -30,11 +30,12 @@ // The app and overlay includes below are why // .github/scripts/levelization/results/loops.txt records -// `xrpld.app <-> xrpld.telemetry` and `xrpld.overlay <-> xrpld.telemetry`, where -// ordering.txt previously had telemetry strictly below both. The observable -// gauges are pull-model: their callbacks sample live state when the reader -// thread fires, so they need the concrete types to call getJqTransOverflow(), -// size(), getPeerDisconnectCharges(), foreach() and txMetrics(). +// `xrpld.app <-> xrpld.telemetry` and `xrpld.overlay <-> xrpld.telemetry` as +// cycles, rather than an acyclic ordering.txt entry placing telemetry strictly +// below both. The observable gauges are pull-model: their callbacks sample live +// state when the reader thread fires, so they need the concrete types to call +// getJqTransOverflow(), size(), getPeerDisconnectCharges(), foreach() and +// txMetrics(). // // The cycle is confined to this translation unit. No telemetry header includes // app or overlay (MetricsRegistry.h forward-declares what it needs and takes a diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 0837bd13ce..f66f9c8fcb 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -308,11 +308,11 @@ public: * phase. Mostly ObservableGauges, plus the ObservableCounters whose * source value is already cumulative. * - * Split from `start()` because the two halves have different - * prerequisites. `start()` needs only config strings; these callbacks - * read live Application services, so this half must run later. + * A separate entry point from `start()` because the two halves have + * different prerequisites. `start()` needs only config strings; these + * callbacks read live Application services, so this half must run later. * Registering an observable also arms the reader thread to invoke its - * callback on the next tick, which is why the split is about ordering + * callback on the next tick, which is why the separation is about ordering * and not just tidiness. * * @pre `start()` has already run (the meter exists). If it has not,