Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics

src/libxrpl/nodestore/backend/NuDBFactory.cpp

@@ -87,10 +87,21 @@ public:
     std::atomic<std::uint64_t> insertMaxUs{0};
 
     /**
-     * Summed depth observed at each insert.
+     * Summed depth observed at each insert, accumulated at insert entry.
      */
     std::atomic<std::uint64_t> depthSum{0};
 
+    /**
+     * How many depth samples make up @ref depthSum.
+     *
+     * Its own counter rather than reusing the completed-insert count, because
+     * the two are taken at different moments: a depth sample exists as soon
+     * as an insert starts, while the insert count only rises when one
+     * finishes. Dividing by the wrong one biases the mean downward under
+     * load, which is when the mean matters.
+     */
+    std::atomic<std::uint64_t> depthSamples{0};
+
     NuDBBackend(
         size_t keyBytes,
         Section const& keyValues,
@@ -270,7 +281,16 @@ public:
         // NuDB takes one global mutex for the whole insert, so the wait is
         // invisible from here. Record the depth we joined at and the wall
         // time we spent; the split follows from Little's Law.
+        //
+        // Depth and its sample count are both folded in HERE, at entry, so
+        // the mean is over the same population. Counting the sample at exit
+        // instead would drop every insert still in flight, and those are the
+        // slow, deep ones -- biasing the mean down exactly when queueing is
+        // worst. With all writers inside their first insert the exit-counted
+        // version reports no depth at all.
         auto const depth = concurrentWriters.fetch_add(1, std::memory_order_relaxed) + 1;
+        depthSum.fetch_add(depth, std::memory_order_relaxed);
+        depthSamples.fetch_add(1, std::memory_order_relaxed);
         auto const begin = std::chrono::steady_clock::now();
 
         // A scope guard rather than straight-line code, because the insert
@@ -369,6 +389,7 @@ public:
         stats.insertTotalUs = insertTotalUs.load(std::memory_order_relaxed);
         stats.insertMaxUs = insertMaxUs.load(std::memory_order_relaxed);
         stats.depthSum = depthSum.load(std::memory_order_relaxed);
+        stats.depthSamples = depthSamples.load(std::memory_order_relaxed);
         return stats;
     }
 
@@ -424,7 +445,6 @@ private:
         concurrentWriters.fetch_sub(1, std::memory_order_relaxed);
         insertCount.fetch_add(1, std::memory_order_relaxed);
         insertTotalUs.fetch_add(elapsedUs, std::memory_order_relaxed);
-        depthSum.fetch_add(depth, std::memory_order_relaxed);
 
         // std::atomic has no fetch_max, so raise the maximum with a CAS
         // loop. Mirrors the clamp loop in OTelCollector.cpp.

src/test/nodestore/DatabaseConfig_test.cpp

@@ -1253,7 +1253,7 @@ public:
     }
 
     /**
-     * Verify the seven acquire_* labels, published unconditionally, and each
+     * Verify the nine acquire_* labels, published unconditionally, and each
      * wired to its own counter.
      *
      * Every expected value below is distinct, so a getter cross-wired to a
@@ -1270,10 +1270,12 @@ public:
             "acquire_completions",
             "acquire_deferrals",
             "acquire_give_ups",
+            "acquire_ledger_deferrals",
+            "acquire_ledger_timeouts",
             "acquire_sweep_evictions",
             "acquire_timeouts"};
 
-        // A quiet node publishes all seven at zero. Unlike a mean, zero is the
+        // A quiet node publishes all nine at zero. Unlike a mean, zero is the
         // meaningful "no such event yet" reading for a counter, so omitting
         // these would lose the ability to see that nothing happened.
         AcquireStats const quiet;
@@ -1288,10 +1290,13 @@ public:
         // without partial work so the subset relationship is exercised: 5
         // aborts of which 2 discarded partly built maps.
         AcquireStats busy;
-        for (int i = 0; i < 3; ++i)
-            busy.recordDeferral();
-        for (int i = 0; i < 7; ++i)
-            busy.recordTimeout();
+        for (int i = 0; i < 2; ++i)
+            busy.recordDeferral(true);
+        busy.recordDeferral(false);
+        for (int i = 0; i < 5; ++i)
+            busy.recordTimeout(true);
+        for (int i = 0; i < 2; ++i)
+            busy.recordTimeout(false);
         busy.recordGiveUp();
         for (int i = 0; i < 2; ++i)
             busy.recordAbort(true);
@@ -1312,6 +1317,13 @@ public:
         BEAST_EXPECT(sink.value("acquire_aborts_partial") == std::int64_t{2});
         BEAST_EXPECT(sink.value("acquire_completions") == std::int64_t{11});
         BEAST_EXPECT(sink.value("acquire_sweep_evictions") == std::int64_t{13});
+
+        // The ledger-scoped pair must be a strict subset of the all-lane
+        // totals, and the numbers are chosen to differ from them: a counter
+        // wired to the wrong lane, or one that ignored the flag and counted
+        // everything, would land on 3 and 7 instead of 2 and 5.
+        BEAST_EXPECT(sink.value("acquire_ledger_deferrals") == std::int64_t{2});
+        BEAST_EXPECT(sink.value("acquire_ledger_timeouts") == std::int64_t{5});
     }
 
     /**

src/tests/libxrpl/nodestore/NuDBFactory.cpp

@@ -1,9 +1,11 @@
 #include <xrpl/basics/ByteUtilities.h>
+#include <xrpl/basics/scope.h>
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/beast/utility/temp_dir.h>
 #include <xrpl/config/BasicConfig.h>
 #include <xrpl/nodestore/DummyScheduler.h>
 #include <xrpl/nodestore/Manager.h>
+#include <xrpl/nodestore/Types.h>
 #include <xrpl/nodestore/WriteStats.h>
 
 #include <gtest/gtest.h>
@@ -76,6 +78,24 @@ constexpr std::uint64_t kOverlapPerThread = 50;
  * doInsert() together rather than one after another; staggered starts are what
  * would let every insert run end to end and never overlap.
  *
+ * The latch is the whole point of the round, and it is also the one thing here
+ * that can hang the test binary rather than fail it. Two rules keep it safe:
+ *
+ *   1. Every batch is built before the first thread exists, so the only thing
+ *      a thread does before arriving is arrive. Building a batch inside the
+ *      thread allocates, so it can throw, and a thread that throws never
+ *      arrives -- leaving the other seven blocked on the latch for good.
+ *   2. Spawning is guarded, so a thread that never starts still has its
+ *      arrival accounted for.
+ *
+ *     batches built here (may throw; no thread waiting yet)
+ *          |
+ *          v
+ *     spawn 8 --> [ latch: 8 arrivals ] --> stores overlap --> join
+ *          |                  ^
+ *          +-- spawn threw ---+  guard counts down the missing arrivals,
+ *                                then joins the threads already running
+ *
  * @param backend Backend to insert into. Must be open.
  * @param round   Round index, mixed into the seeds so every round writes
  *                fresh keys and no insert takes the duplicate short-circuit.
@@ -83,20 +103,44 @@ constexpr std::uint64_t kOverlapPerThread = 50;
 void
 runOverlappingInsertRound(Backend& backend, int round)
 {
+    std::vector<Batch> batches;
+    batches.reserve(kOverlapThreads);
+    for (auto t = 0uz; t < kOverlapThreads; ++t)
+    {
+        batches.push_back(createPredictableBatch(
+            kOverlapPerThread, 1000 + t + (static_cast<std::uint64_t>(round) * 100'000)));
+    }
+
     std::latch start(static_cast<std::ptrdiff_t>(kOverlapThreads));
 
     std::vector<std::thread> threads;
     threads.reserve(kOverlapThreads);
+
+    // Covers a spawn loop that ends early. The latch is built for the full set
+    // because a thread that has already arrived cannot be un-counted, so the
+    // shortfall is counted down instead of the latch being resized. Counting
+    // down comes before joining: a thread left waiting on the latch would never
+    // become joinable.
+    //
+    // Released once every thread exists, so the success path joins below rather
+    // than from a destructor -- join() can throw, and a destructor would turn
+    // that into a terminate.
+    ScopeExit releaseAndJoin([&] {
+        start.count_down(static_cast<std::ptrdiff_t>(kOverlapThreads - threads.size()));
+        for (auto& th : threads)
+            th.join();
+    });
+
     for (auto t = 0uz; t < kOverlapThreads; ++t)
     {
-        threads.emplace_back([&backend, &start, t, round] {
-            auto const batch = createPredictableBatch(
-                kOverlapPerThread, 1000 + t + (static_cast<std::uint64_t>(round) * 100'000));
+        threads.emplace_back([&backend, &start, &batches, t] {
             start.arrive_and_wait();
-            for (auto const& obj : batch)
+            for (auto const& obj : batches[t])
                 backend.store(obj);
         });
     }
+    releaseAndJoin.release();
+
     for (auto& th : threads)
         th.join();
 }
@@ -330,6 +374,7 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
     EXPECT_EQ(initial->insertTotalUs, 0u);
     EXPECT_EQ(initial->insertMaxUs, 0u);
     EXPECT_EQ(initial->depthSum, 0u);
+    EXPECT_EQ(initial->depthSamples, 0u);
     EXPECT_EQ(initial->concurrentWriters, 0u);
 
     // Exactly 10 inserts must be counted as 10, and depthSum must be 10
@@ -352,6 +397,11 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
         FAIL() << "nudb must report write stats after inserts";
     EXPECT_EQ(after->insertCount, kFirstBatch);
     EXPECT_EQ(after->depthSum, kFirstBatch);
+    // The denominator of the published mean depth. One sample per insert, so
+    // with the depth being 1 throughout, depthSum and depthSamples coincide
+    // here -- which is why this pair alone cannot tell a correct depthSum from
+    // a constant 1, and why the overlap test below exists.
+    EXPECT_EQ(after->depthSamples, kFirstBatch);
     EXPECT_GT(after->insertTotalUs, 0u);
     EXPECT_GT(after->insertMaxUs, 0u);
     // A maximum is never below the mean, so max * n >= sum. Catches a field
@@ -376,6 +426,7 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert)
         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->depthSamples, kFirstBatch + kSecondBatch);
     EXPECT_EQ(cumulative->concurrentWriters, 0u);
     // A running maximum never decreases.
     EXPECT_GE(cumulative->insertMaxUs, after->insertMaxUs);
@@ -439,6 +490,10 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts)
     // rounds that stored new data would leave these equal.
     EXPECT_EQ(second->insertCount, first->insertCount + kBatchSize);
     EXPECT_EQ(second->depthSum, first->depthSum + kBatchSize);
+    // Sampled at entry, so the duplicate round is sampled whether or not it
+    // stores anything. An implementation that sampled only new data would
+    // leave this at the first round's figure and skew the mean.
+    EXPECT_EQ(second->depthSamples, first->depthSamples + kBatchSize);
     // The depth returned to zero, so the key_exists early return did not
     // leak a writer.
     EXPECT_EQ(second->concurrentWriters, 0u);
@@ -505,6 +560,15 @@ TEST(NuDBFactory, write_stats_measure_depth_under_real_overlap)
     // under contention fails this.
     EXPECT_EQ(stats->insertCount, completedRounds * kOverlapThreads * kOverlapPerThread);
 
+    // A depth sample is taken when an insert starts and insertCount rises when
+    // one finishes, so the two populations differ only while an insert is in
+    // flight. Every thread has been joined here, so nothing is in flight and
+    // they must agree exactly. That is what licenses comparing depthSum against
+    // insertCount below, and it is an assertion in its own right: a depthSamples
+    // stuck at zero makes the published mean depth vanish rather than read
+    // wrong, because the metric is omitted when its denominator is zero.
+    EXPECT_EQ(stats->depthSamples, stats->insertCount);
+
     // THE assertion this test exists for: strictly greater, so a depthSum fed
     // a constant 1 (or fed nothing, or fed insertCount's own delta) cannot
     // satisfy it however many rounds run.

src/xrpld/app/ledger/AcquireStats.h

@@ -93,9 +93,11 @@ public:
      * advance the retry count toward give-up.
      */
     void
-    recordDeferral()
+    recordDeferral(bool ledgerAcquisition = false)
     {
         deferrals_.fetch_add(1, std::memory_order_relaxed);
+        if (ledgerAcquisition)
+            ledgerDeferrals_.fetch_add(1, std::memory_order_relaxed);
     }
 
     /**
@@ -105,9 +107,11 @@ public:
      * separate from deferrals.
      */
     void
-    recordTimeout()
+    recordTimeout(bool ledgerAcquisition = false)
     {
         timeouts_.fetch_add(1, std::memory_order_relaxed);
+        if (ledgerAcquisition)
+            ledgerTimeouts_.fetch_add(1, std::memory_order_relaxed);
     }
 
     /**
@@ -171,6 +175,32 @@ public:
         return timeouts_.load(std::memory_order_relaxed);
     }
 
+    /**
+     * Deferrals that belong to ledger acquisition only.
+     *
+     * @ref getDeferrals covers every TimeoutCounter subclass, so a busy
+     * replay or transaction-set lane inflates it. Compare this against
+     * @ref getLedgerTimeouts to judge ledger acquisition on its own.
+     */
+    [[nodiscard]] std::uint64_t
+    getLedgerDeferrals() const
+    {
+        return ledgerDeferrals_.load(std::memory_order_relaxed);
+    }
+
+    /**
+     * Timeouts that belong to ledger acquisition only.
+     *
+     * The partner of @ref getLedgerDeferrals: rising deferrals with flat
+     * timeouts here means ledger acquisition's give-up path is disarmed,
+     * which the all-lane counters cannot show.
+     */
+    [[nodiscard]] std::uint64_t
+    getLedgerTimeouts() const
+    {
+        return ledgerTimeouts_.load(std::memory_order_relaxed);
+    }
+
     /**
      * Return the number of acquisitions that exhausted their retry budget.
      */
@@ -224,6 +254,16 @@ private:
      */
     std::atomic<std::uint64_t> deferrals_{0};
 
+    /**
+     * Deferrals attributable to ledger acquisition alone.
+     */
+    std::atomic<std::uint64_t> ledgerDeferrals_{0};
+
+    /**
+     * Timeouts attributable to ledger acquisition alone.
+     */
+    std::atomic<std::uint64_t> ledgerTimeouts_{0};
+
     /**
      * Timer bodies that ran and advanced the retry count.
      */

src/xrpld/app/ledger/detail/InboundLedger.cpp

@@ -87,7 +87,9 @@ InboundLedger::InboundLedger(
           app,
           hash,
           kLedgerAcquireTimeout,
-          {.jobType = JtLedgerData, .jobName = "InboundLedger", .jobLimit = 5},
+          {.jobType = JtLedgerData,
+           .jobName = TimeoutCounter::kLedgerAcquireJobName,
+           .jobLimit = 5},
           app.getJournal("InboundLedger"))
     , clock_(clock)
     , seq_(seq)

src/xrpld/app/ledger/detail/TimeoutCounter.cpp

@@ -67,7 +67,7 @@ TimeoutCounter::queueJob(ScopedLockType& sl)
         // Counted separately from timeouts: this path re-arms the timer
         // without running invokeOnTimer, so timeouts_ does not advance and the
         // give-up test that reads it cannot fire while the lane stays full.
-        app_.getAcquireStats().recordDeferral();
+        app_.getAcquireStats().recordDeferral(isLedgerAcquisition());
         JLOG(journal_.debug()) << "Deferring " << queueJobParameter_.jobName
                                << " timer due to load";
         setTimer(sl);
@@ -92,7 +92,7 @@ TimeoutCounter::invokeOnTimer()
     if (!progress_)
     {
         ++timeouts_;
-        app_.getAcquireStats().recordTimeout();
+        app_.getAcquireStats().recordTimeout(isLedgerAcquisition());
         JLOG(journal_.debug()) << "Timeout(" << timeouts_ << ") "
                                << " acquiring " << hash_;
         onTimer(false, sl);

src/xrpld/app/ledger/detail/TimeoutCounter.h

@@ -54,6 +54,17 @@ namespace xrpl {
 class TimeoutCounter
 {
 public:
+    /**
+     * Job name of the ledger-acquisition lane.
+     *
+     * Shared with InboundLedger, which passes it as its job name, so the
+     * counters that attribute deferrals and timeouts to this lane compare
+     * against the same string the lane is registered under. Two independent
+     * literals would drift apart silently: the metrics would read zero with
+     * no build error and no failing test.
+     */
+    static constexpr char kLedgerAcquireJobName[] = "InboundLedger";
+
     /**
      * Cancel the task by marking it as failed if the task is not done.
      * @note this function does not attempt to cancel the scheduled timer or
@@ -139,6 +150,23 @@ protected:
 
     QueueJobParameter queueJobParameter_;
 
+    /**
+     * Whether this counter belongs to ledger acquisition.
+     *
+     * Deferrals and timeouts are recorded in this base class, so they would
+     * otherwise pool every subclass together: a saturated replay lane would
+     * look exactly like a stalled ledger acquisition. The job name already
+     * identifies the subclass, so it is the cheapest discriminator available
+     * and needs no extra state.
+     *
+     * @return True for the InboundLedger lane, false for every other.
+     */
+    [[nodiscard]] bool
+    isLedgerAcquisition() const
+    {
+        return queueJobParameter_.jobName == kLedgerAcquireJobName;
+    }
+
 private:
     /**
      * Calls onTimer() if in the right state.

src/xrpld/telemetry/MetricsRegistry.cpp

@@ -969,7 +969,7 @@ MetricsRegistry::observeWritePathDetail(node_store::Database const& db, ObserveF
     // above 1.0 even under load. An integral gauge would truncate that to 1
     // and lose the whole signal, hence the fixed-point scale -- which the
     // name states, so nobody reads 140 as 140 writers.
-    if (auto const mean = scaledMean(ws->depthSum, ws->insertCount, 100))
+    if (auto const mean = scaledMean(ws->depthSum, ws->depthSamples, 100))
         observe("nudb_writer_depth_x100", *mean);
 }
 
@@ -982,6 +982,13 @@ MetricsRegistry::observeAcquireStats(AcquireStats const& stats, ObserveFn const&
     // give-up path cannot fire, so an acquisition never ends.
     observe("acquire_deferrals", static_cast<std::int64_t>(stats.getDeferrals()));
     observe("acquire_timeouts", static_cast<std::int64_t>(stats.getTimeouts()));
+
+    // The same two events, narrowed to ledger acquisition. The pair above
+    // sums every TimeoutCounter subclass, so a busy replay lane can imitate
+    // a stalled ledger acquisition; compare these two instead when asking
+    // whether ledger acquisition's give-up path is advancing.
+    observe("acquire_ledger_deferrals", static_cast<std::int64_t>(stats.getLedgerDeferrals()));
+    observe("acquire_ledger_timeouts", static_cast<std::int64_t>(stats.getLedgerTimeouts()));
     observe("acquire_give_ups", static_cast<std::int64_t>(stats.getGiveUps()));
     observe("acquire_aborts", static_cast<std::int64_t>(stats.getAborts()));
     observe("acquire_aborts_partial", static_cast<std::int64_t>(stats.getAbortsWithPartialWork()));