Merge pull request #4274 from SirTyson/bucketlistdb-better-metrics

latobarita · web-flow · commit 60263deeb037 · 2024-05-03T10:45:24.000-07:00
Seperate metrics for bloom miss loads

Reviewed-by: marta-lokhova
diff --git a/docs/metrics.md b/docs/metrics.md
@@ -38,7 +38,7 @@ bucketlistDB.bulk.loads                   | meter     | number of entries Bucket
 bucketlistDB.bulk.inflationWinners        | timer     | time to load inflation winners
 bucketlistDB.bulk.poolshareTrustlines     | timer     | time to load poolshare trustlines by accountID and assetID
 bucketlistDB.bulk.prefetch                | timer     | time to prefetch
-bucketlistDB.point.<X>                    | timer     | time to load single entry of type <X>
+bucketlistDB.point.<X>                    | timer     | time to load single entry of type <X> (if no bloom miss occurred)
 herder.pending[-soroban]-txs.age0         | counter   | number of gen0 pending transactions
 herder.pending[-soroban]-txs.age1         | counter   | number of gen1 pending transactions
 herder.pending[-soroban]-txs.age2         | counter   | number of gen2 pending transactions
diff --git a/src/bucket/BucketListSnapshot.cpp b/src/bucket/BucketListSnapshot.cpp
@@ -125,22 +125,28 @@ SearchableBucketListSnapshot::getLedgerEntry(LedgerKey const& k)
 
     if (threadIsMain())
     {
-        auto timer = mSnapshotManager.getPointLoadTimer(k.type()).TimeScope();
-        return getLedgerEntryInternal(k);
+        mSnapshotManager.startPointLoadTimer();
+        auto [result, bloomMiss] = getLedgerEntryInternal(k);
+        mSnapshotManager.endPointLoadTimer(k.type(), bloomMiss);
+        return result;
     }
     else
     {
-        return getLedgerEntryInternal(k);
+        auto [result, bloomMiss] = getLedgerEntryInternal(k);
+        return result;
     }
 }
 
-std::shared_ptr<LedgerEntry>
+std::pair<std::shared_ptr<LedgerEntry>, bool>
 SearchableBucketListSnapshot::getLedgerEntryInternal(LedgerKey const& k)
 {
     std::shared_ptr<LedgerEntry> result{};
+    auto sawBloomMiss = false;
 
     auto f = [&](BucketSnapshot const& b) {
-        auto be = b.getBucketEntry(k);
+        auto [be, bloomMiss] = b.getBucketEntry(k);
+        sawBloomMiss = sawBloomMiss || bloomMiss;
+
         if (be.has_value())
         {
             result =
@@ -156,7 +162,7 @@ SearchableBucketListSnapshot::getLedgerEntryInternal(LedgerKey const& k)
     };
 
     loopAllBuckets(f);
-    return result;
+    return {result, sawBloomMiss};
 }
 
 std::vector<LedgerEntry>
diff --git a/src/bucket/BucketListSnapshot.h b/src/bucket/BucketListSnapshot.h
@@ -68,7 +68,10 @@ class SearchableBucketListSnapshot : public NonMovableOrCopyable
     std::vector<LedgerEntry>
     loadKeysInternal(std::set<LedgerKey, LedgerEntryIdCmp> const& inKeys);
 
-    std::shared_ptr<LedgerEntry> getLedgerEntryInternal(LedgerKey const& k);
+    // Loads bucket entry for LedgerKey k. Returns <LedgerEntry, bloomMiss>,
+    // where bloomMiss is true if a bloom miss occurred during the load.
+    std::pair<std::shared_ptr<LedgerEntry>, bool>
+    getLedgerEntryInternal(LedgerKey const& k);
 
     SearchableBucketListSnapshot(BucketSnapshotManager const& snapshotManager);
 
diff --git a/src/bucket/BucketManagerImpl.cpp b/src/bucket/BucketManagerImpl.cpp
@@ -126,8 +126,7 @@ BucketManagerImpl::initialize()
         if (mApp.getConfig().isUsingBucketListDB())
         {
             mSnapshotManager = std::make_unique<BucketSnapshotManager>(
-                mApp.getMetrics(),
-                std::make_unique<BucketListSnapshot>(*mBucketList, 0));
+                mApp, std::make_unique<BucketListSnapshot>(*mBucketList, 0));
         }
     }
 }
diff --git a/src/bucket/BucketSnapshot.cpp b/src/bucket/BucketSnapshot.cpp
@@ -31,14 +31,14 @@ BucketSnapshot::isEmpty() const
     return mBucket->isEmpty();
 }
 
-std::optional<BucketEntry>
+std::pair<std::optional<BucketEntry>, bool>
 BucketSnapshot::getEntryAtOffset(LedgerKey const& k, std::streamoff pos,
                                  size_t pageSize) const
 {
     ZoneScoped;
     if (isEmpty())
     {
-        return std::nullopt;
+        return {std::nullopt, false};
     }
 
     auto& stream = getStream();
@@ -49,26 +49,26 @@ BucketSnapshot::getEntryAtOffset(LedgerKey const& k, std::streamoff pos,
     {
         if (stream.readOne(be))
         {
-            return std::make_optional(be);
+            return {std::make_optional(be), false};
         }
     }
     else if (stream.readPage(be, k, pageSize))
     {
-        return std::make_optional(be);
+        return {std::make_optional(be), false};
     }
 
     // Mark entry miss for metrics
     mBucket->getIndex().markBloomMiss();
-    return std::nullopt;
+    return {std::nullopt, true};
 }
 
-std::optional<BucketEntry>
+std::pair<std::optional<BucketEntry>, bool>
 BucketSnapshot::getBucketEntry(LedgerKey const& k) const
 {
     ZoneScoped;
     if (isEmpty())
     {
-        return std::nullopt;
+        return {std::nullopt, false};
     }
 
     auto pos = mBucket->getIndex().lookup(k);
@@ -78,7 +78,7 @@ BucketSnapshot::getBucketEntry(LedgerKey const& k) const
                                 mBucket->getIndex().getPageSize());
     }
 
-    return std::nullopt;
+    return {std::nullopt, false};
 }
 
 // When searching for an entry, BucketList calls this function on every bucket.
@@ -105,8 +105,8 @@ BucketSnapshot::loadKeys(std::set<LedgerKey, LedgerEntryIdCmp>& keys,
         indexIter = newIndexIter;
         if (offOp)
         {
-            auto entryOp = getEntryAtOffset(*currKeyIt, *offOp,
-                                            mBucket->getIndex().getPageSize());
+            auto [entryOp, bloomMiss] = getEntryAtOffset(
+                *currKeyIt, *offOp, mBucket->getIndex().getPageSize());
             if (entryOp)
             {
                 if (entryOp->type() != DEADENTRY)
diff --git a/src/bucket/BucketSnapshot.h b/src/bucket/BucketSnapshot.h
@@ -33,10 +33,12 @@ class BucketSnapshot : public NonMovable
     XDRInputFileStream& getStream() const;
 
     // Loads the bucket entry for LedgerKey k. Starts at file offset pos and
-    // reads until key is found or the end of the page.
-    std::optional<BucketEntry> getEntryAtOffset(LedgerKey const& k,
-                                                std::streamoff pos,
-                                                size_t pageSize) const;
+    // reads until key is found or the end of the page. Returns <BucketEntry,
+    // bloomMiss>, where bloomMiss is true if a bloomMiss occurred during the
+    // load.
+    std::pair<std::optional<BucketEntry>, bool>
+    getEntryAtOffset(LedgerKey const& k, std::streamoff pos,
+                     size_t pageSize) const;
 
     BucketSnapshot(std::shared_ptr<Bucket const> const b);
 
@@ -48,8 +50,10 @@ class BucketSnapshot : public NonMovable
     bool isEmpty() const;
     std::shared_ptr<Bucket const> getRawBucket() const;
 
-    // Loads bucket entry for LedgerKey k.
-    std::optional<BucketEntry> getBucketEntry(LedgerKey const& k) const;
+    // Loads bucket entry for LedgerKey k. Returns <BucketEntry, bloomMiss>,
+    // where bloomMiss is true if a bloomMiss occurred during the load.
+    std::pair<std::optional<BucketEntry>, bool>
+    getBucketEntry(LedgerKey const& k) const;
 
     // Loads LedgerEntry's for given keys. When a key is found, the
     // entry is added to result and the key is removed from keys.
diff --git a/src/bucket/BucketSnapshotManager.cpp b/src/bucket/BucketSnapshotManager.cpp
@@ -13,16 +13,15 @@ namespace stellar
 {
 
 BucketSnapshotManager::BucketSnapshotManager(
-    medida::MetricsRegistry& metrics,
-    std::unique_ptr<BucketListSnapshot const>&& snapshot)
-    : mMetrics(metrics)
+    Application& app, std::unique_ptr<BucketListSnapshot const>&& snapshot)
+    : mApp(app)
     , mCurrentSnapshot(std::move(snapshot))
-    , mBulkLoadMeter(
-          mMetrics.NewMeter({"bucketlistDB", "query", "loads"}, "query"))
-    , mBloomMisses(
-          mMetrics.NewMeter({"bucketlistDB", "bloom", "misses"}, "bloom"))
-    , mBloomLookups(
-          mMetrics.NewMeter({"bucketlistDB", "bloom", "lookups"}, "bloom"))
+    , mBulkLoadMeter(app.getMetrics().NewMeter(
+          {"bucketlistDB", "query", "loads"}, "query"))
+    , mBloomMisses(app.getMetrics().NewMeter(
+          {"bucketlistDB", "bloom", "misses"}, "bloom"))
+    , mBloomLookups(app.getMetrics().NewMeter(
+          {"bucketlistDB", "bloom", "lookups"}, "bloom"))
 {
     releaseAssert(threadIsMain());
 }
@@ -51,31 +50,14 @@ BucketSnapshotManager::recordBulkLoadMetrics(std::string const& label,
     auto iter = mBulkTimers.find(label);
     if (iter == mBulkTimers.end())
     {
-        auto& metric = mMetrics.NewTimer({"bucketlistDB", "bulk", label});
+        auto& metric =
+            mApp.getMetrics().NewTimer({"bucketlistDB", "bulk", label});
         iter = mBulkTimers.emplace(label, metric).first;
     }
 
     return iter->second;
 }
 
-medida::Timer&
-BucketSnapshotManager::getPointLoadTimer(LedgerEntryType t) const
-{
-    // For now, only keep metrics for the main thread. We can decide on what
-    // metrics make sense when more background services are added later.
-    releaseAssert(threadIsMain());
-
-    auto iter = mPointTimers.find(t);
-    if (iter == mPointTimers.end())
-    {
-        auto const& label = xdr::xdr_traits<LedgerEntryType>::enum_name(t);
-        auto& metric = mMetrics.NewTimer({"bucketlistDB", "point", label});
-        iter = mPointTimers.emplace(t, metric).first;
-    }
-
-    return iter->second;
-}
-
 void
 BucketSnapshotManager::maybeUpdateSnapshot(
     std::unique_ptr<BucketListSnapshot const>& snapshot) const
@@ -102,4 +84,39 @@ BucketSnapshotManager::updateCurrentSnapshot(
                                            mCurrentSnapshot->getLedgerSeq());
     mCurrentSnapshot.swap(newSnapshot);
 }
+
+void
+BucketSnapshotManager::startPointLoadTimer() const
+{
+    releaseAssert(threadIsMain());
+    releaseAssert(!mTimerStart);
+    mTimerStart = mApp.getClock().now();
+}
+
+void
+BucketSnapshotManager::endPointLoadTimer(LedgerEntryType t,
+                                         bool bloomMiss) const
+{
+    releaseAssert(threadIsMain());
+    releaseAssert(mTimerStart);
+    auto duration = mApp.getClock().now() - *mTimerStart;
+    mTimerStart.reset();
+
+    // We expect about 0.1% of lookups to encounter a bloom miss. To avoid noise
+    // in disk performance metrics, we only track metrics for entries that did
+    // not encounter a bloom miss.
+    if (!bloomMiss)
+    {
+        auto iter = mPointTimers.find(t);
+        if (iter == mPointTimers.end())
+        {
+            auto const& label = xdr::xdr_traits<LedgerEntryType>::enum_name(t);
+            auto& metric =
+                mApp.getMetrics().NewTimer({"bucketlistDB", "point", label});
+            iter = mPointTimers.emplace(t, metric).first;
+        }
+
+        iter->second.Update(duration);
+    }
+}
 }
diff --git a/src/bucket/BucketSnapshotManager.h b/src/bucket/BucketSnapshotManager.h
@@ -32,7 +32,7 @@ class BucketListSnapshot;
 class BucketSnapshotManager : NonMovableOrCopyable
 {
   private:
-    medida::MetricsRegistry& mMetrics;
+    Application& mApp;
 
     // Snapshot that is maintained and periodically updated by BucketManager on
     // the main thread. When background threads need to generate or refresh a
@@ -49,6 +49,8 @@ class BucketSnapshotManager : NonMovableOrCopyable
     medida::Meter& mBloomMisses;
     medida::Meter& mBloomLookups;
 
+    mutable std::optional<VirtualClock::time_point> mTimerStart;
+
     // Called by main thread to update mCurrentSnapshot whenever the BucketList
     // is updated
     void updateCurrentSnapshot(
@@ -65,7 +67,7 @@ class BucketSnapshotManager : NonMovableOrCopyable
                                                bool restartMerges);
 
   public:
-    BucketSnapshotManager(medida::MetricsRegistry& metrics,
+    BucketSnapshotManager(Application& app,
                           std::unique_ptr<BucketListSnapshot const>&& snapshot);
 
     std::unique_ptr<SearchableBucketListSnapshot>
@@ -76,9 +78,10 @@ class BucketSnapshotManager : NonMovableOrCopyable
     void maybeUpdateSnapshot(
         std::unique_ptr<BucketListSnapshot const>& snapshot) const;
 
+    // All metric recording functions must only be called by the main thread
+    void startPointLoadTimer() const;
+    void endPointLoadTimer(LedgerEntryType t, bool bloomMiss) const;
     medida::Timer& recordBulkLoadMetrics(std::string const& label,
                                          size_t numEntries) const;
-
-    medida::Timer& getPointLoadTimer(LedgerEntryType t) const;
 };
 }

Original file line number	Diff line number	Diff line change
`@@ -125,22 +125,28 @@ SearchableBucketListSnapshot::getLedgerEntry(LedgerKey const& k)`
`125`	`125`
`126`	`126`	`if (threadIsMain())`
`127`	`127`	`{`
`128`		`- auto timer = mSnapshotManager.getPointLoadTimer(k.type()).TimeScope();`
`129`		`- return getLedgerEntryInternal(k);`
	`128`	`+ mSnapshotManager.startPointLoadTimer();`
	`129`	`+ auto [result, bloomMiss] = getLedgerEntryInternal(k);`
	`130`	`+ mSnapshotManager.endPointLoadTimer(k.type(), bloomMiss);`
	`131`	`+ return result;`
`130`	`132`	`}`
`131`	`133`	`else`
`132`	`134`	`{`
`133`		`- return getLedgerEntryInternal(k);`
	`135`	`+ auto [result, bloomMiss] = getLedgerEntryInternal(k);`
	`136`	`+ return result;`
`134`	`137`	`}`
`135`	`138`	`}`
`136`	`139`
`137`		`-std::shared_ptr<LedgerEntry>`
	`140`	`+std::pair<std::shared_ptr<LedgerEntry>, bool>`
`138`	`141`	`SearchableBucketListSnapshot::getLedgerEntryInternal(LedgerKey const& k)`
`139`	`142`	`{`
`140`	`143`	`std::shared_ptr<LedgerEntry> result{};`
	`144`	`+ auto sawBloomMiss = false;`
`141`	`145`
`142`	`146`	`auto f = [&](BucketSnapshot const& b) {`
`143`		`- auto be = b.getBucketEntry(k);`
	`147`	`+ auto [be, bloomMiss] = b.getBucketEntry(k);`
	`148`	`+ sawBloomMiss = sawBloomMiss \|\| bloomMiss;`
	`149`	`+`
`144`	`150`	`if (be.has_value())`
`145`	`151`	`{`
`146`	`152`	`result =`
`@@ -156,7 +162,7 @@ SearchableBucketListSnapshot::getLedgerEntryInternal(LedgerKey const& k)`
`156`	`162`	`};`
`157`	`163`
`158`	`164`	`loopAllBuckets(f);`
`159`		`- return result;`
	`165`	`+ return {result, sawBloomMiss};`
`160`	`166`	`}`
`161`	`167`
`162`	`168`	`std::vector<LedgerEntry>`
Original file line number	Diff line number	Diff line change
`@@ -126,8 +126,7 @@ BucketManagerImpl::initialize()`
`126`	`126`	`if (mApp.getConfig().isUsingBucketListDB())`
`127`	`127`	`{`
`128`	`128`	`mSnapshotManager = std::make_unique<BucketSnapshotManager>(`
`129`		`- mApp.getMetrics(),`
`130`		`- std::make_unique<BucketListSnapshot>(*mBucketList, 0));`
	`129`	`+ mApp, std::make_unique<BucketListSnapshot>(*mBucketList, 0));`
`131`	`130`	`}`
`132`	`131`	`}`
`133`	`132`	`}`
Original file line number	Diff line number	Diff line change
`@@ -31,14 +31,14 @@ BucketSnapshot::isEmpty() const`
`31`	`31`	`return mBucket->isEmpty();`
`32`	`32`	`}`
`33`	`33`
`34`		`-std::optional<BucketEntry>`
	`34`	`+std::pair<std::optional<BucketEntry>, bool>`
`35`	`35`	`BucketSnapshot::getEntryAtOffset(LedgerKey const& k, std::streamoff pos,`
`36`	`36`	`size_t pageSize) const`
`37`	`37`	`{`
`38`	`38`	`ZoneScoped;`
`39`	`39`	`if (isEmpty())`
`40`	`40`	`{`
`41`		`- return std::nullopt;`
	`41`	`+ return {std::nullopt, false};`
`42`	`42`	`}`
`43`	`43`
`44`	`44`	`auto& stream = getStream();`
`@@ -49,26 +49,26 @@ BucketSnapshot::getEntryAtOffset(LedgerKey const& k, std::streamoff pos,`
`49`	`49`	`{`
`50`	`50`	`if (stream.readOne(be))`
`51`	`51`	`{`
`52`		`- return std::make_optional(be);`
	`52`	`+ return {std::make_optional(be), false};`
`53`	`53`	`}`
`54`	`54`	`}`
`55`	`55`	`else if (stream.readPage(be, k, pageSize))`
`56`	`56`	`{`
`57`		`- return std::make_optional(be);`
	`57`	`+ return {std::make_optional(be), false};`
`58`	`58`	`}`
`59`	`59`
`60`	`60`	`// Mark entry miss for metrics`
`61`	`61`	`mBucket->getIndex().markBloomMiss();`
`62`		`- return std::nullopt;`
	`62`	`+ return {std::nullopt, true};`
`63`	`63`	`}`
`64`	`64`
`65`		`-std::optional<BucketEntry>`
	`65`	`+std::pair<std::optional<BucketEntry>, bool>`
`66`	`66`	`BucketSnapshot::getBucketEntry(LedgerKey const& k) const`
`67`	`67`	`{`
`68`	`68`	`ZoneScoped;`
`69`	`69`	`if (isEmpty())`
`70`	`70`	`{`
`71`		`- return std::nullopt;`
	`71`	`+ return {std::nullopt, false};`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`auto pos = mBucket->getIndex().lookup(k);`
`@@ -78,7 +78,7 @@ BucketSnapshot::getBucketEntry(LedgerKey const& k) const`
`78`	`78`	`mBucket->getIndex().getPageSize());`
`79`	`79`	`}`
`80`	`80`
`81`		`- return std::nullopt;`
	`81`	`+ return {std::nullopt, false};`
`82`	`82`	`}`
`83`	`83`
`84`	`84`	`// When searching for an entry, BucketList calls this function on every bucket.`
`@@ -105,8 +105,8 @@ BucketSnapshot::loadKeys(std::set<LedgerKey, LedgerEntryIdCmp>& keys,`
`105`	`105`	`indexIter = newIndexIter;`
`106`	`106`	`if (offOp)`
`107`	`107`	`{`
`108`		`- auto entryOp = getEntryAtOffset(currKeyIt, offOp,`
`109`		`- mBucket->getIndex().getPageSize());`
	`108`	`+ auto [entryOp, bloomMiss] = getEntryAtOffset(`
	`109`	`+ currKeyIt, offOp, mBucket->getIndex().getPageSize());`
`110`	`110`	`if (entryOp)`
`111`	`111`	`{`
`112`	`112`	`if (entryOp->type() != DEADENTRY)`