Max collector should ignore only no result errors (#50)

Signed-off-by: Arjun Naik <arjun.rn@gmail.com>

pkg/collector/max_collector.go

@@ -16,7 +16,7 @@ type MaxWeightedCollector struct {
 	weight     float64
 }
 
-// NewMaxCollector initializes a new MacCollector.
+// NewMaxWeightedCollector initializes a new MaxWeightedCollector.
 func NewMaxWeightedCollector(interval time.Duration, weight float64, collectors ...Collector) *MaxWeightedCollector {
 	return &MaxWeightedCollector{
 		collectors: collectors,
@@ -32,24 +32,24 @@ func (c *MaxWeightedCollector) GetMetrics() ([]CollectedMetric, error) {
 	for _, collector := range c.collectors {
 		values, err := collector.GetMetrics()
 		if err != nil {
-			errors = append(errors, err)
-			continue
-		}
-		for _, v := range values {
-			collectedMetrics = append(collectedMetrics, v)
+			if _, ok := err.(NoResultError); ok {
+				errors = append(errors, err)
+				continue
+			}
+			return nil, err
 		}
+		collectedMetrics = append(collectedMetrics, values...)
 	}
 	if len(collectedMetrics) == 0 {
 		if len(errors) == 0 {
 			return nil, fmt.Errorf("no metrics collected, cannot determine max")
-		} else {
-			errorStrings := make([]string, len(errors))
-			for i, e := range errors {
-				errorStrings[i] = e.Error()
-			}
-			allErrors := strings.Join(errorStrings, ",")
-			return nil, fmt.Errorf("could not determine maximum due to errors: %s", allErrors)
 		}
+		errorStrings := make([]string, len(errors))
+		for i, e := range errors {
+			errorStrings[i] = e.Error()
+		}
+		allErrors := strings.Join(errorStrings, ",")
+		return nil, fmt.Errorf("could not determine maximum due to errors: %s", allErrors)
 	}
 	max := collectedMetrics[0]
 	for _, value := range collectedMetrics {

pkg/collector/max_collector_test.go

@@ -19,7 +19,12 @@ func (c dummyCollector) Interval() time.Duration {
 }
 
 func (c dummyCollector) GetMetrics() ([]CollectedMetric, error) {
-	if c.value > 0 {
+	switch c.value {
+	case 0:
+		return nil, NoResultError{query: "invalid query"}
+	case -1:
+		return nil, fmt.Errorf("test error")
+	default:
 		quantity := resource.NewQuantity(c.value, resource.DecimalSI)
 		return []CollectedMetric{
 			{
@@ -28,8 +33,6 @@ func (c dummyCollector) GetMetrics() ([]CollectedMetric, error) {
 				},
 			},
 		}, nil
-	} else {
-		return nil, fmt.Errorf("test error")
 	}
 }
 
@@ -39,24 +42,40 @@ func TestMaxCollector(t *testing.T) {
 		values   []int64
 		expected int
 		weight   float64
+		errored  bool
 	}{
 		{
 			name:     "basic",
 			values:   []int64{100, 10, 9},
 			expected: 100,
 			weight:   1,
+			errored:  false,
 		},
 		{
 			name:     "weighted",
 			values:   []int64{100, 10, 9},
 			expected: 20,
 			weight:   0.2,
+			errored:  false,
 		},
 		{
-			name:     "with error",
-			values:   []int64{-1, 10, 9},
+			name:    "with error",
+			values:  []int64{10, 9, -1},
+			weight:  0.5,
+			errored: true,
+		},
+		{
+			name:     "some invalid results",
+			values:   []int64{0, 1, 0, 10, 9},
 			expected: 5,
 			weight:   0.5,
+			errored:  false,
+		},
+		{
+			name:    "both invalid results and errors",
+			values:  []int64{0, 1, 0, -1, 10, 9},
+			weight:  0.5,
+			errored: true,
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -66,9 +85,13 @@ func TestMaxCollector(t *testing.T) {
 			}
 			wc := NewMaxWeightedCollector(time.Second, tc.weight, collectors...)
 			metrics, err := wc.GetMetrics()
-			require.NoError(t, err)
-			require.Len(t, metrics, 1)
-			require.EqualValues(t, tc.expected, metrics[0].Custom.Value.Value())
+			if tc.errored {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				require.Len(t, metrics, 1)
+				require.EqualValues(t, tc.expected, metrics[0].Custom.Value.Value())
+			}
 
 		})
 

pkg/collector/prometheus_collector.go

@@ -16,6 +16,14 @@ import (
 	"k8s.io/metrics/pkg/apis/custom_metrics"
 )
 
+type NoResultError struct {
+	query string
+}
+
+func (r NoResultError) Error() string {
+	return fmt.Sprintf("query '%s' did not result a valid response", r.query)
+}
+
 type PrometheusCollectorPlugin struct {
 	promAPI promv1.API
 	client  kubernetes.Interface
@@ -88,7 +96,7 @@ func (c *PrometheusCollector) GetMetrics() ([]CollectedMetric, error) {
 	case model.ValVector:
 		samples := value.(model.Vector)
 		if len(samples) == 0 {
-			return nil, fmt.Errorf("query '%s' returned no samples", c.query)
+			return nil, &NoResultError{query: c.query}
 		}
 
 		sampleValue = samples[0].Value
@@ -98,7 +106,7 @@ func (c *PrometheusCollector) GetMetrics() ([]CollectedMetric, error) {
 	}
 
 	if sampleValue.String() == "NaN" {
-		return nil, fmt.Errorf("query '%s' returned no samples: %s", c.query, sampleValue.String())
+		return nil, &NoResultError{query: c.query}
 	}
 
 	if c.perReplica {