feat: Prometheus Metrics Provider Should Evaluate Prometheus Query Before Sending to Prometheus

[liesenfeldn]

Summary

Currently the prometheus metrics provider simply hands of the Query to prometheus, which will fail if for example you are using expression language to handle multiple cases of queries. It would be great if the prometheus provider could first evaluate the Query field, parse out which part of it is actually the query and then only pass that along to prometheus.

Use Cases

We currently have the use case that a metric needs to be swapped out for our template users. The problem however is 2-fold. On the one hand there will be a timeframe where our template users will have either the old metric or the new one, so they will be split for some time, which yes we could solve by just creating a new template, however that means pushing multiple PRs to our users repositories to migrate them, which can cause disruption. On the other hand we also would be able to reduce the amount of template that we currently have by allowing for this behavior.

Example

In the current state of argo rollouts, a query such as the following fails, because prometheus doesn't know what to do with "some_arg" == "not_some_arg" (see example below)

apiVersion: argoproj.io/v1alpha1
kind: ClusterAnalysisTemplate
metadata:
  name: some-template
spec:
  metrics:
  - name: some-metric
    provider:
      prometheus:
        address: prometheus-provider-url
        query: '"some_arg" == "not_some_arg" ? "query1" : ( "some_arg" == "some_arg" ? 'sum(rate(some_metric{filter1="filter1_value",filter2="filter2_value",filter3=~"filter3_value",filter4=~"filter4_value",filter5!~"filter5_value"}[5m])) by(some_value))' : "query2")'
...

However if the prometheus metrics provider were to evaluate Query first, then in this case it would pass this query to prometheus, which would be a valid prometheus query:

`sum(rate(some_metric{filter1="filter1_value",filter2="filter2_value",filter3=~"filter3_value",filter4=~"filter4_value",filter5!~"filter5_value"}[5m])) by(some_value))`

Expected Changes

• Make Query evaluate the expression first, then run the query (by adding a EvalQuery function)
• Adapt the ResolvedPrometheusQuery field to only show the actually run query
• Add tests

Alternative Designs of this feature

1. Adapt the prometheus metrics provider to handle expression language in Query
2. Add a new field to the struct, that is a boolean that tells the prometheus metrics provider to evaluate the expression first, then run the query
3. Add a new field to the struct that is named e.g. QueryExpression where users can separately set a query that needs to be evaluated first. This could cause a bit of confusion, and it would have to be made clear and programmatically enforced that only Query OR QueryExpression can be used.

I actually already have a PR pretty much ready for these changes (using the first design proposal, adding it to Query). I have written some test and also confirmed the behavior with a kind cluster to make sure our use case would be covered and i have tested some of our current templates against my change to make sure it is backwards compatible.

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

[liesenfeldn]

I've added a PR for this #4004

@zachaller what do you think of this issue and the PR?