Tempo: Add warning message when scope missing in TraceQL (#80472)

* Add warning message when scope missing in TraceQL

* Check for warnings in nodes

* Update formattiing

* Tidy up logic

* Tests

* Rename files and move tests for highlighting queries with errors

public/app/plugins/datasource/tempo/traceql/TraceQLEditor.test.tsx

@@ -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([]);
-  });
-});

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);
         });
       }}

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;
+    },
+  };
+}

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,19 +112,70 @@ 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) => {
+    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 = errorNode.from;
+  let start = from;
-      let end = errorNode.to;
+  let end = to;
 
   while (start > 0) {
     startLine++;
@@ -132,8 +187,8 @@ export const setErrorMarkers = (
   }
 
   return {
-        message: computeErrorMessage(errorNode),
+    message,
-        severity: monaco.MarkerSeverity.Error,
+    severity,
 
     startLineNumber: startLine,
     endLineNumber: endLine,
@@ -142,6 +197,4 @@ export const setErrorMarkers = (
     startColumn: start + model.getLineLength(startLine) + 2,
     endColumn: end + model.getLineLength(endLine) + 2,
   };
-    })
-  );
 };