Merge pull request #29337 from vespa-engine/vekterli/add-metric-for-remove-by-gid-ops

Add fundamental metrics for async "remove by GID" SPI operation

4 files changed, 25 insertions, 4 deletions

diff --git a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
index 96618eb9206..4911ad88692 100644
--- a/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
+++ b/storage/src/tests/persistence/filestorage/filestormanagertest.cpp
@@ -1421,6 +1421,14 @@ void FileStorManagerTest::do_test_delete_bucket(bool use_throttled_delete) {
         StorBucketDatabase::WrappedEntry entry(_node->getStorageBucketDatabase().get(bid, "foo"));
         EXPECT_FALSE(entry.exists());
     }
+    if (use_throttled_delete) {
+        auto& metrics = thread_metrics_of(*c.manager)->remove_by_gid;
+        EXPECT_EQ(metrics.failed.getValue(), 0);
+        EXPECT_EQ(metrics.count.getValue(), 1);
+        // We can't reliably test the actual latency here without wiring mock clock bumping into
+        // the async remove by GID execution, but we can at least test that we updated the metric.
+        EXPECT_EQ(metrics.latency.getCount(), 1);
+    }
 }
 
 // TODO remove once throttled behavior is the default
diff --git a/storage/src/vespa/storage/persistence/asynchandler.cpp b/storage/src/vespa/storage/persistence/asynchandler.cpp
index bb3952c8c33..8e962a59809 100644
--- a/storage/src/vespa/storage/persistence/asynchandler.cpp
+++ b/storage/src/vespa/storage/persistence/asynchandler.cpp
@@ -300,14 +300,25 @@ AsyncHandler::handle_delete_bucket_throttling(api::DeleteBucketCommand& cmd, Mes
         });
         _spi.deleteBucketAsync(spi_bucket, std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, bucket.getBucketId(), std::move(task)));
     });
+
     auto& throttler = _env._fileStorHandler.operation_throttler();
+    auto* remove_by_gid_metric = &_env._metrics.remove_by_gid;
+
     for (auto& meta : meta_entries) {
         auto token = throttler.blocking_acquire_one();
+        remove_by_gid_metric->count.inc();
         std::vector<spi::DocTypeGidAndTimestamp> to_remove = {{meta->getDocumentType(), meta->getGid(), meta->getTimestamp()}};
-        auto task = makeResultTask([bucket = cmd.getBucket(), token = std::move(token), invoke_delete_on_zero_refs]([[maybe_unused]] spi::Result::UP ignored) {
-            LOG(spam, "%s: completed removeByGidAsync operation", bucket.toString().c_str());
-            // Nothing else clever to do here. Throttle token and deleteBucket dispatch refs dropped implicitly.
-        });
+        auto task = makeResultTask([bucket = cmd.getBucket(), token = std::move(token),
+                                    invoke_delete_on_zero_refs, remove_by_gid_metric,
+                                    op_timer = framework::MilliSecTimer(_env._component.getClock())]
+            (spi::Result::UP result) {
+                if (result->hasError()) {
+                    remove_by_gid_metric->failed.inc();
+                }
+                remove_by_gid_metric->latency.addValue(op_timer.getElapsedTimeAsDouble());
+                LOG(spam, "%s: completed removeByGidAsync operation", bucket.toString().c_str());
+                // Nothing else clever to do here. Throttle token and deleteBucket dispatch refs dropped implicitly.
+            });
         LOG(spam, "%s: about to invoke removeByGidAsync(%s, %s, %zu)", cmd.getBucket().toString().c_str(),
             vespalib::string(meta->getDocumentType()).c_str(), meta->getGid().toString().c_str(), meta->getTimestamp().getValue());
         _spi.removeByGidAsync(spi_bucket, std::move(to_remove), std::make_unique<ResultTaskOperationDone>(_sequencedExecutor, cmd.getBucketId(), std::move(task)));
diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp
index 6072a1dbc3e..b8ccb15eb29 100644
--- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp
+++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.cpp
@@ -159,6 +159,7 @@ FileStorThreadMetrics::FileStorThreadMetrics(const std::string& name, const std:
       visit(this),
       createBuckets("createbuckets", "Number of buckets that has been created.", this),
       deleteBuckets("deletebuckets", "Number of buckets that has been deleted.", this),
+      remove_by_gid("remove_by_gid", "Internal single-document remove operations used by DeleteBucket", this),
       repairs("bucketverified", "Number of times buckets have been checked.", this),
       repairFixed("bucketfixed", {}, "Number of times bucket has been fixed because of corruption", this),
       recheckBucketInfo("recheckbucketinfo",
diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h
index 85a3813c9eb..6a60ea44a45 100644
--- a/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h
+++ b/storage/src/vespa/storage/persistence/filestorage/filestormetrics.h
@@ -103,6 +103,7 @@ struct FileStorThreadMetrics : public metrics::MetricSet
     Visitor visit;
     Op createBuckets;
     Op deleteBuckets;
+    Op remove_by_gid;
     Op repairs;
     metrics::LongCountMetric repairFixed;
     Op recheckBucketInfo;