From 7d875c0e763d54f8d5d7b59d5cc53d34863bc113 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:30:13 +0100 Subject: [PATCH 01/19] refactor(telemetry): name the unit and epoch in the close-time span attrs The three close-time span attributes hold NetClock readings, which are whole seconds since the XRP Ledger epoch of 2000-01-01 rather than the Unix epoch. Neither the unit nor the epoch was recoverable from the key, so a consumer rendering one as a wall-clock time without first adding the epoch offset lands roughly thirty years early. The sibling close_resolution_ms already named its unit, so the header disagreed with itself. close_time -> close_time_ripple_epoch_s parent_close_time -> parent_close_time_ripple_epoch_s close_time_self -> close_time_self_ripple_epoch_s Emitted values do not change; only the keys do. The public RPC response fields of the same name are deliberately untouched, as renaming those would break the ledger API. --- include/xrpl/consensus/ConsensusSpanNames.h | 24 ++++++++++++++----- include/xrpl/telemetry/SpanNames.h | 13 +++++++++- .../libxrpl/telemetry/SpanGuardFactory.cpp | 2 +- src/xrpld/app/consensus/RCLConsensus.cpp | 7 +++--- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index fc56adcd44..3db180c463 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -59,10 +59,10 @@ * | | * | +-- consensus.accept.apply [jtACCEPT thread, child of accept] * | Created: Adaptor::doAccept() - * | Attrs: ledger_seq, close_time, close_time_correct, + * | Attrs: ledger_seq, close_time_ripple_epoch_s, close_time_correct, * | close_resolution_ms, consensus_state, proposing, round_time_ms, - * | parent_close_time, close_time_self, close_time_vote_bins, - * | resolution_direction, tx_count + * | parent_close_time_ripple_epoch_s, close_time_self_ripple_epoch_s, + * | close_time_vote_bins, resolution_direction, tx_count * | Events: tx.included (per tx, attrs: tx_id) * | * +~~~ consensus.validation.send [jtACCEPT thread, linked] @@ -140,8 +140,8 @@ namespace attr { * concept, same key, distinguished by span name (not an emitter prefix). */ using ::xrpl::telemetry::attr::closeResolutionMs; -using ::xrpl::telemetry::attr::closeTime; using ::xrpl::telemetry::attr::closeTimeCorrect; +using ::xrpl::telemetry::attr::closeTimeRippleEpochS; using ::xrpl::telemetry::attr::fullValidation; using ::xrpl::telemetry::attr::ledgerHash; using ::xrpl::telemetry::attr::ledgerSeq; @@ -232,8 +232,20 @@ inline constexpr auto positionHashPrefix = makeStr("position_hash_prefix"); * "consensus_state" — domain-qualified (collides with other domains' state). */ inline constexpr auto consensusState = makeStr("consensus_state"); -inline constexpr auto parentCloseTime = makeStr("parent_close_time"); -inline constexpr auto closeTimeSelf = makeStr("close_time_self"); +/** + * Close-time instants, both NetClock readings in whole seconds since the XRP + * Ledger epoch (2000-01-01T00:00:00Z) — see `closeTimeRippleEpochS` in + * SpanNames.h for why the epoch is spelled into the key. + * + * `parentCloseTimeRippleEpochS` is the previous ledger's close time; + * `closeTimeSelfRippleEpochS` is this node's own close-time vote for the round, + * so the pair shows how far the node's position sat from the ledger it built on. + * + * `closeTimeVoteBins` is not a time: it holds the number of distinct close-time + * positions seen from peers this round. + */ +inline constexpr auto parentCloseTimeRippleEpochS = makeStr("parent_close_time_ripple_epoch_s"); +inline constexpr auto closeTimeSelfRippleEpochS = makeStr("close_time_self_ripple_epoch_s"); inline constexpr auto closeTimeVoteBins = makeStr("close_time_vote_bins"); inline constexpr auto resolutionDirection = makeStr("resolution_direction"); inline constexpr auto convergePercent = makeStr("converge_percent"); diff --git a/include/xrpl/telemetry/SpanNames.h b/include/xrpl/telemetry/SpanNames.h index b848f96c03..c24f0b254b 100644 --- a/include/xrpl/telemetry/SpanNames.h +++ b/include/xrpl/telemetry/SpanNames.h @@ -133,8 +133,19 @@ inline constexpr auto ledgerSeq = makeStr("ledger_seq"); /** * Shared close-time attrs — bare names, reused by consensus and ledger. + * + * `closeTimeRippleEpochS` carries a NetClock reading: whole seconds since the + * XRP Ledger epoch (2000-01-01T00:00:00Z), never the Unix epoch. The key names + * both the unit and the epoch because neither is recoverable from the value. + * A consumer rendering it as wall-clock time must first add kEpochOffset + * (946684800 seconds, see basics/chrono.h); read as a Unix timestamp instead, + * it lands roughly 30 years early. + * + * `closeResolutionMs` is a duration, not an instant — the granularity the + * close time is rounded to. NetClock resolution is whole seconds, so this + * value is always a multiple of 1000. */ -inline constexpr auto closeTime = makeStr("close_time"); +inline constexpr auto closeTimeRippleEpochS = makeStr("close_time_ripple_epoch_s"); inline constexpr auto closeTimeCorrect = makeStr("close_time_correct"); inline constexpr auto closeResolutionMs = makeStr("close_resolution_ms"); /** diff --git a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp index 36ab40b1b6..6cec7a5c86 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp @@ -99,7 +99,7 @@ TEST(SpanGuardFactory, consensus_close_time_attributes) auto span = telemetry::SpanGuard::span( telemetry::TraceCategory::Consensus, telemetry::seg::consensus, "accept.apply"); span.setAttribute("ledger_seq", static_cast(42)); - span.setAttribute("close_time", static_cast(780000000)); + span.setAttribute("close_time_ripple_epoch_s", static_cast(780000000)); span.setAttribute("close_time_correct", true); span.setAttribute("close_resolution_ms", static_cast(30000)); span.setAttribute("consensus_state", std::string("finished")); diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index a0e2f6f6d3..460a447631 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -635,7 +635,8 @@ RCLConsensus::Adaptor::doAccept( : telemetry::SpanGuard::childSpan(cs::acceptApply, roundSpanContext_); doAcceptSpan.setAttribute(cs::attr::ledgerSeq, static_cast(prevLedger.seq()) + 1); doAcceptSpan.setAttribute( - cs::attr::closeTime, static_cast(consensusCloseTime.time_since_epoch().count())); + cs::attr::closeTimeRippleEpochS, + static_cast(consensusCloseTime.time_since_epoch().count())); doAcceptSpan.setAttribute(cs::attr::closeTimeCorrect, closeTimeCorrect); doAcceptSpan.setAttribute( cs::attr::closeResolutionMs, @@ -648,10 +649,10 @@ RCLConsensus::Adaptor::doAccept( doAcceptSpan.setAttribute( cs::attr::roundTimeMs, static_cast(result.roundTime.read().count())); doAcceptSpan.setAttribute( - cs::attr::parentCloseTime, + cs::attr::parentCloseTimeRippleEpochS, static_cast(prevLedger.closeTime().time_since_epoch().count())); doAcceptSpan.setAttribute( - cs::attr::closeTimeSelf, + cs::attr::closeTimeSelfRippleEpochS, static_cast(rawCloseTimes.self.time_since_epoch().count())); doAcceptSpan.setAttribute( cs::attr::closeTimeVoteBins, static_cast(rawCloseTimes.peers.size())); From 4f4c9e8e3e5fbe8684de3b07eab977cf731579b8 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:30:30 +0100 Subject: [PATCH 02/19] fix(telemetry): correct span scope, span-guard names and the span docs Review feedback, plus a sweep of the branch for the same defects elsewhere. Scope: the ledger.validate guard was a plain local, so it stayed alive until checkAccept returned and the flag-ledger upgrade check ran inside the measured span. That check reads every trusted validation of the parent, so one span in 256 became a duration outlier for work unrelated to promoting a ledger. The span is now scoped to the promotion. tryAdvance stays inside it because it only sets a flag and posts a job. Attributes: tx.apply now carries ledger_seq, which the runbook already documented. The parent ledger.build span has it, but a child cannot be selected by its parent's attributes, so the span could not be found by ledger. Guard names: each span guard is now named after the span it holds, so proposalReceiveSpan, validationReceiveSpan, storeSpan and validateSpan. The name "span" previously meant the trace root in one inbound-message handler and the job-queue handle in its sibling, which taught a reader the opposite of the truth in the next function. Comments: the StatsD gauge rationale now sits with the initialiser it explains rather than in the constructor. The peer span header described its trust flags as shared when they are in fact re-declared to match the consensus keys; the duplication is intentional and the wording was not. Docs: the ledger and peer span tables disagreed with the code, crediting ledger.build with attributes that are set on tx.apply and omitting several that it does set, and all five source-file line numbers in them were stale. The testing guide listed attribute keys that exist nowhere in the code, so its catalog now points at the runbook instead of keeping a second copy that drifts. --- docker/telemetry/TESTING.md | 46 ++++---- src/libxrpl/beast/insight/StatsDCollector.cpp | 6 +- src/xrpld/app/ledger/detail/BuildLedger.cpp | 6 ++ src/xrpld/app/ledger/detail/LedgerMaster.cpp | 101 ++++++++++-------- src/xrpld/overlay/detail/PeerImp.cpp | 26 +++-- src/xrpld/overlay/detail/PeerSpanNames.h | 9 +- 6 files changed, 112 insertions(+), 82 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 723371a888..098002309e 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -372,28 +372,30 @@ See the "Verification Queries" section below. ## Expected Span Catalog -All 16 production span names: +A smoke-test subset: the spans a short local run reliably produces, and how to trigger each. This is not the full catalog — the authoritative span list, with the attributes each span carries, is [docs/telemetry-runbook.md § Span Reference](../../docs/telemetry-runbook.md#span-reference). -| Span Name | Source File | Key Attributes | How to Trigger | -| --------------------------- | ----------------- | ---------------------------------------------------------------------------------------- | ------------------------- | -| `rpc.http_request` | ServerHandler.cpp | -- | Any HTTP RPC call | -| `rpc.ws_upgrade` | ServerHandler.cpp | -- | WebSocket upgrade | -| `rpc.ws_message` | ServerHandler.cpp | -- | WebSocket RPC message | -| `rpc.process` | ServerHandler.cpp | -- | RPC processing | -| `rpc.command.` | RPCHandler.cpp | `xrpl.rpc.command`, `xrpl.rpc.version`, `xrpl.rpc.role` | Any RPC command | -| `tx.process` | NetworkOPs.cpp | `xrpl.tx.hash`, `xrpl.tx.local`, `xrpl.tx.path` | Submit transaction | -| `tx.receive` | PeerImp.cpp | `xrpl.peer.id` | Peer relays transaction | -| `consensus.proposal.send` | RCLConsensus.cpp | `xrpl.consensus.round` | Consensus proposing phase | -| `consensus.ledger_close` | RCLConsensus.cpp | `xrpl.consensus.ledger.seq`, `xrpl.consensus.mode` | Ledger close event | -| `consensus.accept` | RCLConsensus.cpp | `xrpl.consensus.proposers`, `xrpl.consensus.round_time_ms` | Ledger accepted | -| `consensus.validation.send` | RCLConsensus.cpp | `xrpl.consensus.ledger.seq`, `xrpl.consensus.proposing` | Validation sent | -| `consensus.accept.apply` | RCLConsensus.cpp | `xrpl.consensus.close_time`, `close_time_correct`, `close_resolution_ms`, `state` | Ledger apply + close time | -| `tx.apply` | BuildLedger.cpp | `xrpl.ledger.tx_count`, `xrpl.ledger.tx_failed` | Ledger close (tx set) | -| `ledger.build` | BuildLedger.cpp | `xrpl.ledger.seq`, `xrpl.ledger.close_time`, `close_time_correct`, `close_resolution_ms` | Ledger build | -| `ledger.validate` | LedgerMaster.cpp | `xrpl.ledger.seq`, `xrpl.ledger.validations` | Ledger validated | -| `ledger.store` | LedgerMaster.cpp | `xrpl.ledger.seq` | Ledger stored | -| `peer.proposal.receive` | PeerImp.cpp | `xrpl.peer.id`, `xrpl.peer.proposal.trusted` | Peer sends proposal | -| `peer.validation.receive` | PeerImp.cpp | `xrpl.peer.id`, `xrpl.peer.validation.trusted` | Peer sends validation | +Attributes are deliberately not repeated here. Keeping a second copy is how this table came to list attribute keys that no longer exist anywhere in the code. + +| Span Name | Source File | How to Trigger | +| --------------------------- | ----------------- | ------------------------- | +| `rpc.http_request` | ServerHandler.cpp | Any HTTP RPC call | +| `rpc.ws_upgrade` | ServerHandler.cpp | WebSocket upgrade | +| `rpc.ws_message` | ServerHandler.cpp | WebSocket RPC message | +| `rpc.process` | ServerHandler.cpp | RPC processing | +| `rpc.command.` | RPCHandler.cpp | Any RPC command | +| `tx.process` | NetworkOPs.cpp | Submit transaction | +| `tx.receive` | PeerImp.cpp | Peer relays transaction | +| `consensus.proposal.send` | RCLConsensus.cpp | Consensus proposing phase | +| `consensus.ledger_close` | RCLConsensus.cpp | Ledger close event | +| `consensus.accept` | RCLConsensus.cpp | Ledger accepted | +| `consensus.validation.send` | RCLConsensus.cpp | Validation sent | +| `consensus.accept.apply` | RCLConsensus.cpp | Ledger apply + close time | +| `tx.apply` | BuildLedger.cpp | Ledger close (tx set) | +| `ledger.build` | BuildLedger.cpp | Ledger build | +| `ledger.validate` | LedgerMaster.cpp | Ledger validated | +| `ledger.store` | LedgerMaster.cpp | Ledger stored | +| `peer.proposal.receive` | PeerImp.cpp | Peer sends proposal | +| `peer.validation.receive` | PeerImp.cpp | Peer sends validation | --- @@ -443,7 +445,7 @@ curl -s "$PROM/api/v1/query?query=traces_span_metrics_duration_milliseconds_coun # RPC calls by command curl -s "$PROM/api/v1/query?query=traces_span_metrics_calls_total{span_name=~\"rpc.command.*\"}" | - jq '.data.result[] | {command: .metric["xrpl.rpc.command"], count: .value[1]}' + jq '.data.result[] | {command: .metric.command, count: .value[1]}' # Deployment-tier labels present on metrics (set by the collector's # resource/tier processor and promoted via resource_to_telemetry_conversion). diff --git a/src/libxrpl/beast/insight/StatsDCollector.cpp b/src/libxrpl/beast/insight/StatsDCollector.cpp index dc19aa8953..dc15e16dac 100644 --- a/src/libxrpl/beast/insight/StatsDCollector.cpp +++ b/src/libxrpl/beast/insight/StatsDCollector.cpp @@ -166,6 +166,9 @@ private: std::string name_; GaugeImpl::value_type lastValue_{0}; GaugeImpl::value_type value_{0}; + // Start dirty so the initial value (0) is emitted on the first flush. + // Without this, gauges whose value never changes from 0 would never + // appear in downstream metric stores (e.g. Prometheus via StatsD). bool dirty_{true}; }; @@ -572,9 +575,6 @@ StatsDEventImpl::doNotify(EventImpl::value_type const& value) StatsDGaugeImpl::StatsDGaugeImpl(std::string name, std::shared_ptr impl) : impl_(std::move(impl)), name_(std::move(name)) { - // Start dirty so the initial value (0) is emitted on the first flush. - // Without this, gauges whose value never changes from 0 would never - // appear in downstream metric stores (e.g. Prometheus via StatsD). impl_->add(*this); } diff --git a/src/xrpld/app/ledger/detail/BuildLedger.cpp b/src/xrpld/app/ledger/detail/BuildLedger.cpp index 482721bf6a..e2433f9c1a 100644 --- a/src/xrpld/app/ledger/detail/BuildLedger.cpp +++ b/src/xrpld/app/ledger/detail/BuildLedger.cpp @@ -188,6 +188,12 @@ applyTransactions( // If there are any transactions left, we must have // tried them in at least one final pass XRPL_ASSERT(txns.empty() || !certainRetry, "xrpl::applyTransactions : retry transactions"); + // Repeated from the parent ledger.build span on purpose: TraceQL cannot + // reach a parent's attributes from a child, so without it no query can + // select this span by ledger. `view` is the accumulator over the ledger + // being built and copies its header, so this is the same sequence number + // the parent reports. + applySpan.setAttribute(ledger_span::attr::ledgerSeq, static_cast(view.seq())); applySpan.setAttribute(ledger_span::attr::txCount, static_cast(count)); applySpan.setAttribute(ledger_span::attr::txFailed, static_cast(failed.size())); return count; diff --git a/src/xrpld/app/ledger/detail/LedgerMaster.cpp b/src/xrpld/app/ledger/detail/LedgerMaster.cpp index 5e02b4307a..0c2ffff083 100644 --- a/src/xrpld/app/ledger/detail/LedgerMaster.cpp +++ b/src/xrpld/app/ledger/detail/LedgerMaster.cpp @@ -455,8 +455,9 @@ bool LedgerMaster::storeLedger(std::shared_ptr ledger) { using namespace telemetry; - auto span = SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::store); - span.setAttribute(ledger_span::attr::ledgerSeq, static_cast(ledger->header().seq)); + auto storeSpan = SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::store); + storeSpan.setAttribute( + ledger_span::attr::ledgerSeq, static_cast(ledger->header().seq)); bool const validated = ledger->header().validated; // Returns true if we already had the ledger @@ -978,55 +979,65 @@ LedgerMaster::checkAccept(std::shared_ptr const& ledger) return; } - using namespace telemetry; - auto valSpan = SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::validate); - valSpan.setAttribute(ledger_span::attr::ledgerSeq, static_cast(ledger->header().seq)); - valSpan.setAttribute(ledger_span::attr::validations, static_cast(tvc)); - - JLOG(journal_.info()) << "Advancing accepted ledger to " << ledger->header().seq - << " with >= " << minVal << " validations"; - - ledger->setValidated(); - ledger->setFull(); - setValidLedger(ledger); - if (!pubLedger_) + // Scoped so ledger.validate measures only the promotion itself. The + // flag-ledger upgrade-warning check below runs on one ledger in 256 and + // reads every trusted validation of the parent, so leaving it inside would + // make every 256th span a duration outlier for work that is not part of + // promoting a ledger. tryAdvance() stays inside: it only sets a flag and + // posts a job, so it adds no measurable time. { - pendSaveValidated(app_, ledger, true, true); - setPubLedger(ledger); - app_.getOrderBookDB().setup(ledger); - } + using namespace telemetry; + auto validateSpan = + SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::validate); + validateSpan.setAttribute( + ledger_span::attr::ledgerSeq, static_cast(ledger->header().seq)); + validateSpan.setAttribute(ledger_span::attr::validations, static_cast(tvc)); - std::uint32_t const base = app_.getFeeTrack().getLoadBase(); - auto fees = app_.getValidations().fees(ledger->header().hash, base); - { - auto fees2 = app_.getValidations().fees(ledger->header().parentHash, base); - fees.reserve(fees.size() + fees2.size()); - std::ranges::copy(fees2, std::back_inserter(fees)); - } - std::uint32_t fee = 0; - if (!fees.empty()) - { - std::ranges::sort(fees); - if (auto stream = journal_.debug()) + JLOG(journal_.info()) << "Advancing accepted ledger to " << ledger->header().seq + << " with >= " << minVal << " validations"; + + ledger->setValidated(); + ledger->setFull(); + setValidLedger(ledger); + if (!pubLedger_) { - std::stringstream s; - s << "Received fees from validations: (" << fees.size() << ") "; - for (auto const fee1 : fees) - { - s << " " << fee1; - } - stream << s.str(); + pendSaveValidated(app_, ledger, true, true); + setPubLedger(ledger); + app_.getOrderBookDB().setup(ledger); } - fee = fees[fees.size() / 2]; // median - } - else - { - fee = base; - } - app_.getFeeTrack().setRemoteFee(fee); + std::uint32_t const base = app_.getFeeTrack().getLoadBase(); + auto fees = app_.getValidations().fees(ledger->header().hash, base); + { + auto fees2 = app_.getValidations().fees(ledger->header().parentHash, base); + fees.reserve(fees.size() + fees2.size()); + std::ranges::copy(fees2, std::back_inserter(fees)); + } + std::uint32_t fee = 0; + if (!fees.empty()) + { + std::ranges::sort(fees); + if (auto stream = journal_.debug()) + { + std::stringstream s; + s << "Received fees from validations: (" << fees.size() << ") "; + for (auto const fee1 : fees) + { + s << " " << fee1; + } + stream << s.str(); + } + fee = fees[fees.size() / 2]; // median + } + else + { + fee = base; + } - tryAdvance(); + app_.getFeeTrack().setRemoteFee(fee); + + tryAdvance(); + } if (ledger->seq() % 256 == 0) { diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 1f96a0d629..0859e251ca 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1962,10 +1962,12 @@ PeerImp::onMessage(std::shared_ptr const& m) { using namespace telemetry; // root: inbound peer message entry point (kConsumer); must not inherit - // any span left active on this peer thread. - auto span = + // any span left active on this peer thread. Named after the span it holds, + // peer.proposal.receive, to keep it distinct from `proposalSpan` below, + // which holds the consensus-level span handed to the job worker. + auto proposalReceiveSpan = ScopedSpanGuard::freshRoot(TraceCategory::Peer, seg::peer, peer_span::op::proposalReceive); - span.setAttribute(peer_span::attr::peerId, static_cast(id_)); + proposalReceiveSpan.setAttribute(peer_span::attr::peerId, static_cast(id_)); protocol::TMProposeSet const& set = *m; @@ -1993,7 +1995,7 @@ PeerImp::onMessage(std::shared_ptr const& m) // every time a spam packet is received PublicKey const publicKey{makeSlice(set.nodepubkey())}; auto const isTrusted = app_.getValidators().trusted(publicKey); - span.setAttribute(peer_span::attr::proposalTrusted, isTrusted); + proposalReceiveSpan.setAttribute(peer_span::attr::proposalTrusted, isTrusted); // If the operator has specified that untrusted proposals be dropped then // this happens here I.e. before further wasting CPU verifying the signature @@ -2567,10 +2569,12 @@ PeerImp::onMessage(std::shared_ptr const& m) { using namespace telemetry; // root: inbound peer message entry point (kConsumer); must not inherit - // any span left active on this peer thread. - auto valSpan = ScopedSpanGuard::freshRoot( + // any span left active on this peer thread. Named after the span it holds, + // peer.validation.receive, to keep it distinct from the consensus-level + // span handed to the job worker below. + auto validationReceiveSpan = ScopedSpanGuard::freshRoot( TraceCategory::Peer, seg::peer, peer_span::op::validationReceive); - valSpan.setAttribute(peer_span::attr::peerId, static_cast(id_)); + validationReceiveSpan.setAttribute(peer_span::attr::peerId, static_cast(id_)); if (m->validation().size() < 50) { @@ -2611,11 +2615,11 @@ PeerImp::onMessage(std::shared_ptr const& m) // false when telemetry is compiled out, switched off in the config, or // the Peer trace category is disabled; a span that exists but was // sampled out still pays. - if (valSpan) + if (validationReceiveSpan) { - valSpan.setAttribute( + validationReceiveSpan.setAttribute( peer_span::attr::ledgerHash, to_string(val->getLedgerHash()).c_str()); - valSpan.setAttribute(peer_span::attr::fullValidation, val->isFull()); + validationReceiveSpan.setAttribute(peer_span::attr::fullValidation, val->isFull()); } if (!isCurrent( @@ -2633,7 +2637,7 @@ PeerImp::onMessage(std::shared_ptr const& m) // suppression for 30 seconds to avoid doing a relatively expensive // lookup every time a spam packet is received auto const isTrusted = app_.getValidators().trusted(val->getSignerPublic()); - valSpan.setAttribute(peer_span::attr::validationTrusted, isTrusted); + validationReceiveSpan.setAttribute(peer_span::attr::validationTrusted, isTrusted); // If the operator has specified that untrusted validations be // dropped then this happens here I.e. before further wasting CPU diff --git a/src/xrpld/overlay/detail/PeerSpanNames.h b/src/xrpld/overlay/detail/PeerSpanNames.h index 6212d8fca2..24f80e0adf 100644 --- a/src/xrpld/overlay/detail/PeerSpanNames.h +++ b/src/xrpld/overlay/detail/PeerSpanNames.h @@ -36,7 +36,14 @@ using ::xrpl::telemetry::attr::ledgerHash; using ::xrpl::telemetry::attr::peerId; /** - * Trust flag qualified by message type, shared with consensus.*.receive. + * Trust flag qualified by message type — whether the sending key is on this + * node's UNL. + * + * The literals match consensus::span::attr::proposalTrusted and + * ::validationTrusted, so the peer and consensus receive spans report on one + * spanmetrics dimension instead of two. Unlike the constants above these are + * declared here rather than re-exported from SpanNames.h, so the two spellings + * are only kept equal by hand: change one and the dimension splits silently. */ inline constexpr auto proposalTrusted = makeStr("proposal_trusted"); inline constexpr auto validationTrusted = makeStr("validation_trusted"); 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 03/19] 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 From 6d0e42d4272acc9da8fc310155b66c5aa7438728 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:45:58 +0100 Subject: [PATCH 04/19] docs(telemetry): describe the metrics bridge in the present tense The class docs for the OTel insight bridge described how the code got to its current shape rather than what it does. Those comparisons resolve against a revision that the squash merge does not publish, so they read as confidently wrong once merged. - OTelCollector: state that it is selected by [insight] server=otel as an alternative to StatsDCollector. It replaced nothing; CollectorManager still selects StatsDCollector for server=statsd. - Unit: give the reason an Event needs an explicit unit, and the consequence of omitting one, without narrating what the two backends previously assumed. - OTelEventImpl: say that HistogramBuckets.h is the single owner of the bucket edges and why a copy goes stale, instead of quoting a superseded edge list. - HistogramBuckets: drop 'just as 5 s censors them today'. The millisecond ladder tops at 120000, so nothing is censored at 5 s. Comments only, no behaviour change. --- include/xrpl/beast/insight/OTelCollector.h | 5 +++-- include/xrpl/beast/insight/Unit.h | 18 +++++++++--------- include/xrpl/telemetry/HistogramBuckets.h | 6 +++--- src/libxrpl/beast/insight/OTelCollector.cpp | 9 ++++----- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/include/xrpl/beast/insight/OTelCollector.h b/include/xrpl/beast/insight/OTelCollector.h index b93b9bbf50..0d8d3b1f33 100644 --- a/include/xrpl/beast/insight/OTelCollector.h +++ b/include/xrpl/beast/insight/OTelCollector.h @@ -45,8 +45,9 @@ namespace beast::insight { /** * @brief A Collector that exports metrics via OpenTelemetry OTLP/HTTP. * - * Replaces StatsD-based metric collection with native OTel Metrics SDK - * instruments. Each beast::insight instrument maps to an OTel equivalent: + * Selected by `[insight] server=otel`, as an alternative to StatsDCollector: + * it exports through the native OTel Metrics SDK rather than the StatsD wire + * format. Each beast::insight instrument maps to an OTel equivalent: * * - Counter -> OTel Counter * - Gauge -> OTel ObservableGauge (async callback) diff --git a/include/xrpl/beast/insight/Unit.h b/include/xrpl/beast/insight/Unit.h index fd3516b3ab..bdcbab2562 100644 --- a/include/xrpl/beast/insight/Unit.h +++ b/include/xrpl/beast/insight/Unit.h @@ -7,19 +7,19 @@ namespace beast::insight { /** * @brief What an Event's samples measure. * - * `Event` documents itself as carrying "a millisecond time, or other integral - * value", but both backends used to assume the first case: the OTel bridge - * declared every instrument with unit `ms`, and StatsD tagged every sample - * `|ms`. A size metric therefore exported under a `_milliseconds` name and - * inherited a latency bucket ladder, which censored a quarter of its samples - * and pinned its p95 to a constant. - * - * Naming the unit at creation time is what lets the OTel bridge pick both the - * instrument unit and the matching bucket ladder: + * `Event` carries "a millisecond time, or other integral value", so the unit + * cannot be inferred from the sample. Naming it at creation time is what lets + * the OTel bridge pick both the instrument unit and the matching bucket + * ladder: * * makeEvent("time", Unit::Millis) --> OTel unit "ms" --> millisecond ladder * makeEvent("size", Unit::Bytes) --> OTel unit "By" --> byte ladder * + * Without an explicit unit every instrument declares `ms`, so a size metric + * exports under a `_milliseconds` name and inherits a latency bucket ladder. + * For RPC response sizes that ladder censors about a quarter of the samples + * and pins the p95 to a constant. + * * 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 diff --git a/include/xrpl/telemetry/HistogramBuckets.h b/include/xrpl/telemetry/HistogramBuckets.h index 4072cf64f5..f06d119756 100644 --- a/include/xrpl/telemetry/HistogramBuckets.h +++ b/include/xrpl/telemetry/HistogramBuckets.h @@ -79,9 +79,9 @@ namespace xrpl::telemetry::buckets { * * The 60 s and 120 s edges exceed the collector's 30 s top on purpose, * because jobs outlive spans: the updatepaths job type was measured - * averaging about 60 s, so a 30 s ceiling would censor its quantiles just - * 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. + * averaging about 60 s, so a 30 s ceiling would censor its quantiles. 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, which a single * four-second-wide bucket can only interpolate across. diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index cfa2c36850..700315ddbe 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -175,11 +175,10 @@ private: * The instrument's declared unit is what selects its bucket ladder: the * histogram views registered in Telemetry.cpp match on unit, so a `ms` * instrument gets the millisecond ladder and a `By` instrument the byte - * ladder. The edges themselves live in xrpl/telemetry/HistogramBuckets.h -- - * do not restate them here. An earlier version of this comment listed - * `[1, 5, ..., 1000, 5000] ms` as "matching the SpanMetrics connector"; that - * was true when written and silently became false when the connector's - * ladder was extended, which is why the edges now have one owner. + * ladder. The edges themselves live in xrpl/telemetry/HistogramBuckets.h, + * which is their single owner -- do not restate them here. An edge list copied + * into a comment reads as authoritative and goes stale the moment the + * collector's SpanMetrics ladder is extended, with nothing to flag the drift. * * Thread safety: OTel Histogram::Record() is thread-safe by specification. */ From e0924231c1443707eb1dbb49e62cfd0e4f6b536d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:17 +0100 Subject: [PATCH 05/19] docs(telemetry): state the pathfind.discover span trade-off in the present tense The comment said per-asset timing 'is no longer split into individual spans'. This change introduces the span, so per-asset spans never existed for it to be split out of, and the comparison resolves against nothing once squash-merged. Comments only, no behaviour change. --- src/xrpld/rpc/detail/PathRequest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 827393b071..1154109fc9 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -596,8 +596,8 @@ PathRequest::findPaths( // One `pathfind.discover` span wraps the entire per-source-asset loop so // 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. + // is not measured separately — span count and Tempo storage 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 From bbfde30eb1889916b44b07bcec7c9cd67f93a8a4 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:27 +0100 Subject: [PATCH 06/19] docs(consensus): drop pre-squash comparisons from the tracing comments Both comments pointed at a state the squash merge does not publish. - Consensus.h: 'yields a null guard, same as before' had no antecedent in the round or the function. Say instead that a null guard makes the setAttribute calls below no-ops, which is what SpanGuard's impl_ guard does. - ConsensusSpanLabels.h: 'Split from ConsensusSpanNames.h' describes a split performed entirely within this change; both headers first appear here. The dependency rationale and the diagram are unchanged. Comments only, no behaviour change. --- include/xrpl/consensus/Consensus.h | 5 +++-- include/xrpl/consensus/ConsensusSpanLabels.h | 9 +++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/xrpl/consensus/Consensus.h b/include/xrpl/consensus/Consensus.h index 78750656a4..aced7555ea 100644 --- a/include/xrpl/consensus/Consensus.h +++ b/include/xrpl/consensus/Consensus.h @@ -1689,8 +1689,9 @@ Consensus::updateOurPositions(std::unique_ptr const& // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above using namespace telemetry; // Child of the establish span via its captured context (establishSpan_ is - // a thread-free SpanGuard, so parent explicitly via its context). Null - // context (establish not started) yields a null guard, same as before. + // a thread-free SpanGuard, so parent explicitly via its context). A null + // context — the establish phase has not started — yields a null guard, so + // the setAttribute calls below are no-ops. auto span = SpanGuard::childSpan(consensus::span::updatePositions, establishSpanContext_); span.setAttribute( consensus::span::attr::convergePercent, static_cast(convergePercent_)); diff --git a/include/xrpl/consensus/ConsensusSpanLabels.h b/include/xrpl/consensus/ConsensusSpanLabels.h index cc66639c41..21b867d128 100644 --- a/include/xrpl/consensus/ConsensusSpanLabels.h +++ b/include/xrpl/consensus/ConsensusSpanLabels.h @@ -3,10 +3,11 @@ /** * Enum-to-label mappings for consensus span attribute values. * - * Split from ConsensusSpanNames.h so that header stays dependency-free like - * its siblings: the span-name and attribute-key constants are included by - * overlay and app translation units that have no use for the consensus - * enums, while these mappings are needed only by Consensus.h. + * These mappings live in their own header so ConsensusSpanNames.h stays + * dependency-free like its siblings: the span-name and attribute-key + * constants are included by overlay and app translation units that have no + * use for the consensus enums, while these mappings are needed only by + * Consensus.h. * * ConsensusSpanNames.h (constants only, no domain deps) * ^ From 488617cc0b540ee4a7ceb77a1d791e1a97794ec3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:51:47 +0100 Subject: [PATCH 07/19] refactor(telemetry): mark the unused setter parameter [[maybe_unused]] The default setServiceInstanceId() body ignores its argument. Use the attribute rather than a (void) cast: the codebase already uses it 104 times and the build is C++23. --- include/xrpl/telemetry/Telemetry.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 624fcf15dc..3c487638f2 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -260,10 +260,9 @@ public: * @param id The node's base58-encoded public key or custom identifier. */ virtual void - setServiceInstanceId(std::string const& id) + setServiceInstanceId([[maybe_unused]] std::string const& id) { // Default no-op for NullTelemetry implementations. - (void)id; } /** From 5f53745fbfc6016425109e1d1bc7c9306aca9727 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:52:04 +0100 Subject: [PATCH 08/19] docs(telemetry): correct the TESTING guide and the span attribute tables Review feedback on the testing guide: - rm -rf targeted data/, but this config writes under docker/telemetry/data/, so teardown did nothing and a second run reused the old NuDB and SQLite state. Corrected at both sites, including the Test 2 keygen node, which launches with the same config. - The standalone span table said consensus.* does not fire. It does: ledger_accept drives a simulated round, so consensus.round, .phase.open, .ledger_close, .accept and .accept.apply all appear. Only .establish, .update_positions, .check, .proposal.* , .validation.receive and .mode_change cannot. The test intro claimed the same thing and now agrees with the table. - Three blocks duplicated content the file already had. Test 1 now points at the shared Verification Queries section as Test 2 already did, and the Test 2 submit block checks engine_result like Test 1 does. - The numbered step list was a copy of the script's own Step N headers and had drifted by four entries, so it now points at those headers instead. Also corrects the runbook's ledger and peer span tables against the code: ledger.build was credited with tx_count and tx_failed, which tx.apply sets, and was missing its three close-time attributes; peer.validation.receive was missing ledger_hash and full_validation. The five source line numbers in those two tables were stale, so they now name the file only, as the other nineteen rows do. --- docker/telemetry/TESTING.md | 80 +++++++++++++------------------------ docs/telemetry-runbook.md | 20 +++++----- 2 files changed, 37 insertions(+), 63 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 098002309e..5c181b109a 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -34,8 +34,10 @@ The binary is at `.build/xrpld`. ## Test 1: Single-Node Standalone (Quick Verification) -This test verifies RPC and transaction spans in standalone mode. Consensus -spans will not fire because standalone mode does not run consensus. +This test verifies RPC and transaction spans in standalone mode, plus the +consensus spans that a simulated round still produces. The proposal, voting +and peer-facing consensus spans do not fire — see the expected-spans table at +the end of this test for which do and which do not. ### Step 1: Start the observability stack @@ -113,32 +115,7 @@ curl -s http://localhost:5005 -d '{"method":"ledger_accept"}' ### Step 5: Verify traces in Tempo -Wait 5 seconds for the batch export, then: - -```bash -TEMPO="http://localhost:3200" - -# Check xrpld service is registered -curl -s "$TEMPO/api/v2/search/tag/resource.service.name/values" | jq '.tagValues[].value' - -# Check RPC spans -curl -s "$TEMPO/api/search" \ - --data-urlencode 'q={resource.service.name="xrpld" && name="rpc.http_request"}' \ - --data-urlencode 'limit=5' | jq '.traces | length' - -curl -s "$TEMPO/api/search" \ - --data-urlencode 'q={resource.service.name="xrpld" && name="rpc.process"}' \ - --data-urlencode 'limit=5' | jq '.traces | length' - -curl -s "$TEMPO/api/search" \ - --data-urlencode 'q={resource.service.name="xrpld" && name="rpc.command.server_info"}' \ - --data-urlencode 'limit=5' | jq '.traces | length' - -# Check transaction spans -curl -s "$TEMPO/api/search" \ - --data-urlencode 'q={resource.service.name="xrpld" && name="tx.process"}' \ - --data-urlencode 'limit=5' | jq '.traces | length' -``` +Wait 5 seconds for the batch export, then see the "Verification Queries" section below. Its span loop is a superset of what standalone mode produces, so compare its output against the "Expected spans (standalone mode)" table above rather than running a second, narrower set of queries here. Or open Grafana Explore with Tempo datasource: http://localhost:3000 @@ -152,23 +129,24 @@ kill $(pgrep -f 'xrpld.*xrpld-telemetry') docker compose -f docker/telemetry/docker-compose.yml down # Clean xrpld data -rm -rf data/ +rm -rf docker/telemetry/data/ ``` ### Expected spans (standalone mode) -| Span Name | Expected | Notes | -| --------------------------- | -------- | ----------------------------- | -| `rpc.http_request` | Yes | Every HTTP RPC call | -| `rpc.process` | Yes | Every RPC processing | -| `rpc.command.server_info` | Yes | server_info RPC | -| `rpc.command.server_state` | Yes | server_state RPC | -| `rpc.command.ledger` | Yes | ledger RPC | -| `rpc.command.submit` | Yes | submit RPC | -| `rpc.command.ledger_accept` | Yes | ledger_accept RPC | -| `tx.process` | Yes | Transaction submission | -| `tx.receive` | No | No peers in standalone | -| `consensus.*` | No | Consensus disabled standalone | +| Span Name | Expected | Notes | +| ---------------------------------------------------------------------------------------------------------- | -------- | ------------------------------------------------- | +| `rpc.http_request` | Yes | Every HTTP RPC call | +| `rpc.process` | Yes | Every RPC processing | +| `rpc.command.server_info` | Yes | server_info RPC | +| `rpc.command.server_state` | Yes | server_state RPC | +| `rpc.command.ledger` | Yes | ledger RPC | +| `rpc.command.submit` | Yes | submit RPC | +| `rpc.command.ledger_accept` | Yes | ledger_accept RPC | +| `tx.process` | Yes | Transaction submission | +| `tx.receive` | No | No peers in standalone | +| `consensus.round`, `.phase.open`, `.ledger_close`, `.accept`, `.accept.apply` | Yes | `ledger_accept` drives a simulated round | +| `consensus.establish`, `.update_positions`, `.check`, `.proposal.*`, `.validation.receive`, `.mode_change` | No | `simulate` jumps straight to `Accepted`; no peers | --- @@ -185,17 +163,11 @@ Run the integration test script: bash docker/telemetry/integration-test.sh ``` -The script will: +It checks prerequisites, clears the previous run, brings up the observability stack, generates six validator key pairs and their node configs, starts the nodes, waits for consensus and then for a validated ledger, exercises RPC and submits a transaction, verifies traces in Tempo and both the spanmetrics and the StatsD-derived metrics in Prometheus, then prints a summary and leaves the stack running. -1. Start the observability stack -2. Generate 6 validator key pairs -3. Create config files for each node -4. Start all 6 nodes -5. Wait for consensus ("proposing" state) -6. Exercise RPC, submit transactions -7. Verify all span categories in Tempo -8. Verify spanmetrics in Prometheus -9. Print results and leave the stack running +The script announces each step as it runs, so read its `Step N:` headers for the authoritative sequence — they are not restated here, because a numbered copy of them drifts as soon as a step is added. + +Its Tempo checks cover the RPC, transaction, consensus, ledger and peer span categories from a fixed list, which is narrower than the loop in the "Verification Queries" section below. ### Manual @@ -231,7 +203,7 @@ Kill the temporary node: ```bash kill $TEMP_PID -rm -rf data/ +rm -rf docker/telemetry/data/ ``` #### Step 3: Create node configs @@ -359,9 +331,11 @@ curl -s http://localhost:5005 -d '{ "Amount": "10000000" } }] -}' +}' | jq .result.engine_result ``` +Expected result: `"tesSUCCESS"`, the same as Test 1 Step 4. + Wait 15 seconds for consensus and batch export. #### Step 8: Verify in Tempo diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 208bc700de..86f326ec3e 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -172,18 +172,18 @@ hash); `tx.preflight` is stateless and omits both. ### Ledger Spans -| Span Name | Source File | Attributes | Description | -| ----------------- | -------------------- | ------------------------------------- | ----------------------------- | -| `ledger.build` | BuildLedger.cpp:31 | `ledger_seq`, `tx_count`, `tx_failed` | Ledger build during consensus | -| `ledger.validate` | LedgerMaster.cpp:915 | `ledger_seq`, `validations` | Ledger promoted to validated | -| `ledger.store` | LedgerMaster.cpp:409 | `ledger_seq` | Ledger stored in history | +| Span Name | Source File | Attributes | Description | +| ----------------- | ---------------- | ----------------------------------------------------------------------- | ----------------------------- | +| `ledger.build` | BuildLedger.cpp | `ledger_seq`, `close_time`, `close_time_correct`, `close_resolution_ms` | Ledger build during consensus | +| `ledger.validate` | LedgerMaster.cpp | `ledger_seq`, `validations` | Ledger promoted to validated | +| `ledger.store` | LedgerMaster.cpp | `ledger_seq` | Ledger stored in history | ### Peer Spans -| Span Name | Source File | Attributes | Description | -| ------------------------- | ---------------- | ------------------------------- | ----------------------------- | -| `peer.proposal.receive` | PeerImp.cpp:1667 | `peer_id`, `proposal_trusted` | Proposal received from peer | -| `peer.validation.receive` | PeerImp.cpp:2264 | `peer_id`, `validation_trusted` | Validation received from peer | +| Span Name | Source File | Attributes | Description | +| ------------------------- | ----------- | ----------------------------------------------------------------- | ----------------------------- | +| `peer.proposal.receive` | PeerImp.cpp | `peer_id`, `proposal_trusted` | Proposal received from peer | +| `peer.validation.receive` | PeerImp.cpp | `peer_id`, `ledger_hash`, `full_validation`, `validation_trusted` | Validation received from peer | Both peer receive spans are `kConsumer` inbound entry points started as fresh trace roots. They never inherit an ambient span left active on the peer thread, @@ -459,7 +459,7 @@ all its normal attributes, it just lacks a cross-node parent link. {name=~"tx\\..*"} | tx_hash = "" # Find all spans in a cross-node consensus trace -{rootServiceName="xrpld"} | consensus_round_id = 92345679 +{rootServiceName="xrpld"} | consensus_round_id = "" # Compare latency between sender and receiver for validations {name="consensus.validation.send" || name="consensus.validation.receive"} From d687903c3f12cf708eac4f739144efa517e34185 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:52:42 +0100 Subject: [PATCH 09/19] fix(telemetry): correct the metric names, guard the arithmetic, tidy the collector Harness and docs: - integration-test.sh queried traces_span_metrics_* for spanmetrics, but this branch sets the connector namespace to "span", so those two checks matched nothing and failed. The dashboards and runbook had moved; the script had not. - The same script queried eight native metric names with a product prefix and capitals that formatName() cannot produce: it lowercases, maps '.' and ' ' to '_', and prepends nothing. Corrected against the runbook tables. - TESTING.md carried the same stale spanmetrics names and a jq example reading a Prometheus label that does not exist. - The runbook now records where each part of a derived metric name comes from, since only the namespace is ours to choose. Collector: - OTelCounterImpl::increment silently dropped a negative amount. An OTel counter takes unsigned deltas, so assert and let a release build under-count rather than wrap. - OTelGaugeImpl::increment computed current + amount in int64, which is undefined on overflow, and the clamp ran afterwards so it could not help. Check the headroom first. set() now clamps rather than casting a uint64 above INT64_MAX to a negative, which is what made underflow reachable. - The meter scope was two bare literals. They are constants now, and Telemetry.cpp static_asserts them equal to kMeterName and kMeterVersion: beast cannot include the telemetry header, so a build failure is the only way to catch the copies drifting. - formatName uses views::transform and ranges::to, as Backend.cpp already does. - Unused constructor parameters take [[maybe_unused]] instead of (void) casts. - The destructor logged "shutting down" and "stopped" with nothing between. initMetrics was 79 lines doing four jobs. The exporter and the histogram views are separate functions now, addUnitView is a member rather than a lambda capturing this, and the export interval and timeout are named. It also derived the metrics URL from the traces URL by suffix swap, which sent metrics to the traces path whenever the configured URL had any other shape; both URLs now come from one rule that handles a bare host, a trailing slash and either signal path. --- docker/telemetry/TESTING.md | 12 +- docker/telemetry/integration-test.sh | 36 +++-- docs/telemetry-runbook.md | 15 ++ include/xrpl/beast/insight/Collector.h | 5 +- include/xrpl/beast/insight/OTelCollector.h | 14 +- src/libxrpl/beast/insight/OTelCollector.cpp | 101 ++++++------ src/libxrpl/telemetry/Telemetry.cpp | 167 +++++++++++++------- 7 files changed, 214 insertions(+), 136 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 723371a888..3a0f2e1865 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -433,22 +433,24 @@ Base URL: `http://localhost:9090` ```bash PROM="http://localhost:9090" -# Span call counts (from spanmetrics connector) -curl -s "$PROM/api/v1/query?query=traces_span_metrics_calls_total" | +# Span call counts (from the spanmetrics connector). The span_ prefix is the +# connector's `namespace: "span"` in otel-collector-config.yaml; drop that +# setting and these become traces_span_metrics_*. +curl -s "$PROM/api/v1/query?query=span_calls_total" | jq '.data.result[] | {span: .metric.span_name, count: .value[1]}' # Latency histogram -curl -s "$PROM/api/v1/query?query=traces_span_metrics_duration_milliseconds_count" | +curl -s "$PROM/api/v1/query?query=span_duration_milliseconds_count" | jq '.data.result[] | {span: .metric.span_name, count: .value[1]}' # RPC calls by command -curl -s "$PROM/api/v1/query?query=traces_span_metrics_calls_total{span_name=~\"rpc.command.*\"}" | +curl -s "$PROM/api/v1/query?query=span_calls_total{span_name=~\"rpc.command.*\"}" | jq '.data.result[] | {command: .metric["xrpl.rpc.command"], count: .value[1]}' # Deployment-tier labels present on metrics (set by the collector's # resource/tier processor and promoted via resource_to_telemetry_conversion). # Expect deployment_environment and xrpl_network_type on each series. -curl -s "$PROM/api/v1/query?query=traces_span_metrics_calls_total" | +curl -s "$PROM/api/v1/query?query=span_calls_total" | jq '.data.result[0].metric | {deployment_environment, xrpl_network_type, service_name}' ``` diff --git a/docker/telemetry/integration-test.sh b/docker/telemetry/integration-test.sh index b4b07a0f37..c0386cbcf4 100755 --- a/docker/telemetry/integration-test.sh +++ b/docker/telemetry/integration-test.sh @@ -547,15 +547,18 @@ log "--- Spanmetrics ---" log "Waiting 20s for Prometheus scrape cycle..." sleep 20 -calls_count=$(curl -sf "$PROM/api/v1/query?query=traces_span_metrics_calls_total" | +# Names come from the spanmetrics connector's `namespace: "span"` in +# otel-collector-config.yaml. Without that namespace the connector emits +# traces_span_metrics_*, so these queries must move whenever it changes. +calls_count=$(curl -sf "$PROM/api/v1/query?query=span_calls_total" | jq '.data.result | length' 2>/dev/null || echo 0) if [ "$calls_count" -gt 0 ]; then - ok "Prometheus: traces_span_metrics_calls_total ($calls_count series)" + ok "Prometheus: span_calls_total ($calls_count series)" else - fail "Prometheus: traces_span_metrics_calls_total (0 series)" + fail "Prometheus: span_calls_total (0 series)" fi -duration_count=$(curl -sf "$PROM/api/v1/query?query=traces_span_metrics_duration_milliseconds_count" | +duration_count=$(curl -sf "$PROM/api/v1/query?query=span_duration_milliseconds_count" | jq '.data.result | length' 2>/dev/null || echo 0) if [ "$duration_count" -gt 0 ]; then ok "Prometheus: duration histogram ($duration_count series)" @@ -590,23 +593,30 @@ check_otel_metric() { fi } +# Names are what OTelCollector::formatName() produces: the beast::insight +# name lowercased with '.' and ' ' mapped to '_', any group() segment kept, and +# no prefix. The [insight] prefix knob is logged at startup and never applied on +# this path, and the collector's prometheus exporter sets no namespace, so a +# name carrying a product prefix or capitals cannot match any exported series. + # Node health gauges (ObservableGauge — no _total suffix) -check_otel_metric "rippled_LedgerMaster_Validated_Ledger_Age" -check_otel_metric "rippled_LedgerMaster_Published_Ledger_Age" -check_otel_metric "rippled_jobq_job_count" +check_otel_metric "ledgermaster_validated_ledger_age" +check_otel_metric "ledgermaster_published_ledger_age" +check_otel_metric "jobq_job_count" # State accounting -check_otel_metric "rippled_State_Accounting_Full_duration" +check_otel_metric "state_accounting_full_duration" # Peer finder -check_otel_metric "rippled_Peer_Finder_Active_Inbound_Peers" -check_otel_metric "rippled_Peer_Finder_Active_Outbound_Peers" +check_otel_metric "peer_finder_active_inbound_peers" +check_otel_metric "peer_finder_active_outbound_peers" # RPC counters (Counter — Prometheus adds _total suffix automatically) -check_otel_metric "rippled_rpc_requests_total" +check_otel_metric "rpc_requests_total" -# Overlay traffic -check_otel_metric "rippled_total_Bytes_In" +# Overlay traffic — one series per TrafficCount category; "total" is the +# aggregate category. +check_otel_metric "total_bytes_in" # Verify StatsD receiver is NOT required (no statsd receiver in pipeline) log "" diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 406110ec2b..af44d5d63b 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -471,6 +471,8 @@ The OTel Collector's spanmetrics connector automatically derives RED (Rate, Erro ### Generated Metric Names +These names are deliberately generic: the connector emits **one** metric family covering every span, not a metric per span. Which span a series belongs to comes from the `span_name` label, and the rest of the breakdown from the `dimensions` list in `otel-collector-config.yaml`. So a query always names the span in a label selector rather than in the metric name — `span_calls_total{span_name="ledger.build"}`, never a `ledger_build_calls_total`. + | Prometheus Metric | Type | Description | | ----------------------------------- | --------- | ---------------------------- | | `span_calls_total` | Counter | Total span invocations | @@ -478,6 +480,19 @@ The OTel Collector's spanmetrics connector automatically derives RED (Rate, Erro | `span_duration_milliseconds_count` | Histogram | Latency observation count | | `span_duration_milliseconds_sum` | Histogram | Cumulative latency | +Only one part of those names is ours to choose. Reading a name left to right: + +| Part | Set by | +| ------------------------------------- | ------------------------------------------------------------------------------------ | +| `span_` | the connector's `namespace: "span"` in `otel-collector-config.yaml` — **our choice** | +| `calls`, `duration` | the spanmetrics connector's own metric names | +| `_milliseconds` | the Prometheus exporter, expanding the metric's declared unit | +| `_total`, `_bucket`, `_count`, `_sum` | Prometheus conventions for counters and histograms | + +`_milliseconds` rather than `_ms` is therefore not a style decision taken here. The config declares the histogram in milliseconds (`buckets: [1ms, 5ms, ...]`) and never contains the string `milliseconds`; the exporter writes the unit out in full when it translates OTLP to Prometheus. Shortening it would mean renaming the series after export, which would break every dashboard and leave the exported name and the queried name disagreeing. + +Drop the `namespace` setting and these become `traces_span_metrics_*` instead — the connector's default. Any query, dashboard panel or test that names one of these metrics has to move with that setting. + ### Metric Labels Every metric carries these standard labels: diff --git a/include/xrpl/beast/insight/Collector.h b/include/xrpl/beast/insight/Collector.h index c4ba20e3f4..e95d805cc2 100644 --- a/include/xrpl/beast/insight/Collector.h +++ b/include/xrpl/beast/insight/Collector.h @@ -116,9 +116,8 @@ public: * @param unit What the samples measure. */ virtual Event - makeEvent(std::string const& name, Unit unit) + makeEvent(std::string const& name, [[maybe_unused]] Unit unit) { - (void)unit; return makeEvent(name); } @@ -130,7 +129,7 @@ public: return makeEvent(prefix + "." + name); } - Event + [[nodiscard]] Event makeEvent(std::string const& prefix, std::string const& name, Unit unit) { if (prefix.empty()) diff --git a/include/xrpl/beast/insight/OTelCollector.h b/include/xrpl/beast/insight/OTelCollector.h index 0d8d3b1f33..a84e55cf60 100644 --- a/include/xrpl/beast/insight/OTelCollector.h +++ b/include/xrpl/beast/insight/OTelCollector.h @@ -39,9 +39,21 @@ #include #include +#include namespace beast::insight { +/** + * Instrumentation scope this collector fetches its Meter under. + * + * Must equal xrpl::telemetry::kMeterName and kMeterVersion, or instruments land + * on a different scope than the views. Duplicated because beast sits below the + * telemetry module and cannot include its header; Telemetry.cpp static_asserts + * the two agree. + */ +inline constexpr std::string_view kOTelMeterName{"xrpld"}; +inline constexpr std::string_view kOTelMeterVersion{"1.0.0"}; + /** * @brief A Collector that exports metrics via OpenTelemetry OTLP/HTTP. * @@ -125,7 +137,7 @@ public: * @param journal Journal for logging. * @return Shared pointer to the created Collector. */ - static std::shared_ptr + [[nodiscard]] static std::shared_ptr // NOLINTNEXTLINE(readability-identifier-naming) New(std::string const& endpoint, std::string const& prefix, diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index 700315ddbe..b81c1824bf 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -61,9 +62,12 @@ #include #include #include +#include #include #include +#include #include +#include #include #include @@ -270,7 +274,7 @@ public: * @brief Return the current gauge value for the OTel callback. * @return The most recently set/incremented value. */ - int64_t + [[nodiscard]] int64_t currentValue() const; OTelGaugeImpl& @@ -283,10 +287,13 @@ public: gaugeCallback(opentelemetry::metrics::ObserverResult result, void* state); /** - * Create the observable instrument and register the callback, once. + * Create the observable instrument and register the callback. * * Called when the collector is told collection is ready, because the * callback reads live application state. + * + * Idempotent. Arming twice would register the callback twice, so callers + * need not check; onCollectionReady() iterates a snapshot and may re-arm. */ void arm(); @@ -550,7 +557,7 @@ public: * @brief Get the OTel Meter instance for creating instruments. * @return Shared pointer to the OTel Meter. */ - opentelemetry::nostd::shared_ptr const& + [[nodiscard]] opentelemetry::nostd::shared_ptr const& otelMeter() const; /** @@ -563,8 +570,8 @@ public: * @param name Raw metric name from beast::insight callers. * @return Export-ready metric name. */ - static std::string - formatName(std::string const& name); + [[nodiscard]] static std::string + formatName(std::string_view name); private: /** @@ -648,8 +655,10 @@ OTelCounterImpl::OTelCounterImpl( void OTelCounterImpl::increment(value_type amount) { - // OTel counters require non-negative values. beast::insight CounterImpl - // uses int64_t, so clamp negative values to 0 and cast to uint64_t. + // OTel counters take unsigned deltas only. Assert to catch a decrementing + // caller; skip the Add so a release build under-counts instead of wrapping. + XRPL_ASSERT( + amount >= 0, "beast::insight::detail::OTelCounterImpl::increment : non-negative amount"); if (amount > 0) counter_->Add(static_cast(amount)); } @@ -736,20 +745,25 @@ OTelGaugeImpl::~OTelGaugeImpl() void OTelGaugeImpl::set(value_type value) { - value_.store(static_cast(value), std::memory_order_relaxed); + // value_type is uint64_t, the gauge reports int64_t. Clamp instead of + // wrapping to a negative, which increment() would then floor to 0. + constexpr auto kMax = static_cast(std::numeric_limits::max()); + value_.store(static_cast(std::min(value, kMax)), std::memory_order_relaxed); } void OTelGaugeImpl::increment(difference_type amount) { - // Use compare-exchange loop to safely clamp to [0, MAX]. + // Saturate in [0, INT64_MAX]. Signed overflow is UB, so check the headroom + // before adding. A negative amount cannot underflow: current is never + // negative, so the lowest sum is 0 + INT64_MIN. + constexpr auto kMax = std::numeric_limits::max(); int64_t current = value_.load(std::memory_order_relaxed); int64_t desired = 0; do { - desired = current + amount; - // Clamp to 0 on underflow. - desired = std::max(desired, int64_t{0}); + desired = + (amount > 0 && current > kMax - amount) ? kMax : std::max(current + amount, int64_t{0}); } while (!value_.compare_exchange_weak(current, desired, std::memory_order_relaxed)); } @@ -783,23 +797,19 @@ OTelMeterImpl::increment(value_type amount) OTelCollectorImp::OTelCollectorImp( std::string const& endpoint, std::string prefix, - std::string const& instanceId, - std::string const& serviceName, - std::string const& networkType, + // instanceId/serviceName/networkType are accepted so the New() signature + // stays uniform for callers, but they are not read here: the telemetry + // module owns the resource attributes for the shared metrics pipeline. + [[maybe_unused]] std::string const& instanceId, + [[maybe_unused]] std::string const& serviceName, + [[maybe_unused]] std::string const& networkType, Journal journal) : journal_(journal), prefix_(std::move(prefix)) { - // instanceId/serviceName/networkType are retained on the New() signature - // for back-compat but no longer used here: the telemetry module owns the - // resource attributes for the shared metrics pipeline. - (void)instanceId; - (void)serviceName; - (void)networkType; - if (journal_.info()) { // endpoint is informational: the global telemetry pipeline owns the - // real exporter. It is logged here for back-compat and diagnostics. + // real exporter. It is logged here purely as a startup diagnostic. journal_.info() << "OTelCollector starting: endpoint=" << endpoint << " prefix=" << prefix_; } @@ -808,14 +818,10 @@ OTelCollectorImp::OTelCollectorImp( // periodic reader, histogram view, resource attributes) and registers it // via metrics::Provider::SetMeterProvider() during start(). beast metrics // ride that shared pipeline, so both direct-API and beast-sourced metrics - // export under one resource identity. - // - // The name/version literals MUST match the telemetry module's kMeterName - // ("xrpld") and kMeterVersion ("1.0.0"). They are written as literals (not - // referenced from Telemetry.h) because beast/insight sits below the - // telemetry module in the layering and cannot include its header. + // export under one resource identity. The scope must match the telemetry + // module's; see kOTelMeterName in the header. otelMeter_ = metrics_api::Provider::GetMeterProvider()->GetMeter( - std::string{"xrpld"}, std::string{"1.0.0"}); + std::string{kOTelMeterName}, std::string{kOTelMeterVersion}); if (journal_.info()) { @@ -825,12 +831,8 @@ OTelCollectorImp::OTelCollectorImp( OTelCollectorImp::~OTelCollectorImp() { - if (journal_.info()) - { - journal_.info() << "OTelCollector shutting down"; - } - // No pipeline teardown here: the telemetry module owns the global - // MeterProvider lifecycle (ForceFlush/Shutdown happen in Telemetry::stop()). + // Nothing to tear down: the telemetry module owns the global MeterProvider, + // so ForceFlush and Shutdown happen in Telemetry::stop(). if (journal_.info()) { journal_.info() << "OTelCollector stopped"; @@ -996,25 +998,16 @@ OTelCollectorImp::otelMeter() const } std::string -OTelCollectorImp::formatName(std::string const& name) +OTelCollectorImp::formatName(std::string_view name) { - // Produce a clean, lowercase, Prometheus-compatible metric name. - // No prefix — the OTel resource (service.name) identifies the service. - // Dots and spaces become underscores; everything lowercased. - std::string result; - result.reserve(name.size()); - for (char const c : name) - { - if (c == '.' || c == ' ') - { - result += '_'; - } - else - { - result += static_cast(std::tolower(static_cast(c))); - } - } - return result; + // Lowercase, with '.' and ' ' mapped to '_'. No prefix: the service.name + // resource attribute identifies the service. + return name | std::views::transform([](char c) { + return (c == '.' || c == ' ') + ? '_' + : static_cast(std::tolower(static_cast(c))); + }) | + std::ranges::to(); } } // namespace detail diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index aada42756c..54377858bc 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -77,6 +78,24 @@ namespace xrpl::telemetry { +// beast cannot include this header, so it duplicates the meter scope. Fail the +// build if the copies drift: instruments would land off the views' scope. +static_assert(kMeterName == beast::insight::kOTelMeterName); +static_assert(kMeterVersion == beast::insight::kOTelMeterVersion); + +/** + * OTLP/HTTP path per signal, appended by signalEndpoint(). + */ +constexpr std::string_view kTracesPath{"/v1/traces"}; +constexpr std::string_view kMetricsPath{"/v1/metrics"}; + +/** + * Metric export cadence. The interval matches the 1 s scrape the dashboards + * assume; the timeout bounds a stalled collector. + */ +constexpr auto kMetricExportInterval = std::chrono::milliseconds{1000}; +constexpr auto kMetricExportTimeout = std::chrono::milliseconds{500}; + namespace { namespace trace_api = opentelemetry::trace; @@ -366,74 +385,102 @@ class TelemetryImpl : public Telemetry * * @note Throws whatever the SDK factories throw; the constructor catches. */ + /** + * @brief Full OTLP/HTTP URL for one signal. + * + * `[telemetry] endpoint` is one setting but OTLP/HTTP has a path per + * signal, so both are derived from it by the same rule: drop a trailing + * slash, drop a signal path if one is already there, then append the path + * asked for. A bare host, a traces URL and a metrics URL therefore all + * yield the right endpoint for either signal. + * + * @param configured The `[telemetry] endpoint` value. + * @param signalPath Path to append, e.g. kTracesPath. + * @return Endpoint URL for that signal. + */ + [[nodiscard]] static std::string + signalEndpoint(std::string_view configured, std::string_view signalPath) + { + while (configured.ends_with('/')) + configured.remove_suffix(1); + + for (auto const known : {kTracesPath, kMetricsPath}) + { + if (configured.ends_with(known)) + { + configured.remove_suffix(known.size()); + break; + } + } + return std::string{configured} + std::string{signalPath}; + } + + /** + * @brief Build the OTLP/HTTP metric exporter. + * + * @return Exporter pointed at the metrics endpoint, with the same TLS + * options the trace exporter uses. + */ + [[nodiscard]] auto + makeMetricExporter() const + { + otlp_http::OtlpHttpMetricExporterOptions opts; + opts.url = signalEndpoint(setup_.exporterEndpoint, kMetricsPath); + if (setup_.useTls) + { + opts.ssl_ca_cert_path = setup_.tlsCertPath; + opts.ssl_client_cert_path = setup_.tlsClientCertPath; + opts.ssl_client_key_path = setup_.tlsClientKeyPath; + } + return otlp_http::OtlpHttpMetricExporterFactory::Create(opts); + } + + /** + * @brief Register one histogram view, selected by instrument unit. + * + * The unit is the selector, so an instrument gets the ladder matching what + * it measures and a byte count is never bucketed on a latency ladder. + * + * The view name stays EMPTY: a non-empty one renames every matching + * histogram to it and collapses them into a single series. The meter + * selector must match kMeterName, or the view never applies and + * instruments fall back to the SDK default ladder (ceiling 10,000). + * + * @param unitCode OTel unit code to select on, e.g. "ms". + * @param boundaries Bucket upper bounds, from HistogramBuckets.h. + * @param description Description recorded on the view. + */ + void + addUnitView( + std::string const& unitCode, + std::vector boundaries, + std::string const& description) + { + auto selector = metrics_sdk::InstrumentSelectorFactory::Create( + metrics_sdk::InstrumentType::kHistogram, "*", unitCode); + auto meterSelector = + metrics_sdk::MeterSelectorFactory::Create(std::string(kMeterName), "", ""); + auto config = std::make_shared(); + config->boundaries_ = std::move(boundaries); + auto view = metrics_sdk::ViewFactory::Create( + "", description, metrics_sdk::AggregationType::kHistogram, std::move(config)); + meterProvider_->AddView(std::move(selector), std::move(meterSelector), std::move(view)); + } + void initMetrics() { - // Derive the metrics endpoint from the trace endpoint by swapping - // the trailing "/v1/traces" path for "/v1/metrics". Any other URL - // shape is used as-is. - std::string metricsEndpoint = setup_.exporterEndpoint; - constexpr std::string_view tracesPath{"/v1/traces"}; - if (metricsEndpoint.ends_with(tracesPath)) - { - metricsEndpoint.replace( - metricsEndpoint.size() - tracesPath.size(), tracesPath.size(), "/v1/metrics"); - } - - // Configure OTLP HTTP metric exporter, honoring the same TLS - // options as the trace exporter. - otlp_http::OtlpHttpMetricExporterOptions metricExporterOpts; - metricExporterOpts.url = metricsEndpoint; - if (setup_.useTls) - { - metricExporterOpts.ssl_ca_cert_path = setup_.tlsCertPath; - metricExporterOpts.ssl_client_cert_path = setup_.tlsClientCertPath; - metricExporterOpts.ssl_client_key_path = setup_.tlsClientKeyPath; - } - - auto metricExporter = otlp_http::OtlpHttpMetricExporterFactory::Create(metricExporterOpts); - - // Configure periodic metric reader (1-second export interval, - // matching the beast OTelCollector path). metrics_sdk::PeriodicExportingMetricReaderOptions readerOpts; - readerOpts.export_interval_millis = std::chrono::milliseconds(1000); - readerOpts.export_timeout_millis = std::chrono::milliseconds(500); + readerOpts.export_interval_millis = kMetricExportInterval; + readerOpts.export_timeout_millis = kMetricExportTimeout; auto reader = metrics_sdk::PeriodicExportingMetricReaderFactory::Create( - std::move(metricExporter), readerOpts); + makeMetricExporter(), readerOpts); - // Create MeterProvider with the shared resource, then attach reader. meterProvider_ = metrics_sdk::MeterProviderFactory::Create( std::make_unique(), makeResource()); 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 - // 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 - // collapse them into a single series. - // - // The meter selector MUST match the meter name used by getMeter() and - // the beast OTelCollector, or a view never applies and instruments - // fall back to the SDK default ladder (ceiling 10,000). - auto const addUnitView = [this]( - std::string const& unitCode, - std::vector boundaries, - std::string const& description) { - auto selector = metrics_sdk::InstrumentSelectorFactory::Create( - metrics_sdk::InstrumentType::kHistogram, "*", unitCode); - auto meterSelector = - metrics_sdk::MeterSelectorFactory::Create(std::string(kMeterName), "", ""); - auto config = std::make_shared(); - config->boundaries_ = std::move(boundaries); - auto view = metrics_sdk::ViewFactory::Create( - "", description, metrics_sdk::AggregationType::kHistogram, std::move(config)); - meterProvider_->AddView(std::move(selector), std::move(meterSelector), std::move(view)); - }; - addUnitView( beast::insight::otelUnitCode(beast::insight::Unit::Millis), buckets::toVector(buckets::kMillisecondBuckets), @@ -443,8 +490,8 @@ class TelemetryImpl : public Telemetry buckets::toVector(buckets::kByteBuckets), "Size buckets, 512 B to 1 MiB"); - // Publish as the global meter provider so developers (and the beast - // OTelCollector shim) reach the same pipeline. + // Publish globally so both direct-API and beast-sourced metrics ride + // one pipeline. metrics_api::Provider::SetMeterProvider( opentelemetry::nostd::shared_ptr(meterProvider_)); } @@ -493,7 +540,7 @@ public: // Configure OTLP HTTP exporter otlp_http::OtlpHttpExporterOptions exporterOpts; - exporterOpts.url = setup_.exporterEndpoint; + exporterOpts.url = signalEndpoint(setup_.exporterEndpoint, kTracesPath); if (setup_.useTls) { exporterOpts.ssl_ca_cert_path = setup_.tlsCertPath; From 255a4134e73a629f4a4e81998bab93dc5c8c5977 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:51:20 +0100 Subject: [PATCH 10/19] fix(telemetry): record preclaim errors and apply-stage exceptions The three apply-pipeline stages disagreed on span status. preflight set Error on any non-success TER, preclaim never set it at all, and the apply stage recorded nothing when a transaction threw. - preclaim now sets Error for any non-success result, matching preflight. Routine retryable results (terPRE_SEQ, telINSUF_FEE_P) are included, so stage=preclaim error rates will rise and track normal queueing. - The apply span no longer keys Error on canApply. A dry run reports tesSUCCESS with canApply false, which was recorded as an error. Every other path with canApply false already has a non-success result, so !isTesSuccess covers them. - Exceptions escaping the apply stage set ter_result=tefEXCEPTION and record an exception event, then rethrow unchanged, mirroring what invokePreflight and invokePreclaim already do. The caller still maps the exception to tefEXCEPTION, so behaviour is unchanged. - Two comments claimed every exit funnels through the logger lambda. A throw does not, so they now say each return path. --- src/libxrpl/tx/Transactor.cpp | 247 ++++++++++++++++++---------------- src/libxrpl/tx/applySteps.cpp | 4 + 2 files changed, 135 insertions(+), 116 deletions(-) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 4eb43d1596..4e6dbf33b2 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -49,6 +49,7 @@ #include #include #include +#include #include #include #include @@ -1624,126 +1625,140 @@ Transactor::operator()() trapTransaction(*trap); } - auto result = ctx_.preclaimResult; - if (isTesSuccess(result)) - result = apply(); - - // No transaction can return temUNKNOWN from apply, - // and it can't be passed in from a preclaim. - XRPL_ASSERT(result != temUNKNOWN, "xrpl::Transactor::operator() : result is not temUNKNOWN"); - - if (auto stream = j_.trace()) - stream << "preclaim result: " << transToken(result); - - auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); - bool const canApply = std::invoke([&result, &fee, this] { - bool canApplyTmp = isTesSuccess(result); - - if (ctx_.size() > kOversizeMetaDataCap) - result = tecOVERSIZE; - - if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) - { - // If the TapFailHard flag is set, a tec result - // must not do anything - ctx_.discard(); - canApplyTmp = false; - } - else if ( - (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || - (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) - { - // This is and must remain the only place where `canApplyTmp` can change from false to - // true. Changing from true to false is no problem. - std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); - } - return canApplyTmp; - }); - - // Every exit from this function funnels through here, so this is also where - // the apply span records its outcome: each return path reports the engine - // result and whether the transaction was applied. - auto const logger = [this, &span]( - TER result, - bool canApply, - std::optional&& metadata = std::nullopt) -> ApplyResult { - JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); - - // Also guarded: transToken() is a lookup returning a string, and this - // funnel runs on every exit path. - if (span) - { - span.setAttribute( - telemetry::tx_apply_span::attr::terResult, transToken(result).c_str()); - span.setAttribute(telemetry::tx_apply_span::attr::applied, canApply); - // Mark the span as errored when the transaction was not applied or - // the engine result is not a success, so failed applies surface in - // span-status error counts alongside preflight and preclaim. - if (!canApply || !isTesSuccess(result)) - span.setError(transToken(result)); - } - - return {result, canApply, std::move(metadata)}; - }; - - if (!canApply) - return logger(result, canApply); - - // First invariant pass: both protocol and transaction-specific - // checks run against the transaction's tentative outcome. If it - // does not return tecINVARIANT_FAILED, we can proceed to apply the - // tx. - result = checkInvariants(result, fee, InvariantScope::Full); - if (result == tecINVARIANT_FAILED) + try { - // Fee-claim reset: roll the transaction's effects back so that - // only the fee deduction remains. This is the reset referenced - // by InvariantScope::ProtocolOnly. - auto const resetResult = reset(fee); - if (!isTesSuccess(resetResult.first)) - result = resetResult.first; + auto result = ctx_.preclaimResult; + if (isTesSuccess(result)) + result = apply(); - fee = resetResult.second; + // No transaction can return temUNKNOWN from apply, + // and it can't be passed in from a preclaim. + XRPL_ASSERT( + result != temUNKNOWN, "xrpl::Transactor::operator() : result is not temUNKNOWN"); - // Re-check invariants against the post-reset (fee-claim only) - // state. The transaction's effects are gone, so the - // transaction-specific invariants no longer apply and only the - // protocol invariants are re-run. A failure here escalates to - // tefINVARIANT_FAILED and excludes the tx from the ledger. - if (isTesSuccess(result) || isTecClaim(result)) - result = checkInvariants(result, fee, InvariantScope::ProtocolOnly); + if (auto stream = j_.trace()) + stream << "preclaim result: " << transToken(result); + + auto fee = ctx_.tx.getFieldAmount(sfFee).xrp(); + bool const canApply = std::invoke([&result, &fee, this] { + bool canApplyTmp = isTesSuccess(result); + + if (ctx_.size() > kOversizeMetaDataCap) + result = tecOVERSIZE; + + if (isTecClaim(result) && ((view().flags() & TapFailHard) != 0u)) + { + // If the TapFailHard flag is set, a tec result + // must not do anything + ctx_.discard(); + canApplyTmp = false; + } + else if ( + (result == tecOVERSIZE) || (result == tecKILLED) || (result == tecINCOMPLETE) || + (result == tecEXPIRED) || (isTecClaimHardFail(result, view().flags()))) + { + // This is and must remain the only place where `canApplyTmp` can change from false + // to true. Changing from true to false is no problem. + std::tie(result, fee, canApplyTmp) = processPersistentChanges(result, fee); + } + return canApplyTmp; + }); + + // Each return path funnels through here, so this is also where the apply + // span records its outcome: the engine result and whether the transaction + // was applied. A throw bypasses it and ends the span with no outcome. + auto const logger = [this, &span]( + TER result, + bool canApply, + std::optional&& metadata = std::nullopt) -> ApplyResult { + JLOG(j_.trace()) << (canApply ? "applied " : "not applied ") << transToken(result); + + // Also guarded: transToken() is a lookup returning a string, and this + // funnel runs on every return path. + if (span) + { + span.setAttribute( + telemetry::tx_apply_span::attr::terResult, transToken(result).c_str()); + span.setAttribute(telemetry::tx_apply_span::attr::applied, canApply); + // Mark the span as errored when the engine result is not a success, + // so failed applies surface alongside preflight and preclaim. Not + // keyed on `canApply`: a dry run reports tesSUCCESS with canApply + // false, and that is not a failure. + if (!isTesSuccess(result)) + span.setError(transToken(result)); + } + + return {result, canApply, std::move(metadata)}; + }; + + if (!canApply) + return logger(result, canApply); + + // First invariant pass: both protocol and transaction-specific + // checks run against the transaction's tentative outcome. If it + // does not return tecINVARIANT_FAILED, we can proceed to apply the + // tx. + result = checkInvariants(result, fee, InvariantScope::Full); + if (result == tecINVARIANT_FAILED) + { + // Fee-claim reset: roll the transaction's effects back so that + // only the fee deduction remains. This is the reset referenced + // by InvariantScope::ProtocolOnly. + auto const resetResult = reset(fee); + if (!isTesSuccess(resetResult.first)) + result = resetResult.first; + + fee = resetResult.second; + + // Re-check invariants against the post-reset (fee-claim only) + // state. The transaction's effects are gone, so the + // transaction-specific invariants no longer apply and only the + // protocol invariants are re-run. A failure here escalates to + // tefINVARIANT_FAILED and excludes the tx from the ledger. + if (isTesSuccess(result) || isTecClaim(result)) + result = checkInvariants(result, fee, InvariantScope::ProtocolOnly); + } + + // We ran through the invariant checker, which can, in some cases, + // return a tef error code. Don't apply the transaction in that case. + if (!isTecClaim(result) && !isTesSuccess(result)) + return logger(result, false); + + std::optional metadata; + + // Transaction succeeded fully or (retries are not allowed and the + // transaction could claim a fee) + + // The transactor and invariant checkers guarantee that this will + // *never* trigger but if it, somehow, happens, don't allow a tx + // that charges a negative fee. + if (fee < beast::kZero) + Throw("fee charged is negative!"); + + // Charge whatever fee they specified. The fee has already been + // deducted from the balance of the account that issued the + // transaction. We just need to account for it in the ledger + // header. + if (!view().open() && fee != beast::kZero) + ctx_.destroyXRP(fee); + + // Once we call apply, we will no longer be able to look at view() + metadata = ctx_.apply(result); + + if ((ctx_.flags() & TapDryRun) != 0u) + return logger(result, false, std::move(metadata)); + + return logger(result, canApply, std::move(metadata)); + } + catch (std::exception const& e) + { + // The caller's doApply() maps this to tefEXCEPTION. Record it on the + // span before unwinding so per-stage error counts include exceptions. + span.setAttribute( + telemetry::tx_apply_span::attr::terResult, transToken(tefEXCEPTION).c_str()); + span.recordException(e); + throw; } - - // We ran through the invariant checker, which can, in some cases, - // return a tef error code. Don't apply the transaction in that case. - if (!isTecClaim(result) && !isTesSuccess(result)) - return logger(result, false); - - std::optional metadata; - - // Transaction succeeded fully or (retries are not allowed and the - // transaction could claim a fee) - - // The transactor and invariant checkers guarantee that this will - // *never* trigger but if it, somehow, happens, don't allow a tx - // that charges a negative fee. - if (fee < beast::kZero) - Throw("fee charged is negative!"); - - // Charge whatever fee they specified. The fee has already been - // deducted from the balance of the account that issued the - // transaction. We just need to account for it in the ledger - // header. - if (!view().open() && fee != beast::kZero) - ctx_.destroyXRP(fee); - - // Once we call apply, we will no longer be able to look at view() - metadata = ctx_.apply(result); - - if ((ctx_.flags() & TapDryRun) != 0u) - return logger(result, false, std::move(metadata)); - - return logger(result, canApply, std::move(metadata)); } } // namespace xrpl diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 45758bf616..3ec6c25aa8 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -314,6 +314,10 @@ invokePreclaim(PreclaimContext const& ctx) { span.setAttribute( telemetry::tx_apply_span::attr::terResult, transToken(preclaimTer).c_str()); + // Mark the span as errored when preclaim rejects the transaction so + // failed stages surface in span-status error counts. + if (!isTesSuccess(preclaimTer)) + span.setError(transToken(preclaimTer)); } return preclaimTer; } From ef10ef98139d8e3e6716cee9363707434d877d9f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:51:56 +0100 Subject: [PATCH 11/19] fix(telemetry): separate the validation.receive latency populations consensus.validation.receive is created before the drop decision, so one span name covered three exits with unrelated cost profiles: two drop paths that end in microseconds, and a queued path whose handle is moved into the job and so covers job wait plus checkValidation. No existing attribute separated them. validation_trusted=true implies the queued path, but validation_trusted=false spans both drop paths and the queued path, so any quantile over the span mixed the populations. Both drop paths are live in the default config, which sets relayUntrustedValidations. Adds validation_status, set once on each exit rather than as a default, following the tx_status precedent in the same file: dropped_diverged, queued, dropped_load. On the queued path it is set before the handle is moved into the job. --- include/xrpl/consensus/ConsensusSpanNames.h | 12 +++++++++++ src/xrpld/overlay/detail/PeerImp.cpp | 23 +++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index 3db180c463..f71b332457 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -287,6 +287,14 @@ inline constexpr auto disputesCount = makeStr("disputes_count"); */ inline constexpr auto proposalTrusted = makeStr("proposal_trusted"); inline constexpr auto validationTrusted = makeStr("validation_trusted"); + +/** + * "validation_status" — which exit the inbound validation took. Set once per + * exit, so a dropped validation (microseconds) is separable from a queued one + * (job wait plus checkValidation). Without it the span name reports two + * unrelated latency distributions and every quantile over it is meaningless. + */ +inline constexpr auto validationStatus = makeStr("validation_status"); } // namespace attr // ===== Event names =========================================================== @@ -352,6 +360,10 @@ inline constexpr auto closeAnomaly = makeStr("anomaly"); inline constexpr auto closeOthersClosed = makeStr("others_closed"); inline constexpr auto closeIdle = makeStr("idle"); inline constexpr auto closeNormal = makeStr("normal"); +// validation_status values, one per exit of the inbound validation path. +inline constexpr auto validationQueued = makeStr("queued"); +inline constexpr auto validationDroppedDiverged = makeStr("dropped_diverged"); +inline constexpr auto validationDroppedLoad = makeStr("dropped_load"); } // namespace val } // namespace xrpl::telemetry::consensus::span diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 9505a954be..cfc921d63e 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -2671,12 +2671,29 @@ PeerImp::onMessage(std::shared_ptr const& m) static_cast(val->getSignTime().time_since_epoch().count())); } + // validation_status is set once on each exit below, not as a default + // here, to avoid OTel SDK attribute duplication. It is what separates + // the microsecond drop paths from the queued path, which also covers + // job wait and checkValidation. if (!isTrusted && (tracking_.load() == Tracking::Diverged)) { + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationDroppedDiverged); + } JLOG(pJournal_.debug()) << "Dropping untrusted validation from diverged peer"; } else if (isTrusted || !app_.getFeeTrack().isLoadedLocal()) { + // Set before the handle is moved into the job below. + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationQueued); + } std::string const name = isTrusted ? "ChkTrust" : "ChkUntrust"; std::weak_ptr const weak = shared_from_this(); @@ -2690,6 +2707,12 @@ PeerImp::onMessage(std::shared_ptr const& m) } else { + if (span && *span) + { + span->setAttribute( + telemetry::consensus::span::attr::validationStatus, + telemetry::consensus::span::val::validationDroppedLoad); + } JLOG(pJournal_.debug()) << "Dropping untrusted validation for load"; } } From 9d71dea9723c37da1c354cdaa11f4577e05e3b29 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:00:28 +0100 Subject: [PATCH 12/19] refactor(telemetry): mark the telemetry accessors nodiscard getInstance(), getTracer(), both startSpan() overloads and networkTypeFromId() return values that a caller must use. A discarded startSpan() result destroys the returned span immediately, so the span opens and closes with no content. Six methods in Telemetry.h already carried the attribute, on the base and on every override. The new attributes follow that: the overrides in Telemetry.cpp and NullTelemetry.cpp get it too, because [[nodiscard]] is not inherited and a call bound to the derived type would otherwise be unchecked. No caller anywhere in the chain discards any of these results. --- include/xrpl/telemetry/Telemetry.h | 8 ++++---- src/libxrpl/telemetry/NullTelemetry.cpp | 6 +++--- src/libxrpl/telemetry/Telemetry.cpp | 12 ++++++------ src/libxrpl/telemetry/TelemetryConfig.cpp | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 3c487638f2..a7156c0bfa 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -120,7 +120,7 @@ public: * Get the global Telemetry instance. * @return Pointer to the active instance, or nullptr if not started. */ - static Telemetry* + [[nodiscard]] static Telemetry* getInstance() { return instance.load(std::memory_order_acquire); @@ -322,7 +322,7 @@ public: * @param name Tracer name used to identify the instrumentation library. * @return A shared pointer to the Tracer. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr getTracer(std::string_view name = kTracerName) = 0; /** @@ -340,7 +340,7 @@ public: * - kConsumer: async message receive * @return A shared pointer to the new Span. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::trace::SpanKind kind = opentelemetry::trace::SpanKind::kInternal) = 0; @@ -356,7 +356,7 @@ public: * @param kind The span kind (defaults to kInternal). * @return A shared pointer to the new Span. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::context::Context const& parentContext, diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index 32244af0b5..81ad9e108a 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -104,7 +104,7 @@ public: } #ifdef XRPL_ENABLE_TELEMETRY - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view) override { static auto noopTracer = opentelemetry::nostd::shared_ptr( @@ -112,14 +112,14 @@ public: return noopTracer; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, opentelemetry::trace::SpanKind) override { return opentelemetry::nostd::shared_ptr( new opentelemetry::trace::NoopSpan(nullptr)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan( std::string_view, opentelemetry::context::Context const&, diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 8076146a9c..d07f480a58 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -215,7 +215,7 @@ public: return false; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view) override { static auto noopTracer = @@ -223,13 +223,13 @@ public: return noopTracer; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, trace_api::SpanKind) override { return opentelemetry::nostd::shared_ptr(new trace_api::NoopSpan(nullptr)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view, opentelemetry::context::Context const&, trace_api::SpanKind) override { @@ -428,7 +428,7 @@ public: return setup_.traceLedger; } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr getTracer(std::string_view name = kTracerName) override { if (!sdkProvider_) @@ -438,7 +438,7 @@ public: return sdkProvider_->GetTracer(std::string(name)); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan(std::string_view name, trace_api::SpanKind kind) override { auto tracer = getTracer(); @@ -447,7 +447,7 @@ public: return tracer->StartSpan(std::string(name), opts); } - opentelemetry::nostd::shared_ptr + [[nodiscard]] opentelemetry::nostd::shared_ptr startSpan( std::string_view name, opentelemetry::context::Context const& parentContext, diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index e18a8d4a97..828b303c58 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -65,7 +65,7 @@ constexpr std::uint32_t maxQueueSize = 2048u; * @param networkId The network identifier from [network_id] config. * @return "mainnet", "testnet", "devnet", or "unknown" for other values. */ -std::string +[[nodiscard]] std::string networkTypeFromId(std::uint32_t networkId) { switch (networkId) From 75ccf9a0970ed7019c710de3e23c61fcecdcaa57 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:01:08 +0100 Subject: [PATCH 13/19] refactor(telemetry): mark the span factories and trace-context checks nodiscard The trace-context checks validate bytes received from a peer, so a discarded result means untrusted input was accepted unchecked. hashSpan(), txReceiveSpan() and txProcessSpan() return an RAII guard; discarding one ends the span on the same line it began. The telemetry-disabled twins of both hashSpan overloads already carried the attribute, so the two #ifdef arms now agree. Both hashSpan overloads also gain the @return line their siblings already had, now that the result cannot be dropped. No caller in the chain discards any of these results. --- include/xrpl/telemetry/SpanGuard.h | 8 ++++++-- include/xrpl/telemetry/TraceContextPropagator.h | 2 +- include/xrpl/telemetry/TraceContextValidation.h | 6 +++--- src/xrpld/telemetry/TxTracing.h | 11 +++++++++-- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 6fb60bbac2..6a31ecd83a 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -394,8 +394,10 @@ public: * @param name Full span name (e.g. "tx.receive"). * @param hashData Pointer to at least 16 bytes of hash data. * @param hashSize Size of the hash buffer (must be >= 16). + * @return An active guard, or a null guard when the category is + * disabled or hashSize is under 16. */ - static SpanGuard + [[nodiscard]] static SpanGuard hashSpan( TraceCategory const cat, std::string_view const name, @@ -414,8 +416,10 @@ public: * @param parentSpanId Pointer to 8 bytes of parent span ID. * @param parentSpanSize Size of parent span ID buffer (must be 8). * @param traceFlags Trace flags from remote context. + * @return An active guard, or a null guard when the category is + * disabled, hashSize is under 16, or parentSpanSize is not 8. */ - static SpanGuard + [[nodiscard]] static SpanGuard hashSpan( TraceCategory const cat, std::string_view const name, diff --git a/include/xrpl/telemetry/TraceContextPropagator.h b/include/xrpl/telemetry/TraceContextPropagator.h index 6a698c1933..ca1e0676f1 100644 --- a/include/xrpl/telemetry/TraceContextPropagator.h +++ b/include/xrpl/telemetry/TraceContextPropagator.h @@ -42,7 +42,7 @@ namespace xrpl::telemetry { * @return An OTel Context with the extracted parent span, or an empty * context if the protobuf fields are missing or invalid. */ -inline opentelemetry::context::Context +[[nodiscard]] inline opentelemetry::context::Context extractFromProtobuf(protocol::TraceContext const& proto) { namespace trace = opentelemetry::trace; diff --git a/include/xrpl/telemetry/TraceContextValidation.h b/include/xrpl/telemetry/TraceContextValidation.h index e69b2d17ca..c299ba6cf9 100644 --- a/include/xrpl/telemetry/TraceContextValidation.h +++ b/include/xrpl/telemetry/TraceContextValidation.h @@ -57,7 +57,7 @@ namespace xrpl::telemetry { * @param traceId The raw trace_id bytes from a protobuf TraceContext. * @return true if usable as a trace identifier, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidTraceId(std::string const& traceId) { return traceId.size() == 16 && std::ranges::any_of(traceId, [](char c) { return c != 0; }); @@ -69,7 +69,7 @@ isValidTraceId(std::string const& traceId) * @param spanId The raw span_id bytes from a protobuf TraceContext. * @return true if usable as a span identifier, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidSpanId(std::string const& spanId) { return spanId.size() == 8 && std::ranges::any_of(spanId, [](char c) { return c != 0; }); @@ -86,7 +86,7 @@ isValidSpanId(std::string const& spanId) * @param tc The protobuf TraceContext received from a peer. * @return true if both ids are present and valid, false otherwise. */ -inline bool +[[nodiscard]] inline bool isValidTraceContext(protocol::TraceContext const& tc) { return tc.has_trace_id() && isValidTraceId(tc.trace_id()) && tc.has_span_id() && diff --git a/src/xrpld/telemetry/TxTracing.h b/src/xrpld/telemetry/TxTracing.h index 5baf01df2d..682f482f1b 100644 --- a/src/xrpld/telemetry/TxTracing.h +++ b/src/xrpld/telemetry/TxTracing.h @@ -32,8 +32,12 @@ namespace xrpl::telemetry { * trace_id is derived from txID[0:16]. If the incoming message carries * a protobuf TraceContext with a valid span_id, it is used as the * parent to preserve relay ordering. + * @param txID Transaction id; its first 16 bytes become the trace_id. + * @param msg The received message, read only for its trace context. + * @return An active guard, or a null guard when the Transactions category + * is disabled. Bind it: a discarded guard ends the span immediately. */ -inline SpanGuard +[[nodiscard]] inline SpanGuard txReceiveSpan(uint256 const& txID, [[maybe_unused]] protocol::TMTransaction const& msg) { #ifdef XRPL_ENABLE_TELEMETRY @@ -63,8 +67,11 @@ txReceiveSpan(uint256 const& txID, [[maybe_unused]] protocol::TMTransaction cons /** * Create a "tx.process" span for transaction processing in NetworkOPs. * trace_id is derived from txID[0:16]. + * @param txID Transaction id; its first 16 bytes become the trace_id. + * @return An active guard, or a null guard when the Transactions category + * is disabled. Bind it: a discarded guard ends the span immediately. */ -inline SpanGuard +[[nodiscard]] inline SpanGuard txProcessSpan(uint256 const& txID) { return SpanGuard::hashSpan( From 90bc58a658cffe8a28f56754fb83246807ca5fe1 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:01:21 +0100 Subject: [PATCH 14/19] refactor(telemetry): use ranges algorithms in ValidationTracker, mark accessors nodiscard Nine std::count_if calls over window1h_ and window7d_ took an iterator pair; std::ranges::count_if takes the container. One hand-rolled erase-while-iterating loop becomes a single std::erase_if: pending_ is a hash_map, which is a std::unordered_map alias, so the C++20 overload applies. The hard-trim loop below it is left as a loop on purpose. It re-tests pending_.size() every step to stop as soon as the cap is met, which erase_if cannot express. The accessors return values a caller must use: 13 in ValidationTracker.h, the two Unit.h mappers, the two HistogramBuckets.h helpers, getMeter() and networkTypeFromId(). No caller in the chain discards any of them. --- include/xrpl/beast/insight/Unit.h | 4 +- include/xrpl/telemetry/HistogramBuckets.h | 4 +- include/xrpl/telemetry/Telemetry.h | 4 +- src/xrpld/telemetry/ValidationTracker.h | 26 ++++++------ .../telemetry/detail/ValidationTracker.cpp | 40 ++++++++----------- 5 files changed, 35 insertions(+), 43 deletions(-) diff --git a/include/xrpl/beast/insight/Unit.h b/include/xrpl/beast/insight/Unit.h index bdcbab2562..5697155ec5 100644 --- a/include/xrpl/beast/insight/Unit.h +++ b/include/xrpl/beast/insight/Unit.h @@ -53,7 +53,7 @@ enum class Unit : std::uint8_t { * @param unit The unit to translate. * @return A static, null-terminated UCUM code. */ -constexpr char const* +[[nodiscard]] constexpr char const* otelUnitCode(Unit unit) noexcept { switch (unit) @@ -78,7 +78,7 @@ otelUnitCode(Unit unit) noexcept * @param unit The unit to describe. * @return A static, null-terminated description. */ -constexpr char const* +[[nodiscard]] constexpr char const* otelUnitDescription(Unit unit) noexcept { switch (unit) diff --git a/include/xrpl/telemetry/HistogramBuckets.h b/include/xrpl/telemetry/HistogramBuckets.h index f06d119756..9d67052205 100644 --- a/include/xrpl/telemetry/HistogramBuckets.h +++ b/include/xrpl/telemetry/HistogramBuckets.h @@ -144,7 +144,7 @@ inline constexpr std::array kByteBuckets{ * @return true when the ladder is non-empty, starts at or above zero, and * every later edge is strictly greater than its predecessor. */ -constexpr bool +[[nodiscard]] constexpr bool isAscendingNonNegative(std::span ladder) noexcept { if (ladder.empty() || ladder.front() < 0.0) @@ -167,7 +167,7 @@ static_assert(isAscendingNonNegative(kByteBuckets)); * @param ladder Bucket upper bounds. * @return A vector holding the same edges in the same order. */ -inline std::vector +[[nodiscard]] inline std::vector toVector(std::span ladder) { return std::vector(ladder.begin(), ladder.end()); diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index e5e0d897ad..ebb021f888 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -393,7 +393,7 @@ public: * @param name Meter name used to identify the instrumentation scope. * @return A shared pointer to the Meter. */ - virtual opentelemetry::nostd::shared_ptr + [[nodiscard]] virtual opentelemetry::nostd::shared_ptr getMeter(std::string_view name = kMeterName) = 0; /** @@ -486,7 +486,7 @@ makeTelemetrySetup( * @param networkId The network identifier from [network_id] config. * @return "mainnet" (0), "testnet" (1), "devnet" (2), or "unknown". */ -std::string +[[nodiscard]] std::string networkTypeFromId(std::uint32_t networkId); } // namespace xrpl::telemetry diff --git a/src/xrpld/telemetry/ValidationTracker.h b/src/xrpld/telemetry/ValidationTracker.h index 346e324ae2..278332ad8e 100644 --- a/src/xrpld/telemetry/ValidationTracker.h +++ b/src/xrpld/telemetry/ValidationTracker.h @@ -138,21 +138,21 @@ public: * Agreement percentage over the last 1 hour. * @return Percentage [0.0, 100.0], or 0.0 if no data. */ - double + [[nodiscard]] double agreementPct1h() const; /** * Agreement percentage over the last 24 hours. * @return Percentage [0.0, 100.0], or 0.0 if no data. */ - double + [[nodiscard]] double agreementPct24h() const; /** * Agreement percentage over the last 7 days. * @return Percentage [0.0, 100.0], or 0.0 if no data. */ - double + [[nodiscard]] double agreementPct7d() const; /** @} */ @@ -165,37 +165,37 @@ public: /** * Number of agreements in the 1-hour window. */ - uint64_t + [[nodiscard]] uint64_t agreements1h() const; /** * Number of misses in the 1-hour window. */ - uint64_t + [[nodiscard]] uint64_t missed1h() const; /** * Number of agreements in the 24-hour window. */ - uint64_t + [[nodiscard]] uint64_t agreements24h() const; /** * Number of misses in the 24-hour window. */ - uint64_t + [[nodiscard]] uint64_t missed24h() const; /** * Number of agreements in the 7-day window. */ - uint64_t + [[nodiscard]] uint64_t agreements7d() const; /** * Number of misses in the 7-day window. */ - uint64_t + [[nodiscard]] uint64_t missed7d() const; /** @} */ @@ -208,25 +208,25 @@ public: /** * Total agreements since process start. */ - uint64_t + [[nodiscard]] uint64_t totalAgreements() const; /** * Total misses since process start. */ - uint64_t + [[nodiscard]] uint64_t totalMissed() const; /** * Total validations this node sent. */ - uint64_t + [[nodiscard]] uint64_t totalValidationsSent() const; /** * Total network validations observed for comparison. */ - uint64_t + [[nodiscard]] uint64_t totalValidationsChecked() const; /** @} */ diff --git a/src/xrpld/telemetry/detail/ValidationTracker.cpp b/src/xrpld/telemetry/detail/ValidationTracker.cpp index 94cd517d89..a29fde9c7f 100644 --- a/src/xrpld/telemetry/detail/ValidationTracker.cpp +++ b/src/xrpld/telemetry/detail/ValidationTracker.cpp @@ -116,19 +116,11 @@ void ValidationTracker::evictOldPending(TimePoint now) { auto const cutoff = now - kLateRepairWindow; - for (auto it = pending_.begin(); it != pending_.end();) - { - if (it->second.reconciled && it->second.recordTime < cutoff) - { - it = pending_.erase(it); - } - else - { - ++it; - } - } + std::erase_if(pending_, [cutoff](auto const& entry) { + return entry.second.reconciled && entry.second.recordTime < cutoff; + }); - // Hard trim if still over limit. The loop above already removed every + // Hard trim if still over limit. The pass above already removed every // reconciled entry older than the late-repair window, so here we drop // any remaining reconciled entry as a last resort. if (pending_.size() > kMaxPendingEvents) @@ -155,7 +147,7 @@ ValidationTracker::agreementPct1h() const if (window1h_.empty()) return 0.0; auto const agreed = static_cast( - std::count_if(window1h_.begin(), window1h_.end(), [](auto const& e) { return e.agreed; })); + std::ranges::count_if(window1h_, [](auto const& e) { return e.agreed; })); return (agreed / static_cast(window1h_.size())) * 100.0; } @@ -165,8 +157,8 @@ ValidationTracker::agreementPct24h() const std::scoped_lock const lock(mutex_); if (window24h_.empty()) return 0.0; - auto const agreed = static_cast(std::count_if( - window24h_.begin(), window24h_.end(), [](auto const& e) { return e.agreed; })); + auto const agreed = static_cast( + std::ranges::count_if(window24h_, [](auto const& e) { return e.agreed; })); return (agreed / static_cast(window24h_.size())) * 100.0; } @@ -175,7 +167,7 @@ ValidationTracker::agreements1h() const { std::scoped_lock const lock(mutex_); return static_cast( - std::count_if(window1h_.begin(), window1h_.end(), [](auto const& e) { return e.agreed; })); + std::ranges::count_if(window1h_, [](auto const& e) { return e.agreed; })); } uint64_t @@ -183,23 +175,23 @@ ValidationTracker::missed1h() const { std::scoped_lock const lock(mutex_); return static_cast( - std::count_if(window1h_.begin(), window1h_.end(), [](auto const& e) { return !e.agreed; })); + std::ranges::count_if(window1h_, [](auto const& e) { return !e.agreed; })); } uint64_t ValidationTracker::agreements24h() const { std::scoped_lock const lock(mutex_); - return static_cast(std::count_if( - window24h_.begin(), window24h_.end(), [](auto const& e) { return e.agreed; })); + return static_cast( + std::ranges::count_if(window24h_, [](auto const& e) { return e.agreed; })); } uint64_t ValidationTracker::missed24h() const { std::scoped_lock const lock(mutex_); - return static_cast(std::count_if( - window24h_.begin(), window24h_.end(), [](auto const& e) { return !e.agreed; })); + return static_cast( + std::ranges::count_if(window24h_, [](auto const& e) { return !e.agreed; })); } double @@ -209,7 +201,7 @@ ValidationTracker::agreementPct7d() const if (window7d_.empty()) return 0.0; auto const agreed = static_cast( - std::count_if(window7d_.begin(), window7d_.end(), [](auto const& e) { return e.agreed; })); + std::ranges::count_if(window7d_, [](auto const& e) { return e.agreed; })); return (agreed / static_cast(window7d_.size())) * 100.0; } @@ -218,7 +210,7 @@ ValidationTracker::agreements7d() const { std::scoped_lock const lock(mutex_); return static_cast( - std::count_if(window7d_.begin(), window7d_.end(), [](auto const& e) { return e.agreed; })); + std::ranges::count_if(window7d_, [](auto const& e) { return e.agreed; })); } uint64_t @@ -226,7 +218,7 @@ ValidationTracker::missed7d() const { std::scoped_lock const lock(mutex_); return static_cast( - std::count_if(window7d_.begin(), window7d_.end(), [](auto const& e) { return !e.agreed; })); + std::ranges::count_if(window7d_, [](auto const& e) { return !e.agreed; })); } uint64_t From f68cf0d00989d553cc0b8da07ee30ddc52a72173 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:09:19 +0100 Subject: [PATCH 15/19] refactor(telemetry): name the traces endpoint for its signal [telemetry] endpoint carried one OTLP signal while its name implied it covered every signal. That asymmetry is what let the metrics URL be guessed later by rewriting this one's path suffix, so anything not ending /v1/traces silently posted metrics to the traces path. Renames the key to traces_endpoint and Setup::exporterEndpoint to tracesEndpoint. The default value is unchanged and the URL is still used verbatim, with no path derived from it. The startup log line and the compose-file example name the new key, the latter being where an operator copies it from. No metrics_endpoint is added here: this branch has no metrics pipeline, so the key would parse into a member nothing reads. --- docker/telemetry/docker-compose.yml | 2 +- include/xrpl/telemetry/Telemetry.h | 5 +++-- src/libxrpl/telemetry/Telemetry.cpp | 4 ++-- src/libxrpl/telemetry/TelemetryConfig.cpp | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/docker/telemetry/docker-compose.yml b/docker/telemetry/docker-compose.yml index a910e1a940..0b3f268aaf 100644 --- a/docker/telemetry/docker-compose.yml +++ b/docker/telemetry/docker-compose.yml @@ -15,7 +15,7 @@ # Configure xrpld to export traces by adding to xrpld.cfg: # [telemetry] # enabled=1 -# endpoint=http://localhost:4318/v1/traces +# traces_endpoint=http://localhost:4318/v1/traces services: # OpenTelemetry Collector: receives spans from xrpld via OTLP protocol, diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index a7156c0bfa..678a41c50b 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -168,9 +168,10 @@ public: std::string serviceInstanceId; /** - * OTLP/HTTP endpoint URL where spans are sent. + * Full OTLP/HTTP URL where spans are sent, including the signal path. + * Used verbatim: no other endpoint is derived from it. */ - std::string exporterEndpoint = "http://localhost:4318/v1/traces"; + std::string tracesEndpoint = "http://localhost:4318/v1/traces"; /** * Whether to use TLS for the exporter connection. diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index d07f480a58..6c72b2ed20 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -286,12 +286,12 @@ public: void start() override { - JLOG(journal_.info()) << "Telemetry starting: endpoint=" << setup_.exporterEndpoint + JLOG(journal_.info()) << "Telemetry starting: traces_endpoint=" << setup_.tracesEndpoint << " sampling=" << setup_.samplingRatio; // Configure OTLP HTTP exporter otlp_http::OtlpHttpExporterOptions exporterOpts; - exporterOpts.url = setup_.exporterEndpoint; + exporterOpts.url = setup_.tracesEndpoint; if (setup_.useTls) { exporterOpts.ssl_ca_cert_path = setup_.tlsCertPath; diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 828b303c58..4cbbbf2a98 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -31,7 +31,7 @@ namespace key { constexpr char const* enabled = "enabled"; constexpr char const* serviceName = "service_name"; constexpr char const* serviceInstanceId = "service_instance_id"; -constexpr char const* endpoint = "endpoint"; +constexpr char const* tracesEndpoint = "traces_endpoint"; constexpr char const* useTls = "use_tls"; constexpr char const* tlsCaCert = "tls_ca_cert"; constexpr char const* batchSize = "batch_size"; @@ -54,7 +54,7 @@ constexpr char const* traceLedger = "trace_ledger"; */ namespace dflt { constexpr char const* serviceName = "xrpld"; -constexpr char const* endpoint = "http://localhost:4318/v1/traces"; +constexpr char const* tracesEndpoint = "http://localhost:4318/v1/traces"; constexpr std::uint32_t batchSize = 512u; constexpr std::uint32_t batchDelayMs = 5000u; constexpr std::uint32_t maxQueueSize = 2048u; @@ -97,7 +97,7 @@ makeTelemetrySetup( setup.serviceVersion = version; setup.serviceInstanceId = section.valueOr(key::serviceInstanceId, nodePublicKey); - setup.exporterEndpoint = section.valueOr(key::endpoint, dflt::endpoint); + setup.tracesEndpoint = section.valueOr(key::tracesEndpoint, dflt::tracesEndpoint); setup.useTls = section.valueOr(key::useTls, 0) != 0; setup.tlsCertPath = section.valueOr(key::tlsCaCert, ""); From d00a550895e7866172421356ef00f3e24ba0d2e7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:15:09 +0100 Subject: [PATCH 16/19] test(telemetry): follow the traces_endpoint rename in the config test The rename arrived from phase-1b by merge, which left this test naming a member that no longer exists. Updates both assertions to tracesEndpoint and the section key to traces_endpoint. The key matters as much as the member: had only the member been renamed, the parse would have fallen back to the default and the test would have compared the collector URL against localhost. --- src/tests/libxrpl/telemetry/TelemetryConfig.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 82b0fe70c6..152511e0a5 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -13,7 +13,7 @@ TEST(TelemetryConfig, setup_defaults) EXPECT_EQ(s.serviceName, "xrpld"); EXPECT_TRUE(s.serviceVersion.empty()); EXPECT_TRUE(s.serviceInstanceId.empty()); - EXPECT_EQ(s.exporterEndpoint, "http://localhost:4318/v1/traces"); + EXPECT_EQ(s.tracesEndpoint, "http://localhost:4318/v1/traces"); EXPECT_FALSE(s.useTls); EXPECT_TRUE(s.tlsCertPath.empty()); EXPECT_DOUBLE_EQ(s.samplingRatio, 1.0); @@ -53,7 +53,7 @@ TEST(TelemetryConfig, parse_full_section) section.set("service_name", "my-rippled"); section.set("service_instance_id", "custom-id"); section.set("exporter", "otlp_http"); - section.set("endpoint", "http://collector:4318/v1/traces"); + section.set("traces_endpoint", "http://collector:4318/v1/traces"); section.set("use_tls", "1"); section.set("tls_ca_cert", "/etc/ssl/ca.pem"); section.set("batch_size", "256"); @@ -70,7 +70,7 @@ TEST(TelemetryConfig, parse_full_section) EXPECT_TRUE(setup.enabled); EXPECT_EQ(setup.serviceName, "my-rippled"); EXPECT_EQ(setup.serviceInstanceId, "custom-id"); - EXPECT_EQ(setup.exporterEndpoint, "http://collector:4318/v1/traces"); + EXPECT_EQ(setup.tracesEndpoint, "http://collector:4318/v1/traces"); EXPECT_TRUE(setup.useTls); EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); EXPECT_EQ(setup.batchSize, 256u); From f87dff3eb67e641b44843f0abcf679cff4f7ace6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:16:39 +0100 Subject: [PATCH 17/19] docs(telemetry): follow the traces_endpoint rename in the runbook The rename arrived from phase-1b by merge; the runbook still told operators to set `endpoint`, which the parser no longer reads. Updates the Quick Start ini block and the Configuration Reference row. No metrics_endpoint row is added: this branch exports no metrics, so documenting the key here would describe something the code ignores. --- docs/telemetry-runbook.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 0a42eb3b96..6e6bff0757 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -29,7 +29,7 @@ Add to your `xrpld.cfg`: ```ini [telemetry] enabled=1 -endpoint=http://localhost:4318/v1/traces +traces_endpoint=http://localhost:4318/v1/traces ``` ### 3. Build with telemetry support @@ -45,7 +45,7 @@ cmake --build --preset default | Option | Default | Description | | -------------------------- | --------------------------------- | --------------------------------------------------------- | | `enabled` | `0` | Master switch for telemetry | -| `endpoint` | `http://localhost:4318/v1/traces` | OTLP/HTTP endpoint | +| `traces_endpoint` | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | | `service_name` | `xrpld` | OpenTelemetry service name resource attribute | | `service_instance_id` | node public key | OpenTelemetry service instance ID resource attribute | | `trace_rpc` | `1` | Enable RPC request tracing | From 40877ab6fb22e2eb70ffb409826e7b84b2058ace Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:18:17 +0100 Subject: [PATCH 18/19] docs(telemetry): rename the endpoint option to traces_endpoint The [telemetry] option table documents the config key operators copy. The key is now traces_endpoint, named for the one OTLP signal it carries, so the old row pointed at a key the parser no longer reads. Two prose mentions of "endpoint" further down describe the concept rather than naming the key, and are left alone. --- OpenTelemetryPlan/05-configuration-reference.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 579ee10a66..cf0f993dda 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -18,7 +18,7 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme | Option | Type | Default | Description | | --------------------- | ------ | --------------------------------- | ---------------------------------------------------- | | `enabled` | bool | `false` | Enable/disable telemetry | -| `endpoint` | string | `http://localhost:4318/v1/traces` | OTLP/HTTP collector endpoint | +| `traces_endpoint` | string | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | | `use_tls` | bool | `false` | Enable TLS for exporter connection | | `tls_ca_cert` | string | `""` | Path to CA certificate file | | `batch_size` | uint | `512` | Spans per export batch | From 8c695b69a57f1126c4b50520c98987ef11d525c3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:21:01 +0100 Subject: [PATCH 19/19] fix(telemetry): follow the traces_endpoint rename in the node configs The rename arrived from phase-1b by merge. Four files still wrote the old key, which the parser no longer reads, so each would have silently fallen back to the default collector URL. integration-test.sh is the load-bearing one: it generates the node config the test harness starts, so the stale key would have pointed the node at localhost regardless of the compose network. xrpld-telemetry.cfg is the standalone node config; the other two document the key. Note this cfg has a second, divergent variant on the devnet branches that needs the same fix there. --- OpenTelemetryPlan/09-data-collection-reference.md | 2 +- docker/telemetry/TESTING.md | 2 +- docker/telemetry/integration-test.sh | 2 +- docker/telemetry/xrpld-telemetry.cfg | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index eb907904d5..ee97467ba2 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -963,7 +963,7 @@ prefix=xrpld ```ini [telemetry] enabled=1 -endpoint=http://otel-collector:4318/v1/traces +traces_endpoint=http://otel-collector:4318/v1/traces trace_peer=0 batch_size=1024 max_queue_size=4096 diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 5c181b109a..01843a00d7 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -256,7 +256,7 @@ online_delete=256 [telemetry] enabled=1 -endpoint=http://localhost:4318/v1/traces +traces_endpoint=http://localhost:4318/v1/traces batch_size=512 batch_delay_ms=2000 max_queue_size=2048 diff --git a/docker/telemetry/integration-test.sh b/docker/telemetry/integration-test.sh index fb9f4eda06..c0327a7508 100755 --- a/docker/telemetry/integration-test.sh +++ b/docker/telemetry/integration-test.sh @@ -330,7 +330,7 @@ ${IPS_FIXED} [telemetry] enabled=1 service_instance_id=Node-${i} -endpoint=http://localhost:4318/v1/traces +traces_endpoint=http://localhost:4318/v1/traces exporter=otlp_http batch_size=512 batch_delay_ms=2000 diff --git a/docker/telemetry/xrpld-telemetry.cfg b/docker/telemetry/xrpld-telemetry.cfg index 0a50c3bbaa..3458efeebd 100644 --- a/docker/telemetry/xrpld-telemetry.cfg +++ b/docker/telemetry/xrpld-telemetry.cfg @@ -47,7 +47,7 @@ docker/telemetry/data/debug.log [telemetry] enabled=1 service_instance_id=xrpld-standalone -endpoint=http://localhost:4318/v1/traces +traces_endpoint=http://localhost:4318/v1/traces exporter=otlp_http batch_size=512 batch_delay_ms=5000