diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 7844652a87..c5c25cea75 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -585,8 +585,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 ba92196349..95179dba85 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -228,8 +228,6 @@ compile unchanged either way. > **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 / Base Configuration diff --git a/OpenTelemetryPlan/08-appendix.md b/OpenTelemetryPlan/08-appendix.md index e059cfcf7f..de22c388f2 100644 --- a/OpenTelemetryPlan/08-appendix.md +++ b/OpenTelemetryPlan/08-appendix.md @@ -117,19 +117,18 @@ The full span inventory (names, attributes, parents as instrumented) is in ### 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) | -| [09-data-collection-reference.md](./09-data-collection-reference.md) | Span/metric/dashboard inventory | +| 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 | +| [09-data-collection-reference.md](./09-data-collection-reference.md) | Span/metric/dashboard inventory | ### Task Lists diff --git a/OpenTelemetryPlan/OpenTelemetryPlan.md b/OpenTelemetryPlan/OpenTelemetryPlan.md index e914e8062c..223ec5181f 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"] dataref["09-data-collection-reference.md"] end @@ -69,7 +68,6 @@ flowchart TB config --> phases phases --> backends backends --> appendix - backends --> secure appendix --> dataref style overview fill:#1b5e20,stroke:#0d3d14,color:#fff,stroke-width:2px @@ -85,7 +83,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 style dataref fill:#4a148c,stroke:#2e0d57,color:#fff ``` @@ -106,7 +103,6 @@ flowchart TB | **7** | [Observability Backends](./07-observability-backends.md) | Backend selection guide and production architecture | | **8** | [Appendix](./08-appendix.md) | Glossary, references, version history | | **9** | [Data Collection Reference](./09-data-collection-reference.md) | Complete inventory of spans, attributes, metrics, and dashboards | -| **Sec** | [Securing the OTel Pipeline](./secure-OTel.md) | Threat model and hardening (mTLS, peer trace-context validation) | > Note there is no document 4: `04-code-samples.md` was removed during the > rollout, and the numbering was left as-is rather than renumbering every @@ -240,12 +236,4 @@ A single-source-of-truth reference documenting every piece of telemetry data col --- -## 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 diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index cb0bb5b605..d0736e1a6f 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. # diff --git a/docker/telemetry/grafana/provisioning/datasources/tempo.yaml b/docker/telemetry/grafana/provisioning/datasources/tempo.yaml index 3b9862f722..261899d053 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. # Transaction hash, local/peer origin, status. # Consensus mode, round, ledger sequence, close time. @@ -116,6 +117,41 @@ 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 # Transaction tracing filters - id: tx-hash tag: tx_hash diff --git a/docs/build/telemetry.md b/docs/build/telemetry.md index 134cca5f63..7cd0af58c1 100644 --- a/docs/build/telemetry.md +++ b/docs/build/telemetry.md @@ -234,11 +234,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/include/xrpl/telemetry/ValidationTracker.h b/include/xrpl/telemetry/ValidationTracker.h index ed6e9ed975..91284ed67b 100644 --- a/include/xrpl/telemetry/ValidationTracker.h +++ b/include/xrpl/telemetry/ValidationTracker.h @@ -575,6 +575,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/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 diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index ae5aac6de8..edae2ae6ed 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. */ @@ -352,6 +343,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"; + } } /** @@ -421,17 +422,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/libxrpl/telemetry/detail/ValidationTracker.cpp b/src/libxrpl/telemetry/detail/ValidationTracker.cpp index 9f334a4fcb..b6655c5ece 100644 --- a/src/libxrpl/telemetry/detail/ValidationTracker.cpp +++ b/src/libxrpl/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); // The gross pair records this first classification and is left // alone by the repair branch below, so each only ever rises. @@ -124,6 +136,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();) 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/tests/libxrpl/beast/insight/StatsDCollector.cpp b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp index aaab3e571e..af4b9a3ce5 100644 --- a/src/tests/libxrpl/beast/insight/StatsDCollector.cpp +++ b/src/tests/libxrpl/beast/insight/StatsDCollector.cpp @@ -73,6 +73,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. @@ -80,17 +83,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: @@ -109,6 +117,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_; }; /** @@ -140,6 +154,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) { @@ -148,12 +166,18 @@ TEST(StatsDCollector, UntouchedCounterPublishesNothing) auto collector = StatsDCollector::make(address, "test", Journal(Journal::getNullSink())); auto const counter = collector->makeCounter("untouched"); + auto const control = collector->makeGauge("control"); // Same reason as above: polling must be on, or the empty result proves only // that nothing was polled. collector->onCollectionReady(); - // 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()); } 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); } diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 753188da4a..9f266c4c2a 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -27,6 +27,7 @@ #ifdef XRPL_ENABLE_TELEMETRY #include +#include #include #include #include @@ -521,9 +522,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(); @@ -553,11 +560,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); } diff --git a/src/tests/libxrpl/telemetry/ValidationTracker.cpp b/src/tests/libxrpl/telemetry/ValidationTracker.cpp index 5ba3d24c98..8b78743a1e 100644 --- a/src/tests/libxrpl/telemetry/ValidationTracker.cpp +++ b/src/tests/libxrpl/telemetry/ValidationTracker.cpp @@ -474,6 +474,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/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index 08c323e711..c2c7d10e7d 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -282,15 +282,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); } @@ -1116,18 +1117,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: diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index bb42bbec04..981b97ca80 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 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/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index fc58d81af8..1efcea372d 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,23 +226,33 @@ 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); - span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str()); - // Status::operator bool() returns true when there IS an error - // (code_ != OK), so the ternary correctly maps error->error, ok->success. - 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) + // 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) { - span.setError(rpc_span::val::error); - } - else - { - span.setOk(); + // Read after the handler ran, because a handler may raise its own + // load type (pathfind charges a heavy burden). + span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str()); + // 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; } @@ -234,32 +280,37 @@ 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 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) { - 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} diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index bf3193f8f5..b18c27bdcc 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -409,6 +409,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); 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