diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 0c2e36bdfd1..49dce583b53 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/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); diff --git a/src/core/ext/xds/xds_api.h b/src/core/ext/xds/xds_api.h index d08248021a1..87cfa29a617 100644 --- a/src/core/ext/xds/xds_api.h +++ b/src/core/ext/xds/xds_api.h @@ -273,7 +273,7 @@ class XdsApi { using EdsUpdateMap = std::map; struct ClusterLoadReport { - XdsClusterDropStats::DroppedRequestsMap dropped_requests; + XdsClusterDropStats::Snapshot dropped_requests; std::map, XdsClusterLocalityStats::Snapshot, XdsLocalityName::Less> locality_stats; diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index ee96b9fe5c2..6d448641b03 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/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 (q.second > 0) return false; - } + if (!cluster_snapshot.dropped_requests.IsZero()) 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()) { - load_report_state.deleted_drop_stats[p.first] += p.second; - } + auto dropped_requests = cluster_drop_stats->GetSnapshotAndReset(); + 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()) { - snapshot.dropped_requests[p.first] += p.second; - } + auto dropped_requests = drop_stats->GetSnapshotAndReset(); + snapshot.dropped_requests += dropped_requests; } // Aggregate locality stats. for (auto it = load_report.locality_stats.begin(); diff --git a/src/core/ext/xds/xds_client.h b/src/core/ext/xds/xds_client.h index 6eeb1790e66..3e74c2c38dc 100644 --- a/src/core/ext/xds/xds_client.h +++ b/src/core/ext/xds/xds_client.h @@ -278,7 +278,7 @@ class XdsClient : public DualRefCounted { }; std::set drop_stats; - XdsClusterDropStats::DroppedRequestsMap deleted_drop_stats; + XdsClusterDropStats::Snapshot deleted_drop_stats; std::map, LocalityState, XdsLocalityName::Less> locality_stats; diff --git a/src/core/ext/xds/xds_client_stats.cc b/src/core/ext/xds/xds_client_stats.cc index 1bea78fd4f4..ba29ec085f6 100644 --- a/src/core/ext/xds/xds_client_stats.cc +++ b/src/core/ext/xds/xds_client_stats.cc @@ -29,6 +29,14 @@ namespace grpc_core { +namespace { + +uint64_t GetAndResetCounter(Atomic* from) { + return from->Exchange(0, MemoryOrder::RELAXED); +} + +} // namespace + // // XdsClusterDropStats // @@ -48,15 +56,21 @@ XdsClusterDropStats::~XdsClusterDropStats() { xds_client_.reset(DEBUG_LOCATION, "DropStats"); } -XdsClusterDropStats::DroppedRequestsMap -XdsClusterDropStats::GetSnapshotAndReset() { +XdsClusterDropStats::Snapshot 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* from) { - return from->Exchange(0, MemoryOrder::RELAXED); -} - -} // namespace - XdsClusterLocalityStats::Snapshot XdsClusterLocalityStats::GetSnapshotAndReset() { Snapshot snapshot = {GetAndResetCounter(&total_successful_requests_), diff --git a/src/core/ext/xds/xds_client_stats.h b/src/core/ext/xds/xds_client_stats.h index b0671052399..87d816ba21a 100644 --- a/src/core/ext/xds/xds_client_stats.h +++ b/src/core/ext/xds/xds_client_stats.h @@ -99,7 +99,31 @@ class XdsLocalityName : public RefCounted { // Drop stats for an xds cluster. class XdsClusterDropStats : public RefCounted { public: - using DroppedRequestsMap = std::map; + // The total number of requests dropped for any reason is the sum of + // uncategorized_drops, and dropped_requests map. + using CategorizedDropsMap = std::map; + 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 xds_client, absl::string_view lrs_server_name, @@ -108,8 +132,9 @@ class XdsClusterDropStats : public RefCounted { ~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 { 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 - // dropped_requests_ can be accessed by both the picker (from data plane + Atomic uncategorized_drops_{0}; + // 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.