From af6439a59c41d14cb93d203599bc0d0ee52054d6 Mon Sep 17 00:00:00 2001 From: AhmedGrati Date: Wed, 19 Jul 2023 20:27:42 +0100 Subject: [PATCH] feat: add prometheus timeout attribute Signed-off-by: AhmedGrati --- metricproviders/metricproviders.go | 2 +- metricproviders/prometheus/prometheus.go | 25 +++++++-- metricproviders/prometheus/prometheus_test.go | 54 ++++++++++++++----- pkg/apis/rollouts/v1alpha1/analysis_types.go | 3 ++ 4 files changed, 65 insertions(+), 19 deletions(-) diff --git a/metricproviders/metricproviders.go b/metricproviders/metricproviders.go index ed544a7f04..f73ff15e88 100644 --- a/metricproviders/metricproviders.go +++ b/metricproviders/metricproviders.go @@ -41,7 +41,7 @@ func (f *ProviderFactory) NewProvider(logCtx log.Entry, metric v1alpha1.Metric) if err != nil { return nil, err } - return prometheus.NewPrometheusProvider(api, logCtx), nil + return prometheus.NewPrometheusProvider(api, logCtx, metric) case job.ProviderType: return job.NewJobProvider(logCtx, f.KubeClient, f.JobLister), nil case kayenta.ProviderType: diff --git a/metricproviders/prometheus/prometheus.go b/metricproviders/prometheus/prometheus.go index 83c9ec83f2..4fe8905d61 100644 --- a/metricproviders/prometheus/prometheus.go +++ b/metricproviders/prometheus/prometheus.go @@ -33,8 +33,9 @@ const ( // Provider contains all the required components to run a prometheus query type Provider struct { - api v1.API - logCtx log.Entry + api v1.API + logCtx log.Entry + Timeout time.Duration } // Type indicates provider is a prometheus provider @@ -59,7 +60,7 @@ func (p *Provider) Run(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric) v1alph } //TODO(dthomson) make timeout configurable - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), p.Timeout) defer cancel() response, warnings, err := p.api.Query(ctx, metric.Provider.Prometheus.Query, time.Now()) @@ -138,11 +139,25 @@ func (p *Provider) processResponse(metric v1alpha1.Metric, response model.Value) } // NewPrometheusProvider Creates a new Prometheus client -func NewPrometheusProvider(api v1.API, logCtx log.Entry) *Provider { - return &Provider{ +func NewPrometheusProvider(api v1.API, logCtx log.Entry, metric v1alpha1.Metric) (*Provider, error) { + provider := &Provider{ logCtx: logCtx, api: api, } + + if metric.Provider.Prometheus == nil || metric.Provider.Prometheus.Timeout == nil { + provider.Timeout = 30 * time.Second + return provider, nil + } + + metricTimeout := metric.Provider.Prometheus.Timeout + + if *metricTimeout < 0 { + return nil, errors.New("prometheus timeout should not be negative") + } + + provider.Timeout = time.Duration(*metricTimeout * int(time.Second)) + return provider, nil } // NewPrometheusAPI generates a prometheus API from the metric configuration diff --git a/metricproviders/prometheus/prometheus_test.go b/metricproviders/prometheus/prometheus_test.go index d62f080bcd..7e234443e0 100644 --- a/metricproviders/prometheus/prometheus_test.go +++ b/metricproviders/prometheus/prometheus_test.go @@ -5,6 +5,7 @@ import ( "math" "os" "testing" + "time" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" v1 "github.com/prometheus/client_golang/api/prometheus/v1" @@ -30,8 +31,22 @@ func TestType(t *testing.T) { mock := mockAPI{ value: newScalar(10), } - p := NewPrometheusProvider(mock, e) + timeout := 5 + metric := v1alpha1.Metric{ + Name: "foo", + SuccessCondition: "result == 10", + FailureCondition: "result != 10", + Provider: v1alpha1.MetricProvider{ + Prometheus: &v1alpha1.PrometheusMetric{ + Query: "test", + Timeout: &timeout, + }, + }, + } + p, err := NewPrometheusProvider(mock, e, metric) + assert.NoError(t, err) assert.Equal(t, ProviderType, p.Type()) + assert.Equal(t, p.Timeout, time.Duration(timeout*int(time.Second))) } func TestRunSuccessfully(t *testing.T) { @@ -39,7 +54,6 @@ func TestRunSuccessfully(t *testing.T) { mock := mockAPI{ value: newScalar(10), } - p := NewPrometheusProvider(mock, e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -50,8 +64,11 @@ func TestRunSuccessfully(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, e, metric) + measurement := p.Run(newAnalysisRun(), metric) assert.NotNil(t, measurement.StartedAt) + assert.NoError(t, err) assert.Equal(t, "10", measurement.Value) assert.NotNil(t, measurement.FinishedAt) assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, measurement.Phase) @@ -64,7 +81,6 @@ func TestRunSuccessfullyWithEnv(t *testing.T) { } address := "http://127.0.0.1:9090" os.Setenv(EnvVarArgoRolloutsPrometheusAddress, address) - p := NewPrometheusProvider(mock, e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -75,8 +91,10 @@ func TestRunSuccessfullyWithEnv(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.NotNil(t, measurement.StartedAt) + assert.NoError(t, err) assert.Equal(t, "10", measurement.Value) assert.NotNil(t, measurement.FinishedAt) assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, measurement.Phase) @@ -88,7 +106,6 @@ func TestRunSuccessfullyWithWarning(t *testing.T) { value: newScalar(10), warnings: v1.Warnings([]string{"warning", "warning2"}), } - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -99,8 +116,10 @@ func TestRunSuccessfullyWithWarning(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, *e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.NotNil(t, measurement.StartedAt) + assert.NoError(t, err) assert.Equal(t, "10", measurement.Value) assert.NotNil(t, measurement.FinishedAt) assert.Equal(t, `"warning", "warning2"`, measurement.Metadata["warnings"]) @@ -113,7 +132,6 @@ func TestRunSuccessfullyWithWarningWithEnv(t *testing.T) { value: newScalar(10), warnings: v1.Warnings([]string{"warning", "warning2"}), } - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -124,8 +142,10 @@ func TestRunSuccessfullyWithWarningWithEnv(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, *e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.NotNil(t, measurement.StartedAt) + assert.NoError(t, err) assert.Equal(t, "10", measurement.Value) assert.NotNil(t, measurement.FinishedAt) assert.Equal(t, `"warning", "warning2"`, measurement.Metadata["warnings"]) @@ -138,7 +158,6 @@ func TestRunWithQueryError(t *testing.T) { mock := mockAPI{ err: expectedErr, } - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -149,8 +168,10 @@ func TestRunWithQueryError(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, *e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.Equal(t, expectedErr.Error(), measurement.Message) + assert.NoError(t, err) assert.NotNil(t, measurement.StartedAt) assert.Equal(t, "", measurement.Value) assert.NotNil(t, measurement.FinishedAt) @@ -163,7 +184,6 @@ func TestRunWithResolveArgsError(t *testing.T) { mock := mockAPI{ err: expectedErr, } - p := NewPrometheusProvider(mock, e) metric := v1alpha1.Metric{ Name: "foo", Provider: v1alpha1.MetricProvider{ @@ -172,8 +192,10 @@ func TestRunWithResolveArgsError(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.Equal(t, expectedErr.Error(), measurement.Message) + assert.NoError(t, err) assert.NotNil(t, measurement.StartedAt) assert.Equal(t, "", measurement.Value) assert.NotNil(t, measurement.FinishedAt) @@ -183,7 +205,6 @@ func TestRunWithResolveArgsError(t *testing.T) { func TestGetStatusReturnsResolvedQuery(t *testing.T) { e := log.Entry{} mock := mockAPI{} - p := NewPrometheusProvider(mock, e) metric := v1alpha1.Metric{ Name: "foo", Provider: v1alpha1.MetricProvider{ @@ -192,15 +213,16 @@ func TestGetStatusReturnsResolvedQuery(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, e, metric) metricsMetadata := p.GetMetadata(metric) assert.NotNil(t, metricsMetadata) + assert.NoError(t, err) assert.Equal(t, "resolved-query", metricsMetadata["ResolvedPrometheusQuery"]) } func TestRunWithEvaluationError(t *testing.T) { e := log.WithField("", "") mock := mockAPI{} - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -211,8 +233,10 @@ func TestRunWithEvaluationError(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, *e, metric) measurement := p.Run(newAnalysisRun(), metric) assert.Equal(t, "Prometheus metric type not supported", measurement.Message) + assert.NoError(t, err) assert.NotNil(t, measurement.StartedAt) assert.Equal(t, "", measurement.Value) assert.NotNil(t, measurement.FinishedAt) @@ -222,7 +246,6 @@ func TestRunWithEvaluationError(t *testing.T) { func TestResume(t *testing.T) { e := log.WithField("", "") mock := mockAPI{} - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{ Name: "foo", SuccessCondition: "result == 10", @@ -233,6 +256,8 @@ func TestResume(t *testing.T) { }, }, } + p, err := NewPrometheusProvider(mock, *e, metric) + assert.NoError(t, err) now := metav1.Now() previousMeasurement := v1alpha1.Measurement{ StartedAt: &now, @@ -245,8 +270,9 @@ func TestResume(t *testing.T) { func TestTerminate(t *testing.T) { e := log.NewEntry(log.New()) mock := mockAPI{} - p := NewPrometheusProvider(mock, *e) metric := v1alpha1.Metric{} + p, err := NewPrometheusProvider(mock, *e, metric) + assert.NoError(t, err) now := metav1.Now() previousMeasurement := v1alpha1.Measurement{ StartedAt: &now, @@ -259,8 +285,10 @@ func TestTerminate(t *testing.T) { func TestGarbageCollect(t *testing.T) { e := log.NewEntry(log.New()) mock := mockAPI{} - p := NewPrometheusProvider(mock, *e) - err := p.GarbageCollect(nil, v1alpha1.Metric{}, 0) + metric := v1alpha1.Metric{} + p, err := NewPrometheusProvider(mock, *e, metric) + assert.NoError(t, err) + err = p.GarbageCollect(nil, v1alpha1.Metric{}, 0) assert.NoError(t, err) } diff --git a/pkg/apis/rollouts/v1alpha1/analysis_types.go b/pkg/apis/rollouts/v1alpha1/analysis_types.go index 72364344de..43fee98a4f 100644 --- a/pkg/apis/rollouts/v1alpha1/analysis_types.go +++ b/pkg/apis/rollouts/v1alpha1/analysis_types.go @@ -213,6 +213,9 @@ type PrometheusMetric struct { // Sigv4 Config is the aws SigV4 configuration to use for SigV4 signing if using Amazon Managed Prometheus // +optional Authentication PrometheusAuth `json:"authentication,omitempty" protobuf:"bytes,3,opt,name=authentication"` + // Timeout represents the duration within which a prometheus query should complete. It is expressed in seconds. + // +optional + Timeout *int `json:"timeout,omitempty" protobuf:"bytes,4,opt,name=timeout"` } // PrometheusMetric defines the prometheus query to perform canary analysis