From db0eae1b3580328872bbc0f95aa7040b79188b88 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:05:33 +0100 Subject: [PATCH] fix(telemetry): pass StartOptions, not a string, in telemetry-off tests MetricsRegistry::start() takes a StartOptions aggregate. Six call sites in the #ifndef XRPL_ENABLE_TELEMETRY block still passed a std::string, so the block did not compile. Nothing in CI compiles it: telemetry defaults ON, and the block is skipped whenever the macro is defined. Add one shared kTestStartOptions carrying just the endpoint -- the other fields are never read on the no-op path -- and correct two comments that still described the old signature and a #else stub that does not exist. --- .../libxrpl/telemetry/MetricsRegistry.cpp | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index 11fbbfaed8..af3eb68ddf 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -603,13 +603,23 @@ using namespace xrpl; namespace { /** - * OTLP/HTTP endpoint passed to every start() call below. Nothing ever dials - * it -- these tests exercise the no-op path -- it just has to be a plausible - * URL. start() takes `std::string const&`, so call sites construct one from - * this view rather than repeating the literal. + * OTLP/HTTP endpoint used by every start() call below. Nothing ever dials it + * -- these tests exercise the no-op path -- it just has to be a plausible URL. + * It reaches start() through @ref kTestStartOptions. */ constexpr std::string_view kTestEndpoint{"http://localhost:4318/v1/metrics"}; +/** + * The only StartOptions field these tests need. + * + * start() takes the StartOptions aggregate, not a string. The other fields -- + * resource identity, network id, TLS paths -- are never read on the no-op + * path, and their defaults already mean "unset". One shared value keeps all + * six call sites on the same endpoint. + */ +telemetry::MetricsRegistry::StartOptions const kTestStartOptions{ + .endpoint = std::string{kTestEndpoint}}; + /** * Minimal mock ServiceRegistry for MetricsRegistry testing. * @@ -903,7 +913,7 @@ TEST_F(MetricsRegistryTest, disabled_start_stop) telemetry::MetricsRegistry registry(false, mockApp_, j_); // start() and stop() should be no-ops when disabled. - registry.start(std::string{kTestEndpoint}); + registry.start(kTestStartOptions); registry.stop(); // Double stop should be safe. @@ -924,8 +934,9 @@ TEST_F(MetricsRegistryTest, disabled_start_stop) // (src/tests/libxrpl/CMakeLists.txt:117-126 -- the `else()` branch; when it is // ON the .cpp needs concrete xrpld types such as LedgerMaster, TxQ, NetworkOPs, // Overlay and node_store::Database, which a standalone GTest binary cannot -// link). Both start() and startAsyncGauges() therefore compile here to their -// `#else` branch, which only (void)-casts its arguments. So these tests pin the +// link). Both start() and startAsyncGauges() have a single definition whose +// whole body sits inside #ifdef XRPL_ENABLE_TELEMETRY, so here they compile to +// an empty body with a [[maybe_unused]] parameter. So these tests pin the // API SURFACE -- that both entry points exist, are callable in either order, // and leave the object usable -- and NOT the gauge behaviour. Real coverage of // "gauges observe values only after startAsyncGauges()" is unreachable from @@ -943,7 +954,7 @@ TEST_F(MetricsRegistryTest, async_gauges_start_after_start_is_safe) telemetry::MetricsRegistry registry(false, mockApp_, j_); // The documented order: provider/sync instruments first, gauges second. - registry.start(std::string{kTestEndpoint}); + registry.start(kTestStartOptions); registry.startAsyncGauges(); // State: the enable flag is untouched by either phase. Exact value, not @@ -973,7 +984,7 @@ TEST_F(MetricsRegistryTest, async_gauges_before_start_does_not_break_start) EXPECT_EQ(registry.isEnabled(), false); // Phase 1 still works afterwards, so the bad call left no state behind. - registry.start(std::string{kTestEndpoint}); + registry.start(kTestStartOptions); registry.recordJobQueued("ledgerData", "ProcessLData"); EXPECT_EQ(registry.isEnabled(), false); @@ -994,7 +1005,7 @@ TEST_F(MetricsRegistryTest, async_gauges_respect_the_compile_time_guard) // return. EXPECT_EQ(registry.isEnabled(), true); - EXPECT_NO_THROW(registry.start(std::string{kTestEndpoint})); + EXPECT_NO_THROW(registry.start(kTestStartOptions)); EXPECT_NO_THROW(registry.startAsyncGauges()); EXPECT_NO_THROW(registry.stop()); @@ -1004,7 +1015,7 @@ TEST_F(MetricsRegistryTest, async_gauges_respect_the_compile_time_guard) TEST_F(MetricsRegistryTest, disabled_recording_methods) { telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start(std::string{kTestEndpoint}); + registry.start(kTestStartOptions); // All recording methods should be no-ops (not crash). registry.recordRpcStarted("server_info"); @@ -1022,7 +1033,7 @@ TEST_F(MetricsRegistryTest, destructor_calls_stop) { // Let the destructor handle cleanup. telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start(std::string{kTestEndpoint}); + registry.start(kTestStartOptions); } // If we get here without crash, the destructor handled stop. }