From 46dbc92b5fb561cd01535740392cdf71074d0c0d Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 12 Jun 2026 20:18:12 +0100 Subject: [PATCH] code review comments addressed Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- .../03-implementation-strategy.md | 34 ++++++++++----- .../05-configuration-reference.md | 15 ++++--- .../07-observability-backends.md | 9 ++-- OpenTelemetryPlan/OpenTelemetryPlan.md | 12 +++--- OpenTelemetryPlan/presentation.md | 43 +++++++++++-------- 5 files changed, 69 insertions(+), 44 deletions(-) diff --git a/OpenTelemetryPlan/03-implementation-strategy.md b/OpenTelemetryPlan/03-implementation-strategy.md index 697e8af019..d51e30b211 100644 --- a/OpenTelemetryPlan/03-implementation-strategy.md +++ b/OpenTelemetryPlan/03-implementation-strategy.md @@ -233,6 +233,14 @@ xychart-beta - Maximum memory is bounded: ~8.3 MB static (dominated by worker thread stack) + 2048 queued spans x ~500 bytes (~1 MB) + active spans (~0.8 MB) ≈ **~10 MB ceiling** - The worker thread stack (~8 MB) is virtual memory; actual RSS depends on stack usage (typically much less) +> **Measured outcome**: A perf-iac comparison (telemetry compiled-in + enabled vs compiled-out, +> 9 nodes — validators and client-handlers — under sustained payment load) recorded **no measurable +> RSS increase over the telemetry-off baseline** (~15 GiB mean / ~18–19 GiB peak on both sides), +> with no OOM, no swap, and no leak across the run. The ~10 MB ceiling above is therefore a +> provisioning safety margin (dominated by virtual thread-stack address space), not an expected +> resident-memory increase. Steady-state cost shows up as throughput (~3–4% at head sampling 1.0), +> not memory. + ### 3.5.4 Performance Data Sources The overhead estimates in Sections 3.3-3.5 are derived from the following sources: @@ -335,7 +343,7 @@ This section provides a detailed assessment of how intrusive the OpenTelemetry i | Component | Files Modified | Lines Added | Lines Changed | Architectural Impact | | --------------------- | -------------- | ----------- | ------------- | -------------------- | -| **Core Telemetry** | 5 new files | ~800 | 0 | None (new module) | +| **Core Telemetry** | 11 new files | ~980 | 0 | None (new module) | | **Application Init** | 2 files | ~30 | ~5 | Minimal | | **RPC Layer** | 3 files | ~80 | ~20 | Minimal | | **Transaction Relay** | 4 files | ~120 | ~40 | Low | @@ -345,7 +353,7 @@ This section provides a detailed assessment of how intrusive the OpenTelemetry i | **PathFinding** | 2 | ~80 | ~5 | Minimal | | **TxQ/Fee** | 2 | ~60 | ~5 | Minimal | | **Validator/Amend** | 3 | ~40 | ~5 | Minimal | -| **Total** | **~28 files** | **~1,490** | **~120** | **Low** | +| **Total** | **~34 files** | **~1,670** | **~120** | **Low** | ### 3.9.2 Detailed File Impact @@ -365,15 +373,19 @@ pie title Code Changes by Component #### New Files (No Impact on Existing Code) -| File | Lines | Purpose | -| ---------------------------------------------- | ----- | -------------------- | -| `include/xrpl/telemetry/Telemetry.h` | ~160 | Main interface | -| `include/xrpl/telemetry/SpanGuard.h` | ~120 | RAII wrapper | -| `include/xrpl/telemetry/TraceContext.h` | ~80 | Context propagation | -| `src/xrpld/telemetry/TracingInstrumentation.h` | ~60 | Macros | -| `src/libxrpl/telemetry/Telemetry.cpp` | ~200 | Implementation | -| `src/libxrpl/telemetry/TelemetryConfig.cpp` | ~60 | Config parsing | -| `src/libxrpl/telemetry/NullTelemetry.cpp` | ~40 | No-op implementation | +| File | Lines | Purpose | +| ------------------------------------------------ | ----- | ------------------------ | +| `include/xrpl/telemetry/Telemetry.h` | ~160 | Main interface | +| `include/xrpl/telemetry/TelemetryConfig.h` | ~80 | Configuration structures | +| `include/xrpl/telemetry/TraceContext.h` | ~80 | Context propagation | +| `include/xrpl/telemetry/SpanGuard.h` | ~120 | RAII wrapper | +| `include/xrpl/telemetry/SpanAttributes.h` | ~60 | Attribute helpers | +| `src/libxrpl/telemetry/Telemetry.cpp` | ~200 | Implementation | +| `src/libxrpl/telemetry/TelemetryConfig.cpp` | ~60 | Config parsing | +| `src/libxrpl/telemetry/TraceContext.cpp` | ~80 | Context serialization | +| `src/libxrpl/telemetry/NullTelemetry.cpp` | ~40 | No-op implementation | +| `src/xrpld/telemetry/TracingInstrumentation.h` | ~60 | Macros | +| `src/xrpld/telemetry/TracingInstrumentation.cpp` | ~40 | Instrumentation impl | #### Modified Files (Existing Xrpld Code) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 2eeca72dcb..c08d0510c9 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -36,13 +36,14 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme documents but are not parsed by `TelemetryConfig.cpp` in Phase 1b and later phases. They will be added as the corresponding subsystems are instrumented: -| Option | Planned Phase | Purpose | -| ----------------- | ------------- | ---------------------------------------- | -| `exporter` | Future | Select between OTLP/HTTP and OTLP/gRPC | -| `trace_pathfind` | Phase 2 | Path computation tracing toggle | -| `trace_txq` | Phase 3 | Transaction queue tracing toggle | -| `trace_validator` | Future | Validator list / manifest update tracing | -| `trace_amendment` | Future | Amendment voting tracing | +| Option | Planned Phase | Purpose | +| -------------------------- | ------------- | ----------------------------------------------------------------------- | +| `exporter` | Future | Select between OTLP/HTTP and OTLP/gRPC | +| `trace_pathfind` | Phase 2 | Path computation tracing toggle | +| `trace_txq` | Phase 3 | Transaction queue tracing toggle | +| `trace_validator` | Future | Validator list / manifest update tracing | +| `trace_amendment` | Future | Amendment voting tracing | +| `consensus_trace_strategy` | Phase 4 | Trace ID strategy for consensus rounds (`deterministic` \| `attribute`) | --- diff --git a/OpenTelemetryPlan/07-observability-backends.md b/OpenTelemetryPlan/07-observability-backends.md index a8bd897723..d397fcb69d 100644 --- a/OpenTelemetryPlan/07-observability-backends.md +++ b/OpenTelemetryPlan/07-observability-backends.md @@ -258,10 +258,13 @@ Grafana provisions three TraceQL-based alert rules (group `xrpld-tracing-alerts` {resource.service.name="xrpld" && name="consensus.round"} | avg(duration) > 5s ``` -- **RPC Error Rate Spike** (critical, `for: 2m`): fires when the error rate across `rpc.command.*` spans exceeds 5%. +- **RPC Error Rate Spike** (critical, `for: 2m`): fires when the error rate across `rpc.command.*` spans exceeds 5%. Error _rate_ is a ratio, so it must divide the error-span rate by the total-span rate — a single TraceQL `rate()` returns spans/second, not a percentage, and would fire on traffic volume alone. This uses span metrics emitted by the collector's `spanmetrics` connector (Prometheus datasource), not a TraceQL query: ``` - {resource.service.name="xrpld" && name=~"rpc.command.*" && status.code=error} | rate() > 0.05 + sum(rate(calls_total{service_name="xrpld", span_name=~"rpc.command.*", status_code="STATUS_CODE_ERROR"}[5m])) + / + sum(rate(calls_total{service_name="xrpld", span_name=~"rpc.command.*"}[5m])) + > 0.05 ``` - **Transaction Throughput Drop** (warning, `for: 10m`): fires when the `tx.receive` span rate falls below 10/s. @@ -270,7 +273,7 @@ Grafana provisions three TraceQL-based alert rules (group `xrpld-tracing-alerts` {resource.service.name="xrpld" && name="tx.receive"} | rate() < 10 ``` -> **Note**: The first two rules use TraceQL aggregates (`avg(duration)`, `rate()`), which require Tempo 2.3+ with TraceQL metrics enabled. Verify aggregate query support in your Tempo version before provisioning. +> **Note**: The Consensus Round Slow and Transaction Throughput Drop rules use TraceQL aggregates (`avg(duration)`, `rate()`), which require Tempo 2.3+ with TraceQL metrics enabled. Verify aggregate query support in your Tempo version before provisioning. The RPC Error Rate Spike rule instead queries Prometheus span metrics (collector `spanmetrics` connector), so it needs that connector enabled in the collector pipeline. --- diff --git a/OpenTelemetryPlan/OpenTelemetryPlan.md b/OpenTelemetryPlan/OpenTelemetryPlan.md index 2836070485..eec093e3e0 100644 --- a/OpenTelemetryPlan/OpenTelemetryPlan.md +++ b/OpenTelemetryPlan/OpenTelemetryPlan.md @@ -16,12 +16,12 @@ This document provides a comprehensive implementation plan for integrating OpenT ### Estimated Performance Overhead -| Metric | Overhead | Notes | -| ------------- | ---------- | ----------------------------------- | -| CPU | 1-3% | Span creation and attribute setting | -| Memory | 2-5 MB | Batch buffer for pending spans | -| Network | 10-50 KB/s | Compressed OTLP export to collector | -| Latency (p99) | <2% | With proper sampling configuration | +| Metric | Overhead | Notes | +| ------------- | ---------- | ------------------------------------------------ | +| CPU | 1-3% | Span creation and attribute setting | +| Memory | <10 MB | SDK statics + batch buffer + worker thread stack | +| Network | 10-50 KB/s | Compressed OTLP export to collector | +| Latency (p99) | <2% | With proper sampling configuration | --- diff --git a/OpenTelemetryPlan/presentation.md b/OpenTelemetryPlan/presentation.md index 535807a96e..bc83f82fc2 100644 --- a/OpenTelemetryPlan/presentation.md +++ b/OpenTelemetryPlan/presentation.md @@ -222,7 +222,7 @@ flowchart TB services --> Telemetry end - Telemetry -->|OTLP/gRPC| Collector["OTel Collector"] + Telemetry -->|OTLP/HTTP| Collector["OTel Collector"] Collector --> Tempo["Grafana Tempo"] Collector --> Elastic["Elastic APM"] @@ -237,7 +237,7 @@ flowchart TB - **Core Services (blue, top)**: RPC Server, Overlay, and Consensus are the three primary components that generate trace data — they represent the entry points for client requests, peer messages, and consensus rounds respectively. - **Telemetry Module (green, middle)**: The OpenTelemetry SDK sits below the core services and receives span data from all three; it acts as a single collection point within the xrpld process. -- **OTel Collector (orange, center)**: An external process that receives spans over OTLP/gRPC from the Telemetry Module; it decouples xrpld from backend choices and handles batching, sampling, and routing. +- **OTel Collector (orange, center)**: An external process that receives spans over OTLP/HTTP from the Telemetry Module; it decouples xrpld from backend choices and handles batching, sampling, and routing. - **Backends (bottom row)**: Tempo and Elastic APM are interchangeable — the Collector fans out to any combination, so operators can switch backends without modifying xrpld code. - **Top-to-bottom flow**: Data flows from instrumented code down through the SDK, out over the network to the Collector, and finally into storage/visualization backends. @@ -373,6 +373,13 @@ Per-transaction tracing cost breakdown: > recordable (~250 bytes base), and `std::map`-based attribute storage (~200-500 bytes for 3-5 > string attributes). See [Section 3.5.4](./03-implementation-strategy.md#354-performance-data-sources) for source references. +> **Measured (perf-iac, telemetry on vs off, 9 nodes under payment load)**: the ~10 MB +> above is a theoretical SDK-footprint ceiling, dominated by virtual (not resident) thread-stack +> memory. In practice, per-node RSS showed **no measurable increase over the telemetry-off +> baseline** (~15 GiB mean / ~18–19 GiB peak on both sides), with no OOM, swap, or leak over the +> run. Treat memory overhead as negligible; the ceiling is a provisioning safety margin, not an +> expected increase. + **Network (10-50 KB/s) — Calculation**: Two sources of network overhead: @@ -457,7 +464,7 @@ flowchart LR ```mermaid flowchart LR - A["Head Sampling
10% default"] --> B["Tail Sampling
Keep errors/slow"] --> C["Batch Export
Reduce I/O"] --> D["Conditional Compile
XRPL_ENABLE_TELEMETRY"] + A["Head Sampling
fixed 1.0 (record all)"] --> B["Tail Sampling
Keep errors/slow"] --> C["Batch Export
Reduce I/O"] --> D["Conditional Compile
XRPL_ENABLE_TELEMETRY"] style A fill:#1565c0,stroke:#0d47a1,color:#fff style B fill:#2e7d32,stroke:#1b5e20,color:#fff @@ -496,7 +503,7 @@ flowchart LR | Aspect | Details | | ----------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Where it runs** | Inside xrpld (SDK-level). Configured via `sampling_ratio` in `xrpld.cfg`. | +| **Where it runs** | Inside xrpld (SDK-level). In xrpld the ratio is fixed at 1.0 and not read from config (tail sampling in the collector needs every span) — the example below shows the general head-sampling mechanism. | | **When the decision happens** | At trace creation time — before the first span is even populated. | | **How it works** | `sampling_ratio=0.1` means each trace has a 10% probability of being recorded. Dropped traces incur near-zero overhead (no spans created, no attributes set, no export). | | **Propagation** | Once a trace is sampled, the `trace_flags` field (1 byte in the context header) tells downstream nodes to also sample it. Unsampled traces propagate `trace_flags=0`, so downstream nodes skip them too. | @@ -508,8 +515,10 @@ flowchart LR ```ini [telemetry] -# Record 10% of traces (recommended for production) -sampling_ratio=0.1 +# xrpld fixes head sampling at 1.0 (record every trace). This value is +# NOT read from config — the collector performs tail sampling instead, +# which needs all spans to arrive. See Slide 9 (tail sampling) and §7.4.2. +sampling_ratio=1.0 ``` ### Tail Sampling (Decision at End) @@ -574,16 +583,16 @@ processors: ### Head vs. Tail — Side-by-Side -| | Head Sampling | Tail Sampling | -| ----------------------------- | ---------------------------------------- | ------------------------------------------------ | -| **Decision point** | Trace start (inside xrpld) | Trace end (in OTel Collector) | -| **Knows trace content?** | No (random coin flip) | Yes (evaluates completed trace) | -| **Overhead on xrpld** | Lowest (dropped traces = no-op) | Higher (must export 100% to Collector) | -| **Collector resource usage** | Low (receives only sampled traces) | Higher (buffers all traces before deciding) | -| **Captures all errors?** | No (only if trace was randomly selected) | **Yes** (error policy catches them) | -| **Captures slow operations?** | No (random) | **Yes** (latency policy catches them) | -| **Configuration** | `xrpld.cfg`: `sampling_ratio=0.1` | `otel-collector.yaml`: `tail_sampling` processor | -| **Best for** | High-throughput steady-state | Troubleshooting & anomaly detection | +| | Head Sampling | Tail Sampling | +| ----------------------------- | ----------------------------------------- | ------------------------------------------------ | +| **Decision point** | Trace start (inside xrpld) | Trace end (in OTel Collector) | +| **Knows trace content?** | No (random coin flip) | Yes (evaluates completed trace) | +| **Overhead on xrpld** | Lowest (dropped traces = no-op) | Higher (must export 100% to Collector) | +| **Collector resource usage** | Low (receives only sampled traces) | Higher (buffers all traces before deciding) | +| **Captures all errors?** | No (only if trace was randomly selected) | **Yes** (error policy catches them) | +| **Captures slow operations?** | No (random) | **Yes** (latency policy catches them) | +| **Configuration** | Fixed at `1.0` in xrpld (not config-read) | `otel-collector.yaml`: `tail_sampling` processor | +| **Best for** | High-throughput steady-state | Troubleshooting & anomaly detection | ### Recommended Strategy for xrpld @@ -610,7 +619,7 @@ flowchart LR style storage fill:#2e7d32,stroke:#1b5e20,color:#fff ``` -> **Why this works**: xrpld exports everything (no blind drops), the Collector applies intelligent filtering (keep errors/slow/anomalies, sample the rest), and only ~15-20% of traces reach storage. If Collector resource usage becomes a concern, add head sampling at `sampling_ratio=0.5` to halve the export volume while still giving the Collector enough data for good tail-sampling decisions. +> **Why this works**: xrpld exports everything (no blind drops), the Collector applies intelligent filtering (keep errors/slow/anomalies, sample the rest), and only ~15-20% of traces reach storage. xrpld's head sampling is fixed at 1.0 and not configurable, because tail sampling can only see traces that reach the Collector — any head drop would blind the error/slow policies. To reduce volume, tune the Collector's tail-sampling rules rather than adding head sampling. ---