Loki: Add the ability to prettify logql queries

[gwdawson]

What is this feature?
adds a button giving the user an option to prettify their loki query.

Which issue(s) does this PR fix?:
Fixes #61041

how to test this pr:

in grafana/grafana

1. checkout to this branch
2. make sure you have enabled the feature flag lokiFormatQuery

[github-actions]

Backend code coverage report for PR #64337
No changes

[github-actions]

Frontend code coverage report for PR #64337

Plugin	Main	PR	Difference
loki	85.29%	85.35%	.06%

[matyax]

Good work so far! I will try to come back to this with a potential approach using Lezer to replace the template variables.

[matyax]

Leaving this as a comment to continue later. I've been playing with using the paser to get the node that points to a variable, to be used after formatting to replace the interpolated string with the variable name. For example, I would like to keep a reference to the Range node in this tree:

So I can use it later to replace it with the variable name using the parsed prettified query:

To attempt to do that, I would store a path to the node (node parents until the root of the tree), for every detected variable.

Datasource:

  async formatQuery(query: string) {
    const transformedQuery = transformForFormatting(query, this.interpolateString.bind(this));
    
    let formatted = await this.metadataRequest('format_query', { query: transformedQuery.query  });

    return formatted;
  }

Formatter.ts

import { SyntaxNode } from '@lezer/common';

import { ScopedVars, TypedVariableModel } from '@grafana/data';
import { parser } from '@grafana/lezer-logql';
import { ErrorId } from 'app/plugins/datasource/prometheus/querybuilder/shared/parsingUtils';

import { placeHolderScopedVars } from './components/monaco-query-field/monaco-completion-provider/validation';

type Transformation = {
    original: string;
    replaced: string;
}

type TransformedQuery = {
    transformations: Transformation[];
    query: string;
}

interface ParseError {
    text: string;
    node: SyntaxNode;
}



function parseQuery(query: string) {
    const parseErrors = new Map<SyntaxNode | null, ParseError[]>();
    const tree = parser.parse(query);
    tree.iterate({
      enter: (nodeRef): false | void => {
        if (nodeRef.type.id === ErrorId) {
          const node = nodeRef.node;
          const siblings = parseErrors.get(node.parent) || []; 
          parseErrors.set(node.parent, [...siblings, {
            node,
            text: query.substring(node.from, node.to),
          }]);
        }
      },
    });
    return parseErrors;
  }

export function transformForFormatting(query: string, interpolateString: (string: string, scopedVars?: ScopedVars) => string): TransformedQuery {
    // We get the possible variables from the parse errors
    const errors = parseQuery(query);
    // Nothing to fix, the query is ready for formatting
    if (!errors.size) {
        return {
            transformations: [],
            query,
        };
    }

    errors.forEach((parseError, parent) => {
        if (parseError[0].text === '$') {
            const variable = query.substring(parseError[0].node.from, parseError[parseError.length-1].node.from);
            const chain = [parent?.type.name];
            let lastParent = parent;
            while (lastParent?.parent) {
                chain.push(lastParent.parent.type.name);
                lastParent = lastParent.parent;
            }
            console.log(variable, interpolateString(variable, placeHolderScopedVars), chain);
        }
    });

    return {
        transformations: [],
        query,
    };
}

export function revertTransformations(query: string, transformations: Transformation[]): string {
    return '';
}

Anyway, given the complexity that this could require, I wonder if we should either:

• Ask the backend to be nice with variables somehow.
• Format in the frontend and handle variables better than the endpoint.

In any case, this could be useful to you as an introduction to Lezer. To be continued.

[matyax]

Will follow up with something that might work. Stay tunned.

[matyax]

We might try something like this: #64591 Let me know what you think.

[matyax]

@gwdawson Found the bug that is currently duplicating the output of count(sum(rate({compose_project="tns-custom"}[1s]))).

The root cause of the problem is the function iterateNode. What happens is that in formatVectorAggregationExpr you call iterateNode asking for the following nodes: [VectorOp, Number, MetricExpr, Grouping], and because it's recursive (and unfortunately it's bugged), it returns a lot more that you're expecting.

Instead of getting an array of VectorOp, MetricExpr from:

You get 2 times those two:

All ☝️ are recursive calls over the same nodes. So, you correctly format the first two VectorOp, MetricExpr, and when you're done, you append the extra VectorOp, MetricExpr.

From that, the following change fixes the bug:

function iterateNode(node: SyntaxNode, lookingFor: number[]): SyntaxNode[] {
  const nodes: SyntaxNode[] = [];
  let child = node.firstChild;

  while (child) {
    if (lookingFor.includes(child.type.id)) {
      nodes.push(child);
      lookingFor.splice(lookingFor.indexOf(child.type.id), 1); // <---- this
    }

    nodes.push(...iterateNode(child, lookingFor));
    child = child.nextSibling;
  }

  return nodes;
}

The problem with that change is that it breaks other code that is dependent on iterateNode looking for repeated nodes.

One of the main issues with iterateNode is that it should be either recursive or use a loop, not both, and I'm not sure why you added a recursive call.

For example:

function iterateNode(node: SyntaxNode, lookingFor: number[]): SyntaxNode[] {
  const nodes: SyntaxNode[] = [];
  let child = node.firstChild;

  while (child) {
    if (lookingFor.includes(child.type.id)) {
      nodes.push(child);
    }

    // nodes.push(...iterateNode(child, lookingFor)); This should not be necessary
    child = child.nextSibling; // maybe this while should move to the children if there are no more siblings?
  }

  return nodes;
}

TL;DR

Adding lookingFor.splice(lookingFor.indexOf(child.type.id), 1); fixes the bug for that query, but the root cause of this bug (and maybe other bugs we don't know about yet) is that iterateNode is a buggy implementation that should be fixed.

Other things I noticed

I would suggest that you avoid recursive calls, specially calls to formatLokiQuery, because it's a potential source for bugs. E.g.

grafana/public/app/plugins/datasource/loki/formatter.ts

Lines 329 to 331 in 610c420

	const metricExpr = query.substring(node.from, node.to);
	response += indentMultiline(formatLokiQuery(metricExpr), 1);
	response += '\n)';

There, I would change it to:

const metricExpr = query.substring(node.from, node.to); // slice the MetricExpr part
const metricNode = getNodeFromQuery(metricExpr, MetricExpr); // Get the new MetricExpr node
if (metricNode) {
  response += indentMultiline(formatMetricExpr(metricNode, metricExpr), 1); // Pass it to formatMetricExpr
}

PD: it works really well! Great work so far 🤝

[matyax]

One last thing and we're done here 🚢

[matyax]

Very nice work 👏 👏 👏

this change is related to the api implementation we tested. dismissing to unblock merge