Merge branch 'pratik/otel-phase9-metric-gap-fill' into pratik/otel-phase10-workload-validation

Brings forward the clang-tidy fixes for the new nodestore metric tests.

src/tests/libxrpl/ledger/AcquireStats.cpp

@@ -30,7 +30,7 @@ using xrpl::AcquireStats;
  */
 TEST(AcquireStatsTest, StartsAtZero)
 {
-    AcquireStats stats;
+    AcquireStats const stats;
 
     EXPECT_EQ(stats.getDeferrals(), 0u);
     EXPECT_EQ(stats.getTimeouts(), 0u);

src/tests/libxrpl/nodestore/Backend.cpp

@@ -184,7 +184,8 @@ TEST_P(BackendTypeTest, write_stats_reported_only_when_measured)
 
     if (GetParam() == "nudb")
     {
-        ASSERT_TRUE(stats.has_value());
+        if (!stats.has_value())
+            FAIL() << "nudb must report write stats";
         // Freshly opened and not yet written to.
         EXPECT_EQ(stats->insertCount, 0u);
         EXPECT_EQ(stats->concurrentWriters, 0u);

src/tests/libxrpl/nodestore/Database.cpp

@@ -20,6 +20,7 @@
 #include <algorithm>
 #include <atomic>
 #include <chrono>
+#include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <stdexcept>

src/tests/libxrpl/nodestore/NuDBFactory.cpp

@@ -279,7 +279,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
     // Before any write the stats exist but are all zero, and no writer is
     // in flight.
     auto const initial = backend->getWriteStats();
-    ASSERT_TRUE(initial.has_value());
+    if (!initial.has_value())
+        FAIL() << "nudb must report write stats";
     EXPECT_EQ(initial->insertCount, 0u);
     EXPECT_EQ(initial->insertTotalUs, 0u);
     EXPECT_EQ(initial->insertMaxUs, 0u);
@@ -294,7 +295,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
     storeBatch(*backend, batch);
 
     auto const after = backend->getWriteStats();
-    ASSERT_TRUE(after.has_value());
+    if (!after.has_value())
+        FAIL() << "nudb must report write stats after inserts";
     EXPECT_EQ(after->insertCount, kFirstBatch);
     EXPECT_EQ(after->depthSum, kFirstBatch);
     EXPECT_GT(after->insertTotalUs, 0u);
@@ -316,7 +318,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
     storeBatch(*backend, more);
 
     auto const cumulative = backend->getWriteStats();
-    ASSERT_TRUE(cumulative.has_value());
+    if (!cumulative.has_value())
+        FAIL() << "nudb must report write stats after a second batch";
     EXPECT_EQ(cumulative->insertCount, kFirstBatch + kSecondBatch);
     EXPECT_EQ(cumulative->depthSum, kFirstBatch + kSecondBatch);
     EXPECT_EQ(cumulative->concurrentWriters, 0u);
@@ -347,7 +350,8 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts)
     storeBatch(*backend, batch);
 
     auto const first = backend->getWriteStats();
-    ASSERT_TRUE(first.has_value());
+    if (!first.has_value())
+        FAIL() << "nudb must report write stats";
     ASSERT_EQ(first->insertCount, kBatchSize);
 
     // Re-storing the identical batch writes nothing new, but each call is
@@ -355,7 +359,8 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts)
     storeBatch(*backend, batch);
 
     auto const second = backend->getWriteStats();
-    ASSERT_TRUE(second.has_value());
+    if (!second.has_value())
+        FAIL() << "nudb must report write stats after re-storing";
     EXPECT_EQ(second->insertCount, kBatchSize * 2);
     EXPECT_EQ(second->depthSum, kBatchSize * 2);
     // The depth returned to zero, so the early-return error path did not
@@ -397,7 +402,8 @@ TEST(NuDBFactory, write_stats_observe_concurrent_writers)
         th.join();
 
     auto const stats = backend->getWriteStats();
-    ASSERT_TRUE(stats.has_value());
+    if (!stats.has_value())
+        FAIL() << "nudb must report write stats after concurrent inserts";
     EXPECT_EQ(stats->insertCount, kThreads * kPerThread);
     EXPECT_GE(stats->depthSum, stats->insertCount);
     EXPECT_EQ(stats->concurrentWriters, 0u);

src/tests/libxrpl/telemetry/MetricsRegistry.cpp

@@ -25,10 +25,12 @@
 
 #include <gtest/gtest.h>
 
+#include <algorithm>
 #include <array>
 #include <cstddef>
 #include <cstdint>
 #include <limits>
+#include <optional>
 #include <set>
 #include <string_view>
 
@@ -180,12 +182,9 @@ constexpr std::array kFoldToOtherHandlers = {
 consteval bool
 allPassThroughUnchanged()
 {
-    for (auto const name : kPassThroughHandlers)
-    {
-        if (MetricsRegistry::sanitiseHandler(name) != name)
-            return false;
-    }
-    return true;
+    return std::ranges::all_of(kPassThroughHandlers, [](auto const name) {
+        return MetricsRegistry::sanitiseHandler(name) == name;
+    });
 }
 
 /**
@@ -194,12 +193,9 @@ allPassThroughUnchanged()
 consteval bool
 allFoldToOther()
 {
-    for (auto const name : kFoldToOtherHandlers)
-    {
-        if (MetricsRegistry::sanitiseHandler(name) != MetricsRegistry::kHandlerOther)
-            return false;
-    }
-    return true;
+    return std::ranges::all_of(kFoldToOtherHandlers, [](auto const name) {
+        return MetricsRegistry::sanitiseHandler(name) == MetricsRegistry::kHandlerOther;
+    });
 }
 
 // Compile-time guarantees. Duplicated at runtime below so a failure names
@@ -337,7 +333,7 @@ TEST(MetricsRegistryScaledMean, zero_count_reports_absence_not_zero)
     // same numerator with one sample does produce a value, so the guard is
     // keyed on the count and is not rejecting everything.
     ASSERT_TRUE(Registry::scaledMean(500, 1).has_value());
-    EXPECT_EQ(*Registry::scaledMean(500, 1), 500);
+    EXPECT_EQ(Registry::scaledMean(500, 1), std::optional<std::int64_t>{500});
 }
 
 TEST(MetricsRegistryScaledMean, zero_total_over_real_samples_is_a_genuine_zero)
@@ -348,7 +344,7 @@ TEST(MetricsRegistryScaledMean, zero_total_over_real_samples_is_a_genuine_zero)
     // not suppressed, or a working fast path looks like a dead one.
     auto const mean = Registry::scaledMean(0, 32);
     ASSERT_TRUE(mean.has_value());
-    EXPECT_EQ(*mean, 0);
+    EXPECT_EQ(mean, std::optional<std::int64_t>{0});
 }
 
 TEST(MetricsRegistryScaledMean, exact_means_are_computed_exactly)
@@ -387,13 +383,15 @@ TEST(MetricsRegistryScaledMean, large_inputs_saturate_instead_of_wrapping)
     // would surface as a sudden dip to a healthy-looking small number, which
     // is the failure mode worth preventing.
     auto const saturated = Registry::scaledMean(kU64Max, 1);
-    ASSERT_TRUE(saturated.has_value());
+    if (!saturated.has_value())
+        FAIL() << "a saturated mean must still be reported";
     EXPECT_EQ(*saturated, kI64Max);
 
     // Scaling must not overflow either: this quotient times 100 exceeds
     // int64 range, so it clamps rather than wraps negative.
     auto const scaled = Registry::scaledMean(kU64Max, 2, 100);
-    ASSERT_TRUE(scaled.has_value());
+    if (!scaled.has_value())
+        FAIL() << "a saturated scaled mean must still be reported";
     EXPECT_EQ(*scaled, kI64Max);
 
     // Every result is non-negative; a negative latency or depth is
@@ -404,11 +402,9 @@ TEST(MetricsRegistryScaledMean, large_inputs_saturate_instead_of_wrapping)
     // Just below the boundary the value is exact, not clamped, so the clamp
     // above is a real bound and not a blanket ceiling on everything.
     auto const exact = Registry::scaledMean(static_cast<std::uint64_t>(kI64Max), 1);
-    ASSERT_TRUE(exact.has_value());
-    EXPECT_EQ(*exact, kI64Max);
+    EXPECT_EQ(exact, std::optional<std::int64_t>{kI64Max});
     auto const belowBoundary = Registry::scaledMean(1'000'000, 4, 100);
-    ASSERT_TRUE(belowBoundary.has_value());
-    EXPECT_EQ(*belowBoundary, 25'000'000);
+    EXPECT_EQ(belowBoundary, std::optional<std::int64_t>{25'000'000});
 }
 
 TEST(MetricsRegistryScaledMean, default_scale_is_one)

src/tests/libxrpl/telemetry/NodeStoreMetricNames.cpp

@@ -186,9 +186,9 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives)
 #include <opentelemetry/exporters/memory/in_memory_metric_data.h>
 #include <opentelemetry/exporters/memory/in_memory_metric_exporter_factory.h>
 #include <opentelemetry/metrics/meter.h>
+#include <opentelemetry/nostd/unique_ptr.h>
 #include <opentelemetry/nostd/variant.h>
 #include <opentelemetry/sdk/metrics/aggregation/aggregation_config.h>
-#include <opentelemetry/sdk/metrics/data/metric_data.h>
 #include <opentelemetry/sdk/metrics/data/point_data.h>
 #include <opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_factory.h>
 #include <opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_options.h>
@@ -202,7 +202,7 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives)
 
 #include <array>
 #include <chrono>
-#include <cstdint>
+#include <cstddef>
 #include <memory>
 #include <string>
 #include <utility>
@@ -482,10 +482,10 @@ TEST(NodeStoreReadHistogram, the_two_labels_split_the_series_four_ways)
     // the assertion that a label mix-up would break: swapping two values would
     // put a sample in the wrong series and fail here.
     for (auto const& [isAsync, wasFound, value, bucket] : std::array<Expected, 4>{
-             {{true, true, 3.0, 2},
-              {true, false, 9.0, 3},
-              {false, true, 40.0, 5},
-              {false, false, 800.0, 9}}})
+             {{.isAsync = true, .wasFound = true, .value = 3.0, .bucket = 2},
+              {.isAsync = true, .wasFound = false, .value = 9.0, .bucket = 3},
+              {.isAsync = false, .wasFound = true, .value = 40.0, .bucket = 5},
+              {.isAsync = false, .wasFound = false, .value = 800.0, .bucket = 9}}})
     {
         auto const* point = findPoint(points, isAsync, wasFound);
         ASSERT_NE(point, nullptr) << "missing series for fetch_type="