From 7c848a12826ba9e9ef8bd823d3935878c7ae7af8 Mon Sep 17 00:00:00 2001 From: Arjun Date: Sat, 27 Apr 2019 14:54:20 +0200 Subject: [PATCH] Max collector should ignore only no result errors (#50) Signed-off-by: Arjun Naik --- pkg/collector/max_collector.go | 26 +++++++++--------- pkg/collector/max_collector_test.go | 39 +++++++++++++++++++++------ pkg/collector/prometheus_collector.go | 12 +++++++-- 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/pkg/collector/max_collector.go b/pkg/collector/max_collector.go index cf7fcc5..c194de6 100644 --- a/pkg/collector/max_collector.go +++ b/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 { diff --git a/pkg/collector/max_collector_test.go b/pkg/collector/max_collector_test.go index 776aa69..195ef9c 100644 --- a/pkg/collector/max_collector_test.go +++ b/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()) + } }) diff --git a/pkg/collector/prometheus_collector.go b/pkg/collector/prometheus_collector.go index 1700313..749dee5 100644 --- a/pkg/collector/prometheus_collector.go +++ b/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 {