From 74c4b68997f318059a22a1e4da9f5c442f0d32eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Tue, 23 Jun 2020 22:39:27 +0200 Subject: [PATCH] Remove time from stat struct as it's never sent anyway. (#8410) --- cmd/autoscaler/main.go | 2 +- pkg/autoscaler/metrics/collector.go | 21 +++++------- pkg/autoscaler/metrics/collector_test.go | 14 +++----- pkg/autoscaler/metrics/stats_scraper.go | 2 -- pkg/autoscaler/metrics/stats_scraper_test.go | 35 +------------------- pkg/autoscaler/scaling/multiscaler_test.go | 1 - pkg/autoscaler/statserver/server.go | 2 -- pkg/autoscaler/statserver/server_test.go | 9 ++--- pkg/reconciler/metric/metric_test.go | 2 +- 9 files changed, 18 insertions(+), 70 deletions(-) diff --git a/cmd/autoscaler/main.go b/cmd/autoscaler/main.go index 12d3b91f7d61..56b464c5a9e0 100644 --- a/cmd/autoscaler/main.go +++ b/cmd/autoscaler/main.go @@ -165,7 +165,7 @@ func main() { go func() { for sm := range statsCh { - collector.Record(sm.Key, sm.Stat) + collector.Record(sm.Key, time.Now(), sm.Stat) multiScaler.Poke(sm.Key, sm.Stat) } }() diff --git a/pkg/autoscaler/metrics/collector.go b/pkg/autoscaler/metrics/collector.go index 267d23aa94af..494703b17aab 100644 --- a/pkg/autoscaler/metrics/collector.go +++ b/pkg/autoscaler/metrics/collector.go @@ -48,9 +48,6 @@ type StatsScraperFactory func(*av1alpha1.Metric, *zap.SugaredLogger) (StatsScrap // Stat defines a single measurement at a point in time type Stat struct { - // The time the data point was received by autoscaler. - Time time.Time - // The unique identity of this pod. Used to count how many pods // are contributing to the metrics. PodName string @@ -86,7 +83,7 @@ type Collector interface { // it already exist. CreateOrUpdate(*av1alpha1.Metric) error // Record allows stats to be captured that came from outside the Collector. - Record(key types.NamespacedName, stat Stat) + Record(key types.NamespacedName, now time.Time, stat Stat) // Delete deletes a Metric and halts collection. Delete(string, string) error // Watch registers a singleton function to call when a specific collector's status changes. @@ -169,12 +166,12 @@ func (c *MetricCollector) Delete(namespace, name string) error { } // Record records a stat that's been generated outside of the metric collector. -func (c *MetricCollector) Record(key types.NamespacedName, stat Stat) { +func (c *MetricCollector) Record(key types.NamespacedName, now time.Time, stat Stat) { c.collectionsMutex.RLock() defer c.collectionsMutex.RUnlock() if collection, exists := c.collections[key]; exists { - collection.record(stat) + collection.record(now, stat) } } @@ -319,7 +316,7 @@ func newCollection(metric *av1alpha1.Metric, scraper StatsScraper, tickFactory f callback(key) } if stat != emptyStat { - c.record(stat) + c.record(time.Now(), stat) } } } @@ -375,15 +372,15 @@ func (c *collection) lastError() error { } // record adds a stat to the current collection. -func (c *collection) record(stat Stat) { +func (c *collection) record(now time.Time, stat Stat) { // Proxied requests have been counted at the activator. Subtract // them to avoid double counting. concurr := stat.AverageConcurrentRequests - stat.AverageProxiedConcurrentRequests - c.concurrencyBuckets.Record(stat.Time, concurr) - c.concurrencyPanicBuckets.Record(stat.Time, concurr) + c.concurrencyBuckets.Record(now, concurr) + c.concurrencyPanicBuckets.Record(now, concurr) rps := stat.RequestCount - stat.ProxiedRequestCount - c.rpsBuckets.Record(stat.Time, rps) - c.rpsPanicBuckets.Record(stat.Time, rps) + c.rpsBuckets.Record(now, rps) + c.rpsPanicBuckets.Record(now, rps) } // add adds the stats from `src` to `dst`. diff --git a/pkg/autoscaler/metrics/collector_test.go b/pkg/autoscaler/metrics/collector_test.go index 4c1fda2eaf1b..cfbb2ed19832 100644 --- a/pkg/autoscaler/metrics/collector_test.go +++ b/pkg/autoscaler/metrics/collector_test.go @@ -133,7 +133,6 @@ func TestMetricCollectorScraper(t *testing.T) { wantPRPS = 3 * 20 ) stat := Stat{ - Time: now, PodName: "testPod", AverageConcurrentRequests: reportConcurrency, RequestCount: reportRPS, @@ -221,7 +220,6 @@ func TestMetricCollectorNoScraper(t *testing.T) { metricKey := types.NamespacedName{Namespace: defaultNamespace, Name: defaultName} const wantStat = 0. stat := Stat{ - Time: now, PodName: "testPod", AverageConcurrentRequests: wantStat, RequestCount: wantStat, @@ -268,7 +266,7 @@ func TestMetricCollectorNoScraper(t *testing.T) { stat.RequestCount = wantRC stat.AverageConcurrentRequests = wantAverageRC - coll.Record(metricKey, stat) + coll.Record(metricKey, now, stat) gotConcurrency, _, _ = coll.StableAndPanicConcurrency(metricKey, now) gotRPS, _, err := coll.StableAndPanicRPS(metricKey, now) @@ -290,7 +288,6 @@ func TestMetricCollectorNoDataError(t *testing.T) { metricKey := types.NamespacedName{Namespace: defaultNamespace, Name: defaultName} const wantStat = 0. stat := Stat{ - Time: now, PodName: "testPod", AverageConcurrentRequests: wantStat, RequestCount: wantStat, @@ -323,13 +320,11 @@ func TestMetricCollectorRecord(t *testing.T) { metricKey := types.NamespacedName{Namespace: defaultNamespace, Name: defaultName} const want = 10.0 outdatedStat := Stat{ - Time: oldTime, PodName: "testPod", AverageConcurrentRequests: 100, RequestCount: 100, } stat := Stat{ - Time: now, PodName: "testPod", AverageConcurrentRequests: want + 10, AverageProxiedConcurrentRequests: 10, // this should be subtracted from the above. @@ -360,8 +355,8 @@ func TestMetricCollectorRecord(t *testing.T) { // Add two stats. The second record operation will remove the first outdated one. // After this the concurrencies are calculated correctly. - coll.Record(metricKey, outdatedStat) - coll.Record(metricKey, stat) + coll.Record(metricKey, oldTime, outdatedStat) + coll.Record(metricKey, now, stat) stable, panic, err := coll.StableAndPanicConcurrency(metricKey, now) if err != nil { t.Fatal("StableAndPanicConcurrency:", err) @@ -508,12 +503,11 @@ func TestMetricCollectorAggregate(t *testing.T) { now := time.Now() for i := time.Duration(0); i < 10; i++ { stat := Stat{ - Time: now.Add(i * time.Second), PodName: "testPod", AverageConcurrentRequests: float64(i + 5), RequestCount: float64(i + 5), } - c.record(stat) + c.record(now.Add(i*time.Second), stat) } now = now.Add(9 * time.Second) diff --git a/pkg/autoscaler/metrics/stats_scraper.go b/pkg/autoscaler/metrics/stats_scraper.go index 45cbaa2693e1..7d529e620694 100644 --- a/pkg/autoscaler/metrics/stats_scraper.go +++ b/pkg/autoscaler/metrics/stats_scraper.go @@ -297,7 +297,6 @@ func (s *serviceScraper) scrapePods(readyPods int) (Stat, error) { func computeAverages(results <-chan Stat, sample, total float64) Stat { ret := Stat{ - Time: time.Now(), PodName: scraperPodName, } @@ -379,7 +378,6 @@ func (s *serviceScraper) scrapeService(window time.Duration, readyPods int) (Sta close(youngStatCh) ret := Stat{ - Time: time.Now(), PodName: scraperPodName, } diff --git a/pkg/autoscaler/metrics/stats_scraper_test.go b/pkg/autoscaler/metrics/stats_scraper_test.go index f3338291214a..e66cd7126e76 100644 --- a/pkg/autoscaler/metrics/stats_scraper_test.go +++ b/pkg/autoscaler/metrics/stats_scraper_test.go @@ -189,16 +189,11 @@ func TestPodDirectScrapeSuccess(t *testing.T) { } fake.Endpoints(3, fake.TestService) makePods(3) - now := time.Now() - got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) - if err != nil { + if _, err := scraper.Scrape(defaultMetric.Spec.StableWindow); err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want greater than %v", got.Time, now) - } if !scraper.podsAddressable { t.Error("PodAddressable switched to false") } @@ -216,16 +211,12 @@ func TestPodDirectScrapeSomeFailButSuccess(t *testing.T) { } fake.Endpoints(5, fake.TestService) makePods(5) - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want greater than %v", got.Time, now) - } // Checking one of the metrics is enough here. if got.AverageConcurrentRequests != 20.0 { t.Errorf("stat.AverageConcurrentRequests=%v, want %v", @@ -256,16 +247,12 @@ func TestPodDirectScrapeNoneSucceed(t *testing.T) { } fake.Endpoints(4, fake.TestService) makePods(4) - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want greater than %v", got.Time, now) - } // Checking one of the metrics is enough here. if got.AverageConcurrentRequests != 20.0 { t.Errorf("stat.AverageConcurrentRequests=%v, want %v", @@ -309,16 +296,11 @@ func TestScrapeReportStatWhenAllCallsSucceed(t *testing.T) { // Make an Endpoints with 3 pods. fake.Endpoints(3, fake.TestService) - // Scrape will set a timestamp bigger than this. - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want greater than %v", got.Time, now) - } checkBaseStat(t, got) } @@ -341,16 +323,11 @@ func TestScrapeAllPodsYoungPods(t *testing.T) { fake.Endpoints(numP, fake.TestService) - // Scrape will set a timestamp bigger than this. - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want bigger than %v", got.Time, now) - } if got.PodName != scraperPodName { t.Errorf("stat.PodName=%v, want %v", got.PodName, scraperPodName) } @@ -374,16 +351,11 @@ func TestScrapeAllPodsOldPods(t *testing.T) { fake.Endpoints(numP, fake.TestService) - // Scrape will set a timestamp bigger than this. - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want bigger than %v", got.Time, now) - } if got.PodName != scraperPodName { t.Errorf("stat.PodName=%v, want %v", got.PodName, scraperPodName) } @@ -408,16 +380,11 @@ func TestScrapeSomePodsOldPods(t *testing.T) { fake.Endpoints(numP, fake.TestService) - // Scrape will set a timestamp bigger than this. - now := time.Now() got, err := scraper.Scrape(defaultMetric.Spec.StableWindow) if err != nil { t.Fatal("Unexpected error from scraper.Scrape():", err) } - if got.Time.Before(now) { - t.Errorf("stat.Time=%v, want bigger than %v", got.Time, now) - } if got.PodName != scraperPodName { t.Errorf("stat.PodName=%v, want %v", got.PodName, scraperPodName) } diff --git a/pkg/autoscaler/scaling/multiscaler_test.go b/pkg/autoscaler/scaling/multiscaler_test.go index 56bdd7de2414..c27eec56885c 100644 --- a/pkg/autoscaler/scaling/multiscaler_test.go +++ b/pkg/autoscaler/scaling/multiscaler_test.go @@ -354,7 +354,6 @@ func TestMultiScalerScaleFromZero(t *testing.T) { } testStat := metrics.Stat{ - Time: time.Now(), PodName: "test-pod", AverageConcurrentRequests: 1, RequestCount: 1, diff --git a/pkg/autoscaler/statserver/server.go b/pkg/autoscaler/statserver/server.go index be5bb3215681..c58b17d13f8a 100644 --- a/pkg/autoscaler/statserver/server.go +++ b/pkg/autoscaler/statserver/server.go @@ -175,8 +175,6 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { continue } - sm.Stat.Time = time.Now() - s.logger.Debugf("Received stat message: %+v", sm) s.statsCh <- sm } diff --git a/pkg/autoscaler/statserver/server_test.go b/pkg/autoscaler/statserver/server_test.go index d5708a47f5bd..2b14398aff7d 100644 --- a/pkg/autoscaler/statserver/server_test.go +++ b/pkg/autoscaler/statserver/server_test.go @@ -29,7 +29,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/gorilla/websocket" "go.uber.org/zap" "golang.org/x/sync/errgroup" @@ -220,12 +219,8 @@ func assertReceivedOK(t *testing.T, sm metrics.StatMessage, statSink *websocket. if !ok { t.Fatal("Statistic not received") } - if recv.Stat.Time == (time.Time{}) { - t.Fatal("Stat time is nil") - } - ignoreTimeField := cmpopts.IgnoreFields(metrics.StatMessage{}, "Stat.Time") - if !cmp.Equal(sm, recv, ignoreTimeField) { - t.Fatalf("StatMessage mismatch: diff (-got, +want) %s", cmp.Diff(recv, sm, ignoreTimeField)) + if !cmp.Equal(sm, recv) { + t.Fatalf("StatMessage mismatch: diff (-got, +want) %s", cmp.Diff(recv, sm)) } return true } diff --git a/pkg/reconciler/metric/metric_test.go b/pkg/reconciler/metric/metric_test.go index becaaf5eac93..fd1f4559da1c 100644 --- a/pkg/reconciler/metric/metric_test.go +++ b/pkg/reconciler/metric/metric_test.go @@ -276,7 +276,7 @@ func (c *testCollector) CreateOrUpdate(metric *av1alpha1.Metric) error { return c.createOrUpdateError } -func (c *testCollector) Record(key types.NamespacedName, stat metrics.Stat) { +func (c *testCollector) Record(key types.NamespacedName, now time.Time, stat metrics.Stat) { if c.recordCalls != nil { c.recordCalls <- struct{}{} }