feat: Web metric provider #318
Conversation
|
|
There was a problem hiding this comment.
Hi @tomsanbear, this is awesome, and thank you for contributing! I like the overall approach, but I have some implementation questions and a couple of minor style comments.
| 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" | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No problem with this, will run the linter this time around.
| return "", v1alpha1.AnalysisPhaseError, fmt.Errorf("Could not find JSONPath in body: %s", err) | ||
| } | ||
| out := buf.String() | ||
| outAsFloat, err := strconv.ParseFloat(buf.String(), 64) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the comments @dthomson25 Cheers! |
…ted, check for 2xx status code, minor naming changes for Go best practices
…ized/extended test suite.
|
Alright, found some time today to fit this in. Got a smarter parser in there for interpreting between ints, floats, bools and strings. Haven't bothered yet with adding in the alternate HTTP methods, I'll wait to hear back on the discussion. |
| Method: "GET", // TODO maybe make this configurable....also implies we will need body templates | ||
| } | ||
|
|
||
| request.URL, err = url.Parse(metric.Provider.Web.URL) |
There was a problem hiding this comment.
Style Nix: We can get rid of the err variable declaration at line 41 if we use the variable short assignment := here
There was a problem hiding this comment.
This one I actually did since we can't have a ':=' assignment combined with the field assignment I have there. I could swap that out with a new var that then gets assigned to request.URL, I'll push that in the next commit, and we can nitpick that if it's a problem also.
| } | ||
|
|
||
| value, status, err := p.parseResponse(metric, response) | ||
| if err != nil || response.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
No need for that now, will remove and just check for error with the response parsing.
| func parsePrimitiveFromString(in string) interface{} { | ||
| // Chain ordering as follows: | ||
| // int -> float -> bool -> string | ||
|
|
||
| // 64 bit Int conversion | ||
| inAsInt, err := strconv.ParseInt(in, 10, 64) | ||
| if err == nil { | ||
| return inAsInt | ||
| } | ||
|
|
||
| // Float conversion | ||
| inAsFloat, err := strconv.ParseFloat(in, 64) | ||
| if err == nil { | ||
| return inAsFloat | ||
| } | ||
|
|
||
| // Bool conversion | ||
| inAsBool, err := strconv.ParseBool(in) | ||
| if err == nil { | ||
| return inAsBool | ||
| } | ||
|
|
||
| // Else | ||
| return in |
There was a problem hiding this comment.
I'm concerned this approach maybe a little too implicit. For example, what if the user expects to evaluate a string, but the function converts the response into an int? In that case, they would have no options. I think we can make it a little more explicit by adding some functions to the condition language similar to https://github.com/antonmedv/expr/blob/master/docs/examples/demo.go#L23.
We would need to add these functions here: https://github.com/argoproj/argo-rollouts/blob/master/utils/evaluate/evaluate.go#L9-L13. This would allow us to have int, float, bool, and string functions that users can use to convert their results as they please.
There was a problem hiding this comment.
Ok, I'll take a look into this, thanks for the feedback on this portion!
|
Also, can you rebase on master? We made a CI change in another commit that this PR can't use until rebasing with master. The change will make sure the Circle has enough resources to run the tests, and there shouldn't be any conflicts |
* Fix Infinite loop with PreviewReplicaCount set * Use xlarge instead of large
…sg fault in syncReplicasOnly (argoproj#314)
* Fix panic if rollout cannot create a new RS * Enable controller to handle panics with crashing
Codecov Report
@@ Coverage Diff @@
## master #318 +/- ##
==========================================
+ Coverage 83.59% 83.76% +0.16%
==========================================
Files 67 70 +3
Lines 6146 6456 +310
==========================================
+ Hits 5138 5408 +270
- Misses 727 751 +24
- Partials 281 297 +16
Continue to review full report at Codecov.
|
| Web: &v1alpha1.WebMetric{ | ||
| // URL: server.URL, | ||
| JSONPath: "{$.key[0].key2.value}", | ||
| Headers: []v1alpha1.WebMetricHeader{v1alpha1.WebMetricHeader{Key: "key", Value: "value"}}, |
There was a problem hiding this comment.
File is not gofmt-ed with -s (from gofmt)
| Headers: []v1alpha1.WebMetricHeader{v1alpha1.WebMetricHeader{Key: "key", Value: "value"}}, | |
| Headers: []v1alpha1.WebMetricHeader{{Key: "key", Value: "value"}}, |
|
I made the easy changes and added some additional testing. But will have to revisit the evaluation logic as discussed above when I have some time this week. 👍 |
|
Hey @tomsanbear, I wanted to check in and see if you had a chance to get those last changes in. I totally understand if not with the holidays and everything the past couple of weeks. Please let me know if there's anything I can do to help! |
Hey @dthomson25 it's as you said holidays have been particularly busy for me. I think this weekend at the latest I should be able to get this last bit finished up. I took a look at the code for the evaluation mechanism during some downtime and it seems fairly straight forward to add for this PR. Thanks for reaching out! |
|
@dthomson25 So I did a quick code of what we discussed... not sure if it's along the same idea as what you were thinking. Just a few of my thoughts:
If you had a different approach in mind I'll be interested to hear it. |
|
We were trying this out, and if metric.Provider.Web != nil {
numProviders++
}But once we got this change, things seemed to be working. |
utils/evaluate/evaluate.go
Outdated
| "asInt": func(in string) int64 { | ||
| inAsInt, err := strconv.ParseInt(in, 10, 64) | ||
| if err == nil { | ||
| return inAsInt | ||
| } | ||
| panic(err) | ||
| }, | ||
| "asFloat": func(in string) float64 { | ||
| inAsFloat, err := strconv.ParseFloat(in, 64) | ||
| if err == nil { | ||
| return inAsFloat | ||
| } | ||
| panic(err) | ||
| }, |
There was a problem hiding this comment.
This is definitely in line with what I was thinking! For the error handling, we should add a recover at line 28 to catch these panics. From there, we can return an error saying Unable to convert to Float and capture that in the status. Otherwise, the panic will break the rest of the analysis run. I think it will make the comparisons more explicit for the users and gives them flexibility if needed.
There was a problem hiding this comment.
This should also be a good candidate for some unit tests using the assert.Panic function
There was a problem hiding this comment.
Sounds good! I'll add that in later today 👍
There was a problem hiding this comment.
So, I added tests, and I did also add the recover clause. But it may be unecessary after playing around with the evaluation library. From the looks of it, it already wraps panics from internal functions as errors with nice pretty print debug messages.
got expected error: strconv.ParseFloat: parsing "NotANum": invalid syntax (1:9)
| asFloat(result) == 1.1
| ........^
… a recover clause to the eval logic. NOTE: this clause might not be necessary.
|
Nope! I am happy with this for a first implementation as this achieves what I was looking to have for pulling other metrics and stats. |
Today I ran into a situation where it'd be nice to link external metrics to Argo Rollouts, but unfortunately my platform is not supported. I notice the issue open for the generic WebMetric and created a basic implementation (Issue #177)
I would appreciate some feedback before I spend any more time on it. If there are glaring implementation issues, or schema changes you might suggest that would be great (I'm new-ish to Golang).
Some notes on my implementation:
Cheers!