From 7273b06d35c02d430f7f062b7860d6ad87a65191 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 14 Aug 2026 22:19:24 +0100 Subject: [PATCH] docs(telemetry): describe metrics instead of citing plan task numbers These comments were indexed by task, use-case and limitation numbers that are defined only in planning documents outside the shipped tree. Nothing in the repository defined them, so the cross-references resolved nowhere. Each comment now states what the code does. --- src/tests/libxrpl/telemetry/MetricMacros.cpp | 16 ++++---- src/xrpld/app/consensus/RCLConsensus.cpp | 2 +- src/xrpld/perflog/detail/PerfLogImp.cpp | 33 ++++++++-------- src/xrpld/telemetry/MetricMacros.h | 35 +++++++++-------- src/xrpld/telemetry/MetricsRegistry.cpp | 40 ++++++++++---------- src/xrpld/telemetry/MetricsRegistry.h | 40 ++++++++++---------- 6 files changed, 82 insertions(+), 84 deletions(-) diff --git a/src/tests/libxrpl/telemetry/MetricMacros.cpp b/src/tests/libxrpl/telemetry/MetricMacros.cpp index ddcd2af33c..988cf1f1ba 100644 --- a/src/tests/libxrpl/telemetry/MetricMacros.cpp +++ b/src/tests/libxrpl/telemetry/MetricMacros.cpp @@ -277,9 +277,9 @@ TEST(MetricMacros, observable_gauge_register_reports_current_value) FakeApp app; wire(app, /*enabled=*/true); - // Own the state exactly as a real caller would (Use Case 5 in the - // design doc) -- the macro's callback reads through this atomic on - // every collection tick, it does not own the value itself. + // Own the state exactly as a real caller would -- the macro's callback + // reads through this atomic on every collection tick, it does not own + // the value itself. std::atomic queueDepth{0}; XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER( app, @@ -287,11 +287,11 @@ TEST(MetricMacros, observable_gauge_register_reports_current_value) "Test observable gauge for macro unit test", [&queueDepth] { return queueDepth.load(); }); - // There is no application-level read-back API (Use Case 4) -- this - // test can only prove registration doesn't crash and that meter() was - // consulted to create the observable instrument. It does NOT assert the - // observed value reaches Prometheus; that is Task 3b's docker-harness - // job, not this hermetic unit test. + // There is no application-level read-back API -- this test can only + // prove registration doesn't crash and that meter() was consulted to + // create the observable instrument. It does NOT assert the observed + // value reaches Prometheus; that is the docker-harness integration + // test's job, not this hermetic unit test. queueDepth.store(42); EXPECT_EQ(app.registry().meterCalls(), 1); } diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 5af4412ce8..8157a55c09 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -745,7 +745,7 @@ RCLConsensus::Adaptor::doAccept( // Record ledger close for OTel dashboard parity counter. Uses the // call-site macro (see MetricMacros.h) rather than a MetricsRegistry - // member -- proof-of-concept for tasks/metric-macro-plan.md. + // member. XRPL_METRIC_COUNTER_INC(app_, "ledgers_closed_total", "Total ledgers closed by consensus"); //------------------------------------------------------------------------- diff --git a/src/xrpld/perflog/detail/PerfLogImp.cpp b/src/xrpld/perflog/detail/PerfLogImp.cpp index 106c36f157..b3cd723872 100644 --- a/src/xrpld/perflog/detail/PerfLogImp.cpp +++ b/src/xrpld/perflog/detail/PerfLogImp.cpp @@ -336,17 +336,16 @@ PerfLogImp::rpcStart(std::string const& method, std::uint64_t const requestId) counters_.methods[requestId] = {counter->first.c_str(), steady_clock::now()}; } - // Task 9.4: Record RPC start in OTel metrics pipeline. Recorded after the - // locks above are released: the OTel call path allocates and takes locks + // Record RPC start in OTel metrics pipeline. Recorded after the locks + // above are released: the OTel call path allocates and takes locks // inside the SDK, so holding methodsMutex across it would widen a // process-wide critical section for no reason. Mirrors rpcEnd(). if (auto* mr = app_.getMetricsRegistry()) mr->recordRpcStarted(method); - // Proof-of-concept for tasks/metric-macro-plan.md Use Case 2: a value - // that must be able to decrease (UpDownCounter), added at its call - // site with no MetricsRegistry member/init-line/method. Paired with the - // matching -1 in rpcEnd(). Runs on the same path as recordRpcStarted + // A value that must be able to decrease (UpDownCounter), added at its + // call site with no MetricsRegistry member/init-line/method. Paired with + // the matching -1 in rpcEnd(). Runs on the same path as recordRpcStarted // above, i.e. only after a methods-map entry exists for this request. XRPL_METRIC_UPDOWN_ADD(app_, "rpc_in_flight_requests", "RPC requests currently executing", 1); } @@ -397,9 +396,9 @@ PerfLogImp::rpcEnd(std::string const& method, std::uint64_t const requestId, boo counter->second.value.duration += durationUs; } - // Task 9.4: Record RPC completion in OTel metrics pipeline. - // Mirrors the rpcStart() instrumentation so the finished/errored - // counters and duration histogram advance with every call. + // Record RPC completion in OTel metrics pipeline. Mirrors the + // rpcStart() instrumentation so the finished/errored counters and + // duration histogram advance with every call. if (auto* mr = app_.getMetricsRegistry()) { if (finish) @@ -434,8 +433,8 @@ PerfLogImp::jobQueue(JobType const type, std::string const& name) ++counter->second.value.queued; } - // Task 9.5: Record job enqueue in OTel metrics pipeline, after the lock - // above is released so the SDK's work stays outside the critical section. + // Record job enqueue in OTel metrics pipeline, after the lock above is + // released so the SDK's work stays outside the critical section. if (auto* mr = app_.getMetricsRegistry()) mr->recordJobQueued(JobTypes::name(type), name); } @@ -468,10 +467,10 @@ PerfLogImp::jobStart( counters_.jobs[instance] = {type, startTime}; } - // Task 9.5: Record job start in OTel metrics pipeline, after the locks - // above are released. jobsMutex is process-wide and taken by every worker - // thread on every job, so the SDK's allocation and internal locking must - // not run inside it. + // Record job start in OTel metrics pipeline, after the locks above are + // released. jobsMutex is process-wide and taken by every worker thread + // on every job, so the SDK's allocation and internal locking must not + // run inside it. if (auto* mr = app_.getMetricsRegistry()) mr->recordJobStarted(JobTypes::name(type), name, dur.count()); } @@ -499,8 +498,8 @@ PerfLogImp::jobFinish(JobType const type, std::string const& name, microseconds counters_.jobs[instance] = {JtInvalid, steady_time_point()}; } - // Task 9.5: Record job finish in OTel metrics pipeline, after the locks - // above are released, for the same reason as jobStart(). + // Record job finish in OTel metrics pipeline, after the locks above + // are released, for the same reason as jobStart(). if (auto* mr = app_.getMetricsRegistry()) mr->recordJobFinished(JobTypes::name(type), name, dur.count()); } diff --git a/src/xrpld/telemetry/MetricMacros.h b/src/xrpld/telemetry/MetricMacros.h index 6bea92d8e3..a870df3319 100644 --- a/src/xrpld/telemetry/MetricMacros.h +++ b/src/xrpld/telemetry/MetricMacros.h @@ -87,27 +87,26 @@ * @note A histogram whose values can exceed ~10,000 units (e.g. a * microsecond duration beyond 10ms) needs an explicit-bucket View, which * OTel can only register at MeterProvider construction time -- this - * cannot be done from a call site. See Limitation 2. Register such a - * view in MetricsRegistry::initExporterAndProvider() as today; the + * cannot be done from a call site. Register such a view in + * MetricsRegistry::initExporterAndProvider() as today; the * histogram-record call itself can still use the macro. * * @note Only call the SYNCHRONOUS macros (Counter/UpDownCounter/ * Histogram/Gauge) from code that runs AFTER MetricsRegistry::start() has * completed (RPC handlers, job callbacks, consensus rounds, tx apply, peer - * message handlers). See Limitation 1. + * message handlers). * * @note The OBSERVABLE registration macros are the opposite: call them * EAGERLY, exactly once, from constructor/init code -- never from a hot * path. Repeated calls at the same call site register a NEW callback * each time (no create-once caching, unlike the synchronous macros), - * which leaks callbacks. See Limitation 3. + * which leaks callbacks. * * @note There is no way to read back a synchronous instrument's current * accumulated value from application code -- the OTel API is * write-only/push-based by design. If your logic needs both to record a * metric AND read its running value, keep your own state (std::atomic or - * similar) and separately feed OTel via these macros. See "Use Case 4" in - * tasks/metric-macro-plan.md. + * similar) and separately feed OTel via these macros. */ // On Windows, OTel's spin_lock_mutex.h (transitively included from @@ -208,11 +207,11 @@ } while (false) // UpDownCounter: like COUNTER_ADD, but the underlying instrument permits a -// negative amount (Use Case 2 -- e.g. in-flight request count, +1 on start -// / -1 on finish from two different points in the same or different call -// sites). A plain Counter's Add() must never see a negative value per the -// OTel API contract; use this macro, not COUNTER_ADD, whenever the value -// can decrease. +// negative amount (e.g. in-flight request count, +1 on start / -1 on +// finish from two different points in the same or different call sites). +// A plain Counter's Add() must never see a negative value per the OTel +// API contract; use this macro, not COUNTER_ADD, whenever the value can +// decrease. #define XRPL_METRIC_UPDOWN_ADD(app, name, description, amount) \ do \ { \ @@ -354,15 +353,15 @@ #endif // OPENTELEMETRY_ABI_VERSION_NO >= 2 // ----------------------------------------------------------------- -// Observable/async instrument registration (Use Case 5). Unlike the -// synchronous macros above, these do NOT lazily create-on-first-call -- -// they register a callback with the SDK immediately, at the call site, -// the moment the macro executes. Callers MUST invoke this during +// Observable/async instrument registration. Unlike the synchronous +// macros above, these do NOT lazily create-on-first-call -- they +// register a callback with the SDK immediately, at the call site, the +// moment the macro executes. Callers MUST invoke this during // construction/init, before the server is fully live (same timing rule // MetricsRegistry::registerAsyncGauges() already follows for its own -// gauges -- see Limitation 3). Calling it from a hot-path function -// instead of an init path re-registers a new callback on every call, -// which leaks callbacks and is NOT what this macro is for. +// gauges). Calling it from a hot-path function instead of an init path +// re-registers a new callback on every call, which leaks callbacks and +// is NOT what this macro is for. // // The callable is captured in a heap-allocated std::function, and its // address is passed as the `void* state` to AddCallback (whose signature, diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index 40e434416d..c07817e41e 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -382,7 +382,7 @@ MetricsRegistry::initSyncInstruments() jobRunningDurationHistogram_ = meter_->CreateDoubleHistogram(kJobRunningDurationUs, "Job execution time in microseconds"); - // --- External dashboard parity counters (Task 7.14) --- + // --- External dashboard parity counters --- ledgersClosedCounter_ = meter_->CreateUInt64Counter("ledgers_closed_total", "Total ledgers closed by consensus"); validationsSentCounter_ = meter_->CreateUInt64Counter( @@ -441,7 +441,7 @@ MetricsRegistry::stop() } // ----------------------------------------------------------------- -// Synchronous instrument recording — RPC metrics (Task 9.4) +// Synchronous instrument recording — RPC metrics // ----------------------------------------------------------------- void @@ -500,7 +500,7 @@ MetricsRegistry::recordRpcErrored(std::string_view method, std::int64_t duration } // ----------------------------------------------------------------- -// Synchronous instrument recording — Job Queue metrics (Task 9.5) +// Synchronous instrument recording — Job Queue metrics // ----------------------------------------------------------------- void @@ -579,7 +579,7 @@ MetricsRegistry::recordJobFinished( } // ----------------------------------------------------------------- -// Observable gauge callbacks (Tasks 9.1, 9.2, 9.3, 9.6, 9.7) +// Observable gauge callbacks // ----------------------------------------------------------------- #ifdef XRPL_ENABLE_TELEMETRY @@ -647,7 +647,7 @@ MetricsRegistry::registerJqTransOverflowCounter() void MetricsRegistry::registerCacheHitRateGauge() { - // --- Task 9.2: Cache hit rate and size gauges --- + // --- Cache hit rate and size gauges --- cacheHitRateGauge_ = meter_->CreateDoubleObservableGauge("cache_metrics", "Cache hit rates and sizes"); cacheHitRateGauge_->AddCallback( @@ -718,7 +718,7 @@ MetricsRegistry::registerCacheHitRateGauge() void MetricsRegistry::registerTxqGauge() { - // --- Task 9.3: TxQ metrics gauges --- + // --- TxQ metrics gauges --- txqGauge_ = meter_->CreateDoubleObservableGauge("txq_metrics", "Transaction queue metrics"); txqGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { @@ -765,7 +765,7 @@ MetricsRegistry::registerTxqGauge() void MetricsRegistry::registerObjectCountGauge() { - // --- Task 9.6: Counted object instance gauges --- + // --- Counted object instance gauges --- objectCountGauge_ = meter_->CreateInt64ObservableGauge( "object_count", "Live instance counts for key internal object types"); objectCountGauge_->AddCallback( @@ -797,7 +797,7 @@ MetricsRegistry::registerObjectCountGauge() void MetricsRegistry::registerLoadFactorGauge() { - // --- Task 9.7: Load factor breakdown gauges --- + // --- Load factor breakdown gauges --- loadFactorGauge_ = meter_->CreateDoubleObservableGauge("load_factor_metrics", "Fee load factor breakdown"); loadFactorGauge_->AddCallback( @@ -961,7 +961,7 @@ MetricsRegistry::observeReadQueue(node_store::Database& db, ObserveFn const& obs void MetricsRegistry::registerNodeStoreGauge() { - // --- Task 9.1: NodeStore I/O gauges --- + // --- NodeStore I/O gauges --- // The cumulative counters (reads, writes, bytes) are also exposed here // as observable gauges. This avoids adding an xrpld dependency into the // libxrpl nodestore code — the MetricsRegistry reads the existing atomic @@ -1009,7 +1009,7 @@ MetricsRegistry::registerNodeStoreGauge() void MetricsRegistry::registerServerInfoGauge() { - // --- Task 9.7a: Server info gauges --- + // --- Server info gauges --- serverInfoGauge_ = meter_->CreateInt64ObservableGauge("server_info", "Server-level health metrics"); serverInfoGauge_->AddCallback( @@ -1094,7 +1094,7 @@ MetricsRegistry::registerServerInfoGauge() void MetricsRegistry::registerBuildInfoGauge() { - // --- Task 9.7b: Build info gauge --- + // --- Build info gauge --- buildInfoGauge_ = meter_->CreateInt64ObservableGauge("build_info", "Build version information"); buildInfoGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* /* state */) { @@ -1114,7 +1114,7 @@ MetricsRegistry::registerBuildInfoGauge() void MetricsRegistry::registerCompleteLedgersGauge() { - // --- Task 9.7c: Complete ledgers range gauge --- + // --- Complete ledgers range gauge --- completeLedgersGauge_ = meter_->CreateInt64ObservableGauge( "complete_ledgers", "Complete ledger range start/end pairs"); completeLedgersGauge_->AddCallback( @@ -1173,7 +1173,7 @@ MetricsRegistry::registerCompleteLedgersGauge() void MetricsRegistry::registerDbMetricsGauge() { - // --- Task 9.7d: Database size and fetch rate gauges --- + // --- Database size and fetch rate gauges --- dbMetricsGauge_ = meter_->CreateInt64ObservableGauge("db_metrics", "Database storage sizes and fetch rates"); dbMetricsGauge_->AddCallback( @@ -1212,7 +1212,7 @@ MetricsRegistry::registerDbMetricsGauge() void MetricsRegistry::registerValidatorHealthGauge() { - // --- Task 7.9: Validator health gauges --- + // --- Validator health gauges --- validatorHealthGauge_ = meter_->CreateDoubleObservableGauge("validator_health", "Validator health indicators"); validatorHealthGauge_->AddCallback( @@ -1259,7 +1259,7 @@ MetricsRegistry::registerValidatorHealthGauge() void MetricsRegistry::registerPeerQualityGauge() { - // --- Task 7.10: Peer quality gauges --- + // --- Peer quality gauges --- // Uses Peer::json() to read latency and version since those accessors // are not on the abstract Peer interface (they live on PeerImp). peerQualityGauge_ = @@ -1413,7 +1413,7 @@ MetricsRegistry::registerReduceRelayGauge() void MetricsRegistry::registerLedgerEconomyGauge() { - // --- Task 7.11: Ledger economy gauges --- + // --- Ledger economy gauges --- ledgerEconomyGauge_ = meter_->CreateDoubleObservableGauge("ledger_economy", "Ledger fee and economy metrics"); ledgerEconomyGauge_->AddCallback( @@ -1478,7 +1478,7 @@ MetricsRegistry::registerLedgerEconomyGauge() void MetricsRegistry::registerStateTrackingGauge() { - // --- Task 7.12: State tracking gauges --- + // --- State tracking gauges --- stateTrackingGauge_ = meter_->CreateDoubleObservableGauge("state_tracking", "Node state and mode tracking"); stateTrackingGauge_->AddCallback( @@ -1532,7 +1532,7 @@ MetricsRegistry::registerStateTrackingGauge() void MetricsRegistry::registerStorageDetailGauge() { - // --- Task 7.13: Storage detail gauges --- + // --- Storage detail gauges --- // Reports the cumulative payload bytes handed to the NodeStore. See the // note at the observe() call below: this is logical bytes stored, not // on-disk file size, because no accessor for the latter exists. The label @@ -1584,7 +1584,7 @@ MetricsRegistry::registerStorageDetailGauge() void MetricsRegistry::registerValidationAgreementGauge() { - // --- Task 7.15: Validation agreement gauges --- + // --- Validation agreement gauges --- // Reports rolling-window agreement percentages and counts from // ValidationTracker. reconcile() is called at the start of the // callback so that pending ledger events are resolved before the @@ -1696,7 +1696,7 @@ MetricsRegistry::registerValidationTotalsCounters() #endif // XRPL_ENABLE_TELEMETRY // ----------------------------------------------------------------- -// External dashboard parity counter increments (Task 7.14) +// External dashboard parity counter increments // ----------------------------------------------------------------- void diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 017e2c7bae..3662f1960b 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -231,8 +231,8 @@ namespace telemetry { * edit needed. Fall back to a dedicated member + init line + record * method (the pattern below) only when the metric needs to be read * back by other code (e.g. ValidationTracker-style accumulation) or - * needs a custom histogram bucket View (see MetricMacros.h Limitation - * 2 in tasks/metric-macro-plan.md). + * needs a custom histogram bucket View (see the histogram note in + * MetricMacros.h). * - Adding a new OBSERVABLE gauge still requires eager central * registration -- pull-model instruments cannot be lazily created. */ @@ -575,7 +575,7 @@ public: std::int64_t runningDurUs); // ----------------------------------------------------------------- - // External dashboard parity counters (Tasks 7.9-7.14) + // External dashboard parity counters // ----------------------------------------------------------------- /** @@ -862,7 +862,7 @@ private: */ opentelemetry::nostd::shared_ptr dbMetricsGauge_; - // --- External dashboard parity gauges (Tasks 7.9-7.13) --- + // --- External dashboard parity gauges --- /** * Observable gauge for validator health indicators (amendment blocked, * UNL blocked, quorum, UNL expiry). @@ -905,7 +905,7 @@ private: opentelemetry::nostd::shared_ptr validationAgreementGauge_; - // --- External dashboard parity counters (Task 7.14) --- + // --- External dashboard parity counters --- /** * Counter: ledgers_closed_total — incremented each consensus round. */ @@ -1003,15 +1003,15 @@ private: void registerJqTransOverflowCounter(); // gap-fill: overlay overflow total void - registerCacheHitRateGauge(); // Task 9.2 + registerCacheHitRateGauge(); void - registerTxqGauge(); // Task 9.3 + registerTxqGauge(); void - registerObjectCountGauge(); // Task 9.6 + registerObjectCountGauge(); void - registerLoadFactorGauge(); // Task 9.7 + registerLoadFactorGauge(); void - registerNodeStoreGauge(); // Task 9.1 + registerNodeStoreGauge(); // The four nodestore_state helpers and their ObserveFn sink are public // (above), so a test can drive each one with a recording sink and assert @@ -1019,27 +1019,27 @@ private: // arguments, so exposing them widens no state. void - registerServerInfoGauge(); // Task 9.7a + registerServerInfoGauge(); void - registerBuildInfoGauge(); // Task 9.7b + registerBuildInfoGauge(); void - registerCompleteLedgersGauge(); // Task 9.7c + registerCompleteLedgersGauge(); void - registerDbMetricsGauge(); // Task 9.7d + registerDbMetricsGauge(); void - registerValidatorHealthGauge(); // Task 7.9 + registerValidatorHealthGauge(); void - registerPeerQualityGauge(); // Task 7.10 + registerPeerQualityGauge(); void registerReduceRelayGauge(); // Reduce-relay efficiency void - registerLedgerEconomyGauge(); // Task 7.11 + registerLedgerEconomyGauge(); void - registerStateTrackingGauge(); // Task 7.12 + registerStateTrackingGauge(); void - registerStorageDetailGauge(); // Task 7.13 + registerStorageDetailGauge(); void - registerValidationAgreementGauge(); // Task 7.15 + registerValidationAgreementGauge(); void registerValidationTotalsCounters(); // gap-fill: lifetime agree/miss _total #endif // XRPL_ENABLE_TELEMETRY