feat(metricprovider): add prometheus range query support #3704

mclarke47 · 2024-07-04T20:02:50Z

fixes: #3702

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

github-actions · 2024-07-04T20:12:24Z

Go Published Test Results

2 171 tests 2 171 ✅ 2m 54s ⏱️
119 suites 0 💤
1 files 0 ❌

Results for commit 98df9ed.

♻️ This comment has been updated with latest results.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

codecov · 2024-07-04T20:29:39Z

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.30%. Comparing base (4f1edbe) to head (98df9ed).
Report is 89 commits behind head on master.

Files with missing lines	Patch %	Lines
utils/evaluate/evaluate.go	80.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3704      +/-   ##
==========================================
+ Coverage   80.27%   80.30%   +0.03%     
==========================================
  Files         156      156              
  Lines       17964    18018      +54     
==========================================
+ Hits        14420    14470      +50     
- Misses       2631     2634       +3     
- Partials      913      914       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

github-actions · 2024-07-04T21:26:01Z

E2E Tests Published Test Results

4 files 4 suites 3h 27m 32s ⏱️
111 tests 100 ✅ 6 💤 5 ❌
452 runs 420 ✅ 24 💤 8 ❌

For more details on these failures, see this check.

Results for commit 98df9ed.

♻️ This comment has been updated with latest results.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

mclarke47 · 2024-07-05T18:11:58Z

It would be nice if we could parameterize lookBackDuration through args also, but not 100% sure how to do this. I could also do that in a follow up PR.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

zachaller · 2024-07-08T13:53:51Z

What's the difference between this and say doing a query like http_requests_total{job="prometheus"}[5m]?

Is the golang QueryRange function just some syntactic sugar for a raw query?

https://www.robustperception.io/step-and-query_range/

mclarke47 · 2024-07-08T15:14:57Z

Query ranges are syntactic sugar for the query endpoint as it says in that blog post. However I think there are 2 main benefits to adding query ranges:

They allow writing queries similar to how they are viewed in graphs and so IMO makes it make the Prometheus Argo Rollouts provider more intuitive to use, since you can re-use queries from other sources without needing to first convert them to queries that would work with the query endpoint
Query ranges support the step argument which allows you to specify the windows in which the queries over the range are executed so you get a little more control over how the data it returned from prometheus

zachaller · 2024-07-08T15:21:35Z

That's fair, I do see it being a bit easier to reason about, also do you think it make sense to expose the start and end parameters vs hiding that behind lookBackDuration?

mclarke47 · 2024-07-08T15:33:43Z

I think that's would be good, although it should probably be relative to time.Now() otherwise we would be hard coding a datetime which seems not very useful.

@zachaller is there a way to access args of the analysis template inside the provider so that we can parameterize things?

zachaller · 2024-07-08T15:37:47Z

I think that's would be good, although it should probably be relative to time.Now() otherwise we would be hard coding a datetime which seems not very useful.

@zachaller is there a way to access args of the analysis template inside the provider so that we can parameterize things?

Pretty sure this works today because we do things like:

provider:
        prometheus:
          address: "http://prometheus:{{args.prometheus-port}}"
          query: |-
            (quantile(0.5, quantile_over_time(0.5, namespace_pod_cpu_utilization{namespace="{{args.namespace}}", pod=~".*-{{args.canary-hash}}-.*"}[11m])))

Might be worth checking if it still works with your code as well just incase the template system explicity looks for those fields etc.

I would also need to double check if we have access to say time.Now() or something in the templating system as well.

mclarke47 · 2024-07-08T17:26:13Z

Will double check if this already works

mclarke47 · 2024-07-08T17:37:37Z

Confirming:

...
  - name: lookback_duration
    value: "5m"
  - name: lookback_step
    value: "1m"
...    
    provider:
      prometheus:
        rangeQuery:
          lookBackDuration: "{{args.lookback_duration}}"
          step: "{{args.lookback_step}}"
...

does work

mclarke47 · 2024-07-08T17:52:08Z

There are time functions in expr, I'm going to change the API to be start/end/step, with start's default being now(). I'll test this out then update the docs.

mclarke47 · 2024-07-08T18:19:19Z

hmmm, it seems this expression logic is only evaluated in success/failure conditions not in the actual fields of the provider, only args substitution works there.

I could:

Stick with a lookback duration as is currently implemented
Put in some logic to Evaluate these fields with util/evaluate

WDYT @zachaller?

(the image doesn't seem to be embedding it's at https://github.com/argoproj/argo-rollouts/assets/6514980/29710477-b02a-4a11-a71e-cbd4690ffea1)

zachaller · 2024-07-08T19:03:05Z

Yea, it looks like Analysis only use fasttemplate not expr as seen here

It might actually be fairly easy to add expr support directly to parsing analysis. I could see it being pretty useful in other cases for other providers too. Let me know your thoughts if adding expr support to analysis makes sense I would probably just start with time feature support.

Also, I think your example is missing query section.

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

mclarke47 · 2024-07-08T19:09:41Z

Hey @zachaller, I pushed that change to treat the start/end fields as expr expressions. 🙏

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

zachaller · 2024-07-08T20:21:14Z

utils/evaluate/evaluate.go

+		"asInt":   asInt,
+		"asFloat": asFloat,


Let's remove asInt, asFloat and just use the built in funcs https://expr-lang.org/docs/language-definition#int

zachaller · 2024-07-08T20:55:07Z

utils/evaluate/evaluate.go

+	case time.Time:
+		return val, nil
+	default:
+		return time.Time{}, fmt.Errorf("expected string, but got %T", val)


expected time.Time

zachaller · 2024-07-08T21:01:08Z

metricproviders/prometheus/prometheus.go

@@ -112,29 +136,51 @@ func (p *Provider) GarbageCollect(run *v1alpha1.AnalysisRun, metric v1alpha1.Met
 	return nil
 }

+func sampleValuesToFloatSlice(SampleValues []model.SampleValue) []float64 {


can we lower case SampleValues

zachaller · 2024-07-08T21:01:19Z

metricproviders/prometheus/prometheus.go

+	return results
+}
+
+func sampleValuesToResultStr(SampleValues []model.SampleValue) string {


Can we lowercase SampleValues

zachaller · 2024-07-08T21:07:19Z

Just a few small changes then this LGTM

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

mclarke47 · 2024-07-09T20:41:11Z

@zachaller have addressed your comments 🙏

zachaller · 2024-07-10T16:55:19Z

Thank you for the contribution!

feat: add prometheus range query support

957d8ac

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

fix: code-gen

80c2219

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

fix: handle matrix results from prometheus

ffcd819

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

refactor: remove duplicated code

9db3ca9

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

mclarke47 changed the title ~~WIP feat(metricprovider): add prometheus range query support~~ feat(metricprovider): add prometheus range query support WIP Jul 4, 2024

mclarke47 added 4 commits July 4, 2024 17:49

feat: configurable step

22be7ef

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

fix: codegen

189d729

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

test: more tests

887ffb3

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

Merge branch 'master' into add-prom-range-query-support

2e1744c

mclarke47 marked this pull request as ready for review July 5, 2024 18:00

mclarke47 changed the title ~~feat(metricprovider): add prometheus range query support WIP~~ feat(metricprovider): add prometheus range query support Jul 5, 2024

docs: range query docs

ecfa8e0

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

zachaller added this to the v1.8 milestone Jul 8, 2024

IvanManz approved these changes Jul 8, 2024

View reviewed changes

refactor: expr for prometheus range query start/end

422c89b

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

mclarke47 added 2 commits July 8, 2024 15:13

fix: lint issue

8bc2d8e

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

docs: missing codegen

d3804cc

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

zachaller reviewed Jul 8, 2024

View reviewed changes

fix: PR Fixes

98df9ed

Signed-off-by: Matthew Clarke <mclarke@spotify.com>

zachaller approved these changes Jul 10, 2024

View reviewed changes

zachaller merged commit 3e4ea74 into argoproj:master Jul 10, 2024
23 checks passed

mclarke47 deleted the add-prom-range-query-support branch July 10, 2024 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(metricprovider): add prometheus range query support #3704

feat(metricprovider): add prometheus range query support #3704

mclarke47 commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

mclarke47 commented Jul 5, 2024 •

edited

Loading

zachaller commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 8, 2024 •

edited

Loading

zachaller commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 8, 2024

zachaller commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024 •

edited

Loading

zachaller commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

zachaller Jul 8, 2024

mclarke47 Jul 9, 2024

zachaller Jul 8, 2024

mclarke47 Jul 9, 2024

zachaller Jul 8, 2024

mclarke47 Jul 9, 2024

zachaller Jul 8, 2024

mclarke47 Jul 9, 2024

zachaller commented Jul 8, 2024

mclarke47 commented Jul 9, 2024 •

edited

Loading

zachaller commented Jul 10, 2024

feat(metricprovider): add prometheus range query support #3704

feat(metricprovider): add prometheus range query support #3704

Conversation

mclarke47 commented Jul 4, 2024 • edited Loading

github-actions bot commented Jul 4, 2024 • edited Loading

Go Published Test Results

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

github-actions bot commented Jul 4, 2024 • edited Loading

E2E Tests Published Test Results

mclarke47 commented Jul 5, 2024 • edited Loading

zachaller commented Jul 8, 2024 • edited Loading

mclarke47 commented Jul 8, 2024 • edited Loading

zachaller commented Jul 8, 2024 • edited Loading

mclarke47 commented Jul 8, 2024

zachaller commented Jul 8, 2024 • edited Loading

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

mclarke47 commented Jul 8, 2024 • edited Loading

zachaller commented Jul 8, 2024

mclarke47 commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachaller commented Jul 8, 2024

mclarke47 commented Jul 9, 2024 • edited Loading

zachaller commented Jul 10, 2024

mclarke47 commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

codecov bot commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024 •

edited

Loading

mclarke47 commented Jul 5, 2024 •

edited

Loading

zachaller commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 8, 2024 •

edited

Loading

zachaller commented Jul 8, 2024 •

edited

Loading

zachaller commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 8, 2024 •

edited

Loading

mclarke47 commented Jul 9, 2024 •

edited

Loading