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