diff --git a/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.test.tsx b/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.test.tsx deleted file mode 100644 index 6af62515efd..00000000000 --- a/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.test.tsx +++ /dev/null @@ -1,90 +0,0 @@ -import { computeErrorMessage, getErrorNodes } from './errorHighlighting'; - -describe('Check for syntax errors in query', () => { - it.each([ - ['{span.http.status_code = }', 'Invalid value after comparison or arithmetic operator.'], - ['{span.http.status_code 200}', 'Invalid comparison operator after field expression.'], - ['{span.http.status_code ""}', 'Invalid operator after field expression.'], - ['{span.http.status_code @ 200}', 'Invalid comparison operator after field expression.'], - ['{span.http.status_code span.http.status_code}', 'Invalid operator after field expression.'], - [ - '{span.http.status_code = 200} {span.http.status_code = 200}', - 'Invalid spanset combining operator after spanset expression.', - ], - [ - '{span.http.status_code = 200} + {span.http.status_code = 200}', - 'Invalid spanset combining operator after spanset expression.', - ], - ['{span.http.status_code = 200} &&', 'Invalid spanset expression after spanset combining operator.'], - [ - '{span.http.status_code = 200} && {span.http.status_code = 200} | foo() > 3', - 'Invalid aggregation operator after pipepile operator.', - ], - [ - '{span.http.status_code = 200} && {span.http.status_code = 200} | avg() > 3', - 'Invalid expression for aggregator operator.', - ], - ['{ 1 + 1 = 2 + }', 'Invalid value after comparison or arithmetic operator.'], - ['{ .a && }', 'Invalid value after logical operator.'], - ['{ .a || }', 'Invalid value after logical operator.'], - ['{ .a + }', 'Invalid value after comparison or arithmetic operator.'], - ['{ 200 = 200 200 }', 'Invalid comparison operator after field expression.'], - ['{.foo 300}', 'Invalid comparison operator after field expression.'], - ['{.foo 300 && .bar = 200}', 'Invalid operator after field expression.'], - ['{.foo 300 && .bar 200}', 'Invalid operator after field expression.'], - ['{.foo=1} {.bar=2}', 'Invalid spanset combining operator after spanset expression.'], - ['{ span.http.status_code = 200 && }', 'Invalid value after logical operator.'], - ['{ span.http.status_code = 200 || }', 'Invalid value after logical operator.'], - ['{ .foo = 200 } && ', 'Invalid spanset expression after spanset combining operator.'], - ['{ .foo = 200 } || ', 'Invalid spanset expression after spanset combining operator.'], - ['{ .foo = 200 } >> ', 'Invalid spanset expression after spanset combining operator.'], - ['{.foo=1} | avg()', 'Invalid expression for aggregator operator.'], - ['{.foo=1} | avg(.foo) > ', 'Invalid value after comparison operator.'], - ['{.foo=1} | avg() < 1s', 'Invalid expression for aggregator operator.'], - ['{.foo=1} | max() = 3', 'Invalid expression for aggregator operator.'], - ['{.foo=1} | by()', 'Invalid expression for aggregator operator.'], - ['{.foo=1} | select()', 'Invalid expression for aggregator operator.'], - ['{foo}', 'Invalid expression for spanset.'], - ['{.}', 'Invalid expression for spanset.'], - ['{ resource. }', 'Invalid expression for spanset.'], - ['{ span. }', 'Invalid expression for spanset.'], - ['{.foo=}', 'Invalid value after comparison or arithmetic operator.'], - ['{.foo="}', 'Invalid value after comparison or arithmetic operator.'], - ['{.foo=300} |', 'Invalid aggregation operator after pipepile operator.'], - ['{.foo=300} && {.bar=200} |', 'Invalid aggregation operator after pipepile operator.'], - ['{.foo=300} && {.bar=300} && {.foo=300} |', 'Invalid aggregation operator after pipepile operator.'], - ['{.foo=300} | avg(.value)', 'Invalid comparison operator after aggregator operator.'], - ['{.foo=300} && {.foo=300} | avg(.value)', 'Invalid comparison operator after aggregator operator.'], - ['{.foo=300} | avg(.value) =', 'Invalid value after comparison operator.'], - ['{.foo=300} && {.foo=300} | avg(.value) =', 'Invalid value after comparison operator.'], - ['{.foo=300} | max(duration) > 1hs', 'Invalid value after comparison operator.'], - ['{ span.http.status_code', 'Invalid comparison operator after field expression.'], - ['{ .foo = "bar"', 'Invalid comparison operator after field expression.'], - ['abcxyz', 'Invalid query.'], - ])('error message for invalid query - %s, %s', (query: string, expectedErrorMessage: string) => { - const errorNode = getErrorNodes(query)[0]; - expect(computeErrorMessage(errorNode)).toBe(expectedErrorMessage); - }); - - it.each([ - ['123'], - ['abc'], - ['1a2b3c'], - ['{span.status = $code}'], - ['{span.${attribute} = "GET"}'], - ['{span.${attribute:format} = ${value:format} }'], - ['{true} >> {true}'], - ['{true} << {true}'], - ['{true} !>> {true}'], - ['{true} !<< {true}'], - [ - `{ true } /* && { false } && */ && { true } // && { false } - && { true }`, - ], - ['{span.s"t\\"at"us}'], - ['{span.s"t\\\\at"us}'], - ['{ span.s"tat"us" = "GET123 }'], // weird query, but technically valid - ])('valid query - %s', (query: string) => { - expect(getErrorNodes(query)).toStrictEqual([]); - }); -}); diff --git a/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.tsx b/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.tsx index 4b85b0d3960..64aa15acfbd 100644 --- a/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.tsx +++ b/public/app/plugins/datasource/tempo/traceql/TraceQLEditor.tsx @@ -11,7 +11,7 @@ import { dispatch } from '../_importedDependencies/store'; import { TempoDatasource } from '../datasource'; import { CompletionProvider, CompletionType } from './autocomplete'; -import { getErrorNodes, setErrorMarkers } from './errorHighlighting'; +import { getErrorNodes, setMarkers } from './highlighting'; import { languageDefinition } from './traceql'; interface Props { @@ -71,7 +71,7 @@ export function TraceQLEditor(props: Props) { const model = editor.getModel(); if (model) { const errorNodes = getErrorNodes(model.getValue()); - setErrorMarkers(monaco, model, errorNodes); + setMarkers(monaco, model, errorNodes); } // Register callback for query changes @@ -90,7 +90,7 @@ export function TraceQLEditor(props: Props) { // Immediately updates the squiggles, in case the user fixed an error, // excluding the error around the cursor position - setErrorMarkers( + setMarkers( monaco, model, errorNodes.filter((errorNode) => !(errorNode.from <= cursorPosition && cursorPosition <= errorNode.to)) @@ -98,7 +98,7 @@ export function TraceQLEditor(props: Props) { // Later on, show all errors errorTimeoutId.current = window.setTimeout(() => { - setErrorMarkers(monaco, model, errorNodes); + setMarkers(monaco, model, errorNodes); }, 500); }); }} diff --git a/public/app/plugins/datasource/tempo/traceql/highlighting.test.ts b/public/app/plugins/datasource/tempo/traceql/highlighting.test.ts new file mode 100644 index 00000000000..1a3a8b90c35 --- /dev/null +++ b/public/app/plugins/datasource/tempo/traceql/highlighting.test.ts @@ -0,0 +1,192 @@ +import { monacoTypes } from '@grafana/ui'; + +import { computeErrorMessage, getErrorNodes, getWarningMarkers } from './highlighting'; + +describe('Highlighting', () => { + describe('gets correct warning markers for', () => { + const message = 'Add resource or span scope to attribute to improve query performance.'; + + describe('no warnings', () => { + it('for span scope', () => { + const { model } = setup('{ span.component = "http" }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual(expect.objectContaining([])); + }); + it('for resource scope', () => { + const { model } = setup('{ resource.component = "http" }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual(expect.objectContaining([])); + }); + it('for parent scope', () => { + const { model } = setup('{ parent.component = "http" }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual(expect.objectContaining([])); + }); + }); + + it('single warning', () => { + const { model } = setup('{ .component = "http" }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual( + expect.objectContaining([ + { + message, + severity: 4, + startLineNumber: 1, + endLineNumber: 1, + startColumn: 3, + endColumn: 3, + }, + ]) + ); + }); + + it('multiple warnings', () => { + const { model } = setup('{ .component = "http" || .http.status_code = 200 }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual( + expect.objectContaining([ + { + message, + severity: 4, + startLineNumber: 1, + endLineNumber: 1, + startColumn: 3, + endColumn: 3, + }, + { + message, + severity: 4, + startLineNumber: 1, + endLineNumber: 1, + startColumn: 26, + endColumn: 26, + }, + ]) + ); + }); + + it('multiple parts, single warning', () => { + const { model } = setup('{ resource.component = "http" || .http.status_code = 200 }'); + const marker = getWarningMarkers(4, model); + expect(marker).toEqual( + expect.objectContaining([ + { + message, + severity: 4, + startLineNumber: 1, + endLineNumber: 1, + startColumn: 34, + endColumn: 34, + }, + ]) + ); + }); + }); + + describe('check for syntax errors in query', () => { + it.each([ + ['{span.http.status_code = }', 'Invalid value after comparison or arithmetic operator.'], + ['{span.http.status_code 200}', 'Invalid comparison operator after field expression.'], + ['{span.http.status_code ""}', 'Invalid operator after field expression.'], + ['{span.http.status_code @ 200}', 'Invalid comparison operator after field expression.'], + ['{span.http.status_code span.http.status_code}', 'Invalid operator after field expression.'], + [ + '{span.http.status_code = 200} {span.http.status_code = 200}', + 'Invalid spanset combining operator after spanset expression.', + ], + [ + '{span.http.status_code = 200} + {span.http.status_code = 200}', + 'Invalid spanset combining operator after spanset expression.', + ], + ['{span.http.status_code = 200} &&', 'Invalid spanset expression after spanset combining operator.'], + [ + '{span.http.status_code = 200} && {span.http.status_code = 200} | foo() > 3', + 'Invalid aggregation operator after pipepile operator.', + ], + [ + '{span.http.status_code = 200} && {span.http.status_code = 200} | avg() > 3', + 'Invalid expression for aggregator operator.', + ], + ['{ 1 + 1 = 2 + }', 'Invalid value after comparison or arithmetic operator.'], + ['{ .a && }', 'Invalid value after logical operator.'], + ['{ .a || }', 'Invalid value after logical operator.'], + ['{ .a + }', 'Invalid value after comparison or arithmetic operator.'], + ['{ 200 = 200 200 }', 'Invalid comparison operator after field expression.'], + ['{.foo 300}', 'Invalid comparison operator after field expression.'], + ['{.foo 300 && .bar = 200}', 'Invalid operator after field expression.'], + ['{.foo 300 && .bar 200}', 'Invalid operator after field expression.'], + ['{.foo=1} {.bar=2}', 'Invalid spanset combining operator after spanset expression.'], + ['{ span.http.status_code = 200 && }', 'Invalid value after logical operator.'], + ['{ span.http.status_code = 200 || }', 'Invalid value after logical operator.'], + ['{ .foo = 200 } && ', 'Invalid spanset expression after spanset combining operator.'], + ['{ .foo = 200 } || ', 'Invalid spanset expression after spanset combining operator.'], + ['{ .foo = 200 } >> ', 'Invalid spanset expression after spanset combining operator.'], + ['{.foo=1} | avg()', 'Invalid expression for aggregator operator.'], + ['{.foo=1} | avg(.foo) > ', 'Invalid value after comparison operator.'], + ['{.foo=1} | avg() < 1s', 'Invalid expression for aggregator operator.'], + ['{.foo=1} | max() = 3', 'Invalid expression for aggregator operator.'], + ['{.foo=1} | by()', 'Invalid expression for aggregator operator.'], + ['{.foo=1} | select()', 'Invalid expression for aggregator operator.'], + ['{foo}', 'Invalid expression for spanset.'], + ['{.}', 'Invalid expression for spanset.'], + ['{ resource. }', 'Invalid expression for spanset.'], + ['{ span. }', 'Invalid expression for spanset.'], + ['{.foo=}', 'Invalid value after comparison or arithmetic operator.'], + ['{.foo="}', 'Invalid value after comparison or arithmetic operator.'], + ['{.foo=300} |', 'Invalid aggregation operator after pipepile operator.'], + ['{.foo=300} && {.bar=200} |', 'Invalid aggregation operator after pipepile operator.'], + ['{.foo=300} && {.bar=300} && {.foo=300} |', 'Invalid aggregation operator after pipepile operator.'], + ['{.foo=300} | avg(.value)', 'Invalid comparison operator after aggregator operator.'], + ['{.foo=300} && {.foo=300} | avg(.value)', 'Invalid comparison operator after aggregator operator.'], + ['{.foo=300} | avg(.value) =', 'Invalid value after comparison operator.'], + ['{.foo=300} && {.foo=300} | avg(.value) =', 'Invalid value after comparison operator.'], + ['{.foo=300} | max(duration) > 1hs', 'Invalid value after comparison operator.'], + ['{ span.http.status_code', 'Invalid comparison operator after field expression.'], + ['{ .foo = "bar"', 'Invalid comparison operator after field expression.'], + ['abcxyz', 'Invalid query.'], + ])('error message for invalid query - %s, %s', (query: string, expectedErrorMessage: string) => { + const errorNode = getErrorNodes(query)[0]; + expect(computeErrorMessage(errorNode)).toBe(expectedErrorMessage); + }); + + it.each([ + ['123'], + ['abc'], + ['1a2b3c'], + ['{span.status = $code}'], + ['{span.${attribute} = "GET"}'], + ['{span.${attribute:format} = ${value:format} }'], + ['{true} >> {true}'], + ['{true} << {true}'], + ['{true} !>> {true}'], + ['{true} !<< {true}'], + [ + `{ true } /* && { false } && */ && { true } // && { false } + && { true }`, + ], + ['{span.s"t\\"at"us}'], + ['{span.s"t\\\\at"us}'], + ['{ span.s"tat"us" = "GET123 }'], // weird query, but technically valid + ])('valid query - %s', (query: string) => { + expect(getErrorNodes(query)).toStrictEqual([]); + }); + }); +}); + +function setup(value: string) { + const model = makeModel(value); + return { model } as unknown as { model: monacoTypes.editor.ITextModel }; +} + +function makeModel(value: string) { + return { + id: 'test_monaco', + getValue() { + return value; + }, + getLineLength() { + return value.length; + }, + }; +} diff --git a/public/app/plugins/datasource/tempo/traceql/errorHighlighting.ts b/public/app/plugins/datasource/tempo/traceql/highlighting.ts similarity index 59% rename from public/app/plugins/datasource/tempo/traceql/errorHighlighting.ts rename to public/app/plugins/datasource/tempo/traceql/highlighting.ts index c3974d4300d..eb82b861a59 100644 --- a/public/app/plugins/datasource/tempo/traceql/errorHighlighting.ts +++ b/public/app/plugins/datasource/tempo/traceql/highlighting.ts @@ -7,12 +7,16 @@ import { ComparisonOp, FieldExpression, FieldOp, + Identifier, IntrinsicField, Or, + Parent, parser, Pipe, + Resource, ScalarExpression, ScalarFilter, + Span, SpansetFilter, SpansetPipelineExpression, } from '@grafana/lezer-traceql'; @@ -108,40 +112,89 @@ export const getErrorNodes = (query: string): SyntaxNode[] => { * Use red markers (squiggles) to highlight syntax errors in queries. * */ -export const setErrorMarkers = ( +export const setMarkers = ( monaco: typeof monacoTypes, model: monacoTypes.editor.ITextModel, errorNodes: SyntaxNode[] ) => { + const markers = [ + ...getErrorMarkers(monaco.MarkerSeverity.Error, model, errorNodes), + ...getWarningMarkers(monaco.MarkerSeverity.Warning, model), + ]; monaco.editor.setModelMarkers( model, 'owner', // default value - errorNodes.map((errorNode) => { - let startLine = 0; - let endLine = 0; - let start = errorNode.from; - let end = errorNode.to; - - while (start > 0) { - startLine++; - start -= model.getLineLength(startLine) + 1; // new lines don't count for getLineLength() but they still count as a character for the parser - } - while (end > 0) { - endLine++; - end -= model.getLineLength(endLine) + 1; - } - - return { - message: computeErrorMessage(errorNode), - severity: monaco.MarkerSeverity.Error, - - startLineNumber: startLine, - endLineNumber: endLine, - - // `+ 2` because of the above computations - startColumn: start + model.getLineLength(startLine) + 2, - endColumn: end + model.getLineLength(endLine) + 2, - }; - }) + markers ); }; + +export const getErrorMarkers = (severity: number, model: monacoTypes.editor.ITextModel, errorNodes: SyntaxNode[]) => { + return errorNodes.map((errorNode) => { + const message = computeErrorMessage(errorNode); + return getMarker(severity, message, model, errorNode.from, errorNode.to); + }); +}; + +export const getWarningMarkers = (severity: number, model: monacoTypes.editor.ITextModel) => { + let markers = []; + + // Check if there are issues that should result in a warning marker + const text = model.getValue(); + const tree = parser.parse(text); + const indexOfDot = text.indexOf('.'); + if (indexOfDot > -1) { + const cur = tree.cursorAt(0); + do { + const { node } = cur; + if (node.type.id === Identifier) { + // Make sure prevSibling is using the proper scope + if ( + node.prevSibling?.type.id !== Parent && + node.prevSibling?.type.id !== Resource && + node.prevSibling?.type.id !== Span + ) { + const from = node.prevSibling ? node.prevSibling.from : node.from - 1; + const to = node.prevSibling ? node.prevSibling.to : node.from - 1; + const message = 'Add resource or span scope to attribute to improve query performance.'; + markers.push(getMarker(severity, message, model, from, to)); + } + } + } while (cur.next()); + } + + return markers; +}; + +export const getMarker = ( + severity: number, + message: string, + model: monacoTypes.editor.ITextModel, + from: number, + to: number +) => { + let startLine = 0; + let endLine = 0; + let start = from; + let end = to; + + while (start > 0) { + startLine++; + start -= model.getLineLength(startLine) + 1; // new lines don't count for getLineLength() but they still count as a character for the parser + } + while (end > 0) { + endLine++; + end -= model.getLineLength(endLine) + 1; + } + + return { + message, + severity, + + startLineNumber: startLine, + endLineNumber: endLine, + + // `+ 2` because of the above computations + startColumn: start + model.getLineLength(startLine) + 2, + endColumn: end + model.getLineLength(endLine) + 2, + }; +};