From 56cadaff6dc43f60d36b76ceeabf42b952bfd6a1 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:50:34 +0100 Subject: [PATCH 1/4] fix(telemetry): stop path-find tracing from altering request handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Telemetry must read state, never change it. Two defects here did change it, plus three smaller correctness and privacy fixes. doPathFind and doRipplePathFind read source_account / destination_account off context.params to hash them into span attributes. context.params is non-const, so those reads selected json::Value's non-const operator[], which inserts a null for a missing key. The same object is later validated by PathRequest::parseJson, whose first checks are isMember(source_account) and isMember(destination_account) — so a request that omitted either field looked present and the client received Malformed instead of Missing. Reads now go through std::as_const, whose overload returns kNull without inserting. PathRequest::doUpdate emitted pathfind_dest_currency as to_string(saDstAmount_.asset()). For a non-XRP asset that renders as "/" with the issuer as a plaintext Base58 address, so a plain account address reached the span pipeline even though every other account here is hashed first. The issuer is now redacted and the currency kept; an MPT asset renders as its issuance ID and carries no address. PathRequestManager::updateAll created pathfind.update_all with an unscoped SpanGuard. An unscoped guard takes the ambient span as its own parent but does not itself become the ambient parent, so the pathfind.compute spans that doUpdate creates never nested under it, contradicting the documented hierarchy. It is now a scoped guard, held in std::optional because ScopedSpanGuard is deliberately non-movable and so cannot be produced by a ternary. The skip when there are no active subscriptions is preserved. updateAll is dispatched via addJob and doUpdate runs synchronously, so the guard is constructed and destroyed under the same context store, as ScopedSpanGuard requires. The WebSocket entry point emitted the client-supplied command string directly. That value becomes a Prometheus label, so arbitrary request input could drive unbounded label cardinality. It is now resolved against the handler registry, collapsing anything unrecognized to "unknown", matching what the HTTP path already does. Also: the pathfind.discover comment claimed future child spans could be parented off it, which its unscoped guard cannot do — corrected to say what would be required instead. Config-reference and task-list docs named the parser setupTelemetry(); the API is makeTelemetrySetup(). --- .../05-configuration-reference.md | 8 +-- OpenTelemetryPlan/Phase2_taskList.md | 2 +- src/xrpld/rpc/detail/PathFindSpanNames.h | 11 ++-- src/xrpld/rpc/detail/PathRequest.cpp | 26 ++++++++-- src/xrpld/rpc/detail/PathRequestManager.cpp | 25 +++++---- src/xrpld/rpc/detail/ServerHandler.cpp | 52 ++++++++++++++++--- src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 13 +++-- .../rpc/handlers/orderbook/RipplePathFind.cpp | 11 ++-- 8 files changed, 112 insertions(+), 36 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index caf381e8f6..3f6dab5774 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -51,7 +51,7 @@ phases. They will be added as the corresponding subsystems are instrumented: > **TxQ** = Transaction Queue -The parser `setupTelemetry()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` reads the `[telemetry]` `Section` and populates a `Telemetry::Setup` struct, applying the defaults listed in Section 5.1.2 via `section.value_or(...)`. It derives `serviceInstanceId` from the node public key when not overridden, selects the exporter endpoint default by exporter type, and leaves the sampling ratio at its fixed 1.0 default (not read from config — see Section 7.4.2). +The parser `makeTelemetrySetup()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` reads the `[telemetry]` `Section` and populates a `Telemetry::Setup` struct, applying the defaults listed in Section 5.1.2 via `section.value_or(...)`. It derives `serviceInstanceId` from the node public key when not overridden, selects the exporter endpoint default by exporter type, and leaves the sampling ratio at its fixed 1.0 default (not read from config — see Section 7.4.2). --- @@ -65,7 +65,7 @@ The parser `setupTelemetry()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` rea > constructed with an empty `serviceInstanceId` and patched via > `setServiceInstanceId()` once `setup()` has called `getNodeIdentity()`. -`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_`. It is built in the member initializer list via `makeTelemetry(setupTelemetry(...))` with an empty `serviceInstanceId`, then patched in `setup()` by calling `setServiceInstanceId()` with the Base58 node public key (unless the user supplied a custom `service_instance_id`). `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. +`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_`. It is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with an empty `serviceInstanceId`, then patched in `setup()` by calling `setServiceInstanceId()` with the Base58 node public key (unless the user supplied a custom `service_instance_id`). `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. ### 5.3.2 ServiceRegistry Interface Addition @@ -129,7 +129,7 @@ flowchart TB end subgraph init["Initialization"] - parse["setupTelemetry()"] + parse["makeTelemetrySetup()"] factory["makeTelemetry()"] end @@ -163,7 +163,7 @@ flowchart TB **Reading the diagram:** - **Configuration Sources**: `xrpld.cfg` provides runtime settings (endpoint, per-component trace toggles) while the CMake flag controls whether telemetry is compiled in at all. Head sampling is fixed at 1.0 and is not a config option; volume reduction happens via tail sampling in the collector. -- **Initialization**: `setupTelemetry()` parses config values, then `makeTelemetry()` constructs the provider, processor, and exporter objects. +- **Initialization**: `makeTelemetrySetup()` parses config values, then `makeTelemetry()` constructs the provider, processor, and exporter objects. - **Runtime Components**: The `TracerProvider` creates spans, the `BatchProcessor` buffers them, and the `OTLP Exporter` serializes and sends them over the wire. - **OTLP arrow to Collector**: Trace data leaves the xrpld process via OTLP/HTTP and enters the external Collector pipeline. (OTLP/gRPC is future work — see design decisions §2.2.2.) - **Collector Pipeline**: `Receivers` ingest OTLP data, `Processors` apply sampling/filtering/enrichment, and `Exporters` forward traces to storage backends (Tempo, etc.). diff --git a/OpenTelemetryPlan/Phase2_taskList.md b/OpenTelemetryPlan/Phase2_taskList.md index 0e19be9e9c..ed48f10d75 100644 --- a/OpenTelemetryPlan/Phase2_taskList.md +++ b/OpenTelemetryPlan/Phase2_taskList.md @@ -67,7 +67,7 @@ - `src/tests/libxrpl/telemetry/TelemetryConfig.cpp`: - Test Setup defaults (all fields have correct initial values) - - Test `setupTelemetry` config parser (empty section, full section, edge cases) + - Test `makeTelemetrySetup` config parser (empty section, full section, edge cases) - Test `samplingRatio` clamping (values outside 0.0-1.0) - `src/tests/libxrpl/telemetry/SpanGuardFactory.cpp`: diff --git a/src/xrpld/rpc/detail/PathFindSpanNames.h b/src/xrpld/rpc/detail/PathFindSpanNames.h index 65ce81bb96..6d8fedc7ab 100644 --- a/src/xrpld/rpc/detail/PathFindSpanNames.h +++ b/src/xrpld/rpc/detail/PathFindSpanNames.h @@ -65,10 +65,13 @@ inline constexpr auto discover = makeStr("discover"); } // namespace op // ===== Attribute keys ====================================================== -// -// All pathfind attributes are namespaced under `pathfind_*` (underscore form, -// per Phase 1c naming spec rule 5). Avoids collisions with bare keys like -// `fast` or `num_paths` that other subsystems may introduce. + +/** + * All pathfind attributes are namespaced under `pathfind_*`, in underscore + * form, per the span attribute naming convention in CONTRIBUTING.md. Avoids + * collisions with bare keys like `fast` or `num_paths` that other subsystems + * may introduce. + */ namespace attr { /** diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 28a6ad3c5c..155f9e28d0 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include #include @@ -587,9 +588,13 @@ PathRequest::findPaths( // that a single RPC call produces one discover span instead of N (one per // candidate source asset). Trade-off: per-asset discovery/ranking timing // is no longer split into individual spans — span count and Tempo storage - // are bounded per RPC at the cost of per-asset visibility. If per-asset - // breakdown is needed in the future, add child spans inside the loop body - // (`Pathfinder::findPaths`/`computePathRanks`) parented off this span. + // are bounded per RPC at the cost of per-asset visibility. + // + // This is an unscoped guard: it takes the ambient span as its own parent, + // but does not itself become the ambient parent. Adding per-asset child + // spans inside the loop body therefore requires either making this a + // ScopedSpanGuard or passing its context explicitly via childSpan(); a + // child created here today would parent to this span's parent, not to it. using namespace telemetry; auto span = SpanGuard::span( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::discover); @@ -744,7 +749,20 @@ PathRequest::doUpdate( auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::compute); span.setAttribute(pathfind_span::attr::fast, fast); - span.setAttribute(pathfind_span::attr::destCurrency, to_string(saDstAmount_.asset()).c_str()); + // to_string(Issue) renders a non-XRP asset as "/" with the + // issuer as a plaintext Base58 address, so it cannot be emitted as-is: every + // account reaching a span is hashed first. Redact just the issuer and keep + // the currency, which is what this attribute is for. An MPT asset renders as + // its issuance ID and carries no address, so it needs no redaction. + span.setAttribute( + pathfind_span::attr::destCurrency, + saDstAmount_.asset().visit( + [](Issue const& issue) { + return isXRP(issue.account) + ? to_string(issue.currency) + : redactAccount(toBase58(issue.account)) + "/" + to_string(issue.currency); + }, + [](MPTIssue const& mpt) { return to_string(mpt.getMptID()); })); JLOG(journal_.debug()) << iIdentifier_ << " update " << (fast ? "fast" : "normal"); diff --git a/src/xrpld/rpc/detail/PathRequestManager.cpp b/src/xrpld/rpc/detail/PathRequestManager.cpp index 9141aaded4..c8c6c12fc2 100644 --- a/src/xrpld/rpc/detail/PathRequestManager.cpp +++ b/src/xrpld/rpc/detail/PathRequestManager.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -77,15 +78,21 @@ PathRequestManager::updateAll(std::shared_ptr const& inLedger) using namespace telemetry; // updateAll runs on every ledger close. Skip span emission when there are // no active path subscriptions, to avoid a steady stream of empty spans at - // mainnet close cadence. A null guard is used in that case; all other work - // still runs unchanged (notably the isNewPathRequest() flag reset below), - // so behaviour matches the pre-span code path. - auto span = requests.empty() - ? SpanGuard{} - : SpanGuard::span( - TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::updateAll); - span.setAttribute(pathfind_span::attr::ledgerIndex, static_cast(inLedger->seq())); - span.setAttribute(pathfind_span::attr::numRequests, static_cast(requests.size())); + // mainnet close cadence. All other work still runs unchanged (notably the + // isNewPathRequest() flag reset below), so behaviour matches the pre-span + // code path. + // + // Scoped, so the pathfind.compute spans that doUpdate() creates below on + // this thread nest under it. std::optional because ScopedSpanGuard is + // deliberately non-movable, so it cannot be produced by a ternary. + std::optional span; + if (!requests.empty()) + { + span.emplace( + TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::updateAll); + span->setAttribute(pathfind_span::attr::ledgerIndex, static_cast(inLedger->seq())); + span->setAttribute(pathfind_span::attr::numRequests, static_cast(requests.size())); + } bool newRequests = app_.getLedgerMaster().isNewPathRequest(); bool mustBreak = false; diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 25bacc6d73..c12e50fd95 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -107,6 +108,42 @@ statusRequestResponse(http_request_type const& request, boost::beast::http::stat return handoff; } +/** + * Resolve the command attribute for a WebSocket message to a bounded value. + * + * The command/method field is client-supplied and is promoted to a Prometheus + * label by the spanmetrics connector, so the raw string must never be emitted: + * arbitrary request input would drive unbounded label cardinality. Resolving + * against the handler registry keeps per-command attribution for real commands + * and collapses everything else to a single "unknown" series. + * + * A request naming both fields with different values is not a real command + * (processSession rejects it below), so it also resolves to "unknown". + * + * @param jv The parsed WebSocket request object. + * @param config Node config, for the beta-RPC-API flag used to look up the + * handler for the request's API version. + * @return The canonical handler name, or "unknown" for an unrecognized, + * missing, non-string, or self-contradictory command. + */ +static std::string_view +resolveWsCommandSpanName(json::Value const& jv, Config const& config) +{ + bool const hasCommand = jv.isMember(jss::command) && jv[jss::command].isString(); + bool const hasMethod = jv.isMember(jss::method) && jv[jss::method].isString(); + if (!hasCommand && !hasMethod) + return rpc_span::val::unknownCommand; + + std::string const cmd = hasCommand ? jv[jss::command].asString() : jv[jss::method].asString(); + if (hasCommand && hasMethod && cmd != jv[jss::method].asString()) + return rpc_span::val::unknownCommand; + + auto const* handler = + RPC::getHandler(RPC::getAPIVersionNumber(jv, config.betaRpcApi), config.betaRpcApi, cmd); + return (handler != nullptr) ? std::string_view{handler->name} + : std::string_view{rpc_span::val::unknownCommand}; +} + // VFALCO TODO Rewrite to use boost::beast::http::fields static bool authorized(Port const& port, std::map const& h) @@ -434,14 +471,13 @@ ServerHandler::processSession( // Fresh root so each WS message is its own trace. auto span = ScopedSpanGuard::freshRoot( TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); - if (jv.isMember(jss::command) && jv[jss::command].isString()) - { - span.setAttribute(rpc_span::attr::command, jv[jss::command].asString().c_str()); - } - else if (jv.isMember(jss::method) && jv[jss::method].isString()) - { - span.setAttribute(rpc_span::attr::command, jv[jss::method].asString().c_str()); - } + // The command is client-supplied and becomes a Prometheus label via the + // spanmetrics connector, so it is resolved against the handler registry + // before emission: a recognized command keeps its canonical name, anything + // else collapses to "unknown". Emitting the raw string would let request + // input drive unbounded label cardinality. Mirrors the HTTP path's + // resolveCommandSpanName(). + span.setAttribute(rpc_span::attr::command, resolveWsCommandSpanName(jv, app_.config())); auto is = std::static_pointer_cast(session->appDefined); if (is->getConsumer().disconnect(journal_)) diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index bf7fdfe264..9053fdda90 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -13,6 +13,8 @@ #include #include +#include + namespace xrpl { json::Value @@ -23,10 +25,15 @@ doPathFind(RPC::JsonContext& context) // thread) nest under it. doPathFind does not yield, so scoping is safe. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Addresses are hashed before emission for privacy. - if (auto const& src = context.params[jss::source_account]; src.isString()) + // Addresses are hashed before emission for privacy. Read through a const + // reference: the non-const json::Value::operator[] inserts a null for a + // missing key, which would make PathRequest::parseJson's isMember() checks + // see an absent field as present and return Malformed instead of Missing. + // Reading for telemetry must not alter what the request looks like. + auto const& params = std::as_const(context.params); + if (auto const& src = params[jss::source_account]; src.isString()) span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = context.params[jss::destination_account]; dst.isString()) + if (auto const& dst = params[jss::destination_account]; dst.isString()) span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); if (context.app.config().pathSearchMax == 0) diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index f65a41a911..b264fde9ef 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -34,10 +34,15 @@ doRipplePathFind(RPC::JsonContext& context) // span's log lines stay trace-correlated. auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); - // Addresses are hashed before emission for privacy. - if (auto const& src = context.params[jss::source_account]; src.isString()) + // Addresses are hashed before emission for privacy. Read through a const + // reference: the non-const json::Value::operator[] inserts a null for a + // missing key, which would make PathRequest::parseJson's isMember() checks + // see an absent field as present and return Malformed instead of Missing. + // Reading for telemetry must not alter what the request looks like. + auto const& params = std::as_const(context.params); + if (auto const& src = params[jss::source_account]; src.isString()) span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); - if (auto const& dst = context.params[jss::destination_account]; dst.isString()) + if (auto const& dst = params[jss::destination_account]; dst.isString()) span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); if (context.app.config().pathSearchMax == 0) From 5b7081c3b70bdd9ba33ea0fb616e9a67a36b0da8 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 30 Jul 2026 19:47:58 +0100 Subject: [PATCH 2/4] fix(telemetry): start tracing before the first spans are emitted telemetry_->start() ran at the end of ApplicationImp::start(), after overlay_->start(). Spans are emitted well before that, during setup(): beginConsensus() runs the first consensus round there. SpanGuard drops a span whenever the global Telemetry instance is not yet live, so that round's spans were never recorded. Move the start into setup(), behind a new startTelemetry() seam, right after the node identity is known. getNodeIdentity() needs only the cmdline, the config, or the wallet DB, and initRelationalDatabase() has already created the latter -- the adjacent peerReservations_ load proves it is usable -- so the identity block moves up with it. The new position is bounded on both sides: after initRelationalDatabase() because the identity needs the wallet DB and a DB failure aborts setup(), and before beginConsensus() because that emits the first spans. --- src/xrpld/app/main/Application.cpp | 52 ++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index c8de63f914..9494144f48 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1141,6 +1141,22 @@ private: void startGenesisLedger(); + /** + * Start the tracing pipeline. + * + * Called once from setup(), as soon as the node identity is known. + * Starting here rather than in start() means spans emitted during the + * rest of setup() are recorded: SpanGuard drops a span whenever the + * global Telemetry instance is not yet live, and the first consensus + * round runs inside setup(). + * + * @pre nodeIdentity_ is populated, so setServiceInstanceId() has + * already supplied the service.instance.id resource attribute + * (the Telemetry resource is fixed once start() builds it). + */ + void + startTelemetry(); + std::shared_ptr getLastFullLedger(); @@ -1227,6 +1243,27 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) return false; } + nodeIdentity_ = getNodeIdentity(*this, cmdline); + + // Now that the node identity is known, inject it into the telemetry + // resource attributes — but only if the user didn't already set a + // custom service_instance_id in [telemetry]. The Telemetry object + // was constructed with an empty serviceInstanceId because + // nodeIdentity_ is not available in the member initializer list. + if (!config_->section("telemetry").exists("service_instance_id")) + telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); + + // Start telemetry here, not in start(). Spans are emitted during the rest + // of setup() — the first consensus round in beginConsensus() below — and + // are dropped unless the global Telemetry instance is already live. + // + // The position is bounded on both sides: + // - After initRelationalDatabase(): the wallet DB must exist for the node + // identity above, and a DB failure aborts setup(), so starting earlier + // would export a partial trace stream for a run that never comes up. + // - Before beginConsensus(): that call emits the first consensus spans. + startTelemetry(); + if (validatorKeys_.keys) setMaxDisallowedLedger(); @@ -1314,16 +1351,6 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) orderBookDB_->setup(getLedgerMaster().getCurrentLedger()); - nodeIdentity_ = getNodeIdentity(*this, cmdline); - - // Now that the node identity is known, inject it into the telemetry - // resource attributes — but only if the user didn't already set a - // custom service_instance_id in [telemetry]. The Telemetry object - // was constructed with an empty serviceInstanceId because - // nodeIdentity_ is not available in the member initializer list. - if (!config_->section("telemetry").exists("service_instance_id")) - telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); - if (!cluster_->load(config().section(Sections::kClusterNodes))) { JLOG(journal_.fatal()) << "Invalid entry in cluster configuration."; @@ -1536,6 +1563,11 @@ ApplicationImp::start(bool withTimers) ledgerCleaner_->start(); perfLog_->start(); +} + +void +ApplicationImp::startTelemetry() +{ telemetry_->start(); } From 2a5fdf08574a51021f330f4d795befa9d0c6611b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 30 Jul 2026 19:48:41 +0100 Subject: [PATCH 3/4] fix(telemetry): create the meter before the first metric is recorded MetricsRegistry::start() fused three steps with different prerequisites: the exporter/provider and the synchronous instruments need only config strings, while the observable gauges read live Application services. The whole pipeline therefore waited on the latest prerequisite and ran near the end of setup() -- after beginConsensus() had already recorded the process's only operating-mode transition. state_changes_total was emitted into a pipeline that did not exist yet, so the series never appeared at all. Split the two halves. start() keeps the provider and the synchronous instruments and now runs as soon as the registry is constructed; the new startAsyncGauges() registers the observable callbacks and runs once overlay_ exists, still before the first consensus round. Application gains a matching startTelemetryGauges() so each phase has its own call site and its own precondition. Move the jq_trans_overflow_total observable out of initSyncInstruments() into the gauge phase. Its callback reads getOverlay(), which asserts overlay_ is non-null, so creating it in the early phase armed the reader thread against a half-built application -- an assert is not caught by the callback's catch-all. The instrument is an observable counter rather than a gauge, which is how it was mistaken for a push-only instrument. Both start log lines are kept, one per phase, because that timeline is what made the original ordering bug diagnosable. Comments and preconditions are corrected to state the rule rather than the current arrangement: start() may only create instruments whose values are pushed, and any observable whose callback reads a service belongs in the gauge phase. The gauge precondition now lists the services the callbacks actually read. --- .../libxrpl/telemetry/MetricsRegistry.cpp | 117 ++++++++++++++++-- src/xrpld/app/main/Application.cpp | 93 ++++++++++---- src/xrpld/telemetry/MetricsRegistry.cpp | 94 ++++++++++---- src/xrpld/telemetry/MetricsRegistry.h | 89 +++++++++++-- 4 files changed, 322 insertions(+), 71 deletions(-) diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index f21ddfe831..c6a707cb0c 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -14,11 +14,12 @@ * on the nodestore_state gauge. Also a public static constexpr inline, * so it runs in both builds for the same reason. * - * 3. The no-op / telemetry-disabled path — construction, start()/stop() - * lifecycle, and the synchronous record*() methods. Guarded, because - * when XRPL_ENABLE_TELEMETRY is defined MetricsRegistry.cpp is not - * compiled into this binary (see src/tests/libxrpl/CMakeLists.txt) and - * its out-of-line symbols are unresolvable here. + * 3. The no-op / telemetry-disabled path — construction, the two-phase + * start() / startAsyncGauges() / stop() lifecycle, and the synchronous + * record*() methods. Guarded, because when XRPL_ENABLE_TELEMETRY is + * defined MetricsRegistry.cpp is not compiled into this binary (see + * src/tests/libxrpl/CMakeLists.txt) and its out-of-line symbols are + * unresolvable here. */ #include @@ -430,11 +431,20 @@ TEST(MetricsRegistryScaledMean, default_scale_is_one) #include #include #include +#include using namespace xrpl; namespace { +/** + * OTLP/HTTP endpoint passed to every start() call below. Nothing ever dials + * it -- these tests exercise the no-op path -- it just has to be a plausible + * URL. start() takes `std::string const&`, so call sites construct one from + * this view rather than repeating the literal. + */ +constexpr std::string_view kTestEndpoint{"http://localhost:4318/v1/metrics"}; + /** * Minimal mock ServiceRegistry for MetricsRegistry testing. * @@ -728,17 +738,108 @@ TEST_F(MetricsRegistryTest, disabled_start_stop) telemetry::MetricsRegistry registry(false, mockApp_, j_); // start() and stop() should be no-ops when disabled. - registry.start("http://localhost:4318/v1/metrics"); + registry.start(std::string{kTestEndpoint}); registry.stop(); // Double stop should be safe. registry.stop(); } +// --------------------------------------------------------------------------- +// The two-phase startup split: start() then startAsyncGauges(). +// +// Why the split exists: start() reads only config strings, while the +// observable-instrument callbacks registered by startAsyncGauges() read live +// Application services (getOverlay() asserts overlay_ is non-null). The split +// lets the meter go live before the first consensus round records its +// mode-transition counter, while the callbacks still wait for the subsystems. +// +// SCOPE OF THESE TESTS -- read before adding to them. MetricsRegistry.cpp is +// compiled into this binary ONLY when telemetry is OFF +// (src/tests/libxrpl/CMakeLists.txt:117-126 -- the `else()` branch; when it is +// ON the .cpp needs concrete xrpld types such as LedgerMaster, TxQ, NetworkOPs, +// Overlay and node_store::Database, which a standalone GTest binary cannot +// link). Both start() and startAsyncGauges() therefore compile here to their +// `#else` branch, which only (void)-casts its arguments. So these tests pin the +// API SURFACE -- that both entry points exist, are callable in either order, +// and leave the object usable -- and NOT the gauge behaviour. Real coverage of +// "gauges observe values only after startAsyncGauges()" is unreachable from +// this target; it needs the enabled path plus an in-memory metric reader. +// +// Two properties the production code does NOT have, so nothing below asserts +// them: startAsyncGauges() has no idempotency guard (a second call on the +// enabled path would create a second set of same-named instruments), and +// callbacksDetached_ is one-way, so detachCallbacks() followed by +// startAsyncGauges() would register permanently-dead instruments. +// --------------------------------------------------------------------------- + +TEST_F(MetricsRegistryTest, async_gauges_start_after_start_is_safe) +{ + telemetry::MetricsRegistry registry(false, mockApp_, j_); + + // The documented order: provider/sync instruments first, gauges second. + registry.start(std::string{kTestEndpoint}); + registry.startAsyncGauges(); + + // State: the enable flag is untouched by either phase. Exact value, not + // merely "falsy" -- a phase that flipped it would be a real defect. + EXPECT_EQ(registry.isEnabled(), false); + + // Synchronous recording must work off phase 1 alone. This is the whole + // point of the split: nothing here needs the gauges to be registered. + registry.recordRpcStarted("server_info"); + registry.recordRpcFinished("server_info", 1000); + + registry.stop(); + EXPECT_EQ(registry.isEnabled(), false); +} + +TEST_F(MetricsRegistryTest, async_gauges_before_start_does_not_break_start) +{ + telemetry::MetricsRegistry registry(false, mockApp_, j_); + + // Negative path: the mis-ordered call, gauges before the provider exists. + // In THIS build it reaches the (void)-cast stub, so what is actually + // proven is only that the entry point tolerates being called first and + // leaves the object usable -- not that the enabled path's `if (!meter_)` + // guard works, since that guard is inside #ifdef XRPL_ENABLE_TELEMETRY and + // is not compiled here. + registry.startAsyncGauges(); + EXPECT_EQ(registry.isEnabled(), false); + + // Phase 1 still works afterwards, so the bad call left no state behind. + registry.start(std::string{kTestEndpoint}); + registry.recordJobQueued("ledgerData", "ProcessLData"); + EXPECT_EQ(registry.isEnabled(), false); + + registry.stop(); +} + +TEST_F(MetricsRegistryTest, async_gauges_respect_the_compile_time_guard) +{ + // Constructed with enabled=true, which on the enabled path would register + // instruments for real. In this build XRPL_ENABLE_TELEMETRY is undefined, + // so both phases compile to the (void)-cast stub branch and neither + // touches the mock -- every MockServiceRegistry accessor throws, so a + // callback that actually ran would surface as a thrown exception here. + telemetry::MetricsRegistry registry(true, mockApp_, j_); + + // Cause, not just state: the flag really is true, so the no-op below is + // attributable to the compile-time guard and not to an early enabled_ + // return. + EXPECT_EQ(registry.isEnabled(), true); + + EXPECT_NO_THROW(registry.start(std::string{kTestEndpoint})); + EXPECT_NO_THROW(registry.startAsyncGauges()); + EXPECT_NO_THROW(registry.stop()); + + EXPECT_EQ(registry.isEnabled(), true); +} + TEST_F(MetricsRegistryTest, disabled_recording_methods) { telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start("http://localhost:4318/v1/metrics"); + registry.start(std::string{kTestEndpoint}); // All recording methods should be no-ops (not crash). registry.recordRpcStarted("server_info"); @@ -756,7 +857,7 @@ TEST_F(MetricsRegistryTest, destructor_calls_stop) { // Let the destructor handle cleanup. telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start("http://localhost:4318/v1/metrics"); + registry.start(std::string{kTestEndpoint}); } // If we get here without crash, the destructor handled stop. } diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index c543e7e628..bb999fc9c1 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1176,28 +1176,56 @@ private: startGenesisLedger(); /** - * Start the tracing and metrics pipelines. + * Start the tracing pipeline and the metrics provider and synchronous + * instruments. First of the two telemetry startup phases. * - * Called once from setup(), just before the [rpc_startup] loop. Starting - * here (rather than in start()) guarantees the OTel MeterProvider is live - * before any metric-emitting code runs — including startup RPCs, whose - * PerfLog instrumentation records a call-site metric. A call-site metric - * macro caches its instrument on first use via std::call_once; if that - * first use happens while the meter is still empty, the instrument latches - * null for the process lifetime and the metric silently never records. + * Called once from setup(), immediately after metricsRegistry_ is + * constructed. Starting here (rather than in start()) guarantees the OTel + * MeterProvider is live before any metric-emitting code runs — including + * the first consensus round, which records a mode-transition counter, and + * the startup RPCs, whose PerfLog instrumentation records a call-site + * metric. A call-site metric macro caches its instrument on first use via + * std::call_once; if that first use happens while the meter is still + * empty, the instrument latches null for the process lifetime and the + * metric silently never records. * - * The call site sits after overlay_ and the other subsystems are - * constructed, because the metrics reader thread starts here and its - * observable-gauge callbacks read that state (e.g. getOverlay()); starting - * earlier would let the reader observe a half-built application. + * Rule for keeping this call site valid: only telemetry work that reads + * NO application subsystem may run here. That holds today — this phase + * uses the config strings and the node identity, and creates only + * push-model counters and histograms, which app code records into once + * it is ready. Anything that registers a callback reading a subsystem + * must go in startTelemetryGauges() instead, because a callback + * registered here can fire on the metrics reader thread while the rest of + * the application is still being built. * * @pre nodeIdentity_ is populated (needed for the service_instance_id - * fallback), metricsRegistry_ is constructed, and overlay_ (and the other - * subsystems read by observable-gauge callbacks) are constructed. + * fallback) and metricsRegistry_ is constructed. */ void startTelemetry(); + /** + * Register the pull-model observable instruments. Second telemetry phase. + * + * Called once from setup(), immediately after overlay_ is constructed. + * Registering an observable instrument arms the metrics reader thread to + * invoke its callback, and those callbacks read application services — + * getOverlay() asserts overlay_ is non-null, and an assert is not caught + * by the callbacks' own try/catch — so this cannot run as early as + * startTelemetry(). + * + * @pre startTelemetry() has run, and every service the callbacks read is + * constructed. overlay_ is the binding one: the rest (networkOPs_, + * ledgerMaster_, openLedger_, txQ_, nodeStore_, nodeFamily_, + * validators_, acceptedLedgerCache_, cachedSLEs_, acquireStats_, + * timeKeeper_, relationalDatabase_, inboundLedgers_, feeTrack_) are + * already live by the time startTelemetry() is callable, and + * overlay_ is the only one built after it. See + * MetricsRegistry::startAsyncGauges() for the full list. + */ + void + startTelemetryGauges(); + std::shared_ptr getLastFullLedger(); @@ -1382,11 +1410,18 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); // Create the OTel MetricsRegistry for gap-fill metrics (counters, - // histograms, observable gauges). It is started later, just before the - // [rpc_startup] loop (see startTelemetry()). + // histograms, observable gauges). metricsRegistry_ = std::make_unique( telemetry_->isEnabled(), *this, logs_->journal("MetricsRegistry")); + // Start tracing and the metrics provider right away, so the meter exists + // before anything records a metric. beginConsensus() below emits a + // mode-transition counter, and it is the only mode transition the process + // ever makes — a meter created after it would lose that series entirely. + // Only the observable gauges have to wait; they are registered by + // startTelemetryGauges() once overlay_ exists. + startTelemetry(); + if (!cluster_->load(config().section(Sections::kClusterNodes))) { JLOG(journal_.fatal()) << "Invalid entry in cluster configuration."; @@ -1462,6 +1497,15 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) collectorManager_->collector()); add(*overlay_); // add to PropertyStream + // Register the observable instruments now that overlay_ exists. This arms + // the metrics reader thread to invoke their callbacks, several of which + // read getOverlay() — registering earlier would let the reader observe a + // half-built application. The reader thread itself already started in + // startTelemetry() above; this is as early as the callbacks can safely be + // attached, and it is still before beginConsensus() so the gauges cover + // the first round. + startTelemetryGauges(); + // start first consensus round if (!networkOPs_->beginConsensus(ledgerMaster_->getClosedLedger()->header().hash, {})) { @@ -1528,16 +1572,6 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) JLOG(journal_.warn()) << "*** standalone signing solution as soon as possible."; } - // Start telemetry and metrics now — before the [rpc_startup] loop below — - // so the OTel meter is live before any metric-emitting code runs. Startup - // RPCs invoke PerfLog instrumentation that records a call-site metric; a - // metric macro caches its instrument on first use, so a first use before - // the meter exists would latch null for the process lifetime. Placed here, - // after overlay_ and the other subsystems the observable-gauge callbacks - // read are constructed, so the metrics reader thread never observes a - // half-built application. - startTelemetry(); - // // Execute start up rpc commands. // @@ -1636,6 +1670,13 @@ ApplicationImp::startTelemetry() } } +void +ApplicationImp::startTelemetryGauges() +{ + if (metricsRegistry_) + metricsRegistry_->startAsyncGauges(); +} + void ApplicationImp::run() { diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index 5424e7a8b7..a0eb50ced3 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -229,14 +229,45 @@ MetricsRegistry::start(std::string const& endpoint, std::string const& instanceI JLOG(journal_.info()) << "MetricsRegistry: starting, endpoint=" << endpoint << ", instanceId=" << instanceId; + // Rule for anything added below: this phase may create only instruments + // whose recording is PUSHED from app code -- counters and histograms. An + // instrument registered here is live immediately, and the reader thread + // may invoke a registered callback before the rest of the Application is + // built, so any observable whose callback reads an Application service + // belongs in startAsyncGauges(), not here. That includes observable + // COUNTERS, not just gauges: jq_trans_overflow_total was created here and + // its callback read getOverlay(), which asserts overlay_ is non-null. initExporterAndProvider(endpoint, instanceId); initSyncInstruments(); + + JLOG(journal_.info()) << "MetricsRegistry: provider and instruments ready"; +#else + (void)endpoint; + (void)instanceId; + (void)enabled_; +#endif // XRPL_ENABLE_TELEMETRY +} + +void +MetricsRegistry::startAsyncGauges() +{ +#ifdef XRPL_ENABLE_TELEMETRY + if (!enabled_) + return; + + // A mis-ordered call must not crash: without a meter there is nothing to + // create instruments on, so registration is skipped entirely. + if (!meter_) + { + JLOG(journal_.warn()) << "MetricsRegistry: startAsyncGauges() called " + "before start(); no gauges registered"; + return; + } + registerAsyncGauges(); JLOG(journal_.info()) << "MetricsRegistry: started successfully"; #else - (void)endpoint; - (void)instanceId; (void)enabled_; #endif // XRPL_ENABLE_TELEMETRY } @@ -340,30 +371,6 @@ MetricsRegistry::initSyncInstruments() "validations_checked_total", "Total network validations received and checked"); stateChangesCounter_ = meter_->CreateUInt64Counter("state_changes_total", "Total operating mode changes"); - // jq_trans_overflow_total is observed from Overlay's existing cumulative - // atomic (Overlay::getJqTransOverflow()) rather than pushed. The overlay - // owns the only increment site (PeerImp), so an ObservableCounter reads the - // live total each collection cycle without threading a push path through - // develop-owned overlay code. - jqTransOverflowObservable_ = meter_->CreateInt64ObservableCounter( - "jq_trans_overflow_total", "Total job queue transaction overflows"); - jqTransOverflowObservable_->AddCallback( - [](opentelemetry::metrics::ObserverResult result, void* state) { - auto* self = static_cast(state); - if (self->callbacksDetached_.load(std::memory_order_acquire)) - return; - try - { - opentelemetry::nostd::get>>(result) - ->Observe(static_cast(self->app_.getOverlay().getJqTransOverflow())); - } - catch (...) // NOLINT(bugprone-empty-catch) - { - // Silently skip on error. - } - }, - this); ledgerHistoryMismatchCounter_ = meter_->CreateUInt64Counter( "ledger_history_mismatch_total", "Total built-vs-validated ledger mismatches by reason"); txqExpiredCounter_ = meter_->CreateUInt64Counter( @@ -560,6 +567,7 @@ MetricsRegistry::registerAsyncGauges() // Each helper creates one observable instrument and attaches one // callback. Keeping the registration bodies in separate methods // preserves the 80-line-per-function limit enforced by CLAUDE.md. + registerJqTransOverflowCounter(); registerCacheHitRateGauge(); registerTxqGauge(); registerObjectCountGauge(); @@ -579,6 +587,40 @@ MetricsRegistry::registerAsyncGauges() registerValidationTotalsCounters(); } +void +MetricsRegistry::registerJqTransOverflowCounter() +{ + // jq_trans_overflow_total is observed from Overlay's existing cumulative + // atomic (Overlay::getJqTransOverflow()) rather than pushed. The overlay + // owns the only increment site (PeerImp), so an ObservableCounter reads the + // live total each collection cycle without threading a push path through + // develop-owned overlay code. + // + // Registered with the gauges, not with the synchronous instruments: the + // callback reads getOverlay(), which asserts overlay_ is non-null. Arming + // it any earlier would let a reader tick fire before the overlay exists, + // and an assert is not caught by the try block below. + jqTransOverflowObservable_ = meter_->CreateInt64ObservableCounter( + "jq_trans_overflow_total", "Total job queue transaction overflows"); + jqTransOverflowObservable_->AddCallback( + [](opentelemetry::metrics::ObserverResult result, void* state) { + auto* self = static_cast(state); + if (self->callbacksDetached_.load(std::memory_order_acquire)) + return; + try + { + opentelemetry::nostd::get>>(result) + ->Observe(static_cast(self->app_.getOverlay().getJqTransOverflow())); + } + catch (...) // NOLINT(bugprone-empty-catch) + { + // Silently skip on error. + } + }, + this); +} + void MetricsRegistry::registerCacheHitRateGauge() { diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 22d7fe2eba..20494999fa 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -94,11 +94,17 @@ * Example usage: * * @code - * // In Application::setup(), after telemetry_ is created: + * // In Application::setup(), after telemetry_ is created. Phase 1 needs + * // only the config strings, so it runs immediately and the meter is live + * // before any metric-emitting code: * metricsRegistry_ = std::make_unique( * telemetry_->isEnabled(), app, journal); * metricsRegistry_->start(setup.exporterEndpoint); * + * // Later in setup(), once overlay_ exists (the last of the services the + * // callbacks read). Phase 2 registers the observable instruments: + * metricsRegistry_->startAsyncGauges(); + * * // In PerfLogImp::rpcStart(): * if (auto* mr = app_.getMetricsRegistry()) * mr->recordRpcStarted("server_info"); @@ -207,9 +213,9 @@ namespace telemetry { * catch-all try block so a transient failure never crashes * the reader thread. * - ValidationTracker protects its rolling windows internally. - * - start() and stop() are NOT thread-safe with each other and - * must be called from the single Application lifecycle - * thread. + * - start(), startAsyncGauges() and stop() are NOT thread-safe + * with each other and must all be called, in that order, from + * the single Application lifecycle thread. * * @note Lifetime: * - Must be constructed AFTER telemetry_ (reads isEnabled()). @@ -254,7 +260,24 @@ public: operator=(MetricsRegistry const&) = delete; /** - * Initialize the OTel metrics pipeline and register all instruments. + * Initialize the OTel metrics pipeline and create the SYNCHRONOUS + * instruments (counters and histograms). + * + * This is the first of two startup phases, and it can be called as soon + * as the registry is constructed — which is what makes the meter live + * before the first metric-emitting code runs. Startup RPCs and the first + * consensus round both record metrics; a call-site metric macro caches + * its instrument on first use, so a first use before the meter exists + * latches null for the process lifetime. + * + * @note Invariant for future changes: this phase may create only + * instruments with NO Application-reading callback. Push-model + * counters and histograms qualify; app code records into them + * when it is ready. Any observable instrument whose callback + * reads an Application service belongs in `startAsyncGauges()`, + * because registering it here arms the reader thread to invoke + * that callback against a half-built Application. This applies + * to observable COUNTERS as well as gauges. * * @param endpoint OTLP/HTTP endpoint URL for metric export * (e.g. "http://localhost:4318/v1/metrics"). @@ -266,6 +289,37 @@ public: void start(std::string const& endpoint, std::string const& instanceId = {}); + /** + * Register the pull-model observable instruments — the second startup + * 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. + * Registering an observable also arms the reader thread to invoke its + * callback on the next tick, which is why the split is about ordering + * and not just tidiness. + * + * @pre `start()` has already run (the meter exists). If it has not, + * this is a logged no-op rather than a crash. + * @pre Every service the callbacks read is constructed. The full set, + * from the `app.get*()` calls in the registration helpers, is: + * Overlay, OPs (NetworkOPs), LedgerMaster, OpenLedger, TxQ, + * NodeStore, NodeFamily, Validators, AcceptedLedgerCache, + * CachedSLEs, AcquireStats, TimeKeeper, RelationalDatabase, + * InboundLedgers and FeeTrack. + * All but Overlay already exist by the time `start()` is + * callable, so Overlay is what fixes this call's position: + * `ServiceRegistry::getOverlay()` `XRPL_ASSERT`s that + * `overlay_` is non-null, and a reader-thread tick before the + * overlay exists aborts a Debug build. The callbacks' catch-all + * try block does not catch an assert. `getTxQ()` and + * `getRelationalDatabase()` assert likewise. + */ + void + startAsyncGauges(); + /** * Detach all ObservableGauge callbacks so they no-op on the next * reader-thread tick. @@ -277,9 +331,15 @@ public: * guarantees that once `detachCallbacks()` returns, no subsequent * callback invocation will dereference an already-stopped service. * - * Idempotent. Safe to call multiple times. Safe to call before - * `start()` (has no effect). The actual SDK-level provider - * shutdown still happens in `stop()`. + * Idempotent, and safe to call multiple times: the flag is one-way, + * only ever set to true, and nothing clears it. The actual + * SDK-level provider shutdown still happens in `stop()`. + * + * @note One-way means this is a shutdown-only call. Calling it before + * `startAsyncGauges()` does not "have no effect" — it + * permanently disarms every gauge the later call registers, so + * the instruments exist but never observe a value. Only call it + * once the process is shutting down. */ void detachCallbacks() noexcept; @@ -927,15 +987,22 @@ private: * Register all observable gauge callbacks with the OTel SDK. * Dispatches to one helper per metric domain so that each helper * stays well under the 80-line-per-function limit. + * + * Called only from `startAsyncGauges()`, which owns the enabled_ and + * meter_ guards and the Application-state precondition. */ void registerAsyncGauges(); - // Per-domain gauge registration helpers. Each creates its instrument - // and attaches a single ObservableGauge callback that reads current - // values from Application services. The callbacks run on the OTel + // Per-domain registration helpers for the async (pull-model) phase. + // Each creates its instrument -- an ObservableGauge, or an + // ObservableCounter where the underlying value is cumulative -- and + // attaches a single callback that reads current values from Application + // services. The callbacks run on the OTel // PeriodicExportingMetricReader background thread (~10 s tick). void + registerJqTransOverflowCounter(); // gap-fill: overlay overflow total + void registerCacheHitRateGauge(); // Task 9.2 void registerTxqGauge(); // Task 9.3 From ef185b35d2494381723e553ab7b04f5a75745ba7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 30 Jul 2026 19:49:12 +0100 Subject: [PATCH 4/4] fix(telemetry): report base_fee_xrp as the reference transaction cost base_fee_xrp was observed from LoadFeeTrack::getLocalFee(), which is the local load-scaled fee escalation, not the ledger's base fee. The panel built on it therefore tracked this node's load state rather than the network's cost of a reference transaction, and read as a flat line whenever the node was unloaded. Read it from the validated ledger's fee settings instead, alongside the reserve values already taken from there. The observation now only reports when a validated ledger is available, which is correct: before that there is no network fee to report. --- src/xrpld/telemetry/MetricsRegistry.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index a0eb50ced3..2670ffdc5a 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -1392,14 +1392,13 @@ MetricsRegistry::registerLedgerEconomyGauge() ->Observe(value, {{"metric", name}}); }; - // Local fee (drops). - observe("base_fee_xrp", static_cast(app.getFeeTrack().getLocalFee())); - - // Reserve values from the validated ledger. + // Fee and reserve values from the validated ledger. auto const ledger = app.getLedgerMaster().getValidatedLedger(); if (ledger) { auto const& fees = ledger->fees(); + // Cost of a reference transaction (drops). + observe("base_fee_xrp", static_cast(fees.base.drops())); // Base reserve = one account, zero owned objects: // accountReserve(ownerCount=0, accountCount=1) == reserve. observe(