PromQL: Allow operators between range vector selectors and scalars

[darshanime]

Allow operators between range vector selectors and scalars. Specifically,

• Arithmetic binary operators

  • + (addition)
  • - (subtraction)
  • * (multiplication)
  • / (division)
  • % (modulo)
  • ^ (power/exponentiation)

• Comparison binary operators

  • == (equal)
  • != (not-equal)
  • > (greater-than)
  • < (less-than)
  • >= (greater-or-equal)
  • <= (less-or-equal)

Examples:

• avg_over_time(probe_duration_seconds[10m] > 0 < 10)
• rate(my_metric[5m] != 0)

closes #10399

(This PR does not update the UI)

[juliusv]

Thanks! I don't feel qualified to give a full review of PromQL engine code these days, but a few questions stand out to me at a glance:

• The operator is modeled as a field in the matrix selector, rather than just using the existing binary operator AST node. Unless I'm missing something important, I'd want to really see it modeled as a regular binary operator node, and then we extend the existing parameter type checking code to allow range-vector-to-scalar pairings (which currently throws a parse error). Maybe that makes the eval code more hairy, but adding this as a special-case extension of the matrix selector itself seems a bit odd. Also keep in mind that this AST structure is also visualized to users by tools like PromLens, so it makes a difference beyond internal code consistency whether operators are AST nodes or a property of an AST node.
• In a similar vein, I get that this is restricted to scalar 2nd operands for now (although I think matrix/vector ops would make sense in the future), but can we allow any scalar expression rather than just scalar literals? I feel like this possibility would follow more naturally if the operator was just a normal binop AST node.
• Should atan2 be supported as well? I don't know of any use case, but it would seem inconsistent to be the only binary (non-set) operator that's not supported.
• Should this work / do anything for native histogram samples? @beorn7 would be the best to give input on that...
• Before merging, it would be good to at least have a plan for UI updates: either included in this PR, or a commitment by someone to provide them before the next release.

[darshanime]

tfr!

The operator is modeled as a field in the matrix selector, rather than just using the existing binary operator AST node.

I initially started out modelling this as a BinaryExpr like you suggest, but hit the type checks for the function arguments in expressions like rate(my_selector[5m] > 0), where rate takes a ValueTypeMatrix and not BinaryExpr. I then came across @roidelapluie's comment suggesting this be modeled as part of the matrix selector. To me too, it makes sense semantically to have these operators (MatrixSelectorBinOps) be a part of the MatrixSelector node in the AST, just like we currently have VectorSelector, since they are to be applied to the matrix selector samples...

it makes a difference beyond internal code consistency whether operators are AST nodes or a property of an AST node.

Note, the MatrixSelectorBinOps is already a node in the AST (see this test). Does this AST hinder tools like PromLens? Do you have suggestions about what can make it better? Do you still believe BinaryExpr would be the best way to model this?

but can we allow any scalar expression rather than just scalar literals

We have similar restrictions of accepting only number literals in a bunch of other places too; timestamp of @/offset modifier, time range of range vectors etc. Can we pick this up once the duration<>number literal PR is merged? This is tracked in #12318.

Should atan2 be supported as well?

Added!

it would be good to at least have a plan for UI updates

Agree, does @Nexucis know someone who can help with this?

[juliusv]

I initially started out modelling this as a BinaryExpr like you suggest, but hit the type checks for the function arguments in expressions like rate(my_selector[5m] > 0), where rate takes a ValueTypeMatrix and not BinaryExpr. I then came across @roidelapluie's comment suggesting this be modeled as part of the matrix selector. To me too, it makes sense semantically to have these operators (MatrixSelectorBinOps) be a part of the MatrixSelector node in the AST, just like we currently have VectorSelector, since they are to be applied to the matrix selector samples...

Ah, thanks for the additional historical context! Could you elaborate on what the worries about function argument types are? If this was a regular binop and the ValueType() method for the BinaryExpr node was adjusted to correctly return a matrix type for matrix-to-scalar operands, wouldn't everything just keep working as is for the function argument types?

Ok, I can see that some functions like rate() peek into the argument's AST node type and assume that their argument is specifically a MatrixSelector AST node, not just any AST node that evaluates to a matrix type. But expressions like rate(rate(foo[5m])[1h:]) still work fine because subqueries handle this case by replacing the subquery AST node with a populated MatrixSelector node upon evaluation. This looks like the right approach to me here as well when using the BinaryExpr AST node for foo[5m] > 0. Maybe foo[5m] > 0 could even be looked at like a subquery for (foo > 0)[5m:], but with a shorter syntax and an optimized evaluation model (and well, slightly different output values due to keeping original sample timestamps etc.) 😆

The part about remote read hints for selectors is an interesting consideration I hadn't thought about. However, I'm not convinced that we should create more special-case operator-type constructs that are different (but in many ways the same) from our existing binary operators, just to make that part easier for a rare case. If a remote read hint for this is necessary in the future, we can still do so even when it's a normal BinaryExpr node - we already have other remote read hints which require analyzing parts of the AST outside of the immediate selector node itself (like whether there is a rate() function wrapping a selector), and that also works.

Note, the MatrixSelectorBinOps is already a node in the AST (see this test). Does this AST hinder tools like PromLens? Do you have suggestions about what can make it better? Do you still believe BinaryExpr would be the best way to model this?

As far as I understand the code, it's not an AST node by itself - it's just a field of the MatrixSelector AST node. An AST node is one that implements the Node interface and which can be independently evaluated and tracked as a child node, e.g. in places like the Children function that is used to walk the AST. An AST node is one which I can take independently and make it the root of the query itself, and it'll evaluate as a full PromQL expression.

Yeah, so far I still think a normal BinaryExpr node would be the way to implement this. I just don't want us to introduce a lot of special-case constructs into the AST that look and act almost the same as established generalized ones that we already have and which could be reused (and which have more powerful functionality already implemented for them which this would automatically inherit without extra effort, like non-literal scalar arguments).

PromLens is more of a side consideration for me, but things would show up differently there depending on the implementation: with the current code of this PR, there would be only a single selectable AST node displayed for the matrix selector, and it would include the binary operator:

__________
| rate() |
‾‾|‾‾‾‾‾‾‾
  |  _______________
  +--| foo[5m] > 0 |
     ‾‾‾‾‾‾‾‾‾‾‾‾‾‾‾

Implemented as a normal binary operator, it would look like this:

__________
| rate() |
‾‾|‾‾‾‾‾‾‾
  |       ___________
  |    +--| foo[5m] |
  |    |  ‾‾‾‾‾‾‾‾‾‾‾
  |  __|__
  +--| > |
     ‾‾|‾‾
       |  _____
       +--| 0 |
          ‾‾‾‾‾

In this second case, I can now click on each of these three nodes individually to see either the unfiltered range vector, the filtered result, or the filter operand value. And in this second example, the 0 could also be not just a scalar literal, but any other arbitrarily nested PromQL expression that the engine can evaluate, as long as it evaluates to a scalar value type.

We have similar restrictions of accepting only number literals in a bunch of other places too; timestamp of @/offset modifier, time range of range vectors etc. Can we pick this up once the duration<>number literal PR is merged? This is tracked in #12318.

I don't feel too strongly on whether an initial solution has to support non-literal scalar values, but I would assume that this would just automatically happen as a result of implementing the operator as a normal BinaryExpr AST node.

For the @ modifier it's true what you're saying, but that modifier is so different from everything else that implementing it a bit more specially made more sense to me.

Should atan2 be supported as well?

Added!

Nice 👍