From 83fd6ac70ad3c7b6e5e30dc992bd9a613a95fe88 Mon Sep 17 00:00:00 2001 From: ismail simsek Date: Fri, 12 Apr 2024 13:46:17 +0200 Subject: [PATCH] apply same changes in core prometheus frontend --- .../monaco-completion-provider/situation.ts | 101 ++++------------- .../prometheus/querybuilder/parsing.test.ts | 8 +- .../prometheus/querybuilder/parsing.ts | 102 ++++++++---------- .../prometheus/querybuilder/parsingUtils.ts | 7 +- 4 files changed, 76 insertions(+), 142 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/components/monaco-query-field/monaco-completion-provider/situation.ts b/public/app/plugins/datasource/prometheus/components/monaco-query-field/monaco-completion-provider/situation.ts index f1e2d522f94a..47d2a70f9467 100644 --- a/public/app/plugins/datasource/prometheus/components/monaco-query-field/monaco-completion-provider/situation.ts +++ b/public/app/plugins/datasource/prometheus/components/monaco-query-field/monaco-completion-provider/situation.ts @@ -2,6 +2,7 @@ import type { SyntaxNode, Tree } from '@lezer/common'; import { AggregateExpr, AggregateModifier, + BinaryExpr, EqlRegex, EqlSingle, FunctionCallBody, @@ -9,11 +10,9 @@ import { Identifier, LabelMatcher, LabelMatchers, - LabelMatchList, LabelName, MatchOp, MatrixSelector, - MetricIdentifier, Neq, NeqRegex, parser, @@ -35,9 +34,7 @@ type NodeTypeId = | typeof Identifier | typeof LabelMatcher | typeof LabelMatchers - | typeof LabelMatchList | typeof LabelName - | typeof MetricIdentifier | typeof PromQL | typeof StringLiteral | typeof VectorSelector @@ -183,6 +180,10 @@ const RESOLVERS: Resolver[] = [ path: [StringLiteral, LabelMatcher], fun: resolveLabelMatcher, }, + { + path: [ERROR_NODE_NAME, BinaryExpr, PromQL], + fun: resolveTopLevel, + }, { path: [ERROR_NODE_NAME, LabelMatcher], fun: resolveLabelMatcher, @@ -251,30 +252,8 @@ function getLabels(labelMatchersNode: SyntaxNode, text: string): Label[] { return []; } - let listNode: SyntaxNode | null = walk(labelMatchersNode, [['firstChild', LabelMatchList]]); - - const labels: Label[] = []; - - while (listNode !== null) { - const matcherNode = walk(listNode, [['lastChild', LabelMatcher]]); - if (matcherNode === null) { - // unexpected, we stop - return []; - } - - const label = getLabel(matcherNode, text); - if (label !== null) { - labels.push(label); - } - - // there might be more labels - listNode = walk(listNode, [['firstChild', LabelMatchList]]); - } - - // our labels-list is last-first, so we reverse it - labels.reverse(); - - return labels; + const labelNodes = labelMatchersNode.getChildren(LabelMatcher); + return labelNodes.map((ln) => getLabel(ln, text)).filter(notEmpty); } function getNodeChildren(node: SyntaxNode): SyntaxNode[] { @@ -318,17 +297,12 @@ function resolveLabelsForGrouping(node: SyntaxNode, text: string, pos: number): return null; } - const metricIdNode = getNodeInSubtree(bodyNode, MetricIdentifier); + const metricIdNode = getNodeInSubtree(bodyNode, Identifier); if (metricIdNode === null) { return null; } - const idNode = walk(metricIdNode, [['firstChild', Identifier]]); - if (idNode === null) { - return null; - } - - const metricName = getNodeText(idNode, text); + const metricName = getNodeText(metricIdNode, text); return { type: 'IN_GROUPING', metricName, @@ -354,44 +328,11 @@ function resolveLabelMatcher(node: SyntaxNode, text: string, pos: number): Situa const labelName = getNodeText(labelNameNode, text); - // now we need to go up, to the parent of LabelMatcher, - // there can be one or many `LabelMatchList` parents, we have - // to go through all of them - - const firstListNode = walk(parent, [['parent', LabelMatchList]]); - if (firstListNode === null) { + const labelMatchersNode = walk(parent, [['parent', LabelMatchers]]); + if (labelMatchersNode === null) { return null; } - let listNode = firstListNode; - - // we keep going through the parent-nodes - // as long as they are LabelMatchList. - // as soon as we reawch LabelMatchers, we stop - let labelMatchersNode: SyntaxNode | null = null; - while (labelMatchersNode === null) { - const p = listNode.parent; - if (p === null) { - return null; - } - - const { id } = p.type; - - switch (id) { - case LabelMatchList: - //we keep looping - listNode = p; - continue; - case LabelMatchers: - // we reached the end, we can stop the loop - labelMatchersNode = p; - continue; - default: - // we reached some other node, we stop - return null; - } - } - // now we need to find the other names const allLabels = getLabels(labelMatchersNode, text); @@ -400,7 +341,6 @@ function resolveLabelMatcher(node: SyntaxNode, text: string, pos: number): Situa const metricNameNode = walk(labelMatchersNode, [ ['parent', VectorSelector], - ['firstChild', MetricIdentifier], ['firstChild', Identifier], ]); @@ -459,7 +399,7 @@ function resolveLabelKeysWithEquals(node: SyntaxNode, text: string, pos: number) // next false positive: // `something{a="1"^}` - const child = walk(node, [['firstChild', LabelMatchList]]); + const child = walk(node, [['firstChild', LabelMatcher]]); if (child !== null) { // means the label-matching part contains at least one label already. // @@ -476,7 +416,6 @@ function resolveLabelKeysWithEquals(node: SyntaxNode, text: string, pos: number) const metricNameNode = walk(node, [ ['parent', VectorSelector], - ['firstChild', MetricIdentifier], ['firstChild', Identifier], ]); @@ -532,12 +471,12 @@ export function getSituation(text: string, pos: number): Situation | null { }; } - /* - PromQL - Expr - VectorSelector - LabelMatchers - */ + /** + PromQL + Expr + VectorSelector + LabelMatchers + */ const tree = parser.parse(text); // if the tree contains error, it is very probable that @@ -565,3 +504,7 @@ export function getSituation(text: string, pos: number): Situation | null { return null; } + +function notEmpty(value: TValue | null | undefined): value is TValue { + return value !== null && value !== undefined; +} diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts index 08fa72b7b8a1..a168bc6cff2c 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts @@ -434,6 +434,12 @@ describe('buildVisualQueryFromString', () => { to: 27, parentType: 'VectorSelector', }, + { + text: ')', + from: 38, + to: 39, + parentType: 'PromQL', + }, ], query: { metric: '${func_var}', @@ -687,7 +693,7 @@ describe('buildVisualQueryFromString', () => { errors: [ { from: 6, - parentType: 'Expr', + parentType: 'BinaryExpr', text: '(bar + baz)', to: 17, }, diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts index de1486efc2a5..dc4085276c91 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts @@ -4,22 +4,18 @@ import { AggregateModifier, AggregateOp, BinaryExpr, - BinModifiers, - Expr, + BoolModifier, FunctionCall, - FunctionCallArgs, FunctionCallBody, FunctionIdentifier, - GroupingLabel, - GroupingLabelList, GroupingLabels, + Identifier, LabelMatcher, LabelName, + MatchingModifierClause, MatchOp, - MetricIdentifier, NumberLiteral, On, - OnOrIgnoring, ParenExpr, parser, StringLiteral, @@ -101,6 +97,7 @@ interface Context { errors: ParsingError[]; } +// TODO find a better approach for grafana global variables function isValidPromQLMinusGrafanaGlobalVariables(expr: string) { const context: Context = { query: { @@ -141,7 +138,7 @@ export function handleExpression(expr: string, node: SyntaxNode, context: Contex const visQuery = context.query; switch (node.type.id) { - case MetricIdentifier: { + case Identifier: { // Expectation is that there is only one of those per query. visQuery.metric = getString(expr, node); break; @@ -182,8 +179,8 @@ export function handleExpression(expr: string, node: SyntaxNode, context: Contex default: { if (node.type.id === ParenExpr) { - // We don't support parenthesis in the query to group expressions. We just report error but go on with the - // parsing. + // We don't support parenthesis in the query to group expressions. + // We just report error but go on with the parsing. context.errors.push(makeError(expr, node)); } // Any other nodes we just ignore and go to its children. This should be fine as there are lots of wrapper @@ -199,8 +196,9 @@ export function handleExpression(expr: string, node: SyntaxNode, context: Contex } } +// TODO check if we still need this function isIntervalVariableError(node: SyntaxNode) { - return node.prevSibling?.type.id === Expr && node.prevSibling?.firstChild?.type.id === VectorSelector; + return node.prevSibling?.firstChild?.type.id === VectorSelector; } function getLabel(expr: string, node: SyntaxNode): QueryBuilderLabelFilter { @@ -228,7 +226,6 @@ function handleFunction(expr: string, node: SyntaxNode, context: Context) { const funcName = getString(expr, nameNode); const body = node.getChild(FunctionCallBody); - const callArgs = body!.getChild(FunctionCallArgs); const params = []; let interval = ''; @@ -248,13 +245,13 @@ function handleFunction(expr: string, node: SyntaxNode, context: Context) { // We unshift operations to keep the more natural order that we want to have in the visual query editor. visQuery.operations.unshift(op); - if (callArgs) { - if (getString(expr, callArgs) === interval + ']') { + if (body) { + if (getString(expr, body) === '([' + interval + '])') { // This is a special case where we have a function with a single argument and it is the interval. // This happens when you start adding operations in query builder and did not set a metric yet. return; } - updateFunctionArgs(expr, callArgs, context, op); + updateFunctionArgs(expr, body, context, op); } } @@ -283,25 +280,14 @@ function handleAggregation(expr: string, node: SyntaxNode, context: Context) { funcName = `__${funcName}_without`; } - labels.push(...getAllByType(expr, modifier, GroupingLabel)); + labels.push(...getAllByType(expr, modifier, LabelName)); } const body = node.getChild(FunctionCallBody); - const callArgs = body!.getChild(FunctionCallArgs); - const callArgsExprChild = callArgs?.getChild(Expr); - const binaryExpressionWithinAggregationArgs = callArgsExprChild?.getChild(BinaryExpr); - - if (binaryExpressionWithinAggregationArgs) { - context.errors.push({ - text: 'Query parsing is ambiguous.', - from: binaryExpressionWithinAggregationArgs.from, - to: binaryExpressionWithinAggregationArgs.to, - }); - } const op: QueryBuilderOperation = { id: funcName, params: [] }; visQuery.operations.unshift(op); - updateFunctionArgs(expr, callArgs, context, op); + updateFunctionArgs(expr, body, context, op); // We add labels after params in the visual query editor. op.params.push(...labels); } @@ -309,8 +295,7 @@ function handleAggregation(expr: string, node: SyntaxNode, context: Context) { /** * Handle (probably) all types of arguments that function or aggregation can have. * - * FunctionCallArgs are nested bit weirdly basically its [firstArg, ...rest] where rest is again FunctionCallArgs so - * we cannot just get all the children and iterate them as arguments we have to again recursively traverse through + * We cannot just get all the children and iterate them as arguments we have to again recursively traverse through * them. * * @param expr @@ -323,15 +308,16 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont return; } switch (node.type.id) { - // In case we have an expression we don't know what kind so we have to look at the child as it can be anything. - case Expr: - // FunctionCallArgs are nested bit weirdly as mentioned so we have to go one deeper in this case. - case FunctionCallArgs: { + case FunctionCallBody: { let child = node.firstChild; while (child) { - const callArgsExprChild = child.getChild(Expr); - const binaryExpressionWithinFunctionArgs = callArgsExprChild?.getChild(BinaryExpr); + let binaryExpressionWithinFunctionArgs: SyntaxNode | null; + if (child?.type.id === BinaryExpr) { + binaryExpressionWithinFunctionArgs = child; + } else { + binaryExpressionWithinFunctionArgs = child?.getChild(BinaryExpr); + } if (binaryExpressionWithinFunctionArgs) { context.errors.push({ @@ -344,7 +330,6 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont updateFunctionArgs(expr, child, context, op); child = child.nextSibling; } - break; } @@ -377,16 +362,16 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { const visQuery = context.query; const left = node.firstChild!; const op = getString(expr, left.nextSibling); - const binModifier = getBinaryModifier(expr, node.getChild(BinModifiers)); + const binModifier = getBinaryModifier(expr, node.getChild(BoolModifier) ?? node.getChild(MatchingModifierClause)); const right = node.lastChild!; const opDef = binaryScalarOperatorToOperatorName[op]; - const leftNumber = left.getChild(NumberLiteral); - const rightNumber = right.getChild(NumberLiteral); + const leftNumber = left.type.id === NumberLiteral; + const rightNumber = right.type.id === NumberLiteral; - const rightBinary = right.getChild(BinaryExpr); + const rightBinary = right.type.id === BinaryExpr; if (leftNumber) { // TODO: this should be already handled in case parent is binary expression as it has to be added to parent @@ -432,6 +417,7 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { } } +// TODO revisit this function. function getBinaryModifier( expr: string, node: SyntaxNode | null @@ -439,26 +425,26 @@ function getBinaryModifier( | { isBool: true; isMatcher: false } | { isBool: false; isMatcher: true; matches: string; matchType: 'ignoring' | 'on' } | undefined { - if (!node) { - return undefined; - } - if (node.getChild('Bool')) { - return { isBool: true, isMatcher: false }; - } else { - const matcher = node.getChild(OnOrIgnoring); - if (!matcher) { - // Not sure what this could be, maybe should be an error. + if (!node) { return undefined; } - const labels = getString(expr, matcher.getChild(GroupingLabels)?.getChild(GroupingLabelList)); - return { - isMatcher: true, - isBool: false, - matches: labels, - matchType: matcher.getChild(On) ? 'on' : 'ignoring', - }; + if (node.getChild('Bool')) { + return { isBool: true, isMatcher: false }; + } else { + let labels = ''; + const groupingLabels = node.getChild(GroupingLabels); + if (groupingLabels) { + labels = getAllByType(expr, groupingLabels, LabelName).join(', '); + } + + return { + isMatcher: true, + isBool: false, + matches: labels, + matchType: node.getChild(On) ? 'on' : 'ignoring', + }; + } } -} function isEmptyQuery(query: PromVisualQuery) { if (query.labels.length === 0 && query.operations.length === 0 && !query.metric) { diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts index 10be8692ebc6..b5298f7e9ec7 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts @@ -113,11 +113,10 @@ export function makeBinOp( * not be safe is it would also find arguments of nested functions. * @param expr * @param cur - * @param type - can be string or number, some data-sources (loki) haven't migrated over to using numeric constants defined in the lezer parsing library (e.g. lezer-promql). - * @todo Remove string type definition when all data-sources have migrated to numeric constants + * @param type */ -export function getAllByType(expr: string, cur: SyntaxNode, type: number | string): string[] { - if (cur.type.id === type || cur.name === type) { +export function getAllByType(expr: string, cur: SyntaxNode, type: number): string[] { + if (cur.type.id === type) { return [getString(expr, cur)]; } const values: string[] = [];