Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Web metric provider #318

Merged
merged 29 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
296169f
Initial implementation for the WebMetric type of analysis.
tomsanbear Dec 3, 2019
eb42130
Wrote basic test, and fixed some functionality for an initial POC
tomsanbear Dec 3, 2019
56074a1
Clean up debug logging
tomsanbear Dec 3, 2019
94cd16b
Ran codegen
tomsanbear Dec 3, 2019
ff388e4
Made minor fixes suggested in by Rollouts team: WebMetric naming upda…
tomsanbear Dec 9, 2019
9eac5a3
Smarter logic for detecting type in the JSON response. Also parameter…
tomsanbear Dec 9, 2019
3fa6f71
Fix Infinite loop with PreviewReplicaCount set (#308)
dthomson25 Dec 3, 2019
084b1d7
Fix incorrect patch on getting started guide (#315)
dthomson25 Dec 3, 2019
1046115
Create one background analysis per revision (#309)
dthomson25 Dec 3, 2019
a24a733
Bluegreen: allow preview service/replica sets to be replaced and fix …
mathetake Dec 4, 2019
5feb382
Set StableRS hash to current if replicaset does not actually exist (#…
dthomson25 Dec 5, 2019
53ee15b
Update version to v0.6.1
dthomson25 Dec 6, 2019
81f59fe
Add Community Blogs and Presentations section (#322)
saradhis Dec 10, 2019
bdc1ec8
Fix a typo (#321)
bpaquet Dec 10, 2019
867a0f6
Fix link to deployment concepts in docs (#325)
petvaa01 Dec 12, 2019
c61a43b
fix: omitted revisionHistoryLimit was not defaulting to 10 (#330)
jessesuen Dec 16, 2019
f870efe
Fix panics with incorrectly configured rollouts (#328)
dthomson25 Dec 16, 2019
88e59d6
Add Nginx docs for traffic management (#326)
dthomson25 Dec 16, 2019
53b806e
Update version to v0.6.2
dthomson25 Dec 16, 2019
4496241
Fix error handling with HTTP client, change timeout field to timeoutS…
tomsanbear Dec 17, 2019
5dd2fce
Add simple non 2xx test.
tomsanbear Dec 17, 2019
e3321fe
Add test for headers, fix bug where header is unitialized, causing NPE.
tomsanbear Dec 18, 2019
0feb20b
Add a few more corner cases for HTTP responses.
tomsanbear Dec 18, 2019
81b91e1
golang-ci fix
tomsanbear Dec 18, 2019
eb30b36
First pass at a using the evaluation engine to enforce typing.
tomsanbear Jan 7, 2020
6bd5577
Increment numProviders when Web is present.
tomsanbear Jan 12, 2020
99c8430
Deal with merge issues.
tomsanbear Jan 12, 2020
79d8718
Fix leftover merge conflict.
tomsanbear Jan 15, 2020
e476ea7
Add tests to cover panic cases for the type conversions, and also add…
tomsanbear Jan 15, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions metricproviders/metricproviders.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/argoproj/argo-rollouts/metricproviders/kayenta"
"github.com/argoproj/argo-rollouts/metricproviders/webmetric"

log "github.com/sirupsen/logrus"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -50,6 +51,13 @@ func (f *ProviderFactory) NewProvider(logCtx log.Entry, metric v1alpha1.Metric)
} else if metric.Provider.Kayenta != nil {
c := kayenta.NewHttpClient()
return kayenta.NewKayentaProvider(logCtx, c), nil
} else if metric.Provider.WebMetric != nil {
c := webmetric.NewWebMetricHttpClient(metric)
p, err := webmetric.NewWebMetricJsonParser(metric)
if err != nil {
return nil, err
}
return webmetric.NewWebMetricProvider(logCtx, c, p), nil
}
return nil, fmt.Errorf("no valid provider in metric '%s'", metric.Name)
}
203 changes: 203 additions & 0 deletions metricproviders/webmetric/webmetric.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package webmetric

import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"net/url"
"strconv"
"time"

"k8s.io/client-go/util/jsonpath"

"github.com/argoproj/argo-rollouts/utils/evaluate"
metricutil "github.com/argoproj/argo-rollouts/utils/metric"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
log "github.com/sirupsen/logrus"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style Nix: Golang convention for imports is to alphabetize the imports and group them by

import (
standard

external

internal
)

So, you can remove line 14 and remove line 20 to above 18 and run make lint to alphabetize them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem with this, will run the linter this time around.


const (
//ProviderType indicates the provider is prometheus
ProviderType = "WebMetric"
)

// Provider contains all the required components to run a WebMetric query
// Implements the Provider Interface
type Provider struct {
logCtx log.Entry
client *http.Client
jsonParser *jsonpath.JSONPath
}

// Type incidates provider is a WebMetric provider
func (p *Provider) Type() string {
return ProviderType
}

func (p *Provider) Run(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric) v1alpha1.Measurement {
var (
err error
)

startTime := metav1.Now()

// Measurement to pass back
measurement := v1alpha1.Measurement{
StartedAt: &startTime,
}

// Create request
request := &http.Request{
Method: "GET",
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
}

request.URL, err = url.Parse(metric.Provider.WebMetric.Url)
if err != nil {
return metricutil.MarkMeasurementError(measurement, err)
}

for _, header := range metric.Provider.WebMetric.Headers {
request.Header.Set(header.Key, header.Value)
}

// Send Request
response, err := p.client.Do(request)
if err != nil || response.StatusCode != http.StatusOK {
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
return metricutil.MarkMeasurementError(measurement, err)
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
}

value, status, err := p.parseResponse(metric, response)
if err != nil || response.StatusCode != http.StatusOK {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why we check the status code here? I think we can remove that check from the if statement because we check that a couple of lines before that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for that now, will remove and just check for error with the response parsing.

return metricutil.MarkMeasurementError(measurement, err)
}

measurement.Value = value
measurement.Phase = status
finishedTime := metav1.Now()
measurement.FinishedAt = &finishedTime

return measurement
}

func (p *Provider) parseResponse(metric v1alpha1.Metric, response *http.Response) (string, v1alpha1.AnalysisPhase, error) {
var data interface{}

bodyBytes, err := ioutil.ReadAll(response.Body)
if err != nil {
return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Received no bytes in response: %v", err)
}

err = json.Unmarshal(bodyBytes, &data)
if err != nil {
return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Could not parse JSON body: %v", err)
}

buf := new(bytes.Buffer)
err = p.jsonParser.Execute(buf, data)
if err != nil {
return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Could not find JSONPath in body: %s", err)
}
out := buf.String()
outAsFloat, err := strconv.ParseFloat(buf.String(), 64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that only accepting float64 as a result does not provide enough flexibility for the web metrics. For example, if the endpoint pinged returned a struct like { "status": "success" }, the web provider couldn't check if the status field is equal to success. To achieve both goals, the jsonParser should parse the response and pass that parsed object into the evaluate condition method. That should achieve both outcomes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had this in mind as well... I'll dig into the evaluation code a bit more to get a more elegant solution here.

if err != nil {
return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Could not convert response to a number: %s", err)
}
status := p.evaluateResponse(metric, outAsFloat)
return out, status, nil
}

func (p *Provider) evaluateResponse(metric v1alpha1.Metric, result float64) v1alpha1.AnalysisPhase {
successCondition := false
failCondition := false
var err error

if metric.SuccessCondition != "" {
successCondition, err = evaluate.EvalCondition(result, metric.SuccessCondition)
if err != nil {
p.logCtx.Warning(err.Error())
return v1alpha1.AnalysisPhaseError
}
}
if metric.FailureCondition != "" {
failCondition, err = evaluate.EvalCondition(result, metric.FailureCondition)
if err != nil {
return v1alpha1.AnalysisPhaseError
}
}

switch {
case metric.SuccessCondition == "" && metric.FailureCondition == "":
//Always return success unless there is an error
return v1alpha1.AnalysisPhaseSuccessful
case metric.SuccessCondition != "" && metric.FailureCondition == "":
// Without a failure condition, a measurement is considered a failure if the measurement's success condition is not true
failCondition = !successCondition
case metric.SuccessCondition == "" && metric.FailureCondition != "":
// Without a success condition, a measurement is considered a successful if the measurement's failure condition is not true
successCondition = !failCondition
}

if failCondition {
return v1alpha1.AnalysisPhaseFailed
}

if !failCondition && !successCondition {
return v1alpha1.AnalysisPhaseInconclusive
}

// If we reach this code path, failCondition is false and successCondition is true
return v1alpha1.AnalysisPhaseSuccessful
}

// Resume should not be used the WebMetric provider since all the work should occur in the Run method
func (p *Provider) Resume(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric, measurement v1alpha1.Measurement) v1alpha1.Measurement {
p.logCtx.Warn("WebMetric provider should not execute the Resume method")
return measurement
}

// Terminate should not be used the WebMetric provider since all the work should occur in the Run method
func (p *Provider) Terminate(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric, measurement v1alpha1.Measurement) v1alpha1.Measurement {
p.logCtx.Warn("WebMetric provider should not execute the Terminate method")
return measurement
}

// GarbageCollect is a no-op for the WebMetric provider
func (p *Provider) GarbageCollect(run *v1alpha1.AnalysisRun, metric v1alpha1.Metric, limit int) error {
return nil
}

func NewWebMetricHttpClient(metric v1alpha1.Metric) *http.Client {
var (
timeout time.Duration
)

if metric.Provider.WebMetric.Timeout <= 0 {
timeout = time.Duration(10) * time.Second
} else {
timeout = time.Duration(metric.Provider.WebMetric.Timeout) * time.Second
}
c := &http.Client{
Timeout: timeout,
}
return c
}

func NewWebMetricJsonParser(metric v1alpha1.Metric) (*jsonpath.JSONPath, error) {
jsonParser := jsonpath.New("metrics")

err := jsonParser.Parse(metric.Provider.WebMetric.JsonPath)

return jsonParser, err
}

func NewWebMetricProvider(logCtx log.Entry, client *http.Client, jsonParser *jsonpath.JSONPath) *Provider {
return &Provider{
logCtx: logCtx,
client: client,
jsonParser: jsonParser,
}
}
61 changes: 61 additions & 0 deletions metricproviders/webmetric/webmetric_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package webmetric

import (
"io"
"net/http"
"net/http/httptest"
"testing"

"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
)

func TestRunSuccess(t *testing.T) {
input := `
{
"key": [
{
"key2": {
"value": 1
}
}
]
}`

server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
rw.Header().Set("Content-Type", "application/json")
io.WriteString(rw, input)
}))
// Close the server when test finishes
defer server.Close()

e := log.Entry{}

metric := v1alpha1.Metric{
Name: "foo",
SuccessCondition: "result > 0",
FailureCondition: "result <= 0",
Provider: v1alpha1.MetricProvider{
WebMetric: &v1alpha1.WebMetricMetric{
Url: server.URL,
JsonPath: "{$.key[0].key2.value}",
},
},
}

jp, err := NewWebMetricJsonParser(metric)
assert.NoError(t, err)

p := NewWebMetricProvider(e, server.Client(), jp)

measurement := p.Run(newAnalysisRun(), metric)
assert.NotNil(t, measurement.StartedAt)
assert.Equal(t, "1", measurement.Value)
assert.NotNil(t, measurement.FinishedAt)
assert.Equal(t, v1alpha1.AnalysisPhaseSuccessful, measurement.Phase)
}

func newAnalysisRun() *v1alpha1.AnalysisRun {
return &v1alpha1.AnalysisRun{}
}
1 change: 1 addition & 0 deletions pkg/apis/api-rules/violation_exceptions.list
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
API rule violation: names_match,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,MetricProvider,WebMetric
API rule violation: names_match,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutStatus,HPAReplicas
14 changes: 14 additions & 0 deletions pkg/apis/rollouts/v1alpha1/analysis_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ type MetricProvider struct {
// Prometheus specifies the prometheus metric to query
Prometheus *PrometheusMetric `json:"prometheus,omitempty"`
Kayenta *KayentaMetric `json:"kayenta,omitempty"`
WebMetric *WebMetricMetric `json:"webmetric,omitempty"`
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
// Job specifies the job metric run
Job *JobMetric `json:"job,omitempty"`
}
Expand Down Expand Up @@ -267,3 +268,16 @@ type ScopeDetail struct {
Start string `json:"start"`
End string `json:"end"`
}

// I don't like this repetition, but it fits the pattern...
type WebMetricMetric struct {
Url string `json:"url"`
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
Headers []WebMetricHeader `json:"headers,omitempty"`
Timeout int `json:"timeout,omitempty"`
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
JsonPath string `json:"jsonPath"`
dthomson25 marked this conversation as resolved.
Show resolved Hide resolved
}

type WebMetricHeader struct {
Key string `json:"key"`
Value string `json:"value"`
}
Loading