stats: refactor forEachHostMetric to avoid using clusters()

envoy/upstream/cluster_manager.h

@@ -340,6 +340,14 @@ class ClusterManager {
    */
   virtual ClusterInfoMaps clusters() const PURE;
 
+  /**
+   * Iterates over all active clusters and invokes the callback for each.
+   * @param cb the callback to invoke for each active cluster.
+   *
+   * NOTE: This method is only thread safe on the main thread. It should not be called elsewhere.
+   */
+  virtual void forEachActiveCluster(std::function<void(const Cluster&)> cb) const PURE;
+
   /**
    * Receives a cluster name and returns an active cluster (if found).
    * @param cluster_name the name of the cluster.

source/common/upstream/cluster_manager_impl.h

@@ -272,6 +272,13 @@ class ClusterManagerImpl : public ClusterManager,
     return clusters_maps;
   }
 
+  void forEachActiveCluster(std::function<void(const Cluster&)> cb) const override {
+    ASSERT_IS_MAIN_OR_TEST_THREAD();
+    for (const auto& [unused_name, cluster_data] : active_clusters_) {
+      cb(*cluster_data->cluster_);
+    }
+  }
+
   OptRef<const Cluster> getActiveCluster(const std::string& cluster_name) const override {
     ASSERT_IS_MAIN_OR_TEST_THREAD();
     if (const auto& it = active_clusters_.find(cluster_name); it != active_clusters_.end()) {

source/common/upstream/host_utility.cc

@@ -175,15 +175,15 @@ void HostUtility::forEachHostMetric(
     const ClusterManager& cluster_manager,
     const std::function<void(Stats::PrimitiveCounterSnapshot&& metric)>& counter_cb,
     const std::function<void(Stats::PrimitiveGaugeSnapshot&& metric)>& gauge_cb) {
-  for (const auto& [unused_name, cluster_ref] : cluster_manager.clusters().active_clusters_) {
-    Upstream::ClusterInfoConstSharedPtr cluster_info = cluster_ref.get().info();
+  cluster_manager.forEachActiveCluster([&](const Cluster& cluster) {
+    Upstream::ClusterInfoConstSharedPtr cluster_info = cluster.info();
     if (cluster_info->perEndpointStatsEnabled()) {
       const std::string cluster_name =
           Stats::Utility::sanitizeStatsName(cluster_info->observabilityName());
 
       const Stats::TagVector& fixed_tags = cluster_info->statsScope().store().fixedTags();
 
-      for (auto& host_set : cluster_ref.get().prioritySet().hostSetsPerPriority()) {
+      for (auto& host_set : cluster.prioritySet().hostSetsPerPriority()) {
         for (auto& host : host_set->hosts()) {
 
           Stats::TagVector tags;
@@ -234,7 +234,7 @@ void HostUtility::forEachHostMetric(
         }
       }
     }
-  }
+  });
 }
 
 } // namespace Upstream

test/mocks/upstream/cluster_manager.cc

@@ -32,6 +32,12 @@ MockClusterManager::MockClusterManager()
                     OptRef<xds::core::v3::ResourceLocator>,
                     ProtobufMessage::ValidationVisitor&) { return MockOdCdsApiHandle::create(); }));
   ON_CALL(*this, addOrUpdateCluster(_, _, _)).WillByDefault(Return(false));
+  ON_CALL(*this, forEachActiveCluster(_))
+      .WillByDefault(Invoke([this](std::function<void(const Cluster&)> cb) {
+        for (const auto& [unused_name, cluster_ref] : clusters().active_clusters_) {
+          cb(cluster_ref.get());
+        }
+      }));
   ON_CALL(*this, hasActiveClusters()).WillByDefault(Return(false));
 }
 
@@ -56,6 +62,12 @@ void MockClusterManager::initializeClusters(const std::vector<std::string>& acti
   }
 
   ON_CALL(*this, clusters()).WillByDefault(Return(info_map));
+  ON_CALL(*this, forEachActiveCluster(_))
+      .WillByDefault(Invoke([this](std::function<void(const Cluster&)> cb) {
+        for (const auto& [unused_name, cluster] : active_clusters_) {
+          cb(*cluster);
+        }
+      }));
   ON_CALL(*this, getActiveCluster(_))
       .WillByDefault(Invoke([this](const std::string& cluster_name) -> OptRef<const Cluster> {
         if (const auto& it = active_clusters_.find(cluster_name); it != active_clusters_.end()) {

test/mocks/upstream/cluster_manager.h

@@ -43,6 +43,7 @@ class MockClusterManager : public ClusterManager {
   MOCK_METHOD(absl::Status, initializeSecondaryClusters,
               (const envoy::config::bootstrap::v3::Bootstrap& bootstrap));
   MOCK_METHOD(ClusterInfoMaps, clusters, (), (const));
+  MOCK_METHOD(void, forEachActiveCluster, (std::function<void(const Cluster&)>), (const));
   MOCK_METHOD(OptRef<const Cluster>, getActiveCluster, (const std::string& cluster_name), (const));
   MOCK_METHOD(OptRef<const Cluster>, getActiveOrWarmingCluster, (const std::string& cluster_name),
               (const));

test/server/admin/stats_handler_speed_test.cc

@@ -22,6 +22,12 @@ class FastMockClusterManager : public testing::StrictMock<Upstream::MockClusterM
 public:
   ClusterInfoMaps clusters() const override { return clusters_; }
 
+  void forEachActiveCluster(std::function<void(const Upstream::Cluster&)> cb) const override {
+    for (const auto& [unused_name, cluster_ref] : clusters_.active_clusters_) {
+      cb(cluster_ref.get());
+    }
+  }
+
   ClusterInfoMaps clusters_;
   std::vector<std::unique_ptr<FastMockCluster>> clusters_storage_;
   Stats::TestUtil::TestStore store_;

test/server/server_stats_flush_benchmark_test.cc

@@ -24,6 +24,7 @@ namespace Envoy {
 class FastMockClusterManager : public testing::StrictMock<Upstream::MockClusterManager> {
 public:
   ClusterInfoMaps clusters() const override { return ClusterInfoMaps{}; }
+  void forEachActiveCluster(std::function<void(const Upstream::Cluster&)>) const override {}
 };
 
 class TestSinkPredicates : public Stats::SinkPredicates {