From 38f6afbf4c48ba90e45e52b7abee0e7cc027a7e2 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 6 Jul 2026 20:40:12 +0100 Subject: [PATCH] code review comments Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- .../scripts/levelization/results/loops.txt | 3 -- .../scripts/levelization/results/ordering.txt | 1 + OpenTelemetryPlan/Phase3_taskList.md | 50 +++++++++++-------- OpenTelemetryPlan/secure-OTel.md | 19 +++---- include/xrpl/tx/detail/TxApplySpanNames.h | 11 ++-- src/libxrpl/tx/Transactor.cpp | 16 ++++-- src/libxrpl/tx/applySteps.cpp | 4 ++ .../telemetry/TraceContextPropagator.cpp | 18 +++---- src/xrpld/app/misc/NetworkOPs.cpp | 2 +- src/xrpld/app/misc/detail/TxQ.cpp | 9 ++-- src/xrpld/overlay/detail/PeerImp.cpp | 2 +- .../{app/misc => telemetry}/TxSpanNames.h | 0 src/xrpld/telemetry/TxTracing.h | 2 +- 13 files changed, 79 insertions(+), 58 deletions(-) rename src/xrpld/{app/misc => telemetry}/TxSpanNames.h (100%) diff --git a/.github/scripts/levelization/results/loops.txt b/.github/scripts/levelization/results/loops.txt index 852a99f202..a1b2bcd92e 100644 --- a/.github/scripts/levelization/results/loops.txt +++ b/.github/scripts/levelization/results/loops.txt @@ -13,9 +13,6 @@ Loop: xrpld.app xrpld.rpc Loop: xrpld.app xrpld.shamap xrpld.shamap > xrpld.app -Loop: xrpld.app xrpld.telemetry - xrpld.telemetry ~= xrpld.app - Loop: xrpld.overlay xrpld.rpc xrpld.rpc ~= xrpld.overlay diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 13ae5a6999..257b0218c4 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -261,6 +261,7 @@ xrpld.app > xrpl.config xrpld.app > xrpl.core xrpld.app > xrpld.consensus xrpld.app > xrpld.core +xrpld.app > xrpld.telemetry xrpld.app > xrpl.json xrpld.app > xrpl.ledger xrpld.app > xrpl.net diff --git a/OpenTelemetryPlan/Phase3_taskList.md b/OpenTelemetryPlan/Phase3_taskList.md index d765b8655d..79bca77bcc 100644 --- a/OpenTelemetryPlan/Phase3_taskList.md +++ b/OpenTelemetryPlan/Phase3_taskList.md @@ -166,7 +166,8 @@ ## Task 3.6: Context Propagation in Transaction Relay -**Status**: COMPLETE +**Status**: COMPLETE (transaction relay). Consensus proposal/validation +propagation is deferred to Phase 4 — see "Planned (Phase 4)" below. **Objective**: Ensure trace context flows correctly when transactions are relayed between peers, creating linked spans across nodes. @@ -178,17 +179,6 @@ wired in PeerImp) extracts the parent span_id and creates the tx.receive span as a child of the sender's tx.process span. -- **Proposal send/receive**: `RCLConsensus::Adaptor::propose()` injects the - current thread's active span context into the `TMProposeSet` protobuf via - `telemetry::injectToProtobuf()`. PeerImp creates a - `consensus.proposal.receive` span that extracts the sender's trace context - as parent (via `ConsensusReceiveTracing.h`). - -- **Validation send/receive**: `RCLConsensus::Adaptor::validate()` injects - the current thread's active span context into the `TMValidation` protobuf. - PeerImp creates a `consensus.validation.receive` span that extracts the - sender's trace context as parent. - - **Edge cases**: Missing trace context (older peers) degrades gracefully to standalone spans. Invalid/corrupted context is treated as absent. Trace flags are propagated and respected. @@ -200,20 +190,36 @@ - `PropagationHelpers.h` — `injectSpanContext(SpanGuard&, proto)` bridge between SpanGuard and protobuf TraceContext. - `TraceContextPropagator.h` — `injectToProtobuf(ctx, proto)` for - same-thread injection via OTel RuntimeContext (used in propose/validate). -- `ConsensusReceiveTracing.h` — `proposalReceiveSpan()` and - `validationReceiveSpan()` helper functions that create receive spans with - optional parent context extraction from incoming protobuf messages. + same-thread injection via OTel RuntimeContext. **Key modified files**: - `src/xrpld/app/misc/NetworkOPs.cpp` — tx relay injection -- `src/xrpld/app/consensus/RCLConsensus.cpp` — proposal/validation send injection -- `src/xrpld/overlay/detail/PeerImp.cpp` — proposal/validation receive spans - `include/xrpl/telemetry/SpanGuard.h` — `TraceBytes` struct, `getTraceBytes()` - `src/libxrpl/telemetry/SpanGuard.cpp` — `getTraceBytes()` implementation - `src/xrpld/telemetry/PropagationHelpers.h` — inject helpers (new file) -- `src/xrpld/telemetry/ConsensusReceiveTracing.h` — receive span helpers (new file) + +**Planned (Phase 4 — not in this PR)**: + +The consensus proposal/validation propagation below is Phase 4 scope and is +not implemented on this branch. It is listed here only to record the intended +design. + +- **Proposal send/receive**: `RCLConsensus::Adaptor::propose()` injects the + current thread's active span context into the `TMProposeSet` protobuf via + `telemetry::injectToProtobuf()`. PeerImp creates a + `consensus.proposal.receive` span that extracts the sender's trace context + as parent (via `ConsensusReceiveTracing.h`). + +- **Validation send/receive**: `RCLConsensus::Adaptor::validate()` injects + the current thread's active span context into the `TMValidation` protobuf. + PeerImp creates a `consensus.validation.receive` span that extracts the + sender's trace context as parent. + +- Planned files: `src/xrpld/app/consensus/RCLConsensus.cpp` (send injection), + `src/xrpld/overlay/detail/PeerImp.cpp` (receive spans), + `src/xrpld/telemetry/ConsensusReceiveTracing.h` (receive span helpers, + new file). **Reference**: @@ -396,7 +402,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ - `src/xrpld/overlay/detail/PeerImp.cpp` — restructured span creation - `src/xrpld/app/misc/NetworkOPs.cpp` — deterministic context for tx.process -- `src/xrpld/app/misc/TxSpanNames.h` — new `traceStrategy` attribute constant +- `src/xrpld/telemetry/TxSpanNames.h` — new `traceStrategy` attribute constant - New or shared utility for `createDeterministicTxContext()` (location TBD: could be a shared header like `include/xrpl/telemetry/DeterministicContext.h`, or file-local if only used in two places) @@ -431,7 +437,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ - `txSpan(category, group, name, txHash, parentCtx)` — child span (deterministic trace_id combined with protobuf-extracted parent span_id for relay ordering). -- **`TxTracing.h` helper functions** (`src/xrpld/overlay/detail/TxTracing.h`): +- **`TxTracing.h` helper functions** (`src/xrpld/telemetry/TxTracing.h`): File-local helpers that wrap `SpanGuard::txSpan()` for the two main PeerImp call sites: - `txReceiveSpan(txHash, parentCtx)` — creates `tx.receive` span with @@ -495,7 +501,7 @@ This gives the best of both worlds: guaranteed cross-node correlation via determ **Modified files**: -- `src/xrpld/app/misc/TxSpanNames.h` +- `src/xrpld/telemetry/TxSpanNames.h` - `src/xrpld/app/misc/detail/TxQSpanNames.h` - `src/xrpld/app/misc/NetworkOPs.cpp` - `src/xrpld/overlay/detail/PeerImp.cpp` diff --git a/OpenTelemetryPlan/secure-OTel.md b/OpenTelemetryPlan/secure-OTel.md index c570a06ecf..3d22de5452 100644 --- a/OpenTelemetryPlan/secure-OTel.md +++ b/OpenTelemetryPlan/secure-OTel.md @@ -20,7 +20,7 @@ The most effective way to prevent spoofing from external sources is to treat you server { listen 80; - location { + location / { # Clear out untrusted incoming trace headers proxy_set_header traceparent ""; proxy_set_header tracestate ""; @@ -92,10 +92,11 @@ receivers: 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 - auth_type: require_and_verify_client_cert # Rejects unauthorized clients ``` ### **Approach C: Application Layer Authentication (Basic Auth Extension)** @@ -175,7 +176,7 @@ xrpld has **two distinct attack surfaces**, not one. The original guide conflate │ │ OTLP/gRPC │ │ + mTLS │ ▼ - └───────────────────────────────────────── [require_and_verify_client_cert] + └───────────────────────────────────────── [client_ca_file: verify client cert] OTel Collector (in private subnet, NetPol) ``` @@ -196,15 +197,15 @@ The guide's NGINX header stripping and OTTL stale-span filtering target HTTP gat 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; collector verifies with `require_and_verify_client_cert`. 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.** | +| 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, with `auth_type: require_and_verify_client_cert`. +**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. diff --git a/include/xrpl/tx/detail/TxApplySpanNames.h b/include/xrpl/tx/detail/TxApplySpanNames.h index c007ca3c03..2084458cf5 100644 --- a/include/xrpl/tx/detail/TxApplySpanNames.h +++ b/include/xrpl/tx/detail/TxApplySpanNames.h @@ -51,9 +51,11 @@ * @endcode * * @code - * // Transactor::operator() uses span() with prefix + suffix: - * auto span = SpanGuard::span( - * TraceCategory::Transactions, seg::tx, tx_apply_span::op::transactor); + * // Transactor::operator() also uses hashSpan on the same txID so it + * // co-traces with preflight and preclaim under one trace_id: + * auto span = SpanGuard::hashSpan( + * TraceCategory::Transactions, tx_apply_span::transactor, + * txID.data(), txID.kBytes); * span.setAttribute(tx_apply_span::attr::stage, tx_apply_span::val::apply); * @endcode */ @@ -79,6 +81,9 @@ inline constexpr auto transactor = makeStr("transactor"); inline constexpr auto preflight = join(seg::tx, op::preflight); /// "tx.preclaim" — full name for hashSpan() at the preclaim stage. inline constexpr auto preclaim = join(seg::tx, op::preclaim); +/// "tx.transactor" — full name for hashSpan() at the apply stage. Shares the +/// txID-derived trace_id so it co-traces with tx.preflight and tx.preclaim. +inline constexpr auto transactor = join(seg::tx, op::transactor); // ===== Attribute keys ====================================================== diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 0c33995db2..e2752ba67b 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1294,10 +1294,15 @@ Transactor::checkInvariants(TER result, XRPAmount fee) ApplyResult Transactor::operator()() { - auto span = telemetry::SpanGuard::span( + // Derive the trace_id from the transaction id so this apply-stage span + // shares one trace with the preflight and preclaim spans (which also use + // hashSpan on the same id). + auto const txID = ctx_.tx.getTransactionID(); + auto span = telemetry::SpanGuard::hashSpan( telemetry::TraceCategory::Transactions, - telemetry::seg::tx, - telemetry::tx_apply_span::op::transactor); + telemetry::tx_apply_span::transactor, + txID.data(), + txID.kBytes); // "apply" — the third apply-pipeline stage, after preflight and preclaim. span.setAttribute(telemetry::tx_apply_span::attr::stage, telemetry::tx_apply_span::val::apply); if (auto const* fmt = TxFormats::getInstance().findByType(ctx_.tx.getTxnType())) @@ -1428,6 +1433,11 @@ Transactor::operator()() span.setAttribute(telemetry::tx_apply_span::attr::terResult, transToken(result).c_str()); span.setAttribute(telemetry::tx_apply_span::attr::applied, applied); + // Mark the span as errored when the transaction was not applied or the + // engine result is not a success, so failed applies surface in span-status + // error counts alongside preflight and preclaim. + if (!applied || !isTesSuccess(result)) + span.setError(transToken(result)); return {result, applied, metadata}; } diff --git a/src/libxrpl/tx/applySteps.cpp b/src/libxrpl/tx/applySteps.cpp index 77c39d6f49..8a2c97c550 100644 --- a/src/libxrpl/tx/applySteps.cpp +++ b/src/libxrpl/tx/applySteps.cpp @@ -190,6 +190,10 @@ invokePreflight(PreflightContext const& ctx) { span.setAttribute( telemetry::tx_apply_span::attr::terResult, transToken(result.first).c_str()); + // Mark the span as errored when preflight rejects the transaction so + // failed stages surface in span-status error counts. + if (!isTesSuccess(result.first)) + span.setError(transToken(result.first)); } return result; } diff --git a/src/tests/libxrpl/telemetry/TraceContextPropagator.cpp b/src/tests/libxrpl/telemetry/TraceContextPropagator.cpp index 67b4428b75..8755284630 100644 --- a/src/tests/libxrpl/telemetry/TraceContextPropagator.cpp +++ b/src/tests/libxrpl/telemetry/TraceContextPropagator.cpp @@ -81,10 +81,8 @@ TEST(TraceContextPropagator, extract_empty_protobuf) protocol::TraceContext const proto; auto ctx = xrpl::telemetry::extractFromProtobuf(proto); auto span = trace::GetSpan(ctx); - if (span) - { - EXPECT_FALSE(span->GetContext().IsValid()); - } + ASSERT_NE(span, nullptr); + EXPECT_FALSE(span->GetContext().IsValid()); } TEST(TraceContextPropagator, extract_wrong_size_trace_id) @@ -95,10 +93,8 @@ TEST(TraceContextPropagator, extract_wrong_size_trace_id) auto ctx = xrpl::telemetry::extractFromProtobuf(proto); auto span = trace::GetSpan(ctx); - if (span) - { - EXPECT_FALSE(span->GetContext().IsValid()); - } + ASSERT_NE(span, nullptr); + EXPECT_FALSE(span->GetContext().IsValid()); } TEST(TraceContextPropagator, extract_wrong_size_span_id) @@ -109,10 +105,8 @@ TEST(TraceContextPropagator, extract_wrong_size_span_id) auto ctx = xrpl::telemetry::extractFromProtobuf(proto); auto span = trace::GetSpan(ctx); - if (span) - { - EXPECT_FALSE(span->GetContext().IsValid()); - } + ASSERT_NE(span, nullptr); + EXPECT_FALSE(span->GetContext().IsValid()); } TEST(TraceContextPropagator, inject_invalid_span) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 20e4d3758c..3dd384d605 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -17,7 +17,6 @@ #include #include #include -#include #include #include #include @@ -35,6 +34,7 @@ #include #include #include +#include #include #include diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index 124a115556..2e0b37f291 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -1449,9 +1449,6 @@ bool TxQ::accept(Application& app, OpenView& view) { using namespace telemetry; - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); - span.setAttribute(txq_span::attr::queueSize, static_cast(byFee_.size())); /* Move transactions from the queue from largest fee level to smallest. As we add more transactions, the required fee level will increase. @@ -1463,6 +1460,12 @@ TxQ::accept(Application& app, OpenView& view) std::scoped_lock const lock(mutex_); + // Create the span and read byFee_.size() only after taking the lock, since + // byFee_ is guarded by mutex_. + auto span = + SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); + span.setAttribute(txq_span::attr::queueSize, static_cast(byFee_.size())); + auto const metricsSnapshot = feeMetrics_.getSnapshot(); for (auto candidateIter = byFee_.begin(); candidateIter != byFee_.end();) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 57cf163564..ed1d2b5329 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include #include @@ -22,6 +21,7 @@ #include #include #include +#include #include #include diff --git a/src/xrpld/app/misc/TxSpanNames.h b/src/xrpld/telemetry/TxSpanNames.h similarity index 100% rename from src/xrpld/app/misc/TxSpanNames.h rename to src/xrpld/telemetry/TxSpanNames.h diff --git a/src/xrpld/telemetry/TxTracing.h b/src/xrpld/telemetry/TxTracing.h index 47bb30eb54..066c018601 100644 --- a/src/xrpld/telemetry/TxTracing.h +++ b/src/xrpld/telemetry/TxTracing.h @@ -10,7 +10,7 @@ * no-op SpanGuard instances (zero overhead, zero dependencies). */ -#include +#include #include #include