From 973e3dc60e487d9ee4748bd088952722de108aab Mon Sep 17 00:00:00 2001 From: Arjan Singh Bal <46515553+arjan-bal@users.noreply.github.com> Date: Tue, 27 Aug 2024 02:50:00 +0530 Subject: [PATCH] xdsclient: Populate total_issued_requests count in LRS load reports (#7544) (#7565) * Populate issued count in LRS load report * Test success, error and issued counts * Make pass/fail fractions unequal --- .../balancer/clusterimpl/balancer_test.go | 31 +++++++++--- xds/internal/xdsclient/load/store.go | 17 ++++++- xds/internal/xdsclient/load/store_test.go | 50 +++++++++++++------ .../xdsclient/transport/loadreport.go | 1 + .../xdsclient/transport/loadreport_test.go | 2 + 5 files changed, 80 insertions(+), 21 deletions(-) diff --git a/xds/internal/balancer/clusterimpl/balancer_test.go b/xds/internal/balancer/clusterimpl/balancer_test.go index 5a4bb0f270b2..1797f9fd109a 100644 --- a/xds/internal/balancer/clusterimpl/balancer_test.go +++ b/xds/internal/balancer/clusterimpl/balancer_test.go @@ -142,7 +142,7 @@ func (s) TestDropByCategory(t *testing.T) { sc1.UpdateState(balancer.SubConnState{ConnectivityState: connectivity.Ready}) // Test pick with one backend. - const rpcCount = 20 + const rpcCount = 24 if err := cc.WaitForPicker(ctx, func(p balancer.Picker) error { for i := 0; i < rpcCount; i++ { gotSCSt, err := p.Pick(balancer.PickInfo{}) @@ -156,7 +156,13 @@ func (s) TestDropByCategory(t *testing.T) { if err != nil || gotSCSt.SubConn != sc1 { return fmt.Errorf("picker.Pick, got %v, %v, want SubConn=%v", gotSCSt, err, sc1) } - if gotSCSt.Done != nil { + if gotSCSt.Done == nil { + continue + } + // Fail 1/4th of the requests that are not dropped. + if i%8 == 1 { + gotSCSt.Done(balancer.DoneInfo{Err: fmt.Errorf("test error")}) + } else { gotSCSt.Done(balancer.DoneInfo{}) } } @@ -177,7 +183,11 @@ func (s) TestDropByCategory(t *testing.T) { TotalDrops: dropCount, Drops: map[string]uint64{dropReason: dropCount}, LocalityStats: map[string]load.LocalityData{ - assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{Succeeded: rpcCount - dropCount}}, + assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{ + Succeeded: (rpcCount - dropCount) * 3 / 4, + Errored: (rpcCount - dropCount) / 4, + Issued: rpcCount - dropCount, + }}, }, }} @@ -239,7 +249,10 @@ func (s) TestDropByCategory(t *testing.T) { TotalDrops: dropCount2, Drops: map[string]uint64{dropReason2: dropCount2}, LocalityStats: map[string]load.LocalityData{ - assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{Succeeded: rpcCount - dropCount2}}, + assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{ + Succeeded: rpcCount - dropCount2, + Issued: rpcCount - dropCount2, + }}, }, }} @@ -332,7 +345,9 @@ func (s) TestDropCircuitBreaking(t *testing.T) { } dones = append(dones, func() { if gotSCSt.Done != nil { - gotSCSt.Done(balancer.DoneInfo{}) + // Fail these requests to test error counts in the load + // report. + gotSCSt.Done(balancer.DoneInfo{Err: fmt.Errorf("test error")}) } }) } @@ -356,7 +371,11 @@ func (s) TestDropCircuitBreaking(t *testing.T) { Service: testServiceName, TotalDrops: uint64(maxRequest), LocalityStats: map[string]load.LocalityData{ - assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{Succeeded: uint64(rpcCount - maxRequest + 50)}}, + assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{ + Succeeded: uint64(rpcCount - maxRequest), + Errored: 50, + Issued: uint64(rpcCount - maxRequest + 50), + }}, }, }} diff --git a/xds/internal/xdsclient/load/store.go b/xds/internal/xdsclient/load/store.go index 1f266ae20185..f1e265ee7ddf 100644 --- a/xds/internal/xdsclient/load/store.go +++ b/xds/internal/xdsclient/load/store.go @@ -174,6 +174,7 @@ func (ls *perClusterStore) CallStarted(locality string) { p, _ = ls.localityRPCCount.LoadOrStore(locality, tp) } p.(*rpcCountData).incrInProgress() + p.(*rpcCountData).incrIssued() } // CallFinished adds one call finished record for the given locality. @@ -248,6 +249,8 @@ type RequestData struct { Errored uint64 // InProgress is the number of requests in flight. InProgress uint64 + // Issued is the total number requests that were sent. + Issued uint64 } // ServerLoadData contains server load data. @@ -296,7 +299,8 @@ func (ls *perClusterStore) stats() *Data { succeeded := countData.loadAndClearSucceeded() inProgress := countData.loadInProgress() errored := countData.loadAndClearErrored() - if succeeded == 0 && inProgress == 0 && errored == 0 { + issued := countData.loadAndClearIssued() + if succeeded == 0 && inProgress == 0 && errored == 0 && issued == 0 { return true } @@ -305,6 +309,7 @@ func (ls *perClusterStore) stats() *Data { Succeeded: succeeded, Errored: errored, InProgress: inProgress, + Issued: issued, }, LoadStats: make(map[string]ServerLoadData), } @@ -339,6 +344,7 @@ type rpcCountData struct { succeeded *uint64 errored *uint64 inProgress *uint64 + issued *uint64 // Map from load desc to load data (sum+count). Loading data from map is // atomic, but updating data takes a lock, which could cause contention when @@ -353,6 +359,7 @@ func newRPCCountData() *rpcCountData { succeeded: new(uint64), errored: new(uint64), inProgress: new(uint64), + issued: new(uint64), } } @@ -384,6 +391,14 @@ func (rcd *rpcCountData) loadInProgress() uint64 { return atomic.LoadUint64(rcd.inProgress) // InProgress count is not clear when reading. } +func (rcd *rpcCountData) incrIssued() { + atomic.AddUint64(rcd.issued, 1) +} + +func (rcd *rpcCountData) loadAndClearIssued() uint64 { + return atomic.SwapUint64(rcd.issued, 0) +} + func (rcd *rpcCountData) addServerLoad(name string, d float64) { loads, ok := rcd.serverLoads.Load(name) if !ok { diff --git a/xds/internal/xdsclient/load/store_test.go b/xds/internal/xdsclient/load/store_test.go index 88629af0c38b..44618966859c 100644 --- a/xds/internal/xdsclient/load/store_test.go +++ b/xds/internal/xdsclient/load/store_test.go @@ -99,7 +99,12 @@ func TestLocalityStats(t *testing.T) { wantStoreData = &Data{ LocalityStats: map[string]LocalityData{ localities[0]: { - RequestStats: RequestData{Succeeded: 20, Errored: 10, InProgress: 10}, + RequestStats: RequestData{ + Succeeded: 20, + Errored: 10, + InProgress: 10, + Issued: 40, + }, LoadStats: map[string]ServerLoadData{ "net": {Count: 20, Sum: 20}, "disk": {Count: 20, Sum: 40}, @@ -108,7 +113,12 @@ func TestLocalityStats(t *testing.T) { }, }, localities[1]: { - RequestStats: RequestData{Succeeded: 40, Errored: 20, InProgress: 20}, + RequestStats: RequestData{ + Succeeded: 40, + Errored: 20, + InProgress: 20, + Issued: 80, + }, LoadStats: map[string]ServerLoadData{ "net": {Count: 40, Sum: 40}, "disk": {Count: 40, Sum: 80}, @@ -192,7 +202,13 @@ func TestResetAfterStats(t *testing.T) { }, LocalityStats: map[string]LocalityData{ localities[0]: { - RequestStats: RequestData{Succeeded: 20, Errored: 10, InProgress: 10}, + RequestStats: RequestData{ + Succeeded: 20, + Errored: 10, + InProgress: 10, + Issued: 40, + }, + LoadStats: map[string]ServerLoadData{ "net": {Count: 20, Sum: 20}, "disk": {Count: 20, Sum: 40}, @@ -201,7 +217,13 @@ func TestResetAfterStats(t *testing.T) { }, }, localities[1]: { - RequestStats: RequestData{Succeeded: 40, Errored: 20, InProgress: 20}, + RequestStats: RequestData{ + Succeeded: 40, + Errored: 20, + InProgress: 20, + Issued: 80, + }, + LoadStats: map[string]ServerLoadData{ "net": {Count: 40, Sum: 40}, "disk": {Count: 40, Sum: 80}, @@ -298,7 +320,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -308,7 +330,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -327,7 +349,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -337,7 +359,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -347,7 +369,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -357,7 +379,7 @@ func TestStoreStats(t *testing.T) { TotalDrops: 1, Drops: map[string]uint64{"dropped": 1}, LocalityStats: map[string]LocalityData{ "test-locality": { - RequestStats: RequestData{Succeeded: 1}, + RequestStats: RequestData{Succeeded: 1, Issued: 1}, LoadStats: map[string]ServerLoadData{"abc": {Count: 1, Sum: 123}}, }, }, @@ -394,25 +416,25 @@ func TestStoreStatsEmptyDataNotReported(t *testing.T) { { Cluster: "c0", Service: "s0", LocalityStats: map[string]LocalityData{ - "test-locality": {RequestStats: RequestData{Succeeded: 1}}, + "test-locality": {RequestStats: RequestData{Succeeded: 1, Issued: 1}}, }, }, { Cluster: "c0", Service: "s1", LocalityStats: map[string]LocalityData{ - "test-locality": {RequestStats: RequestData{Succeeded: 1}}, + "test-locality": {RequestStats: RequestData{Succeeded: 1, Issued: 1}}, }, }, { Cluster: "c1", Service: "s0", LocalityStats: map[string]LocalityData{ - "test-locality": {RequestStats: RequestData{InProgress: 1}}, + "test-locality": {RequestStats: RequestData{InProgress: 1, Issued: 1}}, }, }, { Cluster: "c1", Service: "s1", LocalityStats: map[string]LocalityData{ - "test-locality": {RequestStats: RequestData{InProgress: 1}}, + "test-locality": {RequestStats: RequestData{InProgress: 1, Issued: 1}}, }, }, } diff --git a/xds/internal/xdsclient/transport/loadreport.go b/xds/internal/xdsclient/transport/loadreport.go index 289fd62cbc75..e47fdd9846ba 100644 --- a/xds/internal/xdsclient/transport/loadreport.go +++ b/xds/internal/xdsclient/transport/loadreport.go @@ -223,6 +223,7 @@ func (t *Transport) sendLoadStatsRequest(stream lrsStream, loads []*load.Data) e TotalSuccessfulRequests: localityData.RequestStats.Succeeded, TotalRequestsInProgress: localityData.RequestStats.InProgress, TotalErrorRequests: localityData.RequestStats.Errored, + TotalIssuedRequests: localityData.RequestStats.Issued, LoadMetricStats: loadMetricStats, UpstreamEndpointStats: nil, // TODO: populate for per endpoint loads. }) diff --git a/xds/internal/xdsclient/transport/loadreport_test.go b/xds/internal/xdsclient/transport/loadreport_test.go index 24770897ea29..0f61c50d5b2a 100644 --- a/xds/internal/xdsclient/transport/loadreport_test.go +++ b/xds/internal/xdsclient/transport/loadreport_test.go @@ -151,12 +151,14 @@ func (s) TestReportLoad(t *testing.T) { // TotalMetricValue is the aggregation of 3.14 + 2.718 = 5.858 {MetricName: testKey1, NumRequestsFinishedWithMetric: 2, TotalMetricValue: 5.858}}, TotalSuccessfulRequests: 1, + TotalIssuedRequests: 1, }, { Locality: &v3corepb.Locality{Region: "test-region2"}, LoadMetricStats: []*v3endpointpb.EndpointLoadMetricStats{ {MetricName: testKey2, NumRequestsFinishedWithMetric: 1, TotalMetricValue: 1.618}}, TotalSuccessfulRequests: 1, + TotalIssuedRequests: 1, }, }, }