Merge pull request #24240 from donnadionne/drop_stats

Wrapping DroppedRequestsMap inside a DroppedRequests strcuture which

src/core/ext/xds/xds_api.cc

@@ -2339,7 +2339,7 @@ grpc_slice XdsApi::CreateLrsRequest(
     }
     // Add dropped requests.
     uint64_t total_dropped_requests = 0;
-    for (const auto& p : load_report.dropped_requests) {
+    for (const auto& p : load_report.dropped_requests.categorized_drops) {
       const std::string& category = p.first;
       const uint64_t count = p.second;
       envoy_config_endpoint_v3_ClusterStats_DroppedRequests* dropped_requests =
@@ -2351,6 +2351,7 @@ grpc_slice XdsApi::CreateLrsRequest(
           dropped_requests, count);
       total_dropped_requests += count;
     }
+    total_dropped_requests += load_report.dropped_requests.uncategorized_drops;
     // Set total dropped requests.
     envoy_config_endpoint_v3_ClusterStats_set_total_dropped_requests(
         cluster_stats, total_dropped_requests);

src/core/ext/xds/xds_api.h

@@ -273,7 +273,7 @@ class XdsApi {
   using EdsUpdateMap = std::map<std::string /*eds_service_name*/, EdsUpdate>;
 
   struct ClusterLoadReport {
-    XdsClusterDropStats::DroppedRequestsMap dropped_requests;
+    XdsClusterDropStats::Snapshot dropped_requests;
     std::map<RefCountedPtr<XdsLocalityName>, XdsClusterLocalityStats::Snapshot,
              XdsLocalityName::Less>
         locality_stats;

src/core/ext/xds/xds_client.cc

@@ -1296,9 +1296,7 @@ namespace {
 bool LoadReportCountersAreZero(const XdsApi::ClusterLoadReportMap& snapshot) {
   for (const auto& p : snapshot) {
     const XdsApi::ClusterLoadReport& cluster_snapshot = p.second;
-    for (const auto& q : cluster_snapshot.dropped_requests) {
+    if (!cluster_snapshot.dropped_requests.IsZero()) return false;
-      if (q.second > 0) return false;
-    }
     for (const auto& q : cluster_snapshot.locality_stats) {
       const XdsClusterLocalityStats::Snapshot& locality_snapshot = q.second;
       if (!locality_snapshot.IsZero()) return false;
@@ -2007,9 +2005,8 @@ void XdsClient::RemoveClusterDropStats(
   if (it != load_report_state.drop_stats.end()) {
     // Record final drop stats in deleted_drop_stats, which will be
     // added to the next load report.
-    for (const auto& p : cluster_drop_stats->GetSnapshotAndReset()) {
+    auto dropped_requests = cluster_drop_stats->GetSnapshotAndReset();
-      load_report_state.deleted_drop_stats[p.first] += p.second;
+    load_report_state.deleted_drop_stats += dropped_requests;
-    }
     load_report_state.drop_stats.erase(it);
   }
 }
@@ -2120,9 +2117,8 @@ XdsApi::ClusterLoadReportMap XdsClient::BuildLoadReportSnapshotLocked(
     // Aggregate drop stats.
     snapshot.dropped_requests = std::move(load_report.deleted_drop_stats);
     for (auto& drop_stats : load_report.drop_stats) {
-      for (const auto& p : drop_stats->GetSnapshotAndReset()) {
+      auto dropped_requests = drop_stats->GetSnapshotAndReset();
-        snapshot.dropped_requests[p.first] += p.second;
+      snapshot.dropped_requests += dropped_requests;
-      }
     }
     // Aggregate locality stats.
     for (auto it = load_report.locality_stats.begin();

src/core/ext/xds/xds_client.h

@@ -278,7 +278,7 @@ class XdsClient : public DualRefCounted<XdsClient> {
     };
 
     std::set<XdsClusterDropStats*> drop_stats;
-    XdsClusterDropStats::DroppedRequestsMap deleted_drop_stats;
+    XdsClusterDropStats::Snapshot deleted_drop_stats;
     std::map<RefCountedPtr<XdsLocalityName>, LocalityState,
              XdsLocalityName::Less>
         locality_stats;

src/core/ext/xds/xds_client_stats.cc

@@ -29,6 +29,14 @@
 
 namespace grpc_core {
 
+namespace {
+
+uint64_t GetAndResetCounter(Atomic<uint64_t>* from) {
+  return from->Exchange(0, MemoryOrder::RELAXED);
+}
+
+}  // namespace
+
 //
 // XdsClusterDropStats
 //
@@ -48,15 +56,21 @@ XdsClusterDropStats::~XdsClusterDropStats() {
   xds_client_.reset(DEBUG_LOCATION, "DropStats");
 }
 
-XdsClusterDropStats::DroppedRequestsMap
+XdsClusterDropStats::Snapshot XdsClusterDropStats::GetSnapshotAndReset() {
-XdsClusterDropStats::GetSnapshotAndReset() {
+  Snapshot snapshot;
+  snapshot.uncategorized_drops = GetAndResetCounter(&uncategorized_drops_);
   MutexLock lock(&mu_);
-  return std::move(dropped_requests_);
+  snapshot.categorized_drops = std::move(categorized_drops_);
+  return snapshot;
+}
+
+void XdsClusterDropStats::AddUncategorizedDrops() {
+  uncategorized_drops_.FetchAdd(1);
 }
 
 void XdsClusterDropStats::AddCallDropped(const std::string& category) {
   MutexLock lock(&mu_);
-  ++dropped_requests_[category];
+  ++categorized_drops_[category];
 }
 
 //
@@ -79,14 +93,6 @@ XdsClusterLocalityStats::~XdsClusterLocalityStats() {
   xds_client_.reset(DEBUG_LOCATION, "LocalityStats");
 }
 
-namespace {
-
-uint64_t GetAndResetCounter(Atomic<uint64_t>* from) {
-  return from->Exchange(0, MemoryOrder::RELAXED);
-}
-
-}  // namespace
-
 XdsClusterLocalityStats::Snapshot
 XdsClusterLocalityStats::GetSnapshotAndReset() {
   Snapshot snapshot = {GetAndResetCounter(&total_successful_requests_),

src/core/ext/xds/xds_client_stats.h

@@ -99,7 +99,31 @@ class XdsLocalityName : public RefCounted<XdsLocalityName> {
 // Drop stats for an xds cluster.
 class XdsClusterDropStats : public RefCounted<XdsClusterDropStats> {
  public:
-  using DroppedRequestsMap = std::map<std::string /* category */, uint64_t>;
+  // The total number of requests dropped for any reason is the sum of
+  // uncategorized_drops, and dropped_requests map.
+  using CategorizedDropsMap = std::map<std::string /* category */, uint64_t>;
+  struct Snapshot {
+    uint64_t uncategorized_drops = 0;
+    // The number of requests dropped for the specific drop categories
+    // outlined in the drop_overloads field in the EDS response.
+    CategorizedDropsMap categorized_drops;
+
+    Snapshot& operator+=(const Snapshot& other) {
+      uncategorized_drops += other.uncategorized_drops;
+      for (const auto& p : other.categorized_drops) {
+        categorized_drops[p.first] += p.second;
+      }
+      return *this;
+    }
+
+    bool IsZero() const {
+      if (uncategorized_drops != 0) return false;
+      for (const auto& p : categorized_drops) {
+        if (p.second != 0) return false;
+      }
+      return true;
+    }
+  };
 
   XdsClusterDropStats(RefCountedPtr<XdsClient> xds_client,
                       absl::string_view lrs_server_name,
@@ -108,8 +132,9 @@ class XdsClusterDropStats : public RefCounted<XdsClusterDropStats> {
   ~XdsClusterDropStats();
 
   // Returns a snapshot of this instance and resets all the counters.
-  DroppedRequestsMap GetSnapshotAndReset();
+  Snapshot GetSnapshotAndReset();
 
+  void AddUncategorizedDrops();
   void AddCallDropped(const std::string& category);
 
  private:
@@ -117,11 +142,12 @@ class XdsClusterDropStats : public RefCounted<XdsClusterDropStats> {
   absl::string_view lrs_server_name_;
   absl::string_view cluster_name_;
   absl::string_view eds_service_name_;
-  // Protects dropped_requests_. A mutex is necessary because the length of
+  Atomic<uint64_t> uncategorized_drops_{0};
-  // dropped_requests_ can be accessed by both the picker (from data plane
+  // Protects categorized_drops_. A mutex is necessary because the length of
+  // dropped_requests can be accessed by both the picker (from data plane
   // mutex) and the load reporting thread (from the control plane combiner).
   Mutex mu_;
-  DroppedRequestsMap dropped_requests_;
+  CategorizedDropsMap categorized_drops_;
 };
 
 // Locality stats for an xds cluster.