Review: error handling in scraper

pkg/autoscaler/collector.go

@@ -259,7 +259,7 @@ func newCollection(metric *av1alpha1.Metric, scraper StatsScraper, logger *zap.S
 				scrapeTicker.Stop()
 				return
 			case <-scrapeTicker.C:
-				message, err := c.getScraper().Scrape()
+				message, err := c.getScraper().Scrape(logger)
 				if err != nil {
 					logger.Errorw("Failed to scrape metrics", zap.Error(err))
 					metric.Status.MarkMetricFailed("FailToScapeMetricsReason", "Metrics scaping failed.")

pkg/autoscaler/collector_test.go

@@ -27,6 +27,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	. "knative.dev/pkg/logging/testing"
 	av1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
+
+	"go.uber.org/zap"
 )
 
 var (
@@ -238,6 +240,6 @@ type testScraper struct {
 	url string
 }
 
-func (s *testScraper) Scrape() (*StatMessage, error) {
+func (s *testScraper) Scrape(logger *zap.SugaredLogger) (*StatMessage, error) {
 	return s.s()
 }

pkg/autoscaler/stats_scraper.go

@@ -22,6 +22,7 @@ import (
 	"sync"
 	"time"
 
+	"go.uber.org/zap"
 	"golang.org/x/sync/errgroup"
 	"k8s.io/apimachinery/pkg/types"
 	"github.com/pkg/errors"
@@ -51,17 +52,17 @@ const (
 var (
 	// errFailedGetEndpoints specifies the error returned by scraper when it fails to
 	// get endpoints.
-	errFailedGetEndpoints = "failed to get endpoints"
+	errFailedGetEndpoints = errors.New("failed to get endpoints")
 
 	// errDidNotReceiveStat specifies the error returned by scraper when it does not receive
 	// stat from an unscraped pod
-	errDidNotReceiveStat = "did not receive stat from an unscraped pod"
+	errDidNotReceiveStat = errors.New("did not receive stat from an unscraped pod")
 )
 
 // StatsScraper defines the interface for collecting Revision metrics
 type StatsScraper interface {
 	// Scrape scrapes the Revision queue metric endpoint.
-	Scrape() (*StatMessage, error)
+	Scrape(logger *zap.SugaredLogger) (*StatMessage, error)
 }
 
 // scrapeClient defines the interface for collecting Revision metrics for a given
@@ -138,10 +139,11 @@ func urlFromTarget(t, ns string) string {
 
 // Scrape calls the destination service then sends it
 // to the given stats channel.
-func (s *ServiceScraper) Scrape() (*StatMessage, error) {
+func (s *ServiceScraper) Scrape(logger *zap.SugaredLogger) (*StatMessage, error) {
 	readyPodsCount, err := s.counter.ReadyCount()
 	if err != nil {
-		return nil, errors.Wrap(err, errFailedGetEndpoints)
+		logger.Errorw(errFailedGetEndpoints.Error(), zap.Error(err))
+		return nil, errFailedGetEndpoints
 	}
 
 	if readyPodsCount == 0 {
@@ -172,7 +174,8 @@ func (s *ServiceScraper) Scrape() (*StatMessage, error) {
 
 	// Return the inner error, if any.
 	if err := grp.Wait(); err != nil {
-		return nil, errors.Wrapf(err, "unsuccessful scrape, sampleSize=%d", sampleSize)
+		logger.Errorw(fmt.Sprintf("unsuccessful scrape, sampleSize=%d", sampleSize), zap.Error(err))
+		return nil, err
 	}
 	close(statCh)
 
@@ -230,7 +233,7 @@ func (s *ServiceScraper) tryScrape(scrapedPods *sync.Map) (*Stat, error) {
 	}
 
 	if _, exists := scrapedPods.LoadOrStore(stat.PodName, struct{}{}); exists {
-		return nil, errors.New(errDidNotReceiveStat)
+		return nil, errDidNotReceiveStat
 	}
 
 	return stat, nil
@@ -239,11 +242,11 @@ func (s *ServiceScraper) tryScrape(scrapedPods *sync.Map) (*Stat, error) {
 // IsErrFailedGetEndpoints determines if the err is an error which indicates
 // the scaper fails to get endpoints
 func IsErrFailedGetEndpoints(err error) bool {
-	return err.Error() == errFailedGetEndpoints
+	return err.Error() == errFailedGetEndpoints.Error()
 }
 
 // IsErrDidNotReceiveStat determines if the err is an error which indicates
 // the scaper did not receive stat from an unscraped pod
 func IsErrDidNotReceiveStat(err error) bool {
-	return err.Error() == errDidNotReceiveStat
+	return err.Error() == errDidNotReceiveStat.Error()
 }

pkg/autoscaler/stats_scraper_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	. "knative.dev/pkg/logging/testing"
 	av1alpha1 "knative.dev/serving/pkg/apis/autoscaling/v1alpha1"
 	"knative.dev/serving/pkg/apis/serving"
 	"knative.dev/serving/pkg/resources"
@@ -129,6 +130,9 @@ func TestNewServiceScraperWithClientErrorCases(t *testing.T) {
 }
 
 func TestScrapeReportStatWhenAllCallsSucceed(t *testing.T) {
+	defer ClearAll()
+	logger := TestLogger(t)
+
 	client := newTestScrapeClient(testStats, []error{nil})
 	scraper, err := serviceScraperForTest(client)
 	if err != nil {
@@ -140,9 +144,9 @@ func TestScrapeReportStatWhenAllCallsSucceed(t *testing.T) {
 
 	// Scrape will set a timestamp bigger than this.
 	now := time.Now()
-	got, err := scraper.Scrape()
+	got, err := scraper.Scrape(logger)
 	if err != nil {
-		t.Fatalf("unexpected error from scraper.Scrape(): %v", err)
+		t.Fatalf("unexpected error from scraper.Scrape(logger): %v", err)
 	}
 
 	if got.Key != testPAKey {
@@ -175,6 +179,9 @@ func TestScrapeReportStatWhenAllCallsSucceed(t *testing.T) {
 }
 
 func TestScrapeReportErrorCannotFindEnoughPods(t *testing.T) {
+	defer ClearAll()
+	logger := TestLogger(t)
+
 	client := newTestScrapeClient(testStats[2:], []error{nil})
 	scraper, err := serviceScraperForTest(client)
 	if err != nil {
@@ -184,13 +191,16 @@ func TestScrapeReportErrorCannotFindEnoughPods(t *testing.T) {
 	// Make an Endpoints with 2 pods.
 	endpoints(2, testService)
 
-	_, err = scraper.Scrape()
+	_, err = scraper.Scrape(logger)
 	if err == nil {
-		t.Errorf("scrape.Scrape() = nil, expected an error")
+		t.Errorf("scrape.Scrape(logger) = nil, expected an error")
 	}
 }
 
 func TestScrapeReportErrorIfAnyFails(t *testing.T) {
+	defer ClearAll()
+	logger := TestLogger(t)
+
 	errTest := errors.New("test")
 
 	// 1 success and 10 failures so one scrape fails permanently through retries.
@@ -204,13 +214,16 @@ func TestScrapeReportErrorIfAnyFails(t *testing.T) {
 	// Make an Endpoints with 2 pods.
 	endpoints(2, testService)
 
-	_, err = scraper.Scrape()
+	_, err = scraper.Scrape(logger)
 	if errors.Cause(err) != errTest {
-		t.Errorf("scraper.Scrape() = %v, want %v", err, errTest)
+		t.Errorf("scraper.Scrape(logger) = %v, want %v", err, errTest)
 	}
 }
 
 func TestScrapeDoNotScrapeIfNoPodsFound(t *testing.T) {
+	defer ClearAll()
+	logger := TestLogger(t)
+
 	client := newTestScrapeClient(testStats, nil)
 	scraper, err := serviceScraperForTest(client)
 	if err != nil {
@@ -220,9 +233,9 @@ func TestScrapeDoNotScrapeIfNoPodsFound(t *testing.T) {
 	// Make an Endpoints with 0 pods.
 	endpoints(0, testService)
 
-	stat, err := scraper.Scrape()
+	stat, err := scraper.Scrape(logger)
 	if err != nil {
-		t.Fatalf("got error from scraper.Scrape() = %v", err)
+		t.Fatalf("got error from scraper.Scrape(logger) = %v", err)
 	}
 	if stat != nil {
 		t.Error("Received unexpected StatMessage.")
@@ -279,3 +292,53 @@ func TestURLFromTarget(t *testing.T) {
 		t.Errorf("urlFromTarget = %s, want: %s, diff: %s", got, want, cmp.Diff(got, want))
 	}
 }
+
+func TestIsErrFailedGetEndpoints(t *testing.T) {
+	testCases := []struct {
+		name           string
+		err            error
+		expectedResult bool
+	}{{
+		name:           "Failed to get endpoints error",
+		err:            errors.New("failed to get endpoints"),
+		expectedResult: true,
+	}, {
+		name:           "Other random error",
+		err:            errors.New("foo"),
+		expectedResult: false,
+	}}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			got := IsErrFailedGetEndpoints(test.err)
+			want := test.expectedResult
+			if got != want {
+				t.Errorf("Got: %v. Want: %v", got, want)
+			}
+		})
+	}
+}
+
+func TestIsErrDidNotReceiveStat(t *testing.T) {
+	testCases := []struct {
+		name           string
+		err            error
+		expectedResult bool
+	}{{
+		name:           "Did not receive stat error",
+		err:            errors.New("did not receive stat from an unscraped pod"),
+		expectedResult: true,
+	}, {
+		name:           "Other random error",
+		err:            errors.New("foo"),
+		expectedResult: false,
+	}}
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			got := IsErrDidNotReceiveStat(test.err)
+			want := test.expectedResult
+			if got != want {
+				t.Errorf("Got: %v. Want: %v", got, want)
+			}
+		})
+	}
+}