From be1e4e3c677ebbad1bee63d4b4cb573f5df6771d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:04:01 +0100 Subject: [PATCH 01/16] docs(telemetry): remove an internal plan doc from the tree The document is internal and is kept outside the repo instead. This also removes every link to it: the document-index rows in OpenTelemetryPlan.md and 08-appendix.md, its node, edge and style lines in the plan's Mermaid map, its own section in OpenTelemetryPlan.md, and the two cross-reference notes in 02-design-decisions.md and 05-configuration-reference.md. --- OpenTelemetryPlan/02-design-decisions.md | 2 - .../05-configuration-reference.md | 2 - OpenTelemetryPlan/08-appendix.md | 23 +- OpenTelemetryPlan/OpenTelemetryPlan.md | 12 - OpenTelemetryPlan/secure-OTel.md | 240 ------------------ 5 files changed, 11 insertions(+), 268 deletions(-) delete mode 100644 OpenTelemetryPlan/secure-OTel.md diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index bd8b831c09..449c4cdb93 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -428,8 +428,6 @@ layer. > **Key Principle**: Telemetry collects **operational metadata** (timing, counts, hashes) — never **sensitive content** (keys, balances, amounts, raw payloads). -> **See also**: [Securing the OTel Pipeline](./secure-OTel.md) covers transport-level protection for telemetry leaving the node — mTLS to the collector and validation of incoming peer trace context. Privacy controls in this section keep sensitive data out of spans; the security doc keeps the spans themselves out of untrusted hands. - --- ## 2.5 Context Propagation Design diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index e83e940cde..a7d73700be 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -99,8 +99,6 @@ The top-level `CMakeLists.txt` adds an `XRPL_ENABLE_TELEMETRY` option (default ` > **OTLP** = OpenTelemetry Protocol | **APM** = Application Performance Monitoring -> **Production hardening**: The configurations in this section are starting points. For production deployments where xrpld ships telemetry across a network to a centrally-hosted collector, see [Securing the OTel Pipeline](./secure-OTel.md) for the required mTLS receiver config, NetworkPolicy, and peer trace-context validation. - The authoritative collector config lives in the repo at `docker/telemetry/otel-collector-config.yaml` (with Tempo backend config in `docker/telemetry/tempo.yaml`). The sections below summarize the development and production shapes of that pipeline. ### 5.5.1 Development Configuration diff --git a/OpenTelemetryPlan/08-appendix.md b/OpenTelemetryPlan/08-appendix.md index 37483a321d..7125809857 100644 --- a/OpenTelemetryPlan/08-appendix.md +++ b/OpenTelemetryPlan/08-appendix.md @@ -170,18 +170,17 @@ flowchart TB ### Plan Documents -| Document | Description | -| ---------------------------------------------------------------- | -------------------------------------------------- | -| [OpenTelemetryPlan.md](./OpenTelemetryPlan.md) | Master overview and executive summary | -| [00-tracing-fundamentals.md](./00-tracing-fundamentals.md) | Distributed tracing concepts and OTel primer | -| [01-architecture-analysis.md](./01-architecture-analysis.md) | xrpld architecture and trace points | -| [02-design-decisions.md](./02-design-decisions.md) | SDK selection, exporters, span conventions | -| [03-implementation-strategy.md](./03-implementation-strategy.md) | Directory structure, performance analysis | -| [05-configuration-reference.md](./05-configuration-reference.md) | xrpld config, CMake, Collector configs | -| [06-implementation-phases.md](./06-implementation-phases.md) | Timeline, tasks, risks, success metrics | -| [07-observability-backends.md](./07-observability-backends.md) | Backend selection and architecture | -| [08-appendix.md](./08-appendix.md) | Glossary, references, version history | -| [secure-OTel.md](./secure-OTel.md) | Threat model and hardening (mTLS, peer validation) | +| Document | Description | +| ---------------------------------------------------------------- | -------------------------------------------- | +| [OpenTelemetryPlan.md](./OpenTelemetryPlan.md) | Master overview and executive summary | +| [00-tracing-fundamentals.md](./00-tracing-fundamentals.md) | Distributed tracing concepts and OTel primer | +| [01-architecture-analysis.md](./01-architecture-analysis.md) | xrpld architecture and trace points | +| [02-design-decisions.md](./02-design-decisions.md) | SDK selection, exporters, span conventions | +| [03-implementation-strategy.md](./03-implementation-strategy.md) | Directory structure, performance analysis | +| [05-configuration-reference.md](./05-configuration-reference.md) | xrpld config, CMake, Collector configs | +| [06-implementation-phases.md](./06-implementation-phases.md) | Timeline, tasks, risks, success metrics | +| [07-observability-backends.md](./07-observability-backends.md) | Backend selection and architecture | +| [08-appendix.md](./08-appendix.md) | Glossary, references, version history | ### Task Lists diff --git a/OpenTelemetryPlan/OpenTelemetryPlan.md b/OpenTelemetryPlan/OpenTelemetryPlan.md index dc2fdde471..51e83b6d65 100644 --- a/OpenTelemetryPlan/OpenTelemetryPlan.md +++ b/OpenTelemetryPlan/OpenTelemetryPlan.md @@ -53,7 +53,6 @@ flowchart TB phases["06-implementation-phases.md"] backends["07-observability-backends.md"] appendix["08-appendix.md"] - secure["secure-OTel.md"] end overview --> fundamentals @@ -68,7 +67,6 @@ flowchart TB config --> phases phases --> backends backends --> appendix - backends --> secure style overview fill:#1b5e20,stroke:#0d3d14,color:#fff,stroke-width:2px style fundamentals fill:#00695c,stroke:#004d40,color:#fff @@ -83,7 +81,6 @@ flowchart TB style phases fill:#4a148c,stroke:#2e0d57,color:#fff style backends fill:#4a148c,stroke:#2e0d57,color:#fff style appendix fill:#4a148c,stroke:#2e0d57,color:#fff - style secure fill:#4a148c,stroke:#2e0d57,color:#fff ``` @@ -102,7 +99,6 @@ flowchart TB | **6** | [Implementation Phases](./06-implementation-phases.md) | 5-phase timeline, tasks, risks, success metrics | | **7** | [Observability Backends](./07-observability-backends.md) | Backend selection guide and production architecture | | **8** | [Appendix](./08-appendix.md) | Glossary, references, version history | -| **Sec** | [Securing the OTel Pipeline](./secure-OTel.md) | Threat model and hardening (mTLS, peer trace-context validation) | --- @@ -200,12 +196,4 @@ The appendix contains a glossary of OpenTelemetry and xrpld-specific terms, refe --- -## Securing the OTel Pipeline - -Threat model and hardening guidance for production deployments where xrpld nodes ship telemetry to a centrally-hosted collector across an untrusted network. Covers the two attack surfaces (collector ingress and peer trace-context spoofing) and the chosen defenses: mTLS as primary collector auth, NetworkPolicy as defense-in-depth, and source-side validation plus per-peer rate limiting for the `protocol::TraceContext` field on peer messages. - -➡️ **[View Securing the OTel Pipeline](./secure-OTel.md)** - ---- - _This document provides a comprehensive implementation plan for integrating OpenTelemetry distributed tracing into the xrpld XRP Ledger node software. For detailed information on any section, follow the links to the corresponding sub-documents._ diff --git a/OpenTelemetryPlan/secure-OTel.md b/OpenTelemetryPlan/secure-OTel.md deleted file mode 100644 index 3d22de5452..0000000000 --- a/OpenTelemetryPlan/secure-OTel.md +++ /dev/null @@ -1,240 +0,0 @@ -# Securing OpenTelemetry Against Trace Context Spoofing - -> **Part of**: [OpenTelemetry Implementation Plan](./OpenTelemetryPlan.md) — see also [Design Decisions § Privacy](./02-design-decisions.md#244-privacy--sensitive-data-policy) (what we don't collect) and [Configuration Reference § 5.5](./05-configuration-reference.md#55-opentelemetry-collector-configuration) (collector base config). - -Trace context spoofing (or poisoning) occurs when untrusted actors inject tampered or stale trace IDs into your system. If these requests are processed, the spans are appended to historical trace buckets, stretching trace durations, ruining p99 latency metrics, and breaking Grafana dashboards. - -This guide outlines two categories of defense: mitigating tampered contexts and locking down the OpenTelemetry (OTel) Collector to trusted clients only. - ---- - -## Part 1: Mitigating Tampered Trace Contexts - -### 1. Perimeter Defense: Strip Headers at the API Gateway - -The most effective way to prevent spoofing from external sources is to treat your API Gateway (Envoy, NGINX, AWS ALB) as a hard boundary. Strip incoming W3C tracing headers (`traceparent`, `tracestate`) from public traffic so the gateway is forced to generate a fresh, legitimate `trace_id`. - -**NGINX Example (Stripping Headers):** - -```nginx -server { - listen 80; - - location / { - # Clear out untrusted incoming trace headers - proxy_set_header traceparent ""; - proxy_set_header tracestate ""; - - proxy_pass http://backend_service; - } -} -``` - -### **2. Timestamp-Anchored Trace IDs and OTTL Filtering** - -If you use a custom trace ID generator that embeds a timestamp in the first few bytes (like AWS X-Ray or UUIDv7), you can use the OTel Collector's OpenTelemetry Transform Language (OTTL) to detect anomalies. -**Collector Configuration (Conceptual OTTL Filter):** - -```yaml -processors: - filter/stale_traces: - error_mode: ignore - traces: - span: - # Example: Drop spans where the start time is significantly different - # from an expected parameter or embedded timestamp logic. - # Note: Standard W3C trace IDs do not contain timestamps by default. - - 'Keep out-of-bounds spans: time.sub(start_time, now()) > duration("1h")' -``` - -## **Part 2: Restricting Access to the OTel Collector** - -Locking down the Collector ensures that only authenticated, trusted clients can submit telemetry data. - -### **Approach A: Network Layer Security (Kubernetes Network Policies)** - -Ensure your Collector is not exposed to the public internet. If running in Kubernetes, use a NetworkPolicy to restrict ingress traffic to specific namespaces. -**Kubernetes NetworkPolicy Example:** - -```yaml -apiVersion: networking.k8s.io/v1 -kind: NetworkPolicy -metadata: - name: allow-internal-otel - namespace: observability -spec: - podSelector: - matchLabels: - app: opentelemetry-collector - policyTypes: - - Ingress - ingress: - - from: - - namespaceSelector: - matchLabels: - environment: production - ports: - - protocol: TCP - port: 4317 # gRPC - - protocol: TCP - port: 4318 # HTTP -``` - -### **Approach B: Transport Layer Security (Mutual TLS / mTLS)** - -Require clients to present a valid cryptographic certificate to connect to the Collector. -**Collector Configuration (mTLS):** - -```yaml -receivers: - otlp: - protocols: - grpc: - endpoint: 0.0.0.0:4317 - tls: - # Setting client_ca_file makes the collector require and verify a - # client cert, rejecting connections without a trusted one. - client_ca_file: /certs/client_ca.pem # CA that signs trusted client certs - cert_file: /certs/collector.pem - key_file: /certs/collector.key -``` - -### **Approach C: Application Layer Authentication (Basic Auth Extension)** - -Use the Collector's extension system to require an API key or Basic Auth credentials. -**Collector Configuration (Basic Auth):** - -```yaml -extensions: - basicauth/collector: - htpasswd: - inline: | - # username:trusted-client, password:SecurePassword123 - trusted-client:$apr1$4v8p76o6$DMTX5Wv6uOmrFAZp2X1N1. - -receivers: - otlp: - protocols: - grpc: - endpoint: 0.0.0.0:4317 - auth: - authenticator: basicauth/collector - -processors: - batch: - -exporters: - otlp: - endpoint: my-backend-storage:4317 - -service: - extensions: [basicauth/collector] - pipelines: - traces: - receivers: [otlp] - processors: [batch] - exporters: [otlp] -``` - -**Client Setup (Environment Variables):** -Developers must pass the authentication header using the standard OTel SDK environment variables: - -```bash -# Base64 encoded "trusted-client:SecurePassword123" -export OTEL_EXPORTER_OTLP_HEADERS="Authorization=Basic dHJ1c3RlZC1jbGllbnQ6U2VjdXJlUGFzc3dvcmQxMjM=" -``` - ---- - -Available routes to build on top of: https://github.com/XRPLF/rippled/pull/6425#discussion_r3234751995 - ---- - -# Analysis: Applying the Guide to xrpld - -The guide above is written for HTTP-fronted web services. xrpld is a P2P node daemon, so the threat model and the applicable defenses differ. This section captures how each approach maps to xrpld and the chosen direction. - -## Threat Model - -xrpld has **two distinct attack surfaces**, not one. The original guide conflates them under "trace context spoofing"; for xrpld they need separate defenses. - -| Surface | Attacker | Vector | Defense | -| ----------------------------------------- | -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | --------------------------------------------- | -| **Collector ingress** (xrpld → collector) | Anyone who can reach `4317`/`4318` on the collector host | Forged OTLP traffic, telemetry exfiltration, DoS on collector | mTLS + network policy | -| **Peer trace context** (peer → xrpld) | Malicious peer in the XRPL overlay | Crafted `protocol::TraceContext` field inside peer protobuf messages (TMTransaction, consensus, etc.) — used to forge `trace_id`/`span_id`, pollute p99, attach spans to historical traces | Validate + rate-limit at the receive boundary | - -**Deployment context:** Across-network. xrpld nodes (potentially run by external operators or in different DCs) ship telemetry to a centrally-hosted collector across an untrusted network. The collector is NOT on the same host or private VPC as every node. - -``` - ┌── peer (untrusted) ── TMTransaction{trace_context} ──▶ xrpld - │ │ - │ [validate + rate-limit] - │ │ - │ ▼ - │ SpanGuard (clean) - │ │ - │ │ OTLP/gRPC - │ │ + mTLS - │ ▼ - └───────────────────────────────────────── [client_ca_file: verify client cert] - OTel Collector - (in private subnet, NetPol) -``` - -## Part 1 Applicability — Peer Trace-Context Validation - -The guide's NGINX header stripping and OTTL stale-span filtering target HTTP gateways and post-hoc cleanup. Neither fits xrpld directly: - -- **NGINX header stripping** — N/A. There is no HTTP gateway between peers and xrpld; trace context arrives inside protobuf peer messages (`protocol::TraceContext`), not as W3C `traceparent` headers. See [src/xrpld/telemetry/PropagationHelpers.h](../src/xrpld/telemetry/PropagationHelpers.h). -- **OTTL stale-span filtering** — Weak fit. Post-hoc cleanup at the collector loses peer identity (you can't tell _which_ peer poisoned the trace). Validation at the receive site is stronger. - -**xrpld-specific Part 1 mitigations:** - -1. **Validate extracted context at the boundary** in [src/xrpld/telemetry/ConsensusReceiveTracing.h](../src/xrpld/telemetry/ConsensusReceiveTracing.h) and any other peer-message receive site. Reject if `trace_id` is all-zero, wrong length, or fails W3C format checks. Treat invalid context as "no propagated context" — start a fresh span — rather than dropping the message. -2. **Per-peer sample rate limiting** so a hostile peer cannot flood the collector with spans bearing a fabricated `trace_id`. Use probabilistic sampling on the receive path keyed by peer identity. - -## Part 2 — Comparison of Collector Hardening Approaches - -Evaluated for the across-network deployment shape: - -| Approach | Across-network fit | Cost | Verdict | -| ------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------- | ---------------------------------- | -| **A. NetworkPolicy / firewall** | Necessary baseline (don't expose `4317`/`4318` to the internet), but insufficient on its own when traffic genuinely crosses networks — you cannot NetworkPolicy the public internet. | Cheap. | **Defense-in-depth, not primary.** | -| **B. mTLS** | Strongest fit. Every xrpld node holds a client cert; the collector verifies it via `client_ca_file` in the receiver's `tls` block. Encrypts in transit (raw OTLP over the internet leaks transaction patterns and validator identity). Compromised node = revoke one cert, no shared secret to rotate everywhere. | Cert issuance + rotation pipeline. | **Primary.** | -| **C. Basic Auth** | Worst shape for this topology. Single shared password across all xrpld nodes — one leaked node config compromises the whole fleet. Doesn't encrypt; you'd need TLS underneath anyway, at which point you're 80% of the way to mTLS. | Cheap to set up, expensive to operate (rotation across N operators). | **Skip.** | - -## Decision - -**Primary defense:** mTLS (Approach B) on the collector's OTLP receivers. The collector requires and verifies each client certificate when `client_ca_file` is set in the receiver's `tls` block (there is no `auth_type` field — setting `client_ca_file` is what enforces client-cert verification). - -**Defense-in-depth:** NetworkPolicy / firewall rules (Approach A) so `4317`/`4318` are never reachable from outside the expected operator subnets even if mTLS were misconfigured. - -**Skipped:** Basic Auth (Approach C) — wrong shape for an across-network, multi-operator topology. - -**Plus xrpld-specific Part 1 work:** trace-context validation and per-peer rate limiting at peer-message receive sites. - -## Decisions Made - -| Decision | Choice | Rationale | -| -------------------- | -------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Cert source for mTLS | **Reuse XRPL node identity key** | One identity per node, no separate PKI to operate. Fits XRPL's existing trust model; requires small CA tooling step to derive/sign the OTel client cert from the node key. | -| Part 1 scope | **Include in this spec** | Collector hardening and peer trace-context validation share one threat model. Coherent design doc; can still be split into multiple PRs at implementation. | -| Dev impact | **Production-only** | Local `docker/telemetry/docker-compose.yml` keeps `insecure: true` and no auth for fast iteration. Only production deployment manifests gain mTLS. Accepted risk: minor dev/prod drift, mitigated by integration tests against a TLS-enabled collector in CI. | - -## Out of Scope - -- NGINX/Envoy header stripping (no HTTP gateway in front of xrpld-to-collector traffic). -- OTTL stale-span filtering at the collector (weaker than source validation; loses peer identity). -- Local development docker-compose hardening. -- Telemetry backend (Tempo) hardening — separate concern, downstream of the collector. - -## Next Step - -Write this up as a design doc with full sections covering: - -1. Threat model & architecture (this section, expanded) -2. Collector hardening — mTLS config, NetworkPolicy -3. Cert pipeline — deriving OTel client cert from XRPL node key -4. Peer trace-context validation — receive-site checks in `ConsensusReceiveTracing.h` -5. Per-peer span rate limiting -6. Testing & rollout From 70ec64e7046e33c2085a1bc2283d1b3be091aa4f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:05:41 +0100 Subject: [PATCH 02/16] fix(telemetry): Leave no queued handler in the StatsD loopback test server receive() built its result in a local and captured it by reference. A handler only ever runs inside the io_context, so cancel() does not retire the pending receive, it queues an operation_aborted completion. That completion survived into the next receive() call still holding a reference to the previous call's destroyed local. Nothing read it, because the handler skips on a non-zero error code, but the helper could not safely be called twice and its own example shows two calls. The result is now a member cleared per call, and the aborted completion is drained before returning, so no handler is queued when receive() returns. That makes the counter test's positive control possible. It asserted only that nothing arrived, which passed just as happily when the channel was dead: pointing the collector at a wrong port did not fail it. An untouched gauge publishes its initial zero, so the test now asserts that exact datagram arrives before asserting the counter stays silent. --- .../libxrpl/beast/insight/StatsDCollector.cpp | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/tests/libxrpl/beast/insight/StatsDCollector.cpp b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp index 5364b2b442..f9a9ac7346 100644 --- a/src/tests/libxrpl/beast/insight/StatsDCollector.cpp +++ b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp @@ -72,6 +72,9 @@ public: /** * Waits for one datagram. * + * Leaves no handler queued, so a later call cannot run this call's + * completion. + * * @param timeout How long to wait before giving up. * @return the datagram's bytes, or an empty string if none arrived in * time. @@ -79,17 +82,22 @@ public: std::string receive(std::chrono::milliseconds timeout) { - std::string received; + received_.clear(); socket_.async_receive( boost::asio::buffer(buffer_), - [&received, this](boost::system::error_code const& ec, std::size_t bytes) { + [this](boost::system::error_code const& ec, std::size_t bytes) { if (!ec) - received.assign(buffer_.data(), bytes); + received_.assign(buffer_.data(), bytes); }); ioContext_.restart(); ioContext_.run_for(timeout); + + // A handler runs only inside the io_context, so cancel() merely queues + // one carrying operation_aborted. Run the context again to retire it. socket_.cancel(); - return received; + ioContext_.restart(); + ioContext_.poll(); + return received_; } private: @@ -108,6 +116,12 @@ private: * 1472-byte packet limit. */ std::array buffer_{}; + + /** + * What the last receive() read, empty when its wait ran out. A member, so + * no queued handler can outlive what it writes to. + */ + std::string received_; }; /** @@ -135,6 +149,10 @@ TEST(StatsDCollector, UntouchedGaugePublishesInitialZero) * This is the other half of the rule above, and it is why the fix is a gauge * starting dirty rather than a flush of everything on the first tick. A counter * reports events, so an unsent counter and a zero counter mean the same thing. + * + * Silence is also what a dead loopback channel looks like, so an untouched gauge + * comes along as a positive control. One flush tick serves both metrics, so a + * counter line would have to travel beside the gauge's. */ TEST(StatsDCollector, UntouchedCounterPublishesNothing) { @@ -143,8 +161,14 @@ TEST(StatsDCollector, UntouchedCounterPublishesNothing) auto collector = StatsDCollector::make(address, "test", Journal(Journal::getNullSink())); auto const counter = collector->makeCounter("untouched"); + auto const control = collector->makeGauge("control"); - // Three seconds spans several one-second flush ticks. + // The control's line, alone: the channel carries a datagram, and the + // counter contributed nothing to it. + EXPECT_EQ(server.receive(std::chrono::seconds(10)), std::string("test.control:0|g\n")); + + // Three seconds spans several one-second flush ticks. The control is clean + // after its first flush, so anything arriving now is the counter. EXPECT_EQ(server.receive(std::chrono::seconds(3)), std::string()); } From c587cf5edf8287a2d89ec44c23707d641969d5cb Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:06:07 +0100 Subject: [PATCH 03/16] fix(rpc): Do not let span naming change the RPC error a client sees resolveCommandSpanName() converted command/method to a string with no type check. json::Value::asString() throws for an array or an object, so a request whose nested method is [] reached that conversion and the throw replaced a clean tooBusy reply with internal. The overloaded path is the only way in: fillHandler() returns tooBusy before anything has read those fields, and every other exit either converted them itself or means neither field is present. The effect is that the error code a client receives depends on whether telemetry was compiled in, which telemetry must never do. The span now falls back to its existing unknown-command label when either present field is not a string. The WebSocket path already validates both fields before dispatch, so it is left alone. The test drives a genuinely overloaded job queue, reading the threshold from the production constant rather than copying it, and asserts the client still gets tooBusy. It lives in the Beast tree because doCommand is daemon code and needs jtx, which the gtest binary cannot reach. --- src/test/rpc/RPCHandler_test.cpp | 176 ++++++++++++++++++++++++++++ src/xrpld/rpc/detail/RPCHandler.cpp | 28 +++-- 2 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 src/test/rpc/RPCHandler_test.cpp diff --git a/src/test/rpc/RPCHandler_test.cpp b/src/test/rpc/RPCHandler_test.cpp new file mode 100644 index 0000000000..d4a9a7c232 --- /dev/null +++ b/src/test/rpc/RPCHandler_test.cpp @@ -0,0 +1,176 @@ +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +namespace xrpl::test { + +/** + * Checks the error a busy server reports for a request it never dispatches. + * + * RPCHandler_test ──doCommand()──> RPC::fillHandler() + * │ │ + * └── fills ──> JobQueue <── reads ─┘ + * + * An overloaded server answers rpcTOO_BUSY before it reads the command name, so + * the request fields still hold whatever json type the client sent. Anything + * that runs afterwards to describe the error has to cope with that and leave + * the answer alone. + * + * @note Each testcase keeps one job-queue worker blocked for as long as it + * runs, and releases it before returning. + */ +class RPCHandler_test : public beast::unit_test::Suite +{ + /** + * How many jobs to queue to hold the server over its overload threshold. + * One job is dispatched straight away, so one spare keeps the waiting + * count above the limit. + */ + static constexpr int kOverloadJobs = rpc::tuning::kMaxJobQueueClients + 2; + + /** + * Dispatches one request on an overloaded server and checks the client is + * told the server is busy. + * + * @param params Request fields, in the form fillHandler() reads them. + */ + void + expectTooBusy(json::Value const& params) + { + using namespace jtx; + Env env{*this}; + auto& app = env.app(); + + // Only one job of this type runs at a time, so every job after the + // first stays queued until the gate opens. They also sort above + // JtClient, the priority the overload check counts from. + std::promise gate; + std::shared_future const open = gate.get_future().share(); + ScopeExit const openGate{[&gate]() { gate.set_value(); }}; + + int queued = 0; + for (int i = 0; i < kOverloadJobs; ++i) + { + if (app.getJobQueue().addJob(JtSweep, "overload", [open]() { open.wait(); })) + ++queued; + } + BEAST_EXPECT(queued == kOverloadJobs); + BEAST_EXPECT(app.getJobQueue().getJobCountGE(JtClient) > rpc::tuning::kMaxJobQueueClients); + + resource::Charge loadType = resource::kFeeReferenceRpc; + resource::Consumer consumer; + rpc::JsonContext context{ + {.j = env.journal, + .app = app, + .loadType = loadType, + .netOps = app.getOPs(), + .ledgerMaster = app.getLedgerMaster(), + .consumer = consumer, + .role = Role::USER, + .coro = {}, + .infoSub = {}, + .apiVersion = rpc::kApiVersionIfUnspecified}, + params, + {}}; + + json::Value result; + rpc::Status status; + std::string thrown; + try + { + status = rpc::doCommand(context, result); + } + catch (std::exception const& e) + { + thrown = e.what(); + } + + if (BEAST_EXPECTS(thrown.empty(), "doCommand threw: " + thrown)) + { + BEAST_EXPECT(status.type() == rpc::Status::Type::ErrorCodeI); + BEAST_EXPECT(status.toErrorCode() == RpcTooBusy); + BEAST_EXPECT(result[jss::error].asString() == "tooBusy"); + BEAST_EXPECT(result[jss::error_code].asInt() == static_cast(RpcTooBusy)); + } + } + + /** + * Checks a well-formed request on an overloaded server. This is the control + * for the two cases below: it shares their fixture and their assertions, + * and differs only in that every field it sends is a string. + */ + void + testRegisteredCommand() + { + testcase("Busy server, registered command"); + + json::Value params = json::ValueType::Object; + params[jss::command] = "ping"; + expectTooBusy(params); + } + + /** + * Checks a request whose "method" field is not a string. + */ + void + testNonStringMethod() + { + testcase("Busy server, method field is not a string"); + + // The HTTP path sets "command" from the outer method name it has + // already checked, and passes the inner request object through + // untouched, so "method" can arrive holding any json type. + json::Value params = json::ValueType::Object; + params[jss::command] = "ping"; + params[jss::method] = json::ValueType::Array; + expectTooBusy(params); + } + + /** + * Checks a request whose "command" field is not a string. + */ + void + testNonStringCommand() + { + testcase("Busy server, command field is not a string"); + + json::Value params = json::ValueType::Object; + params[jss::command] = json::ValueType::Object; + expectTooBusy(params); + } + +public: + void + run() override + { + testRegisteredCommand(); + testNonStringMethod(); + testNonStringCommand(); + } +}; + +BEAST_DEFINE_TESTSUITE(RPCHandler, rpc, xrpl); + +} // namespace xrpl::test diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index dc026c8aa2..dc362f1762 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -235,30 +235,40 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, // Resolve the span suffix / command attribute for a request that failed in // fillHandler. Returns the canonical handler name for a recognized command // (a finite, bounded set) or the literal "unknown" for a request that omits -// both fields or names an unregistered command. The raw request value is -// deliberately NOT used: the command attribute is promoted to a Prometheus -// label by the spanmetrics connector, so an attacker-controlled string would -// let arbitrary request input drive unbounded span-name / label cardinality. +// both fields, supplies one that is not a string, or names an unregistered +// command. The raw request value is deliberately NOT used: the command +// attribute is promoted to a Prometheus label by the spanmetrics connector, so +// an attacker-controlled string would let arbitrary request input drive +// unbounded span-name / label cardinality. // Resolving against the registry keeps per-command error attribution for real // commands (e.g. a submit rejected with rpcTOO_BUSY stays rpc.command.submit) // while collapsing garbage input to a single series. std::string_view resolveCommandSpanName(JsonContext const& context) { - if (!context.params.isMember(jss::command) && !context.params.isMember(jss::method)) + bool const hasCommand = context.params.isMember(jss::command); + bool const hasMethod = context.params.isMember(jss::method); + + if (!hasCommand && !hasMethod) + return rpc_span::val::unknownCommand; + + // A json array or object throws when asked for its string value, and no + // non-string field names a handler. The reply's error code is already + // decided, so naming the span must not be able to change it. + if ((hasCommand && !context.params[jss::command].isString()) || + (hasMethod && !context.params[jss::method].isString())) return rpc_span::val::unknownCommand; // fillHandler() rejects a request that supplies both fields with differing // values as rpcUNKNOWN_COMMAND. Mirror that here, or the span would be // labelled with one of the two names and misattribute the error to a // command that was never dispatched. - if (context.params.isMember(jss::command) && context.params.isMember(jss::method) && + if (hasCommand && hasMethod && context.params[jss::command].asString() != context.params[jss::method].asString()) return rpc_span::val::unknownCommand; - std::string const cmd = context.params.isMember(jss::command) - ? context.params[jss::command].asString() - : context.params[jss::method].asString(); + std::string const cmd = hasCommand ? context.params[jss::command].asString() + : context.params[jss::method].asString(); auto const* handler = getHandler(context.apiVersion, context.app.config().betaRpcApi, cmd); return (handler != nullptr) ? std::string_view{handler->name} From ea521593bb1121fd7752b9d3ee5499806cb3518f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:29:00 +0100 Subject: [PATCH 04/16] docs(telemetry): Document the otel choice in the [insight] section The comment said statsd was the only server choice. otel is also accepted, and an operator reading this had no way to learn that. It also records what does not carry over: the [telemetry] section owns the export destination and the resource attributes on that path, so address and prefix are ignored, and endpoint reaches a startup log line without changing where metrics go. --- cfg/xrpld-example.cfg | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 3116a01420..70d83cbf27 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1267,10 +1267,10 @@ # # "server" # -# Choice of server to send metrics to. Currently the only choice is -# "statsd" which sends UDP packets to a StatsD daemon, which must be -# running while xrpld is running. More information on StatsD is -# available here: +# Choice of server to send metrics to, either "statsd" or "otel". +# +# "statsd" sends UDP packets to a StatsD daemon, which must be running +# while xrpld is running. More information on StatsD is available here: # https://github.com/b/statsd_spec # # When server=statsd, these additional keys are used: @@ -1281,6 +1281,13 @@ # "prefix" A string prepended to each collected metric. This is used # to distinguish between different running instances of xrpld. # +# "otel" exports the same metrics over OTLP instead, and needs a build +# with telemetry compiled in. The [telemetry] section below owns the +# export destination and the resource attributes, so "address" and +# "prefix" do not apply. An "endpoint" key is read on this path but +# only appears in a startup log line; it does not change where metrics +# are sent. +# # If this section is missing, or the server type is unspecified or unknown, # statistics are not collected or reported. # From e8bb4f4657f6ce87e036e775a939d3dde9a81843 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:29:17 +0100 Subject: [PATCH 05/16] fix(telemetry): Catch a non-std exception from the metrics pipeline setup The startup path caught std::exception only. initMetrics() reaches the OTel SDK, which can throw something outside that hierarchy, and escaping a member initializer would stop the node starting. The section states the rule it must not break: a telemetry failure never stops the node, the global provider stays a no-op and every instrument call remains valid. The new clause drops the half-built provider and logs, exactly as the std::exception clause does. ~ApplicationImp() already carries the same pairing for the same reason. --- src/libxrpl/telemetry/Telemetry.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 97ecf11b4d..a667e57601 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -477,6 +477,16 @@ public: "continuing without metrics: " << e.what(); } + catch (...) + { + // initMetrics() reaches third-party SDK code, which may throw + // something outside std::exception. Escaping a constructor on the + // startup path would stop the node starting, so drop the + // half-built provider exactly as the clause above does. + meterProvider_.reset(); + JLOG(journal_.error()) << "Telemetry metrics pipeline failed to initialise, " + "continuing without metrics: unknown exception"; + } } /** From 5cecfc7d0bcdc71c8293642999138b5333263900 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:29:19 +0100 Subject: [PATCH 06/16] fix(insight): Read the StatsD polling gate under the lock it pairs with onTimer read polling_ before taking metricsLock_. A tick that read the flag as set could be preempted before acquiring the lock, letting onCollectionStopping() clear the flag, take the uncontended lock and return. The tick then resumed and ran every hook handler, which read the ledger master, the network operations, the peer finder, the job queue and the overlay after shutdown had been told polling stopped. Collector::onCollectionStopping() promises polling has stopped by the time it returns, and the shutdown ordering in ApplicationImp depends on that. Reading the gate inside the lock makes seeing it set imply holding the lock, so either the tick holds it across the handlers and the stop waits, or the stop wins and the tick polls nothing. The old comment assumed that invariant rather than establishing it. --- src/libxrpl/beast/insight/StatsDCollector.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libxrpl/beast/insight/StatsDCollector.cpp b/src/libxrpl/beast/insight/StatsDCollector.cpp index a1b535035b..9cf0daf2e7 100644 --- a/src/libxrpl/beast/insight/StatsDCollector.cpp +++ b/src/libxrpl/beast/insight/StatsDCollector.cpp @@ -277,8 +277,9 @@ public: { polling_.store(false, std::memory_order_release); - // onTimer holds metricsLock_ across the handler loop, so acquiring it - // here waits for a handler that is already running. + // onTimer reads polling_ under metricsLock_, so a tick that is going to + // call handlers already holds it. Taking it here waits for that tick, + // and any later one reads the cleared flag and polls nothing. std::scoped_lock const _(metricsLock_); } @@ -464,12 +465,17 @@ public: return; } - if (polling_.load(std::memory_order_acquire)) { + // Read the gate under the lock. A tick that sees it set therefore + // holds metricsLock_, which is what lets onCollectionStopping() + // wait for the handlers by taking the same lock. std::scoped_lock const _(metricsLock_); - for (auto& m : metrics_) - m.doProcess(); + if (polling_.load(std::memory_order_acquire)) + { + for (auto& m : metrics_) + m.doProcess(); + } } // The gate above holds back hook handlers, not socket I/O. Events reach From a2ee20b88f160bde4315b0a8578a29844d6d9286 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:22:15 +0100 Subject: [PATCH 07/16] fix(telemetry): Read the RPC span status from the reply, not the Status callMethod decided the rpc.command span's status from the Status the handler returned. Handler.cpp registers 70 of its 72 methods through byRef(), which returns a default Status whatever happened, because an old-style handler reports its error in the reply body instead. So a failed account_info or a refused path_find came out with rpc_status=success and span status Ok. That is the opposite of what the comment above the code claimed it did, and it left a {status.code=error} query blind to every non-throwing RPC error. The status now comes from the reply as well as the Status, which covers both handler styles. byRef() is unchanged and identical to develop: it behaves correctly for its own purpose, and no RPC reply changes. Only telemetry was reading the wrong signal. setOk() is dropped rather than moved. The specification reserves Ok for an operator asserting verified success and warns that a tool may treat it as suppressing errors, so a successful call now leaves the status Unset. No test accompanies this: the Beast tree has no telemetry fixture and the in-memory span exporter is linked only into xrpl_tests, which cannot reach daemon code. Asserting the exported status needs that infrastructure first. --- src/xrpld/rpc/detail/RPCHandler.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index dc362f1762..ca163b22af 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -190,23 +190,21 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, JLOG(context.j.debug()) << "RPC call " << name << " completed in " << ((end - start).count() / 1000000000.0) << "seconds"; perfLog.rpcFinish(name, curId); - // Status::operator bool() returns true when there IS an error - // (code_ != OK), so the ternary correctly maps error->error, ok->success. + // An old-style handler reports its error in the reply, not in the + // Status: byRef() returns a default Status whatever happened. Reading + // both covers every handler. Status::operator bool() is true when there + // IS an error. + bool const failed = static_cast(ret) || containsError(result); span.setAttribute( rpc_span::attr::rpcStatus, - ret ? std::string_view{rpc_span::val::error} - : std::string_view{rpc_span::val::success}); - // Reflect the result in the OTel span status, not just the attribute, - // so non-exception RPC errors (rpcTOO_BUSY, rpcNO_PERMISSION, ...) are - // visible to {status.code=error} queries. - if (ret) - { + failed ? std::string_view{rpc_span::val::error} + : std::string_view{rpc_span::val::success}); + // Error so a failed call answers {status.code=error}, for the codes that + // never throw (rpcTOO_BUSY, rpcNO_PERMISSION, ...). Success stays Unset: + // the spec reserves Ok for an operator asserting verified success, and a + // tool may read it as suppressing errors. + if (failed) span.setError(rpc_span::val::error); - } - else - { - span.setOk(); - } return ret; } catch (std::exception& e) From 89fc3ba450f5bd3757fd21ed99abb4132137305b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:29:58 +0100 Subject: [PATCH 08/16] fix(telemetry): Name the reason an RPC failed in the span status The status description was the fixed string error, so a trace recorded that a request failed but not why. It now carries the error token from the reply, falling back to the status's own error code and then to the old string. Every source is a compile-time literal from the error registry, so no request text reaches it. The rpc_status attribute stays at success and error: that one is a span-metrics dimension, and widening it would mint a series per token per command per node. The work is gated on the span being live, so a build with telemetry compiled out or a disabled guard pays nothing, matching how the neighbouring helper is handled. asCString() is null-checked as well as type-checked, because it returns the raw pointer where asString() guards it. The comment above resolveCommandSpanName now states the invariant rather than how it could be abused. --- src/xrpld/rpc/detail/RPCHandler.cpp | 91 +++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 25 deletions(-) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index ca163b22af..363938367a 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -158,6 +158,42 @@ fillHandler(JsonContext& context, Handler const*& result) return RpcSuccess; } +/** + * Names the reason a command failed, for the span's error description. + * + * jss::error holds the error token, and an old-style handler reports it there + * and nowhere else. A failure the reply does not name is described by the + * status's own error code, which is the only reason left to report. Every + * token is a compile-time string, so no request text reaches the description. + * + * @param status What the handler returned. + * @param result The reply the handler filled in. The returned view can point + * into it, so result must outlive the view. + * @param replyHasError The caller's containsError(result), passed in so the + * reply is not searched twice. + * @return The error token, or "error" where neither source carries one. + */ +std::string_view +errorDescription(Status const& status, json::Value const& result, bool replyHasError) +{ + if (replyHasError && result[jss::error].isString()) + { + // asCString() asserts the type, then hands back the stored pointer + // unchecked, and a string-typed json::Value may hold a null one. Both + // checks are needed before that pointer becomes a view. + if (char const* const token = result[jss::error].asCString(); token != nullptr) + return token; + } + + // A TER or a bare integer code has no token in the error registry, so + // reading one would name an unrelated error. getErrorInfo() returns a + // reference into a static table, so its token outlives this call. + if (status.type() == Status::Type::ErrorCodeI) + return getErrorInfo(status.toErrorCode()).token.cStr(); + + return rpc_span::val::error; +} + Status callMethod(JsonContext& context, Handler::Method method, std::string_view name, json::Value& result) { @@ -190,21 +226,31 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, JLOG(context.j.debug()) << "RPC call " << name << " completed in " << ((end - start).count() / 1000000000.0) << "seconds"; perfLog.rpcFinish(name, curId); - // An old-style handler reports its error in the reply, not in the - // Status: byRef() returns a default Status whatever happened. Reading - // both covers every handler. Status::operator bool() is true when there - // IS an error. - bool const failed = static_cast(ret) || containsError(result); - span.setAttribute( - rpc_span::attr::rpcStatus, - failed ? std::string_view{rpc_span::val::error} - : std::string_view{rpc_span::val::success}); - // Error so a failed call answers {status.code=error}, for the codes that - // never throw (rpcTOO_BUSY, rpcNO_PERMISSION, ...). Success stays Unset: - // the spec reserves Ok for an operator asserting verified success, and a - // tool may read it as suppressing errors. - if (failed) - span.setError(rpc_span::val::error); + // Everything in here only feeds the span, and searching the reply is + // not free, so a null guard pays for none of it. setError() and + // setAttribute() are no-ops on a null guard, but their arguments are + // not: with telemetry compiled out operator bool() is a constant false. + if (span) + { + // An old-style handler reports its error in the reply, not in the + // Status: byRef() returns a default Status whatever happened. + // Reading both covers every handler. Status::operator bool() is + // true when there IS an error. + bool const replyHasError = containsError(result); + bool const failed = static_cast(ret) || replyHasError; + // Two values only. rpc_status is a spanmetrics dimension, so every + // value it can take becomes a Prometheus label and a metric series. + span.setAttribute( + rpc_span::attr::rpcStatus, + failed ? std::string_view{rpc_span::val::error} + : std::string_view{rpc_span::val::success}); + // Error so a failed call answers {status.code=error}, for the codes + // that never throw (rpcTOO_BUSY, rpcNO_PERMISSION, ...). Success + // stays Unset: the spec reserves Ok for an operator asserting + // verified success, and a tool may read it as suppressing errors. + if (failed) + span.setError(errorDescription(ret, result, replyHasError)); + } return ret; } catch (std::exception& e) @@ -231,16 +277,11 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name, #ifdef XRPL_ENABLE_TELEMETRY // Resolve the span suffix / command attribute for a request that failed in -// fillHandler. Returns the canonical handler name for a recognized command -// (a finite, bounded set) or the literal "unknown" for a request that omits -// both fields, supplies one that is not a string, or names an unregistered -// command. The raw request value is deliberately NOT used: the command -// attribute is promoted to a Prometheus label by the spanmetrics connector, so -// an attacker-controlled string would let arbitrary request input drive -// unbounded span-name / label cardinality. -// Resolving against the registry keeps per-command error attribution for real -// commands (e.g. a submit rejected with rpcTOO_BUSY stays rpc.command.submit) -// while collapsing garbage input to a single series. +// fillHandler. The name comes from the handler registry, so only a registered +// handler name or the "unknown" label can reach the span; request text never +// does. That bounded set also bounds the Prometheus label the spanmetrics +// connector derives from it, and a real command still keeps its own error +// attribution: a submit rejected with rpcTOO_BUSY stays rpc.command.submit. std::string_view resolveCommandSpanName(JsonContext const& context) { From cae3f3c477ff74fd8aed3df95542994c16e48c67 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:30:15 +0100 Subject: [PATCH 09/16] test(telemetry): Assert what the consensus-attribute case was only executing The case had no assertions at all, so it could not fail. Its comment also said the attribute constants live in an xrpld-level header a libxrpl test cannot include. ConsensusSpanNames.h is a libxrpl header, two sibling tests already include it, and a comment fifty lines above says so correctly. So the seven literal keys were never forced. They now come from the same constants the emitter uses, and the case asserts the property it was written for: the factory decides its verdict once at creation, and writing either close-time outcome leaves the guard inert and publishing no propagation bytes. The premise is asserted too, so a failure names which exit produced the null guard the rest of the case rests on. --- .../libxrpl/telemetry/SpanGuardFactory.cpp | 66 ++++++++++++------- 1 file changed, 42 insertions(+), 24 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp index 6cec7a5c86..88d045a37c 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp @@ -1,12 +1,14 @@ +#include #include #include +#include #include #include #include #include -#include +#include #include using namespace xrpl; @@ -88,28 +90,44 @@ TEST(SpanGuardFactory, discard_safe_on_null) EXPECT_FALSE(span); } -TEST(SpanGuardFactory, consensus_close_time_attributes) +TEST(SpanGuardFactory, consensus_accept_apply_attributes_are_inert_on_null_guard) { - // Verify the consensus attribute pattern compiles and doesn't crash with a - // null SpanGuard. Attribute keys/values use the underscore convention; the - // canonical consensus::span constants are defined in the xrpld-level - // ConsensusSpanNames.h, which a libxrpl test cannot include, so the keys are - // written as literals here. - { - auto span = telemetry::SpanGuard::span( - telemetry::TraceCategory::Consensus, telemetry::seg::consensus, "accept.apply"); - span.setAttribute("ledger_seq", static_cast(42)); - 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")); - span.setAttribute("proposing", true); - span.setAttribute("round_time_ms", static_cast(3500)); - } - { - auto span = telemetry::SpanGuard::span( - telemetry::TraceCategory::Consensus, telemetry::seg::consensus, "accept.apply"); - span.setAttribute("close_time_correct", false); - span.setAttribute("consensus_state", std::string("moved_on")); - } + namespace cs = consensus::span; + + // Nothing in this binary starts telemetry, so span() returns a null guard + // before it even joins the name. Pinning that here says which of the + // factory's exits produced the null guard the rest of the test relies on. + ASSERT_EQ(Telemetry::getInstance(), nullptr); + + // The attribute set RCLConsensus::doAccept() writes on consensus.accept.apply, + // read from the same constants the emitter uses rather than copied as + // literals. Both close-time outcomes are written below: the values differ, + // the guard's inertness does not. + auto applySpan = SpanGuard::span(TraceCategory::Consensus, seg::consensus, cs::op::acceptApply); + ASSERT_FALSE(applySpan); + + applySpan.setAttribute(cs::attr::ledgerSeq, static_cast(42)); + applySpan.setAttribute(cs::attr::closeTimeRippleEpochS, static_cast(780000000)); + applySpan.setAttribute(cs::attr::closeTimeCorrect, true); + applySpan.setAttribute(cs::attr::closeResolutionMs, static_cast(30000)); + applySpan.setAttribute(cs::attr::consensusState, std::string_view{cs::val::finished}); + applySpan.setAttribute(cs::attr::proposing, true); + applySpan.setAttribute(cs::attr::roundTimeMs, static_cast(3500)); + + // A write cannot activate a guard, so it still holds no span and hands out + // no propagation bytes for an outgoing message to carry. + EXPECT_FALSE(applySpan); + EXPECT_FALSE(applySpan.getTraceBytes().valid); + + // The consensus-failed branch writes the other value over the same two keys, + // and reaches the same inert guard. + auto movedOnSpan = + SpanGuard::span(TraceCategory::Consensus, seg::consensus, cs::op::acceptApply); + ASSERT_FALSE(movedOnSpan); + + movedOnSpan.setAttribute(cs::attr::closeTimeCorrect, false); + movedOnSpan.setAttribute(cs::attr::consensusState, std::string_view{cs::val::movedOn}); + + EXPECT_FALSE(movedOnSpan); + EXPECT_FALSE(movedOnSpan.getTraceBytes().valid); } From 3cfe8d139ebcc8fe50671c33a47fe0d3d8d759bf Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:31:14 +0100 Subject: [PATCH 10/16] feat(telemetry): Add Tempo search filters for the path-finding attributes This branch emits nine pathfind attributes and the datasource offered a dropdown for none of them, so the signal was there but not searchable in Explore. The file's own header states that each phase adds filters for what it introduces. Six filters, for the attributes that select a request. The three counts are measurements read off a span rather than things an operator searches by, so they get none. ledger_index is dynamic because it takes a new value every ledger. --- .../provisioning/datasources/tempo.yaml | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/docker/telemetry/grafana/provisioning/datasources/tempo.yaml b/docker/telemetry/grafana/provisioning/datasources/tempo.yaml index 5b2054a7fd..7b0d4dbb42 100644 --- a/docker/telemetry/grafana/provisioning/datasources/tempo.yaml +++ b/docker/telemetry/grafana/provisioning/datasources/tempo.yaml @@ -7,6 +7,7 @@ # Each phase adds filters for the span attributes it introduces. # Base filters — node identity, service, span name, status. # RPC command, status, role filters. +# Path-finding request, mode and ledger filters. apiVersion: 1 @@ -106,3 +107,38 @@ datasources: operator: "=" scope: span type: dynamic + # Path-finding filters. Only the attributes that select a request get + # a dropdown; pathfind_num_paths, pathfind_num_requests and + # pathfind_num_source_assets are measurements read off a span, not + # things an operator searches by. + - id: pathfind-source-account + tag: pathfind_source_account + operator: "=" + scope: span + type: dynamic + - id: pathfind-dest-account + tag: pathfind_dest_account + operator: "=" + scope: span + type: dynamic + - id: pathfind-dest-currency + tag: pathfind_dest_currency + operator: "=" + scope: span + type: dynamic + - id: pathfind-fast + tag: pathfind_fast + operator: "=" + scope: span + type: dynamic + # Changes every ledger, so it must be dynamic rather than static. + - id: pathfind-ledger-index + tag: pathfind_ledger_index + operator: "=" + scope: span + type: dynamic + - id: pathfind-search-level + tag: pathfind_search_level + operator: ">" + scope: span + type: dynamic From 87560c5157b94a19cada53cad88735f84293fbdf Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:31:15 +0100 Subject: [PATCH 11/16] test(telemetry): Restore the thread's LocalValue store from a scope guard The store swap was undone by trailing statements, which a fatal assertion skips. The thread store is a function-local static, so it outlives every test rather than being reset between them: a skipped restore left it owning a stack object from a dead frame, and the crash then landed in whichever test ran next. The onCoro flag did not protect it either, because the cleanup function reads that flag out of the freed object to decide not to delete it. The guard is declared after both stack stores so it is destroyed before them. --- .../libxrpl/telemetry/SpanGuardScope.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 3fa35bd7c5..f05139410a 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -23,6 +23,7 @@ #ifdef XRPL_ENABLE_TELEMETRY #include +#include #include #include #include @@ -469,9 +470,15 @@ TEST_F(SpanGuardScopeTest, scopedGuard_survives_localvalue_store_swap) xrpl::detail::LocalValues coroStore; xrpl::detail::LocalValues workerStore; - // Detach (do NOT delete) the fixture's active store, run on the coro store, - // and remember the original so teardown gets it back. + // Detach (do NOT delete) the fixture's active store and run on the coro + // store. A failed ASSERT_* returns from the test body, so the restore must be + // RAII or the thread pointer keeps owning a stack store that is about to die. + // Declared after both stack stores, so it is destroyed before either of them. auto* saved = xrpl::detail::getLocalValues().release(); + xrpl::ScopeExit const restoreStore{[saved]() { + xrpl::detail::getLocalValues().release(); + xrpl::detail::getLocalValues().reset(saved); + }}; xrpl::detail::getLocalValues().reset(&coroStore); trc::SpanContext captured = trc::SpanContext::GetInvalid(); @@ -501,11 +508,9 @@ TEST_F(SpanGuardScopeTest, scopedGuard_survives_localvalue_store_swap) auto afterPop = trc::GetSpan(ctx::RuntimeContext::GetCurrent()); EXPECT_FALSE(afterPop->GetContext().IsValid()); - // Restore (re-own) the fixture's store for teardown before any stack store - // leaves scope, so the thread pointer never dangles. - xrpl::detail::getLocalValues().release(); - xrpl::detail::getLocalValues().reset(saved); - + // restoreStore re-owns the fixture's store from here on: it runs on every + // exit path, and the checks below touch no LocalValue. + // // The span ended exactly once, when the scope popped on resume. EXPECT_EQ(countSpans(spanData()->GetSpans(), "rpc.process"), 1u); } From 35db8610cc46064a13c5e658c0c101a74318ab0e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:21 +0100 Subject: [PATCH 12/16] fix(telemetry): Set rpc_status on the invalid-JSON websocket span rpc_status is a span-metrics dimension, so leaving it unset on this path emitted a series with a blank label. Any query selecting on error missed the failure entirely. Co-Authored-By: Claude Opus 5 (1M context) --- src/xrpld/rpc/detail/ServerHandler.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 20587e53e5..325c9c1224 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -352,6 +352,10 @@ ServerHandler::onWSMessage( // Fresh root so each WS message is its own trace. auto span = ScopedSpanGuard::freshRoot( TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); + // rpc_status is a span-metrics dimension, so leaving it unset emits a + // series with a blank label and hides this failure from any query that + // selects on error. + span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error); span.setError(rpc_span::val::invalidJson); json::Value jvResult(json::ValueType::Object); From a293fb66e50f9f35af0c46d167c0fecdd023365f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:30 +0100 Subject: [PATCH 13/16] fix(telemetry): Report pathfind span status from the reply Both pathfind handlers returned on many paths without recording a status, so a failed request produced a span that read as success. Route every exit through one helper that reads the rpc error token off the reply, which also covers the replies built further down the call chain. The token set is fixed by the error registry, so it is safe as a span label; raw request text would not be. Co-Authored-By: Claude Opus 5 (1M context) --- src/xrpld/rpc/detail/PathFindSpanNames.h | 4 +++ src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 30 ++++++++++++------- .../rpc/handlers/orderbook/RipplePathFind.cpp | 24 ++++++++++----- 3 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/xrpld/rpc/detail/PathFindSpanNames.h b/src/xrpld/rpc/detail/PathFindSpanNames.h index 6d8fedc7ab..47376f7e8b 100644 --- a/src/xrpld/rpc/detail/PathFindSpanNames.h +++ b/src/xrpld/rpc/detail/PathFindSpanNames.h @@ -29,6 +29,10 @@ * | +-----------------------------------------------------------+ | * +----------------------------------------------------------------+ * + * pathfind.request ends with status error whenever the handler's reply + * carries an rpc error. The description is that error's registry token, + * never request text. + * * Async recomputation (ledger close): * * +----------------------------------------------------------------+ diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index 44830619e4..af15403030 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -47,18 +47,28 @@ doPathFind(rpc::JsonContext& context) span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); } + // A failed reply carries the rpc error token, so reading the status off the + // reply covers every exit, including the ones whose reply is built further + // down the call chain. The token set is fixed by the error registry, so it + // is safe as a span label; raw request text would not be. + auto const finish = [&span](json::Value&& reply) -> json::Value { + if (span && rpc::containsError(reply)) + span.setError(std::as_const(reply)[jss::error].asString()); + return std::move(reply); + }; + if (context.app.config().pathSearchMax == 0) - return rpcError(RpcNotSupported); + return finish(rpcError(RpcNotSupported)); auto lpLedger = context.ledgerMaster.getClosedLedger(); if (!context.params.isMember(jss::subcommand) || !context.params[jss::subcommand].isString()) { - return rpcError(RpcInvalidParams); + return finish(rpcError(RpcInvalidParams)); } if (!context.infoSub) - return rpcError(RpcNoEvents); + return finish(rpcError(RpcNoEvents)); context.infoSub->setApiVersion(context.apiVersion); @@ -68,8 +78,8 @@ doPathFind(rpc::JsonContext& context) { context.loadType = resource::kFeeHeavyBurdenRpc; context.infoSub->clearRequest(); - return context.app.getPathRequestManager().makePathRequest( - context.infoSub, lpLedger, context.params); + return finish(context.app.getPathRequestManager().makePathRequest( + context.infoSub, lpLedger, context.params)); } if (sSubCommand == "close") @@ -77,10 +87,10 @@ doPathFind(rpc::JsonContext& context) InfoSubRequest::pointer const request = context.infoSub->getRequest(); if (!request) - return rpcError(RpcNoPfRequest); + return finish(rpcError(RpcNoPfRequest)); context.infoSub->clearRequest(); - return request->doClose(); + return finish(request->doClose()); } if (sSubCommand == "status") @@ -88,12 +98,12 @@ doPathFind(rpc::JsonContext& context) InfoSubRequest::pointer const request = context.infoSub->getRequest(); if (!request) - return rpcError(RpcNoPfRequest); + return finish(rpcError(RpcNoPfRequest)); - return request->doStatus(context.params); + return finish(request->doStatus(context.params)); } - return rpcError(RpcInvalidParams); + return finish(rpcError(RpcInvalidParams)); } } // namespace xrpl diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index 49b90a6e4c..9abe40cdcc 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -56,8 +56,18 @@ doRipplePathFind(rpc::JsonContext& context) span.setAttribute(pathfind_span::attr::destAccount, redactAccount(dst.asString())); } + // A failed reply carries the rpc error token, so reading the status off the + // reply covers every exit, including the ones whose reply is built further + // down the call chain. The token set is fixed by the error registry, so it + // is safe as a span label; raw request text would not be. + auto const finish = [&span](json::Value&& reply) -> json::Value { + if (span && rpc::containsError(reply)) + span.setError(std::as_const(reply)[jss::error].asString()); + return std::move(reply); + }; + if (context.app.config().pathSearchMax == 0) - return rpcError(RpcNotSupported); + return finish(rpcError(RpcNotSupported)); context.loadType = resource::kFeeHeavyBurdenRpc; @@ -73,8 +83,8 @@ doRipplePathFind(rpc::JsonContext& context) rpc::tuning::kMaxValidatedLedgerAge) { if (context.apiVersion == 1) - return rpcError(RpcNoNetwork); - return rpcError(RpcNotSynced); + return finish(rpcError(RpcNoNetwork)); + return finish(rpcError(RpcNotSynced)); } PathRequest::pointer request; @@ -175,17 +185,17 @@ doRipplePathFind(rpc::JsonContext& context) jvResult = request->doStatus(context.params); } - return jvResult; + return finish(std::move(jvResult)); } // The caller specified a ledger jvResult = rpc::lookupLedger(lpLedger, context); if (!lpLedger) - return jvResult; + return finish(std::move(jvResult)); rpc::LegacyPathFind const lpf(isUnlimited(context.role), context.app); if (!lpf.isOk()) - return rpcError(RpcTooBusy); + return finish(rpcError(RpcTooBusy)); auto result = context.app.getPathRequestManager().doLegacyPathRequest( context.consumer, lpLedger, context.params); @@ -193,7 +203,7 @@ doRipplePathFind(rpc::JsonContext& context) for (auto& fieldName : jvResult.getMemberNames()) result[fieldName] = std::move(jvResult[fieldName]); - return result; + return finish(std::move(result)); } } // namespace xrpl From daa82cea337988b4faa53712cf9931b96dae7e4c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:38 +0100 Subject: [PATCH 14/16] fix(telemetry): Inject the send span's own context into consensus messages propose() and validate() both injected the ambient span context, but no span is ever activated on the threads that reach them: the round span is deliberately non-ambient and the validation span is parented through a stored context. So no TraceContext was written, every receiving peer took its standalone-span fallback, and the consensus receive spans arrived as orphan trace roots. Inject from the span in hand instead, which is the form the propagation helper documents and the transaction relay path already uses. Co-Authored-By: Claude Opus 5 (1M context) --- src/xrpld/app/consensus/RCLConsensus.cpp | 31 +++++++++++++----------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 5b216dcce5..3763fc9316 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -280,15 +280,16 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) app_.getHashRouter().addSuppression(suppression); - // Inject the current thread's active span context (e.g. the consensus - // round span) so receiving peers can link their proposal.receive span - // as a child of this trace. + // Inject this send span's own context, so receiving peers can parent their + // proposal.receive span to it. Reading the ambient context instead would + // find nothing: no span is activated on either thread that reaches here, + // and the round span is deliberately never ambient. // - // The helper injects only when a span is actually active, so a node with - // telemetry compiled out, disabled by config, or simply not tracing this - // round sends no TraceContext at all rather than an empty one that makes - // every peer take its has_trace_context() branch for nothing. - telemetry::injectCurrentContext(prop); + // Injection writes only when the span is live, so a node with telemetry + // compiled out, disabled by config, or simply not tracing this round sends + // no TraceContext at all rather than an empty one that makes every peer + // take its has_trace_context() branch for nothing. + telemetry::injectSpanContext(span, prop); app_.getOverlay().broadcast(prop); } @@ -1109,18 +1110,20 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns, // Broadcast to all our peers: protocol::TMValidation val; val.set_validation(serialized.data(), serialized.size()); - // Inject the current thread's active span context so receiving - // peers can link their validation.receive span as a child. + // Inject this validation span's own context, so receiving peers can parent + // their validation.receive span to it. Reading the ambient context instead + // would find nothing: valSpan is parented through a stored context and is + // never activated on this thread. // // The trace_context appended below is outside the signature on // `serialized`, so it is not covered by validation authenticity. // Downstream consumers treat it as advisory only. A signature-covered // trace context is a possible future enhancement. // - // As on the proposal path, the helper injects only when a span is actually - // active, so a node that is not tracing sends no TraceContext at all - // rather than an empty one. - telemetry::injectCurrentContext(val); + // Injection writes only when the span is live, so a node that is not + // tracing sends no TraceContext at all rather than an empty one. + if (valSpan) + telemetry::injectSpanContext(*valSpan, val); app_.getOverlay().broadcast(val); // Publish to all our subscribers: From 2d82bce5d4ed0a4c3ea072ca56c13e4362a80eb1 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:49 +0100 Subject: [PATCH 15/16] fix(telemetry): Apply decided validation events oldest minute first pending_ is a hash map, so one reconcile pass handed its entries over in no useful order. The windows only move forward, so an event applied after a later one landed in a bucket the grid had already rolled past and was counted into the wrong window. Collect the decisions, sort by minute, then apply. Co-Authored-By: Claude Opus 5 (1M context) --- .../libxrpl/telemetry/ValidationTracker.cpp | 46 +++++++++++++++++++ src/xrpld/telemetry/ValidationTracker.h | 4 ++ .../telemetry/detail/ValidationTracker.cpp | 20 +++++++- 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/tests/libxrpl/telemetry/ValidationTracker.cpp b/src/tests/libxrpl/telemetry/ValidationTracker.cpp index 57b90efe9d..7001f7f341 100644 --- a/src/tests/libxrpl/telemetry/ValidationTracker.cpp +++ b/src/tests/libxrpl/telemetry/ValidationTracker.cpp @@ -416,6 +416,52 @@ TEST(ValidationTracker, steady_traffic_across_the_grid_boundary_keeps_recent_cou EXPECT_EQ(t.missed7d(), 0u); } +TEST(ValidationTracker, a_stall_across_the_1h_edge_counts_each_event_in_its_own_window) +{ + // Two minutes an hour apart are decided in one pass. pending_ is a hash + // map, so it hands them over in no useful order. Applied newest first, the + // older minute lands behind the 1-hour tail and is counted into that window + // anyway. + auto t = makeTracker(); + + constexpr std::uint64_t kOld = 30; + constexpr std::uint64_t kFresh = 30; + + // No drain happens until the end, so the whole burst has to fit one ring. + static_assert(kOld + kFresh <= Tracker::ringCapacity()); + + for (std::uint64_t i = 0; i < kOld; ++i) + { + t.recordOurValidation(makeHash(9200 + i), static_cast(9200 + i)); + t.recordNetworkValidation(makeHash(9200 + i), static_cast(9200 + i)); + } + + // Past the 1-hour edge with no reconcile in between, which is what puts two + // distant minutes into one pass. + advance(std::chrono::hours(1) + std::chrono::minutes(2)); + + for (std::uint64_t i = 0; i < kFresh; ++i) + { + t.recordOurValidation(makeHash(9300 + i), static_cast(9300 + i)); + t.recordNetworkValidation(makeHash(9300 + i), static_cast(9300 + i)); + } + + settle(t); + + // Nothing was dropped, so all 60 events really did reach that one pass. + ASSERT_EQ(t.droppedEvents(), 0u); + + // Only the fresh minute is inside the hour. Both minutes are inside the day + // and the week, so those two totals are the control: they read the same + // whatever the order was. + EXPECT_EQ(t.agreements1h(), kFresh); + EXPECT_EQ(t.missed1h(), 0u); + EXPECT_DOUBLE_EQ(t.agreementPct1h(), 100.0); + EXPECT_EQ(t.agreements24h(), kOld + kFresh); + EXPECT_EQ(t.missed24h(), 0u); + EXPECT_EQ(t.agreements7d(), kOld + kFresh); +} + // ---- lifetime totals ------------------------------------------------------- TEST(ValidationTracker, lifetime_totals_survive_window_expiry) diff --git a/src/xrpld/telemetry/ValidationTracker.h b/src/xrpld/telemetry/ValidationTracker.h index c5f3142bc0..2cf65b8be1 100644 --- a/src/xrpld/telemetry/ValidationTracker.h +++ b/src/xrpld/telemetry/ValidationTracker.h @@ -556,6 +556,10 @@ private: /** * Decide every pending event past the grace period, repair the ones whose * other half arrived late, and drop entries too old to repair. + * + * Newly decided events reach the windows oldest minute first. The windows + * only move forward, so an event counted after a later one would land in a + * bucket the grid had already rolled past. * @param now Current time point. */ void diff --git a/src/xrpld/telemetry/detail/ValidationTracker.cpp b/src/xrpld/telemetry/detail/ValidationTracker.cpp index d355fb0c76..74c8fd3114 100644 --- a/src/xrpld/telemetry/detail/ValidationTracker.cpp +++ b/src/xrpld/telemetry/detail/ValidationTracker.cpp @@ -10,11 +10,13 @@ #include +#include #include #include #include #include #include +#include namespace xrpl::telemetry { @@ -94,6 +96,16 @@ ValidationTracker::note(Slot const& s, bool ours) void ValidationTracker::decidePending(TimePoint now) { + // A window only ever moves forward, so a bucket the grid has already + // rolled past cannot take a count. pending_ is unordered, so the minutes + // are collected here and applied in order below. + struct Decision + { + std::uint64_t minute{0}; // Bucket the event belongs to. + bool agreed{false}; // True to count it as an agreement. + }; + std::vector decisions; + for (auto& [hash, evt] : pending_) { if (!evt.decided) @@ -104,7 +116,7 @@ ValidationTracker::decidePending(TimePoint now) evt.decided = true; evt.agreed = evt.weValidated && evt.networkValidated; noteTallied(hash); - addToWindows(evt.minute, evt.agreed); + decisions.push_back(Decision{.minute = evt.minute, .agreed = evt.agreed}); (evt.agreed ? totalAgreements_ : totalMissed_).fetch_add(1, std::memory_order_relaxed); } else if ( @@ -120,6 +132,12 @@ ValidationTracker::decidePending(TimePoint now) } } + std::ranges::sort(decisions, [](Decision const& lhs, Decision const& rhs) { + return lhs.minute < rhs.minute; + }); + for (auto const& d : decisions) + addToWindows(d.minute, d.agreed); + // Nothing can be repaired past the window, so the entry is dead weight. auto const cutoff = now - kLateRepairWindow; for (auto it = pending_.begin(); it != pending_.end();) From a9541a7300203b25055bb2ab11b5f566352ad290 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 22 Sep 2026 21:11:58 +0100 Subject: [PATCH 16/16] fix(telemetry): Install the OTel context storage before any thread starts SetRuntimeContextStorage() writes a process-global shared_ptr that every log line reads through RuntimeContext::GetCurrent(). Neither side is atomic, and assigning the wrapper destroys it and placement-news a replacement over the same buffer, so a concurrent reader could make a virtual call through an indeterminate vptr. Install it in main() while the process is still single-threaded instead, and drop the member that held it. Gated on the telemetry section being enabled. Only that one key is read here, because parsing the whole section can throw on a contradictory TLS combination and that error belongs where it already reports. Co-Authored-By: Claude Opus 5 (1M context) --- docs/build/telemetry.md | 10 ++--- .../xrpl/telemetry/CoroAwareContextStorage.h | 9 +++-- src/libxrpl/telemetry/Telemetry.cpp | 24 ++---------- src/xrpld/app/main/Main.cpp | 37 +++++++++++++++++++ 4 files changed, 51 insertions(+), 29 deletions(-) diff --git a/docs/build/telemetry.md b/docs/build/telemetry.md index 696c799b50..f1e108916a 100644 --- a/docs/build/telemetry.md +++ b/docs/build/telemetry.md @@ -194,11 +194,11 @@ flowchart TD ### Coroutine-aware context storage -The active-context stack is not a plain `thread_local`. At telemetry start -xrpld installs `CoroAwareContextStorage`, which keeps the stack in an -`xrpl::LocalValue`. Because `JobQueue::Coro::resume()` swaps the coroutine's -`LocalValue` store in and out with the coroutine, the ambient context _follows -the coroutine_ across every yield and resume — even when it resumes on a +The active-context stack is not a plain `thread_local`. During startup, before +any thread is created, xrpld installs `CoroAwareContextStorage`, which keeps the +stack in an `xrpl::LocalValue`. Because `JobQueue::Coro::resume()` swaps the +coroutine's `LocalValue` store in and out with the coroutine, the ambient context +_follows the coroutine_ across every yield and resume — even when it resumes on a different worker thread. A `ScopedSpanGuard` held across a coroutine yield is therefore safe: its scope rides the coroutine and pops on the same store it was pushed onto, so it never pops the wrong stack. Off a coroutine the `LocalValue` diff --git a/include/xrpl/telemetry/CoroAwareContextStorage.h b/include/xrpl/telemetry/CoroAwareContextStorage.h index d7c047e752..cea330baf7 100644 --- a/include/xrpl/telemetry/CoroAwareContextStorage.h +++ b/include/xrpl/telemetry/CoroAwareContextStorage.h @@ -31,9 +31,10 @@ * | (swapped by Coro) | * +------------------------+ * - * Install once at telemetry start via - * opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(), BEFORE - * any span is created (SDK requirement). + * Install once from main() via + * opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(), before + * any thread is started. That call writes a non-atomic process-global + * shared_ptr which every log line reads back, so a later install would race. * * @note Thread-safety: each thread/coroutine sees its own LocalValue store, so * the stack is never shared across threads — no locking needed. The storage @@ -44,7 +45,7 @@ * the coroutine's store — not a pattern here (spans are created inside their * own coro/job body). * - * Example 1 — install at telemetry start (primary use): + * Example 1 — install from main(), before any thread (primary use): * @code * using opentelemetry::context::RuntimeContext; * RuntimeContext::SetRuntimeContextStorage( diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 97ecf11b4d..4f3e17285f 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -22,14 +22,12 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include #include @@ -316,13 +314,6 @@ class TelemetryImpl : public Telemetry */ std::shared_ptr meterProvider_; - /** - * Coroutine-aware runtime-context storage, installed globally so the OTel - * ambient context follows JobQueue coroutines. Held for the process - * lifetime because it must outlive every span (SDK requirement). - */ - opentelemetry::nostd::shared_ptr contextStorage_; - /** * Set by stop(), so a second call does nothing. */ @@ -541,17 +532,10 @@ public: std::move(sampler), std::make_unique()); - // Install coroutine-aware context storage BEFORE any span is created - // so the OTel ambient context follows JobQueue coroutines across - // yield/resume (fixes wrong-thread scope pop; keeps log-trace - // correlation). Must precede SetTracerProvider and the first span. - // Not reset in stop(): resetting the storage while spans may still - // exist is undefined behaviour (SDK), and by stop() all spans are - // gone, so the storage is simply left installed for process lifetime. - contextStorage_ = - opentelemetry::nostd::shared_ptr( - new CoroAwareContextStorage()); - opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(contextStorage_); + // main() installs the coroutine-aware runtime-context storage while the + // process is single-threaded. It cannot be installed here: start() runs + // from setup(), by which point the io threads read that global pointer + // on every log line. // Set as global provider trace_api::Provider::SetTracerProvider( diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index d72fbf5c77..76c7e6621a 100644 --- a/src/xrpld/app/main/Main.cpp +++ b/src/xrpld/app/main/Main.cpp @@ -32,6 +32,13 @@ #include #include +#ifdef XRPL_ENABLE_TELEMETRY +#include + +#include +#include +#endif // XRPL_ENABLE_TELEMETRY + #include #include #include @@ -822,6 +829,36 @@ run(int argc, char** argv) return -1; } +#ifdef XRPL_ENABLE_TELEMETRY + // Install the coroutine-aware OTel context storage while the process is + // still single-threaded. SetRuntimeContextStorage() writes a + // process-global shared_ptr that every log line reads through + // RuntimeContext::GetCurrent(), and neither side is atomic; the io + // threads start inside makeApplication() below. + // + // Only the one key is read here, not the whole section: parsing it all + // can throw on a contradictory TLS combination, and that error belongs + // to the handler below, where it reports today. Read as int to match + // makeTelemetrySetup(), which treats any non-zero value as on. A value + // that will not convert throws, so install and leave that same parser + // to report it. + bool telemetryEnabled = true; + try + { + telemetryEnabled = config->section("telemetry").valueOr("enabled", 0) != 0; + } + catch (...) + { + } + + if (telemetryEnabled) + { + opentelemetry::context::RuntimeContext::SetRuntimeContextStorage( + opentelemetry::nostd::shared_ptr( + new telemetry::CoroAwareContextStorage())); + } +#endif // XRPL_ENABLE_TELEMETRY + // Application construction runs member initializers that validate // config (for example the [telemetry] section) and can throw. A throw // from a member-initializer list cannot be recovered inside the