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/10] 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 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 02/10] 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 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 03/10] 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 04/10] 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 05/10] 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 06/10] 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 07/10] 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 08/10] 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 09/10] 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 10/10] 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: