Fix: avoid duplicate link metrics (arangodb#20420)

* Labels are part of the metric key, so they should be included in the duplicate metric error msg

* Include indexId in the link metric to ensure uniqueness.

We cannot update a link, but have to drop and recreate it. Until now, this new index had the same set of labels as the old one. However, on followers (and with replication2 also on the leader), the DropIndex and EnsureIndex actions could run concurrently. I.e., we could try to create the new index before the old one was fully removed. In this case we could get a duplicate metric exception, preventing the index from being created. Such errors are not really handled ATM, they are simply logged and otherwise ignored. That means the index will simply not be available on the affected server, since we will also do not retry to create it at a later time.

* Add changelog entry

* Also adjust getLabels

* Also add indexId for inverted index metrics

* Change order of labels

Shard label must be last, because some parsing code relies on it (the shard label is removed when accumulating collection metrics from different shards)

* Fix unit tests

* Adjust aggregation on coordinator

Author: mpoeter

CHANGELOG

@@ -1,6 +1,17 @@
 devel
 -----
 
+* Fix: we cannot update a link, but have to drop and recreate it. Until now,
+  this new index had the same set of labels as the old one. However, on followers
+  (and with replication2 also on the leader), the DropIndex and EnsureIndex
+  actions could run concurrently. I.e., we could try to create the new index
+  before the old one was fully removed. In this case we could get a duplicate
+  metric exception, preventing the index from being created. Such errors are not
+  really handled ATM - they are simply logged and otherwise ignored. That means
+  the index will simply not be available on the affected server, since we will
+  also not retry to create it at a later time.
+  To avoid this, we add a new `indexId` label to the metric to make it unique.
+
 * Expose REST API endpoints for server options discovery:
   - GET `/_admin/options-description`: returns a JSON description of all
     available server options.

arangod/IResearch/IResearchInvertedClusterIndex.cpp

@@ -105,7 +105,8 @@ IResearchDataStore::Stats IResearchInvertedClusterIndex::stats() const {
   auto labels = absl::StrCat(  // clang-format off
       "db=\"", getDbName(), "\","
       "index=\"", name(), "\","
-      "collection=\"", getCollectionName(), "\"");  // clang-format on
+      "collection=\"", getCollectionName(), "\",",
+      "indexId=\"", id().id(), "\"");  // clang-format on
   return {
       metrics.get<std::uint64_t>("arangodb_search_num_docs", labels),
       metrics.get<std::uint64_t>("arangodb_search_num_live_docs", labels),

arangod/IResearch/IResearchLink.cpp

@@ -95,6 +95,7 @@ T getMetric(IResearchLink const& link) {
   metric.addLabel("db", link.getDbName());
   metric.addLabel("view", link.getViewId());
   metric.addLabel("collection", link.getCollectionName());
+  metric.addLabel("indexId", std::to_string(link.index().id().id()));
   metric.addLabel("shard", link.getShardName());
   return metric;
 }
@@ -103,6 +104,7 @@ std::string getLabels(IResearchLink const& link) {
   return absl::StrCat("db=\"", link.getDbName(),                     //
                       "\",view=\"", link.getViewId(),                //
                       "\",collection=\"", link.getCollectionName(),  //
+                      "\",indexId=\"", link.index().id().id(),       //
                       "\",shard=\"", link.getShardName(), "\"");
 }
 

arangod/IResearch/IResearchLinkCoordinator.cpp

@@ -85,7 +85,8 @@ IResearchDataStore::Stats IResearchLinkCoordinator::stats() const {
   auto labels = absl::StrCat(  // clang-format off
       "db=\"", getDbName(), "\","
       "view=\"", getViewId(), "\","
-      "collection=\"", getCollectionName(), "\"");  // clang-format on
+      "collection=\"", getCollectionName(), "\","
+      "indexId=\"", id().id(), "\"");  // clang-format on
   return {
       metrics.get<std::uint64_t>("arangodb_search_num_docs", labels),
       metrics.get<std::uint64_t>("arangodb_search_num_live_docs", labels),

arangod/IResearch/IResearchRocksDBInvertedIndex.cpp

@@ -221,6 +221,7 @@ T getMetric(IResearchRocksDBInvertedIndex const& index) {
   metric.addLabel("db", index.getDbName());
   metric.addLabel("index", index.name());
   metric.addLabel("collection", index.getCollectionName());
+  metric.addLabel("indexId", std::to_string(index.id().id()));
   metric.addLabel("shard", index.getShardName());
   return metric;
 }
@@ -230,6 +231,7 @@ std::string getLabels(IResearchRocksDBInvertedIndex const& index) {
       "db=\"", index.getDbName(), "\","
       "index=\"", index.name(), "\","
       "collection=\"", index.getCollectionName(), "\","
+      "indexId=\"", index.id().id(), "\","
       "shard=\"", index.getShardName(), "\"");  // clang-format on
 }
 

arangod/Metrics/MetricsFeature.cpp

@@ -94,7 +94,8 @@ std::shared_ptr<Metric> MetricsFeature::doAdd(Builder& builder) {
   if (!_registry.try_emplace(key, metric).second) {
     THROW_ARANGO_EXCEPTION_MESSAGE(
         TRI_ERROR_INTERNAL,
-        absl::StrCat(builder.type(), " ", builder.name(), " already exists"));
+        absl::StrCat(builder.type(), " ", metric->name(), ":", metric->labels(),
+                     " already exists"));
   }
   return metric;
 }