merge: bring the review fixes forward from otel-phase7-native-metrics

src/libxrpl/telemetry/NullTelemetry.cpp

@@ -109,7 +109,7 @@ public:
         return false;
     }
 
-    [[nodiscard]] std::string const&
+    [[nodiscard]] ConsensusTraceStrategy
     getConsensusTraceStrategy() const override
     {
         return setup_.consensusTraceStrategy;

src/libxrpl/telemetry/Telemetry.cpp

@@ -244,7 +244,7 @@ public:
         return false;
     }
 
-    [[nodiscard]] std::string const&
+    [[nodiscard]] ConsensusTraceStrategy
     getConsensusTraceStrategy() const override
     {
         return setup_.consensusTraceStrategy;
@@ -504,19 +504,8 @@ public:
                               << " metrics_endpoint=" << setup_.metricsEndpoint
                               << " 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;
@@ -662,7 +651,7 @@ public:
         return setup_.traceLedger;
     }
 
-    [[nodiscard]] std::string const&
+    [[nodiscard]] ConsensusTraceStrategy
     getConsensusTraceStrategy() const override
     {
         return setup_.consensusTraceStrategy;
@@ -714,6 +703,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<Telemetry>
 makeTelemetry(Telemetry::Setup const& setup, beast::Journal journal)
 {

src/libxrpl/telemetry/TelemetryConfig.cpp

@@ -19,6 +19,7 @@
 #include <optional>
 #include <stdexcept>
 #include <string>
+#include <string_view>
 #include <system_error>
 #include <type_traits>
 
@@ -54,6 +55,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
 
 /**
@@ -226,6 +228,61 @@ requirePositive(std::chrono::milliseconds value, 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::runtime_error>(
+        std::string("Invalid value '") + configKey + "' in " + kSectionLabel +
+        ": must start with '" + std::string{kHttpsPrefix} + "' when " + key::tlsClientCert +
+        " is set, but is '" + endpoint + "'.");
+}
+
+/**
+ * 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::runtime_error>(
+        std::string("Invalid value '") + key::consensusTraceStrategy + "' in " + kSectionLabel +
+        ": must be '" + strategyName(ConsensusTraceStrategy::Deterministic) + "' or '" +
+        strategyName(ConsensusTraceStrategy::Random) + "'.");
+}
+
 }  // namespace
 
 /**
@@ -308,6 +365,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
@@ -376,7 +442,7 @@ makeTelemetrySetup(
     setup.traceLedger = section.valueOr<int>(key::traceLedger, 1) != 0;
 
     setup.consensusTraceStrategy =
-        section.valueOr<std::string>("consensus_trace_strategy", "deterministic");
+        readConsensusTraceStrategy(section.valueOr<std::string>(key::consensusTraceStrategy, ""));
 
     return setup;
 }

src/tests/libxrpl/telemetry/SpanGuardScope.cpp

@@ -171,14 +171,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<opentelemetry::trace::Tracer>

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.
  *
@@ -122,6 +141,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
 
 /**
@@ -259,6 +279,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)
@@ -340,6 +361,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);
 
@@ -462,6 +484,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")));
 
@@ -479,6 +502,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);
 
@@ -514,6 +538,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);
@@ -567,6 +592,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(
+        [&section] { mtls::parseSection(section); },
+        ThrowsMessage<std::runtime_error>(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(
+        [&section] { mtls::parseSection(section); },
+        ThrowsMessage<std::runtime_error>(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(
+        [&section] { mtls::parseSection(section); },
+        ThrowsMessage<std::runtime_error>(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 =
@@ -858,6 +1000,71 @@ TEST(TelemetryConfig, metric_export_small_interval_against_default_timeout_throw
             HasSubstr(cadence::keyInterval))));
 }
 
+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;

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 <xrpl/basics/FileUtilities.h>
+#include <xrpl/config/BasicConfig.h>
+#include <xrpl/telemetry/Telemetry.h>
+
+#include <gtest/gtest.h>
+
+#include <fstream>
+#include <string>
+
+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

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<int64_t>(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<int64_t>(prevLgr.seq()) + 1);
     roundSpan_->setAttribute(cs::attr::previousLedgerSeq, static_cast<int64_t>(prevLgr.seq()));
     roundSpan_->setAttribute(cs::attr::previousProposers, static_cast<int64_t>(prevProposers_));

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

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");
 /**