-
Notifications
You must be signed in to change notification settings - Fork 8.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PromQL: Allow operators between range vector selectors and scalars #14095
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
Signed-off-by: darshanime <deathbullet@gmail.com>
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:
|
Signed-off-by: darshanime <deathbullet@gmail.com>
tfr!
I initially started out modelling this as a
Note, the
We have similar restrictions of accepting only number literals in a bunch of other places too; timestamp of
Added!
Agree, does @Nexucis know someone who can help with this? |
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 Ok, I can see that some functions like 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
As far as I understand the code, it's not an AST node by itself - it's just a field of the Yeah, so far I still think a normal 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:
Implemented as a normal binary operator, it would look like this:
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
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 For the
Nice 👍 |
It would be cool to get @codesome 's input on this. |
Allow operators between range vector selectors and scalars. Specifically,
Arithmetic binary operators
Comparison binary operators
Examples:
avg_over_time(probe_duration_seconds[10m] > 0 < 10)
rate(my_metric[5m] != 0)
closes #10399
(This PR does not update the UI)