From fb827dc0f113eee3fc080a8b77d0c56306e505ce Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:39:05 +0100 Subject: [PATCH 1/4] fix(telemetry): make the consensus trace strategy an enum consensus_trace_strategy was read as a std::string and compared against the literal "attribute" in startRoundTracing(), while the runbook documented "deterministic" and "random". The documented value "random" therefore fell through to the default and did nothing. Parse the setting once into ConsensusTraceStrategy, so the consensus code branches on a type. The accepted spellings are now "deterministic" and "random"; anything else fails at startup instead of silently defaulting. The behaviour behind the old "attribute" name is unchanged and is now reached by "random". Document consensus_trace_strategy in xrpld-example.cfg, stating that "random" is experimental and not used: it gives each node its own trace id, so one round arrives as one trace per node. Also state on the tx.included event that it covers the agreed consensus set before the ledger is built, so it is a superset of the accepted ledger. --- OpenTelemetryPlan/02-design-decisions.md | 2 +- cfg/xrpld-example.cfg | 14 ++++ include/xrpl/consensus/ConsensusSpanNames.h | 4 +- include/xrpl/telemetry/Telemetry.h | 77 +++++++++++++++++-- src/libxrpl/telemetry/NullTelemetry.cpp | 2 +- src/libxrpl/telemetry/Telemetry.cpp | 4 +- src/libxrpl/telemetry/TelemetryConfig.cpp | 30 +++++++- .../libxrpl/telemetry/SpanGuardScope.cpp | 9 +-- .../libxrpl/telemetry/TelemetryConfig.cpp | 67 ++++++++++++++++ src/xrpld/app/consensus/RCLConsensus.cpp | 25 +++--- src/xrpld/app/consensus/RCLConsensus.h | 4 +- 11 files changed, 208 insertions(+), 30 deletions(-) diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index a9bb7a3c71..0bd1250f03 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -269,7 +269,7 @@ Establish-phase gap fill and cross-node correlation attributes (Phase 4a): | --------------------- | ------ | --------------------------------------------------------- | | `consensus_round_id` | int64 | Consensus round number | | `consensus_ledger_id` | string | `previousLedger.id()` — shared across nodes | -| `trace_strategy` | string | `"deterministic"` or `"attribute"` | +| `trace_strategy` | string | `"deterministic"` or `"random"` | | `converge_percent` | int64 | Convergence % (0-100+) | | `establish_count` | int64 | Number of establish iterations | | `disputes_count` | int64 | Active disputed transactions | diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index 52242c9cb6..974d525f60 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1775,6 +1775,20 @@ validators.txt # Enable tracing for ledger close and accept operations — ledger # building, state hashing, and write-back to the node store. Default: 1. # +# consensus_trace_strategy=deterministic +# +# How the consensus round span picks its trace id. Two values are +# accepted, and anything else makes xrpld fail to start. +# +# deterministic (the default, and the value to use): the trace id comes +# from the previous ledger hash, so every validator of a round reports +# into one trace and the round can be read end to end across nodes. +# +# random: experimental only, and not used. Each node invents its own +# trace id, so a single round arrives as one separate trace per node. +# Those traces can only be lined up by hand through the +# consensus_ledger_id span attribute. +# # --- Batch processor tuning --- # # batch_size=512 diff --git a/include/xrpl/consensus/ConsensusSpanNames.h b/include/xrpl/consensus/ConsensusSpanNames.h index 3c09dded41..e9d073b5e9 100644 --- a/include/xrpl/consensus/ConsensusSpanNames.h +++ b/include/xrpl/consensus/ConsensusSpanNames.h @@ -317,7 +317,9 @@ namespace event { */ inline constexpr auto disputeResolve = join(makeStr("dispute"), makeStr("resolve")); /** - * "tx.included" + * "tx.included" — one per transaction of the agreed consensus set, recorded + * before the ledger is built. A transaction that then fails to apply still + * has an event, so this is a superset of the accepted ledger's contents. */ inline constexpr auto txIncluded = join(makeStr("tx"), makeStr("included")); diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 4caf67a907..be3f540f56 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -122,6 +122,69 @@ namespace xrpl::telemetry { inline constexpr std::string_view kTracerName{"xrpld"}; #endif +/** + * How a consensus round span picks its trace id. + * + * consensus_trace_strategy (xrpld.cfg) + * | + * v + * makeTelemetrySetup() ──> Setup::consensusTraceStrategy + * | + * v + * RCLConsensus::Adaptor::startRoundTracing() + * | + * +-- Deterministic ──> SpanGuard::hashSpan(prev ledger hash) + * +-- Random ──> SpanGuard::span() / linkedSpan() + * + * Deterministic is the strategy in use. Every validator of a round hashes the + * same previous ledger id, so all of them land in one trace. + * + * Random is experimental and not used. Each node would invent its own trace + * id, so one round would arrive as one trace per node, joinable only by the + * `consensus_ledger_id` attribute. + * + * @code + * // Branch on the strategy rather than on a string. + * if (telemetry.getConsensusTraceStrategy() == ConsensusTraceStrategy::Random) + * span = SpanGuard::span(TraceCategory::Consensus, seg::consensus, op::round); + * else + * span = SpanGuard::hashSpan(TraceCategory::Consensus, name, id.data(), id.kBytes); + * + * // Edge case: the value also goes on a span attribute, so it needs its + * // config spelling back. + * span.setAttribute(attr::traceStrategy, strategyName(ConsensusTraceStrategy::Random)); + * @endcode + * + * @note Adding an enumerator means adding a spelling to strategyName() below + * and to the parser in TelemetryConfig.cpp. Both switch without a default, so + * the compiler catches a missed one. + */ +enum class ConsensusTraceStrategy : std::uint8_t { Deterministic, Random }; + +/** + * Config spelling of a consensus trace strategy. + * + * This is the same text `consensus_trace_strategy` accepts, and it is what + * goes on the `trace_strategy` span attribute, so the two cannot drift. + * + * @param strategy Strategy to name. + * @return "deterministic" or "random", pointing at a string literal. + */ +[[nodiscard]] constexpr char const* +strategyName(ConsensusTraceStrategy strategy) +{ + switch (strategy) + { + case ConsensusTraceStrategy::Deterministic: + return "deterministic"; + case ConsensusTraceStrategy::Random: + return "random"; + } + // The switch covers every enumerator. This return only satisfies the + // compiler, which cannot rule out a value outside the enumeration. + return "deterministic"; +} + class Telemetry { /** @@ -267,12 +330,12 @@ public: bool traceLedger = true; /** - * Strategy for cross-node consensus trace correlation. - * "deterministic" — derive trace_id from ledger hash so all - * validators in the same round share the same trace_id. - * "attribute" — random trace_id, correlate via ledger_id attribute. + * How a consensus round span picks its trace id. + * + * Read from `consensus_trace_strategy`. Deterministic is the strategy + * in use; Random is experimental. See ConsensusTraceStrategy. */ - std::string consensusTraceStrategy = "deterministic"; + ConsensusTraceStrategy consensusTraceStrategy = ConsensusTraceStrategy::Deterministic; }; virtual ~Telemetry() = default; @@ -345,9 +408,9 @@ public: shouldTraceLedger() const = 0; /** - * @return The configured consensus trace correlation strategy. + * @return How a consensus round span picks its trace id. */ - [[nodiscard]] virtual std::string const& + [[nodiscard]] virtual ConsensusTraceStrategy getConsensusTraceStrategy() const = 0; #ifdef XRPL_ENABLE_TELEMETRY diff --git a/src/libxrpl/telemetry/NullTelemetry.cpp b/src/libxrpl/telemetry/NullTelemetry.cpp index f8c3e4eb67..64030b02d7 100644 --- a/src/libxrpl/telemetry/NullTelemetry.cpp +++ b/src/libxrpl/telemetry/NullTelemetry.cpp @@ -104,7 +104,7 @@ public: return false; } - [[nodiscard]] std::string const& + [[nodiscard]] ConsensusTraceStrategy getConsensusTraceStrategy() const override { return setup_.consensusTraceStrategy; diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 6132d043e3..73914a7796 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -216,7 +216,7 @@ public: return false; } - [[nodiscard]] std::string const& + [[nodiscard]] ConsensusTraceStrategy getConsensusTraceStrategy() const override { return setup_.consensusTraceStrategy; @@ -436,7 +436,7 @@ public: return setup_.traceLedger; } - [[nodiscard]] std::string const& + [[nodiscard]] ConsensusTraceStrategy getConsensusTraceStrategy() const override { return setup_.consensusTraceStrategy; diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 7d61954f6f..ea0ba26a0f 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -46,6 +46,7 @@ constexpr char const* traceConsensus = "trace_consensus"; constexpr char const* traceRpc = "trace_rpc"; constexpr char const* tracePeer = "trace_peer"; constexpr char const* traceLedger = "trace_ledger"; +constexpr char const* consensusTraceStrategy = "consensus_trace_strategy"; } // namespace key /** @@ -150,6 +151,33 @@ networkTypeFromId(std::uint32_t networkId) } } +/** + * Map a `consensus_trace_strategy` value onto its enumerator. + * + * Only the two documented spellings are accepted. A typo would otherwise pick + * the default silently, and the operator would never learn the setting had no + * effect. Matching is exact and case-sensitive, like every other value in this + * section. + * + * @param value Raw config value; empty means the key was absent. + * @return The matching strategy, or Deterministic when the key was absent. + * @throws std::runtime_error If the value is neither documented spelling. + */ +[[nodiscard]] ConsensusTraceStrategy +readConsensusTraceStrategy(std::string const& value) +{ + if (value.empty() || value == strategyName(ConsensusTraceStrategy::Deterministic)) + return ConsensusTraceStrategy::Deterministic; + + if (value == strategyName(ConsensusTraceStrategy::Random)) + return ConsensusTraceStrategy::Random; + + Throw( + std::string("Invalid value '") + key::consensusTraceStrategy + "' in " + kSectionLabel + + ": must be '" + strategyName(ConsensusTraceStrategy::Deterministic) + "' or '" + + strategyName(ConsensusTraceStrategy::Random) + "'."); +} + } // namespace Telemetry::Setup @@ -204,7 +232,7 @@ makeTelemetrySetup( setup.traceLedger = section.valueOr(key::traceLedger, 1) != 0; setup.consensusTraceStrategy = - section.valueOr("consensus_trace_strategy", "deterministic"); + readConsensusTraceStrategy(section.valueOr(key::consensusTraceStrategy, "")); return setup; } diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index b14102f96f..5c70f806ed 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -168,14 +168,13 @@ public: } /** - * @return A fixed strategy label; the scope tests do not exercise - * deterministic trace-id correlation, so any stable value works. + * @return A fixed strategy; the scope tests do not exercise trace-id + * correlation, so either value works. */ - [[nodiscard]] std::string const& + [[nodiscard]] ConsensusTraceStrategy getConsensusTraceStrategy() const override { - static std::string const kStrategy{"none"}; - return kStrategy; + return ConsensusTraceStrategy::Deterministic; } opentelemetry::nostd::shared_ptr diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index bef61c3d25..3487bc1e03 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -27,6 +27,7 @@ namespace key { constexpr char const* batchSize = "batch_size"; constexpr char const* batchDelayMs = "batch_delay_ms"; constexpr char const* maxQueueSize = "max_queue_size"; +constexpr char const* consensusTraceStrategy = "consensus_trace_strategy"; } // namespace key /** @@ -104,6 +105,7 @@ TEST(TelemetryConfig, setup_defaults) EXPECT_TRUE(s.traceRpc); EXPECT_TRUE(s.tracePeer); EXPECT_TRUE(s.traceLedger); + EXPECT_EQ(s.consensusTraceStrategy, telemetry::ConsensusTraceStrategy::Deterministic); } TEST(TelemetryConfig, parse_empty_section) @@ -286,6 +288,71 @@ TEST(TelemetryConfig, batch_size_equal_to_max_queue_size_is_accepted) EXPECT_EQ(setup.maxQueueSize, 512u); } +TEST(TelemetryConfig, consensus_trace_strategy_names_match_the_config_spellings) +{ + // strategyName() feeds both the parser and the trace_strategy span + // attribute, so these two strings are the whole public vocabulary. + EXPECT_STREQ( + telemetry::strategyName(telemetry::ConsensusTraceStrategy::Deterministic), "deterministic"); + EXPECT_STREQ(telemetry::strategyName(telemetry::ConsensusTraceStrategy::Random), "random"); +} + +TEST(TelemetryConfig, consensus_trace_strategy_defaults_to_deterministic) +{ + // The key is absent, so the default applies. Deterministic is the only + // strategy in use, and a default of Random would break cross-node + // correlation on every node that omits the key. + EXPECT_EQ( + parseBatch({}).consensusTraceStrategy, telemetry::ConsensusTraceStrategy::Deterministic); +} + +TEST(TelemetryConfig, consensus_trace_strategy_accepts_deterministic) +{ + EXPECT_EQ( + parseBatch({{key::consensusTraceStrategy, "deterministic"}}).consensusTraceStrategy, + telemetry::ConsensusTraceStrategy::Deterministic); +} + +TEST(TelemetryConfig, consensus_trace_strategy_accepts_random) +{ + // Random is experimental and unused, but it is a documented spelling, so + // the parser must still map it to its own enumerator rather than reject it + // or fold it into the default. + EXPECT_EQ( + parseBatch({{key::consensusTraceStrategy, "random"}}).consensusTraceStrategy, + telemetry::ConsensusTraceStrategy::Random); +} + +TEST(TelemetryConfig, consensus_trace_strategy_empty_value_is_the_default) +{ + // `consensus_trace_strategy=` with nothing after it. An empty value means + // the operator wrote the key and no value, which is the default, not a typo. + EXPECT_EQ( + parseBatch({{key::consensusTraceStrategy, ""}}).consensusTraceStrategy, + telemetry::ConsensusTraceStrategy::Deterministic); +} + +TEST(TelemetryConfig, consensus_trace_strategy_rejects_an_undocumented_value) +{ + // "attribute" is not a spelling this parser accepts. Rejecting rather than + // defaulting is the point: a silent fallback would leave the operator + // believing a setting took effect. + EXPECT_EQ( + batchRejection({{key::consensusTraceStrategy, "attribute"}}), + "Invalid value 'consensus_trace_strategy' in [telemetry]: must be 'deterministic' or " + "'random'."); +} + +TEST(TelemetryConfig, consensus_trace_strategy_matching_is_case_sensitive) +{ + // Every other value in this section is matched exactly, so "Random" is a + // typo and must be reported as one. + EXPECT_EQ( + batchRejection({{key::consensusTraceStrategy, "Random"}}), + "Invalid value 'consensus_trace_strategy' in [telemetry]: must be 'deterministic' or " + "'random'."); +} + TEST(TelemetryConfig, null_telemetry_factory) { telemetry::Telemetry::Setup setup; diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index ac627e2519..5b216dcce5 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -695,6 +695,11 @@ RCLConsensus::Adaptor::doAccept( JLOG(j_.debug()) << "Building canonical tx set: " << retriableTxs.key(); + // One tx.included event per transaction of the agreed consensus set, which + // is not yet the accepted ledger: buildLCL() below applies these and some + // may fail, so the events are a superset of what the ledger ends up with. A + // transaction whose bytes cannot be parsed gets no event at all. + // // txCount and the per-transaction event feed the span and nothing else, so // both are guarded on the span being active. Unguarded, every accepted // ledger builds one 64-character hash string per transaction that no one @@ -1338,19 +1343,19 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) if (roundSpan_) roundSpan_.reset(); - auto const& strategy = app_.getTelemetry().getConsensusTraceStrategy(); + auto const strategy = app_.getTelemetry().getConsensusTraceStrategy(); telemetry::SpanContext const* const link = prevRoundSpanContext_.isValid() ? &prevRoundSpanContext_ : nullptr; - if (strategy == "attribute") + if (strategy == telemetry::ConsensusTraceStrategy::Random) { - // Non-deterministic strategy: each node gets a random trace_id, - // correlated via the consensus_ledger_id attribute rather than a - // shared trace_id. Still attach a follows-from link to the prior - // round so consecutive rounds stay navigable. linkedSpan is not - // TraceCategory-aware, so gate it explicitly to match the gating - // of the hashSpan/span factories used below. + // Experimental strategy, not used on a live network: each node gets a + // random trace_id, so one round arrives as one trace per node, joinable + // only by the consensus_ledger_id attribute. Still attach a follows-from + // link to the prior round so consecutive rounds stay navigable. + // linkedSpan is not TraceCategory-aware, so gate it explicitly to match + // the gating of the hashSpan/span factories used below. if (link != nullptr && app_.getTelemetry().shouldTraceConsensus()) { roundSpan_.emplace(telemetry::SpanGuard::linkedSpan(cs::round, *link)); @@ -1364,7 +1369,7 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) } else { - // "deterministic" (the default): derive the trace_id from the previous + // Deterministic (the default): derive the trace_id from the previous // ledger hash so all validators tracing the same round share one trace. roundSpan_.emplace( telemetry::SpanGuard::hashSpan( @@ -1382,7 +1387,7 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) roundSpan_->setAttribute(cs::attr::ledgerId, to_string(prevLgr.id()).c_str()); roundSpan_->setAttribute(cs::attr::ledgerSeq, static_cast(prevLgr.seq()) + 1); - roundSpan_->setAttribute(cs::attr::traceStrategy, strategy.c_str()); + roundSpan_->setAttribute(cs::attr::traceStrategy, telemetry::strategyName(strategy)); roundSpan_->setAttribute(cs::attr::roundId, static_cast(prevLgr.seq()) + 1); roundSpan_->setAttribute(cs::attr::previousLedgerSeq, static_cast(prevLgr.seq())); roundSpan_->setAttribute(cs::attr::previousProposers, static_cast(prevProposers_)); diff --git a/src/xrpld/app/consensus/RCLConsensus.h b/src/xrpld/app/consensus/RCLConsensus.h index 7830622fa8..af85f2bcc7 100644 --- a/src/xrpld/app/consensus/RCLConsensus.h +++ b/src/xrpld/app/consensus/RCLConsensus.h @@ -92,8 +92,8 @@ class RCLConsensus * Span for the current consensus round. * * Created in preStartRound(), ended (via reset()) when the next - * round begins. When consensusTraceStrategy is "deterministic", - * the trace_id is derived from previousLedger.id() so that all + * round begins. Under ConsensusTraceStrategy::Deterministic the + * trace_id is derived from previousLedger.id() so that all * validators in the same round share the same trace_id. * * Thread-free: a SpanGuard owns no thread-local Scope, so it can be From 039c2768ba93a94a3335f641ecb86d8f23f939e2 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:41:39 +0100 Subject: [PATCH 2/4] fix(telemetry): require an https endpoint when a client certificate is set The OTLP/HTTP exporter selects TLS from the endpoint URL scheme alone (HttpSslOptions in the pinned SDK matches "https:" exactly), so a client certificate handed to it alongside an http:// traces_endpoint is loaded and never presented. The parser checked the cert/key pairing, use_tls and file readability, but never the scheme, and the default traces_endpoint is plain HTTP. makeTelemetrySetup() now requires traces_endpoint to start with "https://" whenever tls_client_cert is set, including when the key is left at its default. Nothing asserted the client options reaching the exporter, so a swapped certificate and key would have passed every test. Move the options mapping into makeTraceExporterOptions() and assert it at that boundary with distinct certificate and key paths, plus a one-way-TLS control and a use_tls=0 control. One case runs the whole path from a [telemetry] section. Runbook and example-config fixes: - tx.included is emitted per transaction of the agreed consensus set, before buildLCL() applies anything, so it is a superset of the accepted ledger rather than proof of inclusion. - the dispute.resolve query used the descendant operator, but the event is on the consensus.update_positions span itself, so it matched nothing. - the exhausted-retries query asked for txq_status="retried" with retries_remaining=0, which cannot occur: the attribute is stamped before the attempt and the retried branch only runs while retries are left. Exhaustion is txq_status="failed" with a zero count. - consensus_round_id is an int64, so the two queries comparing it to a quoted string matched nothing. - note that consensus_trace_strategy=random is experimental and not used. - note that a trailing "| attr = value" is rejected by current Tempo; attribute filters belong inside the braces. --- cfg/xrpld-example.cfg | 18 ++- docs/telemetry-runbook.md | 84 ++++++---- include/xrpl/telemetry/Telemetry.h | 49 +++++- src/libxrpl/telemetry/Telemetry.cpp | 31 ++-- src/libxrpl/telemetry/TelemetryConfig.cpp | 38 +++++ .../libxrpl/telemetry/TelemetryConfig.cpp | 140 ++++++++++++++++ .../telemetry/TraceExporterOptions.cpp | 150 ++++++++++++++++++ 7 files changed, 458 insertions(+), 52 deletions(-) create mode 100644 src/tests/libxrpl/telemetry/TraceExporterOptions.cpp diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index e9024b2731..3b52ad8eae 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1722,6 +1722,13 @@ validators.txt # derived from it. # Default: http://localhost:4318/v1/traces. # +# The scheme of this URL is what decides whether the connection is +# encrypted, and it is matched exactly: only a lower-case https:// URL +# gives TLS. Because of that, setting tls_client_cert requires this URL +# to start with https:// — including leaving it at the default above, +# which makes xrpld fail to start rather than export without the client +# identity it was configured with. +# # --- TLS settings for the OTLP exporter connection --- # # use_tls=0 @@ -1755,11 +1762,12 @@ validators.txt # To enable mTLS, both tls_client_cert and tls_client_key must be # specified. If only one is provided, xrpld will fail to start. Providing # them while use_tls=0 also fails to start, rather than being ignored. -# With use_tls=1 each path is opened at startup, so one that does not -# exist or cannot be read fails to start too, rather than failing later -# as an opaque TLS handshake error. All three checks apply only when -# enabled=1; with telemetry disabled these settings are read but never -# validated. +# traces_endpoint must be an https:// URL, because that scheme is what +# makes the exporter present the certificate at all. With use_tls=1 each +# path is opened at startup, so one that does not exist or cannot be read +# fails to start too, rather than failing later as an opaque TLS +# handshake error. All four checks apply only when enabled=1; with +# telemetry disabled these settings are read but never validated. # # tls_client_key= # diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index da42154054..ca2bf2fac6 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -42,27 +42,29 @@ cmake --build --preset default ## Configuration Reference -| Option | Default | Description | -| -------------------------- | --------------------------------- | --------------------------------------------------------- | -| `enabled` | `0` | Master switch for telemetry | -| `traces_endpoint` | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | -| `service_name` | `xrpld` | OpenTelemetry service name resource attribute | -| `service_instance_id` | node public key | OpenTelemetry service instance ID resource attribute | -| `trace_rpc` | `1` | Enable RPC request tracing | -| `trace_transactions` | `1` | Enable transaction tracing | -| `trace_consensus` | `1` | Enable consensus tracing | -| `trace_peer` | `1` | Enable peer message tracing (high volume) | -| `trace_ledger` | `1` | Enable ledger tracing | -| `consensus_trace_strategy` | `deterministic` | Consensus trace ID strategy (`deterministic` or `random`) | -| `batch_size` | `512` | Max spans per batch export | -| `batch_delay_ms` | `5000` | Delay between batch exports | -| `max_queue_size` | `2048` | Max spans queued before dropping | -| `use_tls` | `0` | Use TLS for exporter connection | -| `tls_ca_cert` | (empty) | Path to CA certificate bundle | -| `tls_client_cert` | (empty) | Client cert (PEM) for mTLS; empty = one-way. See note | -| `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert`. See note | +| Option | Default | Description | +| -------------------------- | --------------------------------- | ----------------------------------------------------------------------------------------------------- | +| `enabled` | `0` | Master switch for telemetry | +| `traces_endpoint` | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | +| `service_name` | `xrpld` | OpenTelemetry service name resource attribute | +| `service_instance_id` | node public key | OpenTelemetry service instance ID resource attribute | +| `trace_rpc` | `1` | Enable RPC request tracing | +| `trace_transactions` | `1` | Enable transaction tracing | +| `trace_consensus` | `1` | Enable consensus tracing | +| `trace_peer` | `1` | Enable peer message tracing (high volume) | +| `trace_ledger` | `1` | Enable ledger tracing | +| `consensus_trace_strategy` | `deterministic` | Consensus trace ID strategy. `deterministic` is the value to use; `random` is experimental — see note | +| `batch_size` | `512` | Max spans per batch export | +| `batch_delay_ms` | `5000` | Delay between batch exports | +| `max_queue_size` | `2048` | Max spans queued before dropping | +| `use_tls` | `0` | Use TLS for exporter connection | +| `tls_ca_cert` | (empty) | Path to CA certificate bundle | +| `tls_client_cert` | (empty) | Client cert (PEM) for mTLS; empty = one-way. See note | +| `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert`. See note | -> **mTLS (mutual TLS) note**: `tls_client_cert` and `tls_client_key` are optional — leaving both empty gives one-way (server-only) TLS. **If either one is set**, `enabled=1` requires both of them **and** `use_tls=1`, or the node exits at startup; see the Troubleshooting entry for `Unable to start ...: [telemetry] ...`. When `enabled=0` they are read but never validated. +> **mTLS (mutual TLS) note**: `tls_client_cert` and `tls_client_key` are optional — leaving both empty gives one-way (server-only) TLS. **If either one is set**, `enabled=1` requires both of them, `use_tls=1`, **and** a `traces_endpoint` starting with `https://` — the exporter decides encryption from the URL scheme, so the certificate is only ever presented on an `https://` endpoint. The default `traces_endpoint` is plain HTTP, so mTLS means setting that key too. Breaking any of these makes the node exit at startup; see the Troubleshooting entry for `Unable to start ...: [telemetry] ...`. When `enabled=0` they are read but never validated. + +> **`consensus_trace_strategy` note**: only `deterministic` and `random` are accepted, and anything else makes the node exit at startup. Use `deterministic`: it seeds the round's trace ID from the previous ledger hash, so every validator of a round reports into one trace. `random` is experimental and not used — each node would invent its own trace ID, so a single round would arrive as one separate trace per node, joinable only by hand through `consensus_ledger_id`. ## Span Reference @@ -129,13 +131,22 @@ lifecycle spans be joined to the ledger trace it targeted (`span.current_ledger_ #### Consensus Span Events -| Parent Span | Event Name | Event Attributes | Description | -| ---------------------------- | ----------------- | ----------------------------------------------------------- | ------------------------------------------------------- | -| `consensus.update_positions` | `dispute.resolve` | `tx_id`, `dispute_our_vote`, `dispute_yays`, `dispute_nays` | Emitted per dispute when votes are tallied | -| `consensus.accept.apply` | `tx.included` | `tx_id` | Emitted per transaction included in the accepted ledger | +| Parent Span | Event Name | Event Attributes | Description | +| ---------------------------- | ----------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------------------------ | +| `consensus.update_positions` | `dispute.resolve` | `tx_id`, `dispute_our_vote`, `dispute_yays`, `dispute_nays` | Emitted per dispute when votes are tallied | +| `consensus.accept.apply` | `tx.included` | `tx_id` | Emitted per transaction of the agreed consensus set, before the ledger is built — see note | + +> **`tx.included` note**: the event is recorded while the canonical transaction set is being assembled, which happens before `buildLCL()` applies anything. So a transaction that fails to apply, or is left over to retry in a later ledger, still has a `tx.included` event. Treat the events as the round's **input** set, not as proof a transaction reached the accepted ledger; `tx_count` on the same span counts the same set. A transaction whose bytes cannot be parsed gets no event, and nothing in the accepted ledger is missing one, so the events are always a superset of the ledger's contents. To confirm a transaction actually applied, read `ter_result` and `applied` on its `tx.transactor` span. #### Close Time Queries (Tempo TraceQL) +> **TraceQL syntax**: an attribute filter belongs inside the braces, as +> `{name="x" && span.attr = value}`. The `{name="x"} | attr = value` form used +> by several examples in this document is rejected by current Tempo with a parse +> error, so convert an example to the braced form before running it. Numeric +> attributes such as `consensus_round_id` and `retries_remaining` must be +> compared unquoted. + ``` # Find rounds where validators disagreed on close time {name="consensus.accept.apply"} | close_time_correct = false @@ -149,11 +160,13 @@ lifecycle spans be joined to the ledger trace it targeted (`span.current_ledger_ # Find specific ledger's consensus details {name="consensus.accept.apply"} | ledger_seq = 92345678 -# Find all spans in a consensus round (deterministic trace strategy) -{name="consensus.round"} | consensus_round_id = "" +# Find a consensus round by its id. consensus_round_id is an integer, so it +# must not be quoted, and it is set only on consensus.round. +{name="consensus.round" && span.consensus_round_id = 92345678} -# Find dispute resolutions -{name="consensus.update_positions"} >> {event:name="dispute.resolve"} +# Find dispute resolutions. The event is recorded on the update_positions +# span itself, so it is a condition on that span, not on a descendant. +{name="consensus.update_positions" && event:name="dispute.resolve"} ``` ## Insights and Sample Queries @@ -196,8 +209,11 @@ This section shows what questions you can now answer using the enriched span att # Find ledger closes that applied queued transactions {name="txq.accept"} | ledger_changed = true -# Find transactions that exhausted retries -{name="txq.accept_tx"} | txq_status = "retried" && retries_remaining = 0 +# Find transactions dropped because they had no retries left. +# retries_remaining is recorded before the attempt, and the "retried" branch +# is only reached while retries are left, so exhaustion always shows up as +# "failed" with a zero count. +{name="txq.accept_tx" && span.txq_status = "failed" && span.retries_remaining <= 0} ``` ### RPC Debugging @@ -380,8 +396,12 @@ all its normal attributes, it just lacks a cross-node parent link. # Trace a transaction across the network by its hash {name=~"tx\\..*"} | tx_hash = "" -# Find all spans in a cross-node consensus trace -{rootServiceName="xrpld"} | consensus_round_id = "" +# Find a cross-node consensus trace by round id, then open the returned trace +# to see every node's spans. Under the deterministic strategy all validators of +# a round share one trace id, so one trace holds all of them. The value is an +# integer, so it must not be quoted, and it only matches the consensus.round +# span that carries it. +{name="consensus.round" && span.consensus_round_id = 92345678} # Compare latency between sender and receiver for validations {name="consensus.validation.send" || name="consensus.validation.receive"} diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index d387decfee..b996279d3a 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -102,6 +102,7 @@ #ifdef XRPL_ENABLE_TELEMETRY #include +#include #include #include #include @@ -437,10 +438,12 @@ makeTelemetry(Telemetry::Setup const& setup, beast::Journal journal); * @return A populated Setup struct with defaults for missing values. * @throws std::runtime_error If `enabled` is set and the mutual TLS (mTLS) * settings contradict each other: only one of `tls_client_cert`/`tls_client_key` - * is given, or a client certificate is given while `use_tls` is 0. Also if + * is given, a client certificate is given while `use_tls` is 0, or a client + * certificate is given while `traces_endpoint` is not an `https://` URL — which + * includes leaving `traces_endpoint` at its plain-HTTP default. Also if * `enabled` and `use_tls` are both set and a non-empty `tls_ca_cert`, * `tls_client_cert` or `tls_client_key` cannot be read; an empty path is skipped, - * so an empty `tls_ca_cert` still means "use the system CA store". All three + * so an empty `tls_ca_cert` still means "use the system CA store". All four * checks are skipped when `enabled` is 0. * @throws boost::bad_lexical_cast If any numeric key (`enabled`, `use_tls`, * `batch_size`, the trace switches, ...) holds a value Section::valueOr cannot @@ -454,4 +457,46 @@ makeTelemetrySetup( std::string const& version, std::uint32_t networkId); +#ifdef XRPL_ENABLE_TELEMETRY +/** + * Build the OTLP/HTTP trace exporter options that Setup asks for. + * + * Telemetry::Setup ──> makeTraceExporterOptions() ──> OtlpHttpExporterOptions + * | + * v + * OtlpHttpExporterFactory::Create + * + * Named and declared here rather than left inline in start() so the mapping + * from config to exporter options can be asserted directly. A swapped + * certificate and key, or a CA path written to the wrong field, is invisible + * from outside a running exporter. + * + * TLS fields are set only when `use_tls` is on. Whether the transport is + * actually encrypted is decided by the scheme of the URL, not by this function; + * makeTelemetrySetup() is what rejects a client certificate on a plain-HTTP + * endpoint. + * + * @code + * // Primary use: one-way TLS with a custom CA bundle. + * Telemetry::Setup setup; + * setup.useTls = true; + * setup.tlsCertPath = "/etc/ssl/ca.pem"; + * auto const opts = makeTraceExporterOptions(setup); // ssl_ca_cert_path set + * + * // Edge case: use_tls off leaves every ssl_ field empty, even when paths + * // are configured. + * setup.useTls = false; + * auto const plain = makeTraceExporterOptions(setup); // ssl_ca_cert_path empty + * @endcode + * + * @param setup Parsed [telemetry] configuration. + * @return Options carrying `traces_endpoint` as the URL, plus the TLS paths + * when `use_tls` is on. Every other field keeps its SDK default. + * @note Pure: reads `setup` and touches no global state, so it is safe to call + * from any thread. + */ +[[nodiscard]] opentelemetry::exporter::otlp::OtlpHttpExporterOptions +makeTraceExporterOptions(Telemetry::Setup const& setup); +#endif + } // namespace xrpl::telemetry diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index bc8b2faf3e..b6cd988eec 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -296,19 +296,8 @@ public: JLOG(journal_.info()) << "Telemetry starting: traces_endpoint=" << setup_.tracesEndpoint << " sampling=" << setup_.samplingRatio; - // Configure OTLP HTTP exporter - otlp_http::OtlpHttpExporterOptions exporterOpts; - exporterOpts.url = setup_.tracesEndpoint; - if (setup_.useTls) - { - exporterOpts.ssl_ca_cert_path = setup_.tlsCertPath; - // Present a client cert for mutual TLS. When both paths are - // empty the connection falls back to one-way (server) TLS. - exporterOpts.ssl_client_cert_path = setup_.tlsClientCertPath; - exporterOpts.ssl_client_key_path = setup_.tlsClientKeyPath; - } - - auto exporter = otlp_http::OtlpHttpExporterFactory::Create(exporterOpts); + auto exporter = + otlp_http::OtlpHttpExporterFactory::Create(makeTraceExporterOptions(setup_)); // Configure batch processor trace_sdk::BatchSpanProcessorOptions processorOpts; @@ -481,6 +470,22 @@ public: } // namespace +opentelemetry::exporter::otlp::OtlpHttpExporterOptions +makeTraceExporterOptions(Telemetry::Setup const& setup) +{ + otlp_http::OtlpHttpExporterOptions opts; + opts.url = setup.tracesEndpoint; + if (setup.useTls) + { + opts.ssl_ca_cert_path = setup.tlsCertPath; + // Present a client cert for mutual TLS. When both paths are + // empty the connection falls back to one-way (server) TLS. + opts.ssl_client_cert_path = setup.tlsClientCertPath; + opts.ssl_client_key_path = setup.tlsClientKeyPath; + } + return opts; +} + std::unique_ptr makeTelemetry(Telemetry::Setup const& setup, beast::Journal journal) { diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index c98f3a7151..ef7d430e21 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include namespace xrpl::telemetry { @@ -183,6 +184,34 @@ requireReadableFile(std::string const& path, char const* configKey) } } +/** + * Throw unless an endpoint URL is one the client certificate can be used on. + * + * The OTLP/HTTP exporter turns TLS on from the URL scheme alone, and matches + * "https:" exactly and case-sensitively. So a client certificate only reaches + * the collector on an https endpoint, and this check is what holds that + * invariant: with a client certificate configured, the endpoint is an https URL. + * "https://" is required in full, which is stricter than the exporter's own + * test, so anything this accepts the exporter also treats as TLS. + * + * @param endpoint Endpoint URL from the config, or the built-in default. + * @param configKey Config key the URL came from, named in the message. + * @throws std::runtime_error If the URL does not begin with "https://". + */ +void +requireHttpsEndpoint(std::string const& endpoint, char const* configKey) +{ + constexpr std::string_view kHttpsPrefix{"https://"}; + + if (std::string_view{endpoint}.starts_with(kHttpsPrefix)) + return; + + Throw( + std::string("Invalid value '") + configKey + "' in " + kSectionLabel + + ": must start with '" + std::string{kHttpsPrefix} + "' when " + key::tlsClientCert + + " is set, but is '" + endpoint + "'."); +} + } // namespace Telemetry::Setup @@ -238,6 +267,15 @@ makeTelemetrySetup( "(set use_tls=1 to enable mutual TLS, or remove the cert paths)."); } + // Still inside the enabled branch, and checked before the files are + // opened so a scheme problem is not hidden behind a path problem. The + // exporter reads TLS off the endpoint scheme, so a client certificate is + // only presented on an https endpoint. tls_ca_cert is left out of this + // check: it only names a trust store, while a client certificate is this + // node's own identity and has to reach the collector to mean anything. + if (!setup.tlsClientCertPath.empty()) + requireHttpsEndpoint(setup.tracesEndpoint, key::tracesEndpoint); + // Still inside the enabled branch. The exporter opens these files only // when TLS is on, so check them only then: a bad path behind use_tls=0 // stops nothing. Checking here turns what would otherwise surface much diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 6dca9bdc1e..15cb52a562 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -56,6 +56,25 @@ constexpr char const* pairingError = "must be set together"; constexpr char const* useTlsError = "require use_tls=1"; constexpr char const* readError = "cannot be read"; +/** + * Endpoint values and the message fragment of the scheme guard. + * + * keyEndpoint is the config key, spelled once for the same reason as the two + * client-certificate keys above. httpEndpoint and httpsEndpoint differ only in + * scheme, so a case that swaps them changes nothing else. defaultEndpoint is + * the parser's own default, restated here so the omitted-key case can assert + * that the default is what got rejected; if the default ever changes, the case + * that names it fails rather than quietly testing a different URL. + * + * schemeError occurs in no other message in this file, so matching it proves + * the scheme guard fired and not the pairing, use_tls or readability guard. + */ +constexpr char const* keyEndpoint = "traces_endpoint"; +constexpr char const* httpEndpoint = "http://collector:4318/v1/traces"; +constexpr char const* httpsEndpoint = "https://collector:4318/v1/traces"; +constexpr char const* defaultEndpoint = "http://localhost:4318/v1/traces"; +constexpr char const* schemeError = "must start with 'https://'"; + /** * Build a [telemetry] section carrying only the `enabled` key. * @@ -275,6 +294,7 @@ TEST(TelemetryConfig, mtls_cert_and_key_both_set) auto const key = mtls::writeCertFile(dir.file("client.key")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); section.set(mtls::keyClientCert, cert); section.set(mtls::keyClientKey, key); @@ -397,6 +417,7 @@ TEST(TelemetryConfig, tls_missing_client_cert_file_throws) auto const absentCert = dir.file("absent.pem"); Section section = mtls::makeSection(true); section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); section.set(mtls::keyClientCert, absentCert); section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); @@ -414,6 +435,7 @@ TEST(TelemetryConfig, tls_missing_client_key_file_throws) auto const absentKey = dir.file("absent.key"); Section section = mtls::makeSection(true); section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); section.set(mtls::keyClientKey, absentKey); @@ -449,6 +471,7 @@ TEST(TelemetryConfig, tls_readable_files_are_accepted) auto const key = mtls::writeCertFile(dir.file("k.pem")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); section.set("tls_ca_cert", ca); section.set(mtls::keyClientCert, cert); section.set(mtls::keyClientKey, key); @@ -502,6 +525,123 @@ TEST(TelemetryConfig, tls_ca_cert_not_checked_when_use_tls_off) EXPECT_EQ(setup.tlsCertPath, absentCa); } +TEST(TelemetryConfig, mtls_client_cert_on_a_plain_http_endpoint_throws) +{ + // Full mTLS on an http:// endpoint. Both paths are set and readable and + // use_tls=1, so the pairing, use_tls and readability guards are all + // satisfied and the scheme guard is the only reachable throw. The message + // must name the endpoint key and the rejected URL. + TempDir const dir; + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpEndpoint); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::schemeError), + HasSubstr(mtls::keyEndpoint), + HasSubstr(mtls::httpEndpoint)))); +} + +TEST(TelemetryConfig, mtls_client_cert_with_the_default_endpoint_throws) +{ + // The endpoint key is omitted, so the parser's own default applies — and + // that default is plain HTTP. This is the case an operator reaches by + // configuring mTLS and nothing else, so it must be rejected exactly like + // an explicit http:// URL, naming the default it rejected. + TempDir const dir; + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::schemeError), + HasSubstr(mtls::keyEndpoint), + HasSubstr(mtls::defaultEndpoint)))); +} + +TEST(TelemetryConfig, mtls_client_cert_on_an_https_endpoint_is_accepted) +{ + // The same configuration as the two cases above with only the scheme + // changed, so nothing but the scheme can explain the different outcome. + TempDir const dir; + auto const cert = mtls::writeCertFile(dir.file("c.pem")); + auto const key = mtls::writeCertFile(dir.file("k.pem")); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpsEndpoint); + section.set(mtls::keyClientCert, cert); + section.set(mtls::keyClientKey, key); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_EQ(setup.tracesEndpoint, mtls::httpsEndpoint); + EXPECT_EQ(setup.tlsClientCertPath, cert); + EXPECT_EQ(setup.tlsClientKeyPath, key); +} + +TEST(TelemetryConfig, mtls_scheme_check_is_case_sensitive_like_the_exporter) +{ + // The exporter compares the scheme byte for byte, so "HTTPS://" leaves it + // exporting in the clear. Accepting the upper-case spelling here would let + // a configuration pass validation and still drop the client identity. + TempDir const dir; + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, "HTTPS://collector:4318/v1/traces"); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(HasSubstr(mtls::schemeError))); +} + +TEST(TelemetryConfig, one_way_tls_on_a_plain_http_endpoint_is_accepted) +{ + // The control for the guard's scope: same http:// endpoint and use_tls=1, + // but no client certificate. Only a client identity can be silently + // dropped, so this configuration is left alone. Widen the guard to every + // use_tls=1 node and this case starts failing. + TempDir const dir; + auto const ca = mtls::writeCertFile(dir.file("ca.pem")); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpEndpoint); + section.set("tls_ca_cert", ca); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_EQ(setup.tracesEndpoint, mtls::httpEndpoint); + EXPECT_EQ(setup.tlsCertPath, ca); + EXPECT_TRUE(setup.tlsClientCertPath.empty()); +} + +TEST(TelemetryConfig, mtls_scheme_not_checked_when_telemetry_disabled) +{ + // Telemetry off, so a leftover mTLS block on a plain endpoint must not stop + // the node from booting. use_tls stays 1 and the paths are absent files, so + // the `enabled` gate is the only thing suppressing every guard. + TempDir const dir; + Section section = mtls::makeSection(false); + section.set("use_tls", "1"); + section.set(mtls::keyEndpoint, mtls::httpEndpoint); + section.set(mtls::keyClientCert, mtls::clientCert); + section.set(mtls::keyClientKey, mtls::clientKey); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_FALSE(setup.enabled); + EXPECT_EQ(setup.tracesEndpoint, mtls::httpEndpoint); + EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); +} + TEST(TelemetryConfig, batch_settings_accept_the_lower_bound_exactly) { auto const setup = diff --git a/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp b/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp new file mode 100644 index 0000000000..80f6f22205 --- /dev/null +++ b/src/tests/libxrpl/telemetry/TraceExporterOptions.cpp @@ -0,0 +1,150 @@ +// The whole file is telemetry-only: makeTraceExporterOptions() and the OTel +// exporter options type it returns are both declared behind +// XRPL_ENABLE_TELEMETRY, so without it there is nothing here to test. +#ifdef XRPL_ENABLE_TELEMETRY + +#include +#include +#include + +#include + +#include +#include + +using namespace xrpl; + +namespace { + +/** + * Distinct placeholder paths for the three TLS files. + * + * They are deliberately different from one another in more than a suffix, so a + * certificate written into the key field, or a CA bundle written into either, + * shows up as an inequality naming both paths rather than as a near-miss. + */ +namespace tlsPath { +constexpr char const* ca = "/etc/xrpl/tls/collector-ca-bundle.pem"; +constexpr char const* clientCert = "/etc/xrpl/tls/node-client-certificate.pem"; +constexpr char const* clientKey = "/etc/xrpl/tls/node-client-private-key.pem"; +} // namespace tlsPath + +constexpr char const* kHttpsEndpoint = "https://collector.example:4318/v1/traces"; + +/** + * Build a Setup with mutual TLS configured and nothing else set. + * + * The struct is filled directly rather than parsed, so these cases isolate the + * Setup-to-exporter mapping. The end-to-end case at the bottom of this file + * covers the config-file side. + * + * @param useTls Value for Setup::useTls; the only thing the cases vary. + * @return The populated Setup. + */ +telemetry::Telemetry::Setup +makeMtlsSetup(bool useTls) +{ + telemetry::Telemetry::Setup setup; + setup.enabled = true; + setup.tracesEndpoint = kHttpsEndpoint; + setup.useTls = useTls; + setup.tlsCertPath = tlsPath::ca; + setup.tlsClientCertPath = tlsPath::clientCert; + setup.tlsClientKeyPath = tlsPath::clientKey; + return setup; +} + +/** + * Write a placeholder certificate file at the given path. + * + * makeTelemetrySetup() only needs the file to exist and be readable; nothing + * checks that the contents parse as PEM. + * + * @param path Where to write the file, typically from TempDir::file(). + * @return The same path, ready to pass to Section::set(). + */ +std::string +writeCertFile(std::string const& path) +{ + std::ofstream out{path}; + out << "placeholder\n"; + out.close(); + EXPECT_TRUE(out.good()) << "could not create " << path; + return path; +} + +} // namespace + +TEST(TraceExporterOptions, mtls_paths_reach_the_matching_exporter_fields) +{ + // The assertion the exporter boundary was missing: each configured path + // lands in its own field. The three paths differ, so swapping the client + // certificate and key, or writing the CA bundle into a client field, fails + // here instead of failing as a TLS handshake error on a live collector. + auto const opts = telemetry::makeTraceExporterOptions(makeMtlsSetup(true)); + + EXPECT_EQ(opts.url, kHttpsEndpoint); + EXPECT_EQ(opts.ssl_ca_cert_path, tlsPath::ca); + EXPECT_EQ(opts.ssl_client_cert_path, tlsPath::clientCert); + EXPECT_EQ(opts.ssl_client_key_path, tlsPath::clientKey); +} + +TEST(TraceExporterOptions, one_way_tls_leaves_the_client_fields_empty) +{ + // The one-way TLS control: a CA bundle and no client identity. The client + // fields must stay empty, so an unset client certificate cannot pick up a + // path from somewhere else in Setup. + auto setup = makeMtlsSetup(true); + setup.tlsClientCertPath.clear(); + setup.tlsClientKeyPath.clear(); + + auto const opts = telemetry::makeTraceExporterOptions(setup); + + EXPECT_EQ(opts.url, kHttpsEndpoint); + EXPECT_EQ(opts.ssl_ca_cert_path, tlsPath::ca); + EXPECT_EQ(opts.ssl_client_cert_path, ""); + EXPECT_EQ(opts.ssl_client_key_path, ""); +} + +TEST(TraceExporterOptions, use_tls_off_passes_no_tls_paths_at_all) +{ + // Every path is configured and use_tls is off, so all three fields must + // stay empty while the URL still goes through. This is the only case that + // distinguishes "gated on use_tls" from "always copied". + auto const opts = telemetry::makeTraceExporterOptions(makeMtlsSetup(false)); + + EXPECT_EQ(opts.url, kHttpsEndpoint); + EXPECT_EQ(opts.ssl_ca_cert_path, ""); + EXPECT_EQ(opts.ssl_client_cert_path, ""); + EXPECT_EQ(opts.ssl_client_key_path, ""); +} + +TEST(TraceExporterOptions, config_section_reaches_the_exporter_options) +{ + // The whole path in one case: a [telemetry] section with an https endpoint + // and two different real files, parsed by makeTelemetrySetup() and then + // mapped. Nothing between the config file and the exporter is stubbed, so a + // break anywhere along it lands here. + TempDir const dir; + auto const cert = writeCertFile(dir.file("node-client-certificate.pem")); + auto const key = writeCertFile(dir.file("node-client-private-key.pem")); + ASSERT_NE(cert, key); + + Section section; + section.set("enabled", "1"); + section.set("traces_endpoint", kHttpsEndpoint); + section.set("use_tls", "1"); + section.set("tls_client_cert", cert); + section.set("tls_client_key", key); + + auto const setup = telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); + auto const opts = telemetry::makeTraceExporterOptions(setup); + + EXPECT_EQ(opts.url, kHttpsEndpoint); + EXPECT_EQ(opts.ssl_client_cert_path, cert); + EXPECT_EQ(opts.ssl_client_key_path, key); + // No tls_ca_cert in the section, so the exporter keeps its own trust store. + EXPECT_EQ(opts.ssl_ca_cert_path, ""); +} + +#endif // XRPL_ENABLE_TELEMETRY From 8da882dccb9a234f0055905d103d4f4d08ce694a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:42:05 +0100 Subject: [PATCH 3/4] docs(telemetry): state when retries_remaining is recorded retries_remaining is stamped on the txq.accept_tx span before the transaction is applied and before the retry counter is decremented, so a span with txq_status="retried" always shows a non-zero count and exhaustion shows up as txq_status="failed" with zero. The attribute comment said only "retries left before discard", which reads as a post-decrement value and led to a runbook query that could never match. Also rename the drifted consensus_trace_strategy value in the plan docs from "attribute" to "random", the spelling the parser accepts, and note that it is experimental and not used. --- OpenTelemetryPlan/02-design-decisions.md | 3 +- .../05-configuration-reference.md | 36 +++++++++---------- src/xrpld/app/misc/detail/TxQSpanNames.h | 5 ++- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 70c29093ad..300c3f3f89 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -487,7 +487,8 @@ The first 16 bytes are used as trace_id. See [Phase 4a implementation status](./ and `createDeterministicContext()` in `RCLConsensus.cpp` for the implementation. Switchable via `consensus_trace_strategy` config: -`"deterministic"` (default) or `"attribute"` (random trace_id, correlation via attribute queries). +`"deterministic"` (default) or `"random"` (random trace_id, correlation via attribute queries). +`"random"` is experimental and not used: it would break cross-node trace correlation. #### Why Not Random IDs with Propagation Only? diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 0edf8f20f6..6f643f0f80 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -15,24 +15,24 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme ### 5.1.2 Configuration Options Summary -| Option | Type | Default | Description | -| -------------------------- | ------ | --------------------------------- | ---------------------------------------------------------------------------------------------------------- | -| `enabled` | bool | `false` | Enable/disable telemetry | -| `traces_endpoint` | string | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | -| `use_tls` | bool | `false` | Enable TLS for exporter connection | -| `tls_ca_cert` | string | `""` | Path to CA certificate file | -| `batch_size` | uint | `512` | Spans per export batch | -| `batch_delay_ms` | uint | `5000` | Max delay before sending batch (ms) | -| `max_queue_size` | uint | `2048` | Maximum queued spans | -| `trace_transactions` | 0 or 1 | `1` | Enable transaction tracing | -| `trace_consensus` | 0 or 1 | `1` | Enable consensus tracing | -| `trace_rpc` | 0 or 1 | `1` | Enable RPC tracing | -| `trace_peer` | 0 or 1 | `1` | Enable peer message tracing (high volume) | -| `trace_ledger` | 0 or 1 | `1` | Enable ledger tracing | -| `tx_trace_strategy` | string | `"deterministic"` | TX trace ID strategy: `"deterministic"` (trace_id = txHash[0:16]) or `"attribute"` (random) | -| `consensus_trace_strategy` | string | `"deterministic"` | Consensus trace ID strategy: `"deterministic"` (trace_id = prevLedgerHash[0:16]) or `"attribute"` (random) | -| `service_name` | string | `"xrpld"` | Service name (`service.name`) for traces and metrics | -| `service_instance_id` | string | `` | Instance identifier | +| Option | Type | Default | Description | +| -------------------------- | ------ | --------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | +| `enabled` | bool | `false` | Enable/disable telemetry | +| `traces_endpoint` | string | `http://localhost:4318/v1/traces` | Full OTLP/HTTP URL for spans, used verbatim | +| `use_tls` | bool | `false` | Enable TLS for exporter connection | +| `tls_ca_cert` | string | `""` | Path to CA certificate file | +| `batch_size` | uint | `512` | Spans per export batch | +| `batch_delay_ms` | uint | `5000` | Max delay before sending batch (ms) | +| `max_queue_size` | uint | `2048` | Maximum queued spans | +| `trace_transactions` | 0 or 1 | `1` | Enable transaction tracing | +| `trace_consensus` | 0 or 1 | `1` | Enable consensus tracing | +| `trace_rpc` | 0 or 1 | `1` | Enable RPC tracing | +| `trace_peer` | 0 or 1 | `1` | Enable peer message tracing (high volume) | +| `trace_ledger` | 0 or 1 | `1` | Enable ledger tracing | +| `tx_trace_strategy` | string | `"deterministic"` | TX trace ID strategy: `"deterministic"` (trace_id = txHash[0:16]) or `"attribute"` (random) | +| `consensus_trace_strategy` | string | `"deterministic"` | Consensus trace ID strategy: `"deterministic"` (trace_id = prevLedgerHash[0:16]) or `"random"` (experimental, not used) | +| `service_name` | string | `"xrpld"` | Service name (`service.name`) for traces and metrics | +| `service_instance_id` | string | `` | Instance identifier | **Planned (not yet implemented)**: the following options appear in the design documents but are not parsed by `TelemetryConfig.cpp` in Phase 1b and later diff --git a/src/xrpld/app/misc/detail/TxQSpanNames.h b/src/xrpld/app/misc/detail/TxQSpanNames.h index 6b64b76a57..703553e7b9 100644 --- a/src/xrpld/app/misc/detail/TxQSpanNames.h +++ b/src/xrpld/app/misc/detail/TxQSpanNames.h @@ -121,7 +121,10 @@ inline constexpr auto expiredCount = makeStr("expired_count"); */ inline constexpr auto terCode = makeStr("ter_code"); /** - * "retries_remaining" — retries left before discard. + * "retries_remaining" — retries left as this attempt started, recorded before + * the transaction is applied and before any decrement. A span with + * txq_status="retried" therefore always shows a non-zero count; exhaustion + * shows up as txq_status="failed" with zero. */ inline constexpr auto retriesRemaining = makeStr("retries_remaining"); /** From 2399f5763f6b6ee4ad957596fe1eafb3bb335ea0 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:42:32 +0100 Subject: [PATCH 4/4] docs(telemetry): name the consensus trace strategy value "random" The plan doc offered `"attribute"` as the alternative to `"deterministic"` for consensus_trace_strategy. The parser accepts `"random"`; "attribute" described the correlation mechanism rather than the setting's value. Note also that the alternative is experimental and not used. --- OpenTelemetryPlan/06-implementation-phases.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OpenTelemetryPlan/06-implementation-phases.md b/OpenTelemetryPlan/06-implementation-phases.md index cf1e58f779..c7fc9af1e2 100644 --- a/OpenTelemetryPlan/06-implementation-phases.md +++ b/OpenTelemetryPlan/06-implementation-phases.md @@ -178,7 +178,8 @@ Phase 4a (establish-phase gap fill & cross-node correlation) will add: - **Deterministic trace ID** derived from `previousLedger.id()` so all validators in the same round share the same `trace_id` (switchable via - `consensus_trace_strategy` config: `"deterministic"` or `"attribute"`). + `consensus_trace_strategy` config: `"deterministic"`, or `"random"` which is + experimental and not used). See [Configuration Reference](./05-configuration-reference.md) for full configuration options. - **Round lifecycle spans**: `consensus.round` with round-to-round span links.