Prometheus: Fix parsing logic of prometheus expressions to honor the order of binary operations (#112220)
* change parsing to allow binary expression as long as it is not between two number literals * add clamp function to monaco editor so it can be highlighted * remove warning being thrown for binary operations * modify parsing logic to respect the order of binary operations * fix logic for when to increment offset for binary operations * fix typo in comment * Update packages/grafana-prometheus/src/querybuilder/parsing.ts Fix typo in comment Co-authored-by: ismail simsek <ismailsimsek09@gmail.com> --------- Co-authored-by: ismail simsek <ismailsimsek09@gmail.com>
This commit is contained in:
Jocelyn Collado-Kuri
co-authored by
ismail simsek
ismail simsek
parent
3cbef45851
commit
8cb3d3b552
@@ -74,6 +74,7 @@ const functions = [
|
||||
'absent',
|
||||
'ceil',
|
||||
'changes',
|
||||
'clamp',
|
||||
'clamp_max',
|
||||
'clamp_min',
|
||||
'day_of_month',
|
||||
|
||||
@@ -486,11 +486,6 @@
|
||||
"message-no-metrics-found": "There are no metrics found in the data source.",
|
||||
"name": "Name",
|
||||
"type": "Type"
|
||||
},
|
||||
"update-function-args": {
|
||||
"text": {
|
||||
"query-parsing-is-ambiguous": "Query parsing is ambiguous."
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -200,49 +200,13 @@ describe('buildVisualQueryFromString', () => {
|
||||
});
|
||||
|
||||
describe('nested binary operation errors in visual query editor', () => {
|
||||
// Visual query builder does not currently have support for nested binary operations, for now we should throw an error in the UI letting users know that their query will be misinterpreted
|
||||
it('throws error when visual query parse is ambiguous', () => {
|
||||
it('does not throw error when visual query contains binary ops in function argument', () => {
|
||||
expect(
|
||||
buildVisualQueryFromString('topk(5, node_arp_entries / node_arp_entries{cluster="dev-eu-west-2"})')
|
||||
).toMatchObject({
|
||||
errors: [
|
||||
{
|
||||
from: 8,
|
||||
text: 'Query parsing is ambiguous.',
|
||||
to: 68,
|
||||
},
|
||||
],
|
||||
errors: [],
|
||||
});
|
||||
});
|
||||
|
||||
it('throws error when visual query parse with aggregation is ambiguous (scalar)', () => {
|
||||
expect(buildVisualQueryFromString('topk(5, 1 / 2)')).toMatchObject({
|
||||
errors: [
|
||||
{
|
||||
from: 8,
|
||||
text: 'Query parsing is ambiguous.',
|
||||
to: 13,
|
||||
},
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
it('throws error when visual query parse with functionCall is ambiguous', () => {
|
||||
expect(
|
||||
buildVisualQueryFromString(
|
||||
'clamp_min(sum by(cluster)(rate(X{le="2.5"}[5m]))+sum by (cluster) (rate(X{le="5"}[5m])), 0.001)'
|
||||
)
|
||||
).toMatchObject({
|
||||
errors: [
|
||||
{
|
||||
from: 10,
|
||||
text: 'Query parsing is ambiguous.',
|
||||
to: 87,
|
||||
},
|
||||
],
|
||||
});
|
||||
});
|
||||
|
||||
it('does not throw error when visual query parse is unambiguous', () => {
|
||||
expect(
|
||||
buildVisualQueryFromString('topk(5, node_arp_entries) / node_arp_entries{cluster="dev-eu-west-2"}')
|
||||
@@ -967,6 +931,29 @@ describe('buildVisualQueryFromString', () => {
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it('parses query with functions and binary operations', () => {
|
||||
expect(
|
||||
buildVisualQueryFromString(
|
||||
'clamp(sum(rate(loki_distributor_bytes_received_total{cluster="loki", tenant="kubernetes"}[5m])) / 1024 / 55, 5, 30)'
|
||||
)
|
||||
).toEqual(
|
||||
noErrors({
|
||||
metric: 'loki_distributor_bytes_received_total',
|
||||
labels: [
|
||||
{ label: 'cluster', op: '=', value: 'loki' },
|
||||
{ label: 'tenant', op: '=', value: 'kubernetes' },
|
||||
],
|
||||
operations: [
|
||||
{ id: 'rate', params: ['5m'] },
|
||||
{ id: 'sum', params: [] },
|
||||
{ id: '__divide_by', params: [1024] },
|
||||
{ id: '__divide_by', params: [55] },
|
||||
{ id: 'clamp', params: [5, 30] },
|
||||
],
|
||||
})
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
function noErrors(query: PromVisualQuery) {
|
||||
|
||||
@@ -34,6 +34,7 @@ import {
|
||||
getAllByType,
|
||||
getLeftMostChild,
|
||||
getString,
|
||||
isFunctionOrAggregation,
|
||||
makeBinOp,
|
||||
makeError,
|
||||
replaceBuiltInVariable,
|
||||
@@ -330,24 +331,6 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont
|
||||
let child = node.firstChild;
|
||||
|
||||
while (child) {
|
||||
let binaryExpressionWithinFunctionArgs: SyntaxNode | null;
|
||||
if (child.type.id === BinaryExpr) {
|
||||
binaryExpressionWithinFunctionArgs = child;
|
||||
} else {
|
||||
binaryExpressionWithinFunctionArgs = child.getChild(BinaryExpr);
|
||||
}
|
||||
|
||||
if (binaryExpressionWithinFunctionArgs) {
|
||||
context.errors.push({
|
||||
text: t(
|
||||
'grafana-prometheus.querybuilder.update-function-args.text.query-parsing-is-ambiguous',
|
||||
'Query parsing is ambiguous.'
|
||||
),
|
||||
from: binaryExpressionWithinFunctionArgs.from,
|
||||
to: binaryExpressionWithinFunctionArgs.to,
|
||||
});
|
||||
}
|
||||
|
||||
updateFunctionArgs(expr, child, context, op);
|
||||
child = child.nextSibling;
|
||||
}
|
||||
@@ -392,7 +375,7 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont
|
||||
* @param node
|
||||
* @param context
|
||||
*/
|
||||
function handleBinary(expr: string, node: SyntaxNode, context: Context) {
|
||||
function handleBinary(expr: string, node: SyntaxNode, context: Context, idx = 0) {
|
||||
const visQuery = context.query;
|
||||
const left = node.firstChild!;
|
||||
const op = getString(expr, left.nextSibling);
|
||||
@@ -407,6 +390,16 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) {
|
||||
|
||||
const rightBinary = right.type.id === BinaryExpr;
|
||||
|
||||
// binary operations that are part of a function argument do not get processed and added to the query until the end, this index helps keep track
|
||||
// of where to add the operation in the list rather than just appending it to the end. If the binary operation is just part of a nested binary exp,
|
||||
// we append at the end
|
||||
const parent = node.parent;
|
||||
const child = node.firstChild;
|
||||
const shouldOffsetTail =
|
||||
parent && !parent.type.isTop && (isFunctionOrAggregation(parent) || (child && isFunctionOrAggregation(child)));
|
||||
if (shouldOffsetTail) {
|
||||
idx += 1;
|
||||
}
|
||||
if (leftNumber) {
|
||||
// TODO: this should be already handled in case parent is binary expression as it has to be added to parent
|
||||
// if query starts with a number that isn't handled now.
|
||||
@@ -416,14 +409,18 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) {
|
||||
handleExpression(expr, left, context);
|
||||
}
|
||||
|
||||
// in the case we have an expression like func(...) / 2 or func(...) + 5, the binary expression will be at the top of the tree
|
||||
// in which case, the idx will be 0. In this case it means that the binary operation must be added to the end of the array, and
|
||||
|
||||
const newIdx = idx === 0 ? visQuery.operations.length : -idx;
|
||||
if (rightNumber) {
|
||||
visQuery.operations.push(makeBinOp(opDef, expr, right, !!binModifier?.isBool));
|
||||
visQuery.operations.splice(newIdx, 0, makeBinOp(opDef, expr, right, !!binModifier?.isBool));
|
||||
} else if (rightBinary) {
|
||||
// Due to the way binary ops are parsed we can get a binary operation on the right that starts with a number which
|
||||
// is a factor for a current binary operation. So we have to add it as an operation now.
|
||||
const leftMostChild = getLeftMostChild(right);
|
||||
if (leftMostChild?.type.id === NumberDurationLiteral) {
|
||||
visQuery.operations.push(makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool));
|
||||
visQuery.operations.splice(newIdx, 0, makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool));
|
||||
}
|
||||
|
||||
// If we added the first number literal as operation here we still can continue and handle the rest as the first
|
||||
|
||||
@@ -4,6 +4,7 @@ import { parser } from '@prometheus-io/lezer-promql';
|
||||
import {
|
||||
getLeftMostChild,
|
||||
getString,
|
||||
isFunctionOrAggregation,
|
||||
replaceBuiltInVariable,
|
||||
replaceVariables,
|
||||
returnBuiltInVariable,
|
||||
@@ -123,4 +124,17 @@ describe('builtInTimeVariables', () => {
|
||||
expect(actual2).toBe(testCase.expr);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isFunctionOrAggregation', () => {
|
||||
it('should identify function and aggregation nodes', () => {
|
||||
const tree = parser.parse('clamp(sum(foo[5m]))');
|
||||
const root = tree.topNode;
|
||||
const functionNode = root.firstChild!.lastChild; // clamp
|
||||
const aggregationNode = root.firstChild!.lastChild!.firstChild; // sum
|
||||
|
||||
expect(isFunctionOrAggregation(functionNode!)).toBe(true);
|
||||
expect(isFunctionOrAggregation(aggregationNode!)).toBe(true);
|
||||
expect(isFunctionOrAggregation(root)).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
// Core Grafana history https://github.com/grafana/grafana/blob/v11.0.0-preview/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts
|
||||
import { SyntaxNode, TreeCursor } from '@lezer/common';
|
||||
import { AggregateExpr, FunctionCallBody } from '@prometheus-io/lezer-promql';
|
||||
|
||||
import { QueryBuilderOperation, QueryBuilderOperationParamValue } from './shared/types';
|
||||
|
||||
@@ -194,3 +195,7 @@ export function replaceBuiltInVariable(expr: string): string {
|
||||
export function returnBuiltInVariable(expr: string): string {
|
||||
return expr.replace(builtInReplacementRegex, (match) => replacementToVariable[match]);
|
||||
}
|
||||
|
||||
export function isFunctionOrAggregation(node: SyntaxNode): boolean {
|
||||
return node.type.id === AggregateExpr || node.type.id === FunctionCallBody;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user