Repository: knative/serving
Pull request #7845 (2020-05-04 23:07:03)

Simplify and unify the metric aggreggation when computing averages.
We have 2 copies of the basically same code and in general it does not look very nice.
So instead extract the functions to the Stat so it can aggregate on itself
 and then just reduce the collection to a trivial loops.

/lint

Followup for #5978.
/assign @yanweiguo @markusthoemmes 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/7845#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/autoscaler/OWNERS](https://github.com/knative/serving/blob/master/pkg/autoscaler/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-serving-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/autoscaler/metrics/collector.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/7845/pull-knative-serving-go-coverage/1257446817038077952/artifacts/line-cov.html#file0) | 95.8% | 96.0% | 0.3
[pkg/autoscaler/metrics/stats_scraper.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/7845/pull-knative-serving-go-coverage/1257446817038077952/artifacts/line-cov.html#file1) | 89.9% | 92.4% | 2.5

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-serving-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/autoscaler/metrics/collector.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/7845/pull-knative-serving-go-coverage/1257453612657807361/artifacts/line-cov.html#file0) | 95.8% | 96.0% | 0.3
[pkg/autoscaler/metrics/stats_scraper.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/7845/pull-knative-serving-go-coverage/1257453612657807361/artifacts/line-cov.html#file1) | 89.9% | 92.4% | 2.5


Patch:
diff --git a/config/core/configmaps/leader-election.yaml b/config/core/configmaps/leader-election.yaml
index b3c42f7ba..665c6d53d 100644
--- a/config/core/configmaps/leader-election.yaml
+++ b/config/core/configmaps/leader-election.yaml
@@ -20,11 +20,6 @@ metadata:
   labels:
     serving.knative.dev/release: devel
 data:
-  # An inactive but valid configuration follows; see example.
-  resourceLock: "leases"
-  leaseDuration: "15s"
-  renewDeadline: "10s"
-  retryPeriod: "2s"
   _example: |
     ################################
     #                              #
diff --git a/pkg/autoscaler/metrics/collector.go b/pkg/autoscaler/metrics/collector.go
index 00c539719..f5b1b74b5 100644
--- a/pkg/autoscaler/metrics/collector.go
+++ b/pkg/autoscaler/metrics/collector.go
@@ -406,3 +406,29 @@ func (c *collection) close() {
 	close(c.stopCh)
 	c.grp.Wait()
 }
+
+// add adds the stats from `src` to `dst`.
+func (dst *Stat) add(src Stat) {
+	dst.AverageConcurrentRequests += src.AverageConcurrentRequests
+	dst.AverageProxiedConcurrentRequests += src.AverageProxiedConcurrentRequests
+	dst.RequestCount += src.RequestCount
+	dst.ProxiedRequestCount += src.ProxiedRequestCount
+}
+
+// average reduces the aggregate stat from `sample` pods to an averaged one over
+// `total` pods.
+// The method performs no checks on the data, i.e. that sample is > 0.
+//
+// Assumption: A particular pod can stand for other pods, i.e. other pods
+// have similar concurrency and QPS.
+//
+// Hide the actual pods behind scraper and send only one stat for all the
+// customer pods per scraping. The pod name is set to a unique value, i.e.
+// scraperPodName so in autoscaler all stats are either from activator or
+// scraper.
+func (dst *Stat) average(sample, total float64) {
+	dst.AverageConcurrentRequests = dst.AverageConcurrentRequests / sample * total
+	dst.AverageProxiedConcurrentRequests = dst.AverageProxiedConcurrentRequests / sample * total
+	dst.RequestCount = dst.RequestCount / sample * total
+	dst.ProxiedRequestCount = dst.ProxiedRequestCount / sample * total
+}
diff --git a/pkg/autoscaler/metrics/stats_scraper.go b/pkg/autoscaler/metrics/stats_scraper.go
index 36c135753..6c019570a 100644
--- a/pkg/autoscaler/metrics/stats_scraper.go
+++ b/pkg/autoscaler/metrics/stats_scraper.go
@@ -280,35 +280,22 @@ func (s *serviceScraper) scrapePods(readyPods int) (Stat, error) {
 		return emptyStat, errNoPodsScraped
 	}
 
-	var (
-		avgConcurrency        float64
-		avgProxiedConcurrency float64
-		reqCount              float64
-		proxiedReqCount       float64
-	)
-
-	// Process the stats from individual pods.
-	for stat := range results {
-		avgConcurrency += stat.AverageConcurrentRequests
-		avgProxiedConcurrency += stat.AverageProxiedConcurrentRequests
-		reqCount += stat.RequestCount
-		proxiedReqCount += stat.ProxiedRequestCount
-	}
-
-	avgConcurrency = avgConcurrency / sampleSizeF
-	avgProxiedConcurrency = avgProxiedConcurrency / sampleSizeF
-	reqCount = reqCount / sampleSizeF
-	proxiedReqCount = proxiedReqCount / sampleSizeF
+	return computeAverages(results, sampleSizeF, frpc), nil
+}
 
+func computeAverages(results <-chan Stat, sample, total float64) Stat {
 	ret := Stat{
-		Time:                             time.Now(),
-		PodName:                          scraperPodName,
-		AverageConcurrentRequests:        avgConcurrency * frpc,
-		AverageProxiedConcurrentRequests: avgProxiedConcurrency * frpc,
-		RequestCount:                     reqCount * frpc,
-		ProxiedRequestCount:              proxiedReqCount * frpc,
+		Time:    time.Now(),
+		PodName: scraperPodName,
 	}
-	return ret, nil
+
+	// Sum the stats from individual pods.
+	for stat := range results {
+		ret.add(stat)
+	}
+
+	ret.average(sample, total)
+	return ret
 }
 
 // scrapeService scrapes the metrics using service endpoint
@@ -375,49 +362,23 @@ func (s *serviceScraper) scrapeService(window time.Duration, readyPods int) (Sta
 	close(oldStatCh)
 	close(youngStatCh)
 
-	var (
-		avgConcurrency        float64
-		avgProxiedConcurrency float64
-		reqCount              float64
-		proxiedReqCount       float64
-	)
+	ret := Stat{
+		Time:    time.Now(),
+		PodName: scraperPodName,
+	}
 
+	// Sum the stats from individual pods.
 	oldCnt := len(oldStatCh)
 	for stat := range oldStatCh {
-		avgConcurrency += stat.AverageConcurrentRequests
-		avgProxiedConcurrency += stat.AverageProxiedConcurrentRequests
-		reqCount += stat.RequestCount
-		proxiedReqCount += stat.ProxiedRequestCount
+		ret.add(stat)
 	}
 	for i := oldCnt; i < sampleSize; i++ {
 		// This will always succeed, see reasoning above.
-		stat := <-youngStatCh
-		avgConcurrency += stat.AverageConcurrentRequests
-		avgProxiedConcurrency += stat.AverageProxiedConcurrentRequests
-		reqCount += stat.RequestCount
-		proxiedReqCount += stat.ProxiedRequestCount
+		ret.add(<-youngStatCh)
 	}
 
-	avgConcurrency = avgConcurrency / sampleSizeF
-	avgProxiedConcurrency = avgProxiedConcurrency / sampleSizeF
-	reqCount = reqCount / sampleSizeF
-	proxiedReqCount = proxiedReqCount / sampleSizeF
-
-	// Assumption: A particular pod can stand for other pods, i.e. other pods
-	// have similar concurrency and QPS.
-	//
-	// Hide the actual pods behind scraper and send only one stat for all the
-	// customer pods per scraping. The pod name is set to a unique value, i.e.
-	// scraperPodName so in autoscaler all stats are either from activator or
-	// scraper.
-	return Stat{
-		Time:                             time.Now(),
-		PodName:                          scraperPodName,
-		AverageConcurrentRequests:        avgConcurrency * frpc,
-		AverageProxiedConcurrentRequests: avgProxiedConcurrency * frpc,
-		RequestCount:                     reqCount * frpc,
-		ProxiedRequestCount:              proxiedReqCount * frpc,
-	}, nil
+	ret.average(sampleSizeF, frpc)
+	return ret, nil
 }
 
 // tryScrape runs a single scrape and returns stat if this is a pod that has not been


Test patch:
diff --git a/pkg/leaderelection/config_test.go b/pkg/leaderelection/config_test.go
index 0b0bd6378..e45ba435c 100644
--- a/pkg/leaderelection/config_test.go
+++ b/pkg/leaderelection/config_test.go
@@ -96,8 +96,7 @@ func TestValidateConfig(t *testing.T) {
 	}
 }
 func TestServingConfig(t *testing.T) {
-	actual, example := ConfigMapsFromTestFile(t, "config-leader-election",
-		"resourceLock", "leaseDuration", "renewDeadline", "retryPeriod")
+	actual, example := ConfigMapsFromTestFile(t, "config-leader-election")
 	for _, test := range []struct {
 		name string
 		data *corev1.ConfigMap