From c1a1421aa058f37d555663c86426de4665215a3b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:41:38 +0100 Subject: [PATCH 01/38] docs: Document the [telemetry] trace toggles as 0 or 1 The configuration reference typed the five trace_* switches as bool with default true. An xrpld config section carries integers, and these keys are read with an integer cast, so a literal "true" fails to convert rather than enabling the switch. Type them as 0 or 1 with default 1, matching the other integer-valued keys in the same table. --- OpenTelemetryPlan/05-configuration-reference.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 9ea29ade1d..579ee10a66 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -24,11 +24,11 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme | `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` | bool | `true` | Enable transaction tracing | -| `trace_consensus` | bool | `true` | Enable consensus tracing | -| `trace_rpc` | bool | `true` | Enable RPC tracing | -| `trace_peer` | bool | `true` | Enable peer message tracing (high volume) | -| `trace_ledger` | bool | `true` | Enable ledger tracing | +| `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 | | `service_name` | string | `"xrpld"` | Service name (`service.name`) for traces and metrics | | `service_instance_id` | string | `` | Instance identifier | From 251cd181a7a9cc3315a9cbe208e9dc6cb18b446c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:29:02 +0100 Subject: [PATCH 02/38] fix: Reject unreadable [telemetry] TLS certificate paths at startup With telemetry enabled and use_tls=1, makeTelemetrySetup now reads each non-empty tls_ca_cert / tls_client_cert / tls_client_key path and refuses to start when the file is missing or cannot be read. The message names the config key, the path and the OS error, instead of leaving the problem to surface much later as an opaque TLS handshake failure inside the exporter. Reading the file with getFileContents, as the gRPC server already does for its own ssl_cert and ssl_key pair, proves the file is both present and readable; an existence test alone would miss a permissions problem. The contents are discarded. Both gates are deliberate. The check is skipped when enabled is 0, so a stale cert line still cannot stop a node from booting, and when use_tls is 0, where the exporter never opens the files. An empty path stays valid; for tls_ca_cert it selects the system CA store. Six GTest cases cover the three keys that can fail, the all-readable case, and each gate on its own. --- src/libxrpl/telemetry/TelemetryConfig.cpp | 43 ++++ .../libxrpl/telemetry/TelemetryConfig.cpp | 193 ++++++++++++++++-- 2 files changed, 220 insertions(+), 16 deletions(-) diff --git a/src/libxrpl/telemetry/TelemetryConfig.cpp b/src/libxrpl/telemetry/TelemetryConfig.cpp index 006f062009..f0cc61517f 100644 --- a/src/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/libxrpl/telemetry/TelemetryConfig.cpp @@ -8,6 +8,7 @@ * See cfg/xrpld-example.cfg for the full list of available options. */ +#include #include #include #include @@ -16,6 +17,7 @@ #include #include #include +#include namespace xrpl::telemetry { @@ -85,6 +87,35 @@ networkTypeFromId(std::uint32_t networkId) } } +/** + * Throw unless the given path names a file this process can read. + * + * An empty path means the option is unset, which every caller allows. Reading + * the file proves it is both present and readable; testing existence alone + * would miss a permissions problem. The contents are discarded — nothing here + * checks that they parse as PEM. + * + * @param path Path taken from the config, possibly empty. + * @param configKey Config key the path came from, named in the message. Not + * called `key`, which would hide the `key` namespace above. + * @throws std::runtime_error If the path is non-empty and cannot be read. + */ +void +requireReadableFile(std::string const& path, char const* configKey) +{ + if (path.empty()) + return; + + std::error_code ec; + getFileContents(ec, path); + if (ec) + { + Throw( + std::string{"[telemetry] "} + configKey + " cannot be read: " + path + " - " + + ec.message()); + } +} + } // namespace Telemetry::Setup @@ -139,6 +170,18 @@ makeTelemetrySetup( "[telemetry] tls_client_cert/tls_client_key require use_tls=1 " "(set use_tls=1 to enable mutual TLS, or remove the cert paths)."); } + + // 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 + // later as an opaque handshake failure into a startup error naming the + // key. Each path is optional; an empty one is skipped. + if (setup.useTls) + { + requireReadableFile(setup.tlsCertPath, key::tlsCaCert); + requireReadableFile(setup.tlsClientCertPath, key::tlsClientCert); + requireReadableFile(setup.tlsClientKeyPath, key::tlsClientKey); + } } // Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index 2aff850c84..56a3ef4447 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -5,10 +6,13 @@ #include #include +#include #include +#include using namespace xrpl; +using ::testing::AllOf; using ::testing::HasSubstr; using ::testing::ThrowsMessage; @@ -25,15 +29,18 @@ namespace { * or an unexpected throw. Tests that never set the key are unaffected. One * source of truth still keeps the two files from drifting apart. * - * clientCert and clientKey are the paths written to those keys. They are + * clientCert and clientKey are the paths written to those keys. They name files + * that do not exist, so they suit only the cases the readability check cannot + * reach: telemetry off, or use_tls off. A case with enabled=1 and use_tls=1 + * must write real files with writeCertFile() below instead. They are * declared as `char const*` so they pass to Section::set() (which takes * `std::string const&`) and compare against the parsed std::string members * without an explicit conversion, exactly as a literal would. * - * pairingError and useTlsError are message fragments. Both guards throw - * std::runtime_error, so the exception type alone cannot tell them apart. - * Each fragment occurs in exactly one of the two messages, so matching it - * proves which guard fired. + * pairingError, useTlsError and readError are message fragments. All three + * guards throw std::runtime_error, so the exception type alone cannot tell + * them apart. Each fragment occurs in exactly one of the three messages, so + * matching it proves which guard fired. */ namespace mtls { constexpr char const* keyClientCert = "tls_client_cert"; @@ -42,6 +49,7 @@ constexpr char const* clientCert = "/etc/ssl/client.pem"; constexpr char const* clientKey = "/etc/ssl/client.key"; constexpr char const* pairingError = "must be set together"; constexpr char const* useTlsError = "require use_tls=1"; +constexpr char const* readError = "cannot be read"; /** * Build a [telemetry] section carrying only the `enabled` key. @@ -75,6 +83,27 @@ parseSection(Section const& section) { return telemetry::makeTelemetrySetup(section, "nHUtest123", "2.0.0", 0); } + +/** + * Write a placeholder certificate file at the given path. + * + * The parser only needs the file to exist and be readable, so the contents are + * irrelevant — nothing checks that they parse as PEM. The stream state is + * asserted, so a failed write shows up as a setup failure here rather than as a + * confusing failure in the case under test. + * + * @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 mtls } // namespace @@ -121,6 +150,10 @@ TEST(TelemetryConfig, parse_empty_section) TEST(TelemetryConfig, parse_full_section) { + // The CA path has to name a real file: with enabled=1 and use_tls=1 the + // parser opens it, so a placeholder path would make this case throw. + TempDir const dir; + auto const caCert = mtls::writeCertFile(dir.file("ca.pem")); Section section; section.set("enabled", "1"); section.set("service_name", "my-rippled"); @@ -128,7 +161,7 @@ TEST(TelemetryConfig, parse_full_section) section.set("exporter", "otlp_http"); section.set("endpoint", "http://collector:4318/v1/traces"); section.set("use_tls", "1"); - section.set("tls_ca_cert", "/etc/ssl/ca.pem"); + section.set("tls_ca_cert", caCert); section.set("batch_size", "256"); section.set("batch_delay_ms", "3000"); section.set("max_queue_size", "4096"); @@ -145,7 +178,7 @@ TEST(TelemetryConfig, parse_full_section) EXPECT_EQ(setup.serviceInstanceId, "custom-id"); EXPECT_EQ(setup.exporterEndpoint, "http://collector:4318/v1/traces"); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); + EXPECT_EQ(setup.tlsCertPath, caCert); EXPECT_EQ(setup.batchSize, 256u); EXPECT_EQ(setup.batchDelay, std::chrono::milliseconds{3000}); EXPECT_EQ(setup.maxQueueSize, 4096u); @@ -158,17 +191,24 @@ TEST(TelemetryConfig, parse_full_section) TEST(TelemetryConfig, mtls_cert_and_key_both_set) { - // Telemetry on and use_tls=1, so both guards run and neither may fire. + // Telemetry on and use_tls=1, so all three checks run and none may fire. + // Both paths have to name real files, because the parser opens them here. + // No CA bundle is set, which is the case this covers: mTLS against a + // collector whose certificate the system CA store already vouches for. + TempDir const dir; + auto const cert = mtls::writeCertFile(dir.file("client.pem")); + auto const key = mtls::writeCertFile(dir.file("client.key")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); - section.set(mtls::keyClientCert, mtls::clientCert); - section.set(mtls::keyClientKey, mtls::clientKey); + section.set(mtls::keyClientCert, cert); + section.set(mtls::keyClientKey, key); auto const setup = mtls::parseSection(section); EXPECT_TRUE(setup.enabled); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsClientCertPath, mtls::clientCert); - EXPECT_EQ(setup.tlsClientKeyPath, mtls::clientKey); + EXPECT_TRUE(setup.tlsCertPath.empty()); + EXPECT_EQ(setup.tlsClientCertPath, cert); + EXPECT_EQ(setup.tlsClientKeyPath, key); } TEST(TelemetryConfig, mtls_cert_without_key_throws) @@ -252,20 +292,141 @@ TEST(TelemetryConfig, mtls_default_no_client_tls_is_accepted) TEST(TelemetryConfig, mtls_neither_set_is_one_way_tls) { - // Telemetry is on so the guards run, and this config must pass both: - // one-way TLS with a CA bundle and no client certificate. + // Telemetry is on so the checks run, and this config must pass all of + // them: one-way TLS with a CA bundle and no client certificate. The CA + // path has to name a real file, because the parser opens it here. + TempDir const dir; + auto const caCert = mtls::writeCertFile(dir.file("ca.pem")); Section section = mtls::makeSection(true); section.set("use_tls", "1"); - section.set("tls_ca_cert", "/etc/ssl/ca.pem"); + section.set("tls_ca_cert", caCert); auto const setup = mtls::parseSection(section); EXPECT_TRUE(setup.enabled); EXPECT_TRUE(setup.useTls); - EXPECT_EQ(setup.tlsCertPath, "/etc/ssl/ca.pem"); + EXPECT_EQ(setup.tlsCertPath, caCert); EXPECT_TRUE(setup.tlsClientCertPath.empty()); EXPECT_TRUE(setup.tlsClientKeyPath.empty()); } +TEST(TelemetryConfig, tls_missing_client_cert_file_throws) +{ + // Both client paths are set and use_tls=1, so neither contradiction guard + // can fire and the readability check is the only reachable throw. Only the + // certificate is absent, so the message must name that key and that path. + // + // This case and the two below use an absent file. A file that exists but + // denies read permission is deliberately not covered: a test process + // running as root reads it anyway, so the case would not be reliable. + TempDir const dir; + auto const absentCert = dir.file("absent.pem"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, absentCert); + section.set(mtls::keyClientKey, mtls::writeCertFile(dir.file("k.pem"))); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::readError), HasSubstr(mtls::keyClientCert), HasSubstr(absentCert)))); +} + +TEST(TelemetryConfig, tls_missing_client_key_file_throws) +{ + // The mirror image of the case above: the certificate is readable and only + // the private key is absent, so the key's name must appear instead. + TempDir const dir; + auto const absentKey = dir.file("absent.key"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, mtls::writeCertFile(dir.file("c.pem"))); + section.set(mtls::keyClientKey, absentKey); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage(AllOf( + HasSubstr(mtls::readError), HasSubstr(mtls::keyClientKey), HasSubstr(absentKey)))); +} + +TEST(TelemetryConfig, tls_missing_ca_cert_file_throws) +{ + // One-way TLS with no client certificate, so the CA bundle is the only + // path checked. + TempDir const dir; + auto const absentCa = dir.file("absent-ca.pem"); + Section section = mtls::makeSection(true); + section.set("use_tls", "1"); + section.set("tls_ca_cert", absentCa); + + EXPECT_THAT( + [§ion] { mtls::parseSection(section); }, + ThrowsMessage( + AllOf(HasSubstr(mtls::readError), HasSubstr("tls_ca_cert"), HasSubstr(absentCa)))); +} + +TEST(TelemetryConfig, tls_readable_files_are_accepted) +{ + // Full mTLS with all three files present and readable: parsing must + // succeed and keep every path verbatim. + TempDir const dir; + auto const ca = mtls::writeCertFile(dir.file("ca.pem")); + 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("tls_ca_cert", ca); + section.set(mtls::keyClientCert, cert); + section.set(mtls::keyClientKey, key); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_TRUE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsCertPath, ca); + EXPECT_EQ(setup.tlsClientCertPath, cert); + EXPECT_EQ(setup.tlsClientKeyPath, key); +} + +TEST(TelemetryConfig, tls_paths_not_checked_when_telemetry_disabled) +{ + // Telemetry off, so the files are never opened and absent paths must not + // stop the node from booting. use_tls stays 1 here, so the `enabled` gate + // is the only thing that can be suppressing the check. + TempDir const dir; + auto const absentCert = dir.file("absent.pem"); + auto const absentKey = dir.file("absent.key"); + Section section = mtls::makeSection(false); + section.set("use_tls", "1"); + section.set(mtls::keyClientCert, absentCert); + section.set(mtls::keyClientKey, absentKey); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_FALSE(setup.enabled); + EXPECT_TRUE(setup.useTls); + EXPECT_EQ(setup.tlsClientCertPath, absentCert); + EXPECT_EQ(setup.tlsClientKeyPath, absentKey); +} + +TEST(TelemetryConfig, tls_ca_cert_not_checked_when_use_tls_off) +{ + // With TLS off the exporter never reads the CA path, so a missing file + // must not stop startup. Telemetry stays on here, so the use_tls gate is + // the only thing that can be suppressing the check. The client-cert keys + // cannot be used for this case: they trip the use_tls contradiction guard + // before any file is opened. + TempDir const dir; + auto const absentCa = dir.file("absent-ca.pem"); + Section section = mtls::makeSection(true); + section.set("tls_ca_cert", absentCa); + + telemetry::Telemetry::Setup setup; + ASSERT_NO_THROW(setup = mtls::parseSection(section)); + EXPECT_TRUE(setup.enabled); + EXPECT_FALSE(setup.useTls); + EXPECT_EQ(setup.tlsCertPath, absentCa); +} + TEST(TelemetryConfig, null_telemetry_factory) { telemetry::Telemetry::Setup setup; From a24db2e995b29ea749b298e522ce7ef1de798541 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:29:35 +0100 Subject: [PATCH 03/38] docs: Document the [telemetry] TLS path readability check Bring the three documentation surfaces in line with the new parse-time check: - The @throws clause on makeTelemetrySetup now names the third failure condition and records that an empty path is skipped. - cfg/xrpld-example.cfg states, under all three TLS keys, that with enabled=1 and use_tls=1 a path that does not exist or cannot be read stops startup. The tls_ca_cert wording still says that empty selects the system CA store, since only a path that is set is checked. - The runbook troubleshooting entry gains a third bullet for the "cannot be read" message, whose remedy is the path or its permissions rather than the certificate and key pairing. Documentation only; no behaviour change. --- cfg/xrpld-example.cfg | 19 ++++++++++++++----- docs/telemetry-runbook.md | 21 ++++++++++++++------- include/xrpl/telemetry/Telemetry.h | 5 ++++- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/cfg/xrpld-example.cfg b/cfg/xrpld-example.cfg index a3bd8673ce..9f74c99c82 100644 --- a/cfg/xrpld-example.cfg +++ b/cfg/xrpld-example.cfg @@ -1714,6 +1714,11 @@ validators.txt # Path to a PEM-encoded CA certificate bundle for TLS verification. # Only used when use_tls=1. Default: empty (system CA store). # +# Leaving this empty stays valid and selects the system CA store. A path +# that is set is checked like the client paths below: with enabled=1 and +# use_tls=1, one that does not exist or cannot be read makes xrpld fail +# to start. +# # tls_client_cert= # # Path to this node's PEM-encoded client certificate, presented to the @@ -1723,15 +1728,19 @@ 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. -# Both checks apply only when enabled=1; with telemetry disabled these -# settings are read but never validated. +# 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. # # tls_client_key= # # Path to the PEM-encoded private key for tls_client_cert. Required -# whenever tls_client_cert is set. Requires use_tls=1. Both conditions -# are enforced exactly as described under tls_client_cert above: when -# enabled=1, breaking either one makes xrpld fail to start. +# whenever tls_client_cert is set. Requires use_tls=1, and must be +# readable. All three conditions are enforced exactly as described under +# tls_client_cert above: when enabled=1, breaking any one of them makes +# xrpld fail to start. # Default: empty. # # Head sampling is intentionally fixed at 1.0 (sample everything) and is diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index f30b87eaea..eb3a6e9c28 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -612,12 +612,13 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: exception thrown while the `Application` object is constructed prints the same `Unable to start` prefix, so confirm the text after the colon begins with `[telemetry]` before using this entry -- Cause: the `[telemetry]` mTLS keys (`tls_client_cert` and `tls_client_key`) - contradict each other. Only these two mTLS checks are gated on `enabled=1`; - the rest of the section is still read when telemetry is off, so a malformed - value in any key — including `enabled` itself, which is read before the gate - — still fails startup with a different message -- Fix: the two checks need different remedies, and the printed message says +- Cause: either the `[telemetry]` mTLS keys (`tls_client_cert` and + `tls_client_key`) contradict each other, or one of the TLS certificate paths + cannot be read. Only these three checks are gated on `enabled=1`; the rest of + the section is still read when telemetry is off, so a malformed value in any + key — including `enabled` itself, which is read before the gate — still fails + startup with a different message +- Fix: the three checks need different remedies, and the printed message says which one fired - `tls_client_cert and tls_client_key must be set together` — exactly one of the two paths is set. Either delete the one that is set, or add the missing @@ -626,8 +627,14 @@ Three dashboards are pre-provisioned in `docker/telemetry/grafana/dashboards/`: - `tls_client_cert/tls_client_key require use_tls=1` — both paths are set but TLS is off. Either set `use_tls=1`, or delete **both** paths. Deleting only one of them trips the first check + - ` cannot be read` — the named key (`tls_ca_cert`, `tls_client_cert` or + `tls_client_key`) points at a file the node cannot open; the message also + prints the path and the OS error. Fix the path or its permissions — the + pairing is not what is wrong here. This check runs only when `use_tls=1`, + and an empty `tls_ca_cert` is always accepted (it selects the system CA + store) - If you did not mean to enable telemetry at all, set `enabled=0` — that - clears both checks whichever one fired + clears all three checks whichever one fired ## Performance Tuning diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index a34e3cf19b..bf5af0156c 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -431,7 +431,10 @@ 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. Those two + * is given, or a client certificate is given while `use_tls` is 0. 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 * 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 From d1b80e47a29a040eddb55d68c611e117ec8ffa66 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:25:24 +0100 Subject: [PATCH 04/38] test(telemetry): void span baselines captured on the old span ladder --- docker/telemetry/workload/baselines/README.md | 22 ++++ .../workload/baselines/baseline-timings.json | 102 +----------------- .../workload/regression-thresholds.json | 2 +- 3 files changed, 26 insertions(+), 100 deletions(-) diff --git a/docker/telemetry/workload/baselines/README.md b/docker/telemetry/workload/baselines/README.md index fffea2e34d..4adfbe9b18 100644 --- a/docker/telemetry/workload/baselines/README.md +++ b/docker/telemetry/workload/baselines/README.md @@ -25,6 +25,28 @@ was invoked with. Capture and comparison are profile-agnostic — they only read Prometheus — so all existing profiles (`full-validation`, `quick-smoke`, `stress`) continue to work unchanged. +## Current state: the baseline is a placeholder + +`baseline-timings.json` currently carries `"placeholder": true` and an empty `metrics` object, +so **no metric gates right now**. Its entries were captured on 2026-06-05 against a spanmetrics +ladder that was re-cut on 2026-08-04 in `3860c93db2`, which makes every sub-millisecond quantile +in that capture bucket-edge arithmetic rather than a latency (a p95 of `0.95` ms is `0.95 × 1 ms`). +Because the comparator only flags a metric when the current value _exceeds_ the baseline, a +stale-high baseline passes everything silently — so the entries were voided instead of left in +place. The file's `_note` records why, and which numbers were dropped. + +To restore gating, follow [Bootstrapping the baseline](#bootstrapping-the-baseline) below — a +placeholder is exactly the state that loop expects. Pasting the CI block **replaces the whole +file**, `_note` included; that is intended, and the voided numbers stay retrievable from this +file's git history. + +**Do not let the placeholder outlive one run.** CI stays green the whole time the placeholder +stands, so an un-copied block is not a failure anyone will notice — it is a silent loss of +regression coverage that looks identical to a passing gate. + +Voiding a baseline is the one hand edit this file allows; _setting_ one always comes from a +printed CI block, per the "Refreshing the baseline" rule below. + ## Bootstrapping the baseline 1. Merge a CI run with a `"placeholder": true` baseline. The telemetry-validation diff --git a/docker/telemetry/workload/baselines/baseline-timings.json b/docker/telemetry/workload/baselines/baseline-timings.json index 4e9633568b..0f9b858e5e 100644 --- a/docker/telemetry/workload/baselines/baseline-timings.json +++ b/docker/telemetry/workload/baselines/baseline-timings.json @@ -1,105 +1,9 @@ { - "_note": "job.* entries were removed on 2026-08-21. They were captured against the old microsecond ladder whose first edge was 100us, with 99.3% of job_queued_us samples beneath it, so job.acceptLedger.queued.p95 = 96.79us was 0.95/0.9926 x 100 -- arithmetic on the bucket edge, not a latency. Recapture them on a node running the re-cut ladder (floor 1us); until then the comparer reports them as \"new metric (not in baseline)\" and gates only the span metrics, which are unaffected. Removed values, for reference: job.acceptLedger.queued.p95=96.79us, job.acceptLedger.running.p95=10562.50us, job.transaction.queued.p95=478.97us, job.transaction.running.p95=494.14us.", + "_note": "PLACEHOLDER -- every entry was voided, and the next telemetry-validation run reprints a fresh block to paste back (see baselines/README.md). Span entries were captured 2026-06-05 against a spanmetrics ladder whose first edge was 1ms. That ladder had no edge below 1ms at all, so for the sub-millisecond spans nearly every sample landed in bucket 0 and their quantiles are interpolation across that single edge, not latencies: span.ledger.store read p50/p95/p99 = 0.5/0.95/0.99, exactly quantile x 1ms, and span.tx.process read 0.50428/0.95813/0.99847 for the same reason. Sub-millisecond edges (floor 0.01ms) landed 2026-08-04 in 3860c93db2, so the ladder those numbers came from no longer exists. The gate is one-sided -- compare_to_baseline.py flags a regression only when current exceeds baseline -- so a stale-high baseline passes everything in silence, including a real 10x regression from 0.05ms to 0.5ms hiding under the 0.95ms entry. The 11 entries at or above 1ms (consensus.accept p50/p95/p99, consensus.ledger_close p95/p99, ledger.build p95/p99, ledger.validate p95/p99, tx.apply p95/p99) were undistorted -- but only because every edge from 1ms to 1s is byte-identical between the two ladders and all 11 happen to fall in that band, NOT because the re-cut spared everything above 1ms. It did not: it also added 2s/3s/4s/10s/30s edges above 1s, so a span whose quantiles reach the second scale is distorted just as much. Those 11 are voided with the rest anyway: a half-filled metrics object is not a placeholder, so the gate would stay half-dead with nothing to signal it. Removed span values, for reference (ms, p50/p95/p99): consensus.accept = 1.059405940594059/9.749999999999996/23.704545454545432; consensus.ledger_close = 0.5284697508896797/1.511111111111103/7.878571428571429; ledger.build = 0.7412060301507538/4.611111111111112/7.541666666666674; ledger.store = 0.5/0.95/0.9900000000000001; ledger.validate = 0.5283687943262412/1.3666666666666627/6.699999999999978; rpc.ws_message = 0.5026522773001647/0.9550393268703128/0.9952515090543261; tx.apply = 0.6330472103004292/4.203389830508474/5.083333333333319; tx.process = 0.5042801992591597/0.9581323781882418/0.998474791584883. job.* entries were removed earlier, on 2026-08-21, for the same reason on the native microsecond ladder: its first edge was 100us with 99.3% of job_queued_us samples beneath it, so job.acceptLedger.queued.p95 = 96.79us was 0.95/0.9926 x 100. That earlier note also claimed the gate then \"gates only the span metrics, which are unaffected\" -- that was wrong, for the reasons recorded above. Removed job values, for reference (us): job.acceptLedger.queued.p95=96.79, job.acceptLedger.running.p95=10562.50, job.transaction.queued.p95=478.97, job.transaction.running.p95=494.14. Both groups need recapture on a node running the re-cut ladders. captured_at, git_sha, profile and window below describe the voided run, not a live baseline.", "captured_at": "2026-06-05T18:41:52Z", "git_sha": "fd1c8c6060f7a15cc9e65b16f99629d9ab7ac7dc", - "metrics": { - "span.consensus.accept.p50": { - "unit": "ms", - "value": 1.059405940594059 - }, - "span.consensus.accept.p95": { - "unit": "ms", - "value": 9.749999999999996 - }, - "span.consensus.accept.p99": { - "unit": "ms", - "value": 23.704545454545432 - }, - "span.consensus.ledger_close.p50": { - "unit": "ms", - "value": 0.5284697508896797 - }, - "span.consensus.ledger_close.p95": { - "unit": "ms", - "value": 1.511111111111103 - }, - "span.consensus.ledger_close.p99": { - "unit": "ms", - "value": 7.878571428571429 - }, - "span.ledger.build.p50": { - "unit": "ms", - "value": 0.7412060301507538 - }, - "span.ledger.build.p95": { - "unit": "ms", - "value": 4.611111111111112 - }, - "span.ledger.build.p99": { - "unit": "ms", - "value": 7.541666666666674 - }, - "span.ledger.store.p50": { - "unit": "ms", - "value": 0.5 - }, - "span.ledger.store.p95": { - "unit": "ms", - "value": 0.95 - }, - "span.ledger.store.p99": { - "unit": "ms", - "value": 0.9900000000000001 - }, - "span.ledger.validate.p50": { - "unit": "ms", - "value": 0.5283687943262412 - }, - "span.ledger.validate.p95": { - "unit": "ms", - "value": 1.3666666666666627 - }, - "span.ledger.validate.p99": { - "unit": "ms", - "value": 6.699999999999978 - }, - "span.rpc.ws_message.p50": { - "unit": "ms", - "value": 0.5026522773001647 - }, - "span.rpc.ws_message.p95": { - "unit": "ms", - "value": 0.9550393268703128 - }, - "span.rpc.ws_message.p99": { - "unit": "ms", - "value": 0.9952515090543261 - }, - "span.tx.apply.p50": { - "unit": "ms", - "value": 0.6330472103004292 - }, - "span.tx.apply.p95": { - "unit": "ms", - "value": 4.203389830508474 - }, - "span.tx.apply.p99": { - "unit": "ms", - "value": 5.083333333333319 - }, - "span.tx.process.p50": { - "unit": "ms", - "value": 0.5042801992591597 - }, - "span.tx.process.p95": { - "unit": "ms", - "value": 0.9581323781882418 - }, - "span.tx.process.p99": { - "unit": "ms", - "value": 0.998474791584883 - } - }, + "metrics": {}, + "placeholder": true, "profile": "full-validation", "schema_version": 1, "window": "3m" diff --git a/docker/telemetry/workload/regression-thresholds.json b/docker/telemetry/workload/regression-thresholds.json index 0dba5b6845..7e474569a9 100644 --- a/docker/telemetry/workload/regression-thresholds.json +++ b/docker/telemetry/workload/regression-thresholds.json @@ -1,6 +1,6 @@ { "_description": "Per-metric regression thresholds. A metric regresses when current - baseline exceeds BOTH the percentage and absolute bounds (AND, not OR — this tolerates small-value noise). Defaults apply unless a per-metric override exists.", - "_bucket_note": "SpanMetrics latency histograms use explicit buckets [0.01,0.05,0.1,0.25,0.5,1,5,10,25,50,100,250,500]ms then [1,2,3,4,5,10,30]s (20 edges; docker/telemetry/otel-collector-config.yaml is the authoritative list). An earlier version of this note claimed 15 edges starting at 1ms and justified the 10ms absolute span bound as \"~2 low-end bucket widths\" — that derivation is void, because the sub-millisecond edges make the low-end bucket width 0.01ms, not 5ms. The 10ms bound is retained on its own merit: it is roughly two bucket widths in the 5-25ms band where most span quantiles actually sit, so it still absorbs single-bucket quantization jitter while catching multi-bucket regressions. Second-scale consensus spans have 2s/3s/4s boundaries, so their quantiles quantize to ~1s widths there. The job_queue running bound is widened similarly — per-ledger apply work scales with TxQ burst load. NOTE: the native job_queue histograms are microsecond-valued and their ladder was re-cut (floor 100us → 1us), so any job_queue baseline captured before that change is an interpolation artefact, not a latency.", + "_bucket_note": "SpanMetrics latency histograms use explicit buckets [0.01,0.05,0.1,0.25,0.5,1,5,10,25,50,100,250,500]ms then [1,2,3,4,5,10,30]s (20 edges; docker/telemetry/otel-collector-config.yaml is the authoritative list). An earlier version of this note claimed 15 edges starting at 1ms and justified the 10ms absolute span bound as \"~2 low-end bucket widths\" — that derivation is void, because the sub-millisecond edges make the low-end bucket width 0.01ms, not 5ms. The 10ms bound is retained on its own merit: it is roughly two bucket widths in the 5-25ms band where most span quantiles actually sit, so it still absorbs single-bucket quantization jitter while catching multi-bucket regressions. Second-scale consensus spans have 2s/3s/4s boundaries, so their quantiles quantize to ~1s widths there. The job_queue running bound is widened similarly — per-ledger apply work scales with TxQ burst load. NOTE: BOTH ladders were re-cut, and a baseline captured before its own ladder changed is an interpolation artefact, not a latency. The native job_queue histograms are microsecond-valued and their floor moved 100us → 1us. The span ladder was re-cut too, on 2026-08-04 in 3860c93db2, moving the floor 1ms → 0.01ms; so any sub-millisecond span quantile captured before that date is equally void — a p95 reading 0.95ms is 0.95 × the old 1ms first edge, not a measurement. An earlier note asserted that the surviving span baselines were unaffected by the ladder work; that is wrong for every span quantile below 1ms. Only the band from 1ms to 1s is safe: those edges are byte-identical across the two ladders. The re-cut also ADDED edges above 1s (2s/3s/4s/10s/30s), so a span whose quantiles land in the second-scale range — consensus.round ~3.9s, consensus.establish ~1.9s, the ledger.acquire tail — is distorted just as much, and any pre-2026-08-04 baseline for it is equally void. Do not read this note as licensing a stale second-scale baseline.", "defaults": { "span": { "p50": { "max_pct_increase": 50.0, "max_abs_increase_ms": 10.0 }, From 14badbfdd744e22049243b78de98f6f03f5d7764 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:46:18 +0100 Subject: [PATCH 05/38] docs(telemetry): record rpc_size_bytes and the other unasserted histograms --- docker/telemetry/workload/expected_metrics.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docker/telemetry/workload/expected_metrics.json b/docker/telemetry/workload/expected_metrics.json index 2aa1d0fcb2..078f6caa84 100644 --- a/docker/telemetry/workload/expected_metrics.json +++ b/docker/telemetry/workload/expected_metrics.json @@ -175,7 +175,11 @@ "getobject_request_objects": "GetObjectMetricNames.h:86, emitted from PeerImp.cpp:2926 only while serving an inbound TMGetObjectByHash. The XRPL_METRIC_* macros create their instrument lazily on first use (MetricMacros.h:174-285), so no series exists until a peer actually requests objects by hash — which a 5-node cluster started at genesis and already in sync may never do.", "getobject_lookup_us": "GetObjectMetricNames.h:95, PeerImp.cpp:2929. Same lazy-creation and same inbound-request gate as getobject_request_objects.", "getobject_lookups_total": "GetObjectMetricNames.h:100, PeerImp.cpp:2949/:2956. Same gate.", - "getobject_charge": "GetObjectMetricNames.h:105, PeerImp.cpp:2931. Same gate." + "getobject_charge": "GetObjectMetricNames.h:105, PeerImp.cpp:2931. Same gate.", + "rpc_size_bytes": "ServerHandler.cpp:191, group('rpc')->makeEvent('size', Unit::Bytes). The OTLP Prometheus exporter derives the metric-name suffix from the declared unit, so a byte unit yields rpc_size_bytes; before 24094e427b the event declared no unit and exported as rpc_size_milliseconds on the millisecond bucket ladder. Neither name was ever recorded here, so the harness could confirm neither the rename nor a regression back onto that ladder. Notified from ServerHandler::processRequest:1133, the HTTP JSON-RPC path — it computes an HTTP status and appends a trailing newline — and the load generators are WebSocket-only, the same gate regression-metrics.json:4 records for rpc.process, so only the harness's handful of HTTP health polls reach it. Real coverage needs an HTTP JSON-RPC phase in rpc_load_generator.py; that is a workload change rather than a harness correction, and is deliberately out of scope here.", + "rpc_time_milliseconds": "ServerHandler.cpp:192, group('rpc')->makeEvent('time') with the default millisecond unit. Notified from ServerHandler::processRequest:1129, the same HTTP JSON-RPC call site as rpc_size_bytes and behind the same WebSocket-only gate.", + "ios_latency_milliseconds": "Application.cpp:515, makeEvent('ios_latency') with the default millisecond unit. Emitted and dashboarded, but it measures io-service scheduling delay rather than anything the workload drives, so no workload phase guarantees it.", + "jobq_*_milliseconds, jobq_*_q_milliseconds": "Created per job type in JobTypeData.h:97-98 from info.name() and info.name() + kSuffixQueued ('_q'), so the exported names are jobq__milliseconds and jobq__q_milliseconds with the job type lowercased by formatName. Which job types appear depends on which jobs a run happens to schedule, so no individual name is guaranteed. They are also rounded up to a whole millisecond at source (Event.h:47-51 applies ceil to a millisecond value type), which is why 6e2b2da772 moved the ledger-data-sync q-wait panels off jobq__q_milliseconds_bucket onto job_queued_us_bucket — they are poor assertion targets regardless." } }, "grafana_dashboards": { From 2afae6655ddd3c226825d52cb7fb8929981c8bd9 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 24 Aug 2026 19:53:43 +0100 Subject: [PATCH 06/38] test(telemetry): assert xrpl_node_id reaches the metric series --- docker/telemetry/workload/README.md | 5 +- .../telemetry/workload/expected_metrics.json | 5 +- .../telemetry/workload/validate_telemetry.py | 219 +++++++++++++++--- 3 files changed, 199 insertions(+), 30 deletions(-) diff --git a/docker/telemetry/workload/README.md b/docker/telemetry/workload/README.md index 9221b47713..9d96499051 100644 --- a/docker/telemetry/workload/README.md +++ b/docker/telemetry/workload/README.md @@ -416,7 +416,8 @@ them again — load shape comes entirely from `--profile` and "description": "Top-level doc string — skipped by the validator.", "category_name": { "description": "Human-readable description.", - "metrics": ["metric_1", "metric_2"] + "metrics": ["metric_1", "metric_2"], + "required_labels": ["label_1"] }, "grafana_dashboards": { "uids": ["rpc-performance", "node-health"] @@ -430,6 +431,8 @@ them again — load shape comes entirely from `--profile` and Every metric listed under a `metrics` array must produce > 0 Prometheus series during the validation run. If a metric doesn't fire, the workload generators need to produce enough load to trigger it. +`required_labels` is optional and read for every category that declares one. Each label becomes one additional check, named `metric..label.