Mimirtool: `rules lint` shouldn't merge expression into one line

[d-mankowski-synerise]

Describe the bug

Right now running mimirtool rules lint causes all multi-line expression to be "compressed" into a single line, which, sometimes, drastically reduces readability. We are using it in our CI and this behavior is undesirable.

To Reproduce

Steps to reproduce the behavior:

1. Create a file that contains an example code from Prometheues best practices page:

namespace: test-recordings
groups:
    - name: test.rules
      rules:
        - record: instance_path:request_failures_per_requests:ratio_rate5m
          expr: |2
              instance_path:request_failures:rate5m{job="myjob"}
            /
              instance_path:requests:rate5m{job="myjob"}

and run mimirtool rules lint <file>.

The output is:

namespace: test-recordings
groups:
    - name: test.rules
      rules:
        - record: instance_path:request_failures_per_requests:ratio_rate5m
          expr: |-
            instance_path:request_failures:rate5m{job="myjob"} / instance_path:requests:rate5m{job="myjob"}

Expected behavior

Mimirtool shouldn't change anything.

Environment

• Infrastructure: laptop
• Deployment tool: mimirtool 2.7.0

[edwintye]

We are running into the same issue and this issue makes it quite annoying when using mimirtool in CI. I actually understand the logic of why the file is updated, since it will tidy up an accidental poorly formatted expression, with the downside being that it also changes those that was done deliberately.

This line rules.go#149 performs the overwrite, and can probably be changed to something like

if editExpression {
	r.Groups[i].Rules[j].Expr.Value = exp.String()
}

and the flag can be propagated via an additional cli arg.

However, more interestingly, mimirtool rules prepare does effectively the same job (that adds the namespace and edits the expression) as mimirtool rules lint. The only difference being that prepare adds an aggregation label. An arg --[no]-in-place already exists for prepare which signifies whether the output yaml should replace the existing file. Since both prepare and lint uses the same save function line 698 and line 731 respectively, it is probably more consistent from the user perspective to simply add

	lintCmd.Flag(
		"in-place",
		"edits the rule file in place",
	).Short('i').BoolVar(&r.InPlaceEdit)

and change the save in lint

- if err := save(namespaces, true); err != nil { return err }
+ if err := save(namespaces, r.InPlaceEdit); err != nil { return err }

which allows it to also write the changes to another file.

[dimitarvdimitrov]

Perhaps @d-mankowski-synerise can correct me but the issue seems to come from the fact that mimirtool formats the promQL. Instead sometimes you might want to keep the promQL untouched.

One option is to optionally disable promQL formatting for mimirtool rules lint. But I would first try to introduce better promQL formatting. For example the expression

sum by (x) (
  instance_path:request_failures:rate5m{job="myjob"}
  /
  instance_path:requests:rate5m{job="myjob"}
)

is formatted by Prometheus (using the query formatter in the Prometheus UI) to this

sum by (x) (
  instance_path:request_failures:rate5m{job="myjob"} / instance_path:requests:rate5m{job="myjob"}
)

whereas mimirtool formats it as

sum by (x) (instance_path:request_failures:rate5m{job="myjob"} / instance_path:requests:rate5m{job="myjob"})

What do you think about formatting the expressions as prometheus would instead of formatting them in a single like (which is also in the mimirtool rules lint documentation)?

[narqo]

[..] But I would first try to introduce better promQL formatting

@dimitarvdimitrov since prometheus/prometheus#10544 Prometheus exposes API to format a parsed expression (i.e. parser.Prettify(Node)). From a brief testing it seems it works as you suggested:

     - name: test.rules
       rules:
         - record: instance_path:request_failures_per_requests:ratio_rate5m
-          expr: |2
-              instance_path:request_failures:rate5m{job="myjob"}
-            /
-              instance_path:requests:rate5m{job="myjob"}
-
+          expr: |-
+            instance_path:request_failures:rate5m{job="myjob"} / instance_path:requests:rate5m{job="myjob"}
     - name: test.rules.2
       rules:
         - record: instance_path:request_failures_per_requests:ratio_rate5m
-          expr: |2
+          expr: |-
             sum by (x) (
-              instance_path:request_failures:rate5m{job="myjob"}
-              /
-              instance_path:requests:rate5m{job="myjob"}
+              instance_path:request_failures:rate5m{job="myjob"} / instance_path:requests:rate5m{job="myjob"}
             )

It's simple to update the mimirtool rules [lint, prepare] to output the formatted rules, if that what the users would expect.