Loki: Added query editor and builder support for new Logfmt features #74619

matyax · 2023-09-08T17:31:40Z

This PR adds support for new Loki LogQL features:

--strict flag
--keep-empty flag
Support for parameters (labels)

Additional changes:

Completions have been improved with better handling of trailing spaces and pipes, when inserting text
Situations: path has been renamed to paths to allow better grouping of multiple paths that produce the same situation.

Which issue(s) does this PR fix?:

Special notes for your reviewer:

No special steps to follow. Just install the updated dependencies, write LogQL queries, and expect to be helped by the autocomplete suggestions.

Some examples:

Exception:

In this case we cannot offer labels because the extracted log query has errors: {place="luna"} | logfmt --keep-empty float, [$__auto]. We could attempt to improve this in a follow up PR by using error nodes to clean up the query.

matyax · 2023-09-12T12:59:57Z

Opening for review. Take it for a spin and let me know how it feels. I will be submitting the query builder changes in another PR.

matyax · 2023-09-12T13:03:47Z

...lugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/situation.ts

@@ -451,7 +507,10 @@ function resolveDurations(node: SyntaxNode, text: string, pos: number): Situatio
 }

 function resolveLogRange(node: SyntaxNode, text: string, pos: number): Situation | null {
-  return resolveLogOrLogRange(node, text, pos, false);


After pipe used to be hardcoded, now se use the current query to decide. This prevents duplicated pipes in metric queries.

public/app/plugins/datasource/loki/queryUtils.ts

…aths

ivanahuckova

Nice! Left couple of comments and questions bellow. 🙂

public/app/plugins/datasource/loki/queryUtils.ts

...lugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/situation.ts

ivanahuckova · 2023-09-13T12:53:36Z

...datasource/loki/components/monaco-query-field/monaco-completion-provider/completions.test.ts

+    };
+
+    expect(await getCompletions(situation, completionProvider)).toMatchInlineSnapshot(`
+      [


Here it seems like we are mixing mock completion (with mock docs) and real completion (with real docs). Why is that?

Also these snapshot tests are quite long (100s of lines of code). What if we just checked that it has correct labels, rather than checking everything, as that's what we care about.

Here it seems like we are mixing mock completion (with mock docs) and real completion (with real docs). Why is that?

Because the mocks come from:

jest.mock('../../../querybuilder/operations', () => ({ explainOperator: () => 'Operator docs', }));

The reason is so tests doesn't break because we change the documentation. The usual "tests should be resilient to change".

Also these snapshot tests are quite long (100s of lines of code). What if we just checked that it has correct labels, rather than checking everything, as that's what we care about.

The whole way we test the completion output here is very cumbersome, to say the least. After having worked multiple times on this, I'm now more inclined to use inline snapshots, as they give you a readable output of the completion in a "similar" way of using the editor and seeing what it offers. I find that with snapshots it's easier to see that the output is good, unlike

grafana/public/app/plugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/completions.test.ts

Lines 361 to 362 in e16617d

const expected = buildAfterSelectorCompletions('logfmt', 'json', afterPipe, true);

expect(completions).toEqual(expected);

where the expectations are invisible.

We were also doing "snapshots" in the form of hardcoded json objects in places like this:

grafana/public/app/plugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/completions.test.ts

Lines 231 to 255 in e16617d

test('Returns completion options when the situation is IN_GROUPING', async () => {

const situation: Situation = { type: 'IN_GROUPING', logQuery: '' };

const completions = await getCompletions(situation, completionProvider);

expect(completions).toEqual([

{

insertText: 'extracted',

label: 'extracted',

triggerOnInsert: false,

type: 'LABEL_NAME',

},

{

insertText: 'place',

label: 'place',

triggerOnInsert: false,

type: 'LABEL_NAME',

},

{

insertText: 'source',

label: 'source',

triggerOnInsert: false,

type: 'LABEL_NAME',

},

]);

});

If I were to test this again, at the moment I would change everything to snapshots or json objects, but I'm 100% open to suggestions. Right now I can't see a way of testing completions without having to use very large strings or objects.

I would think here to check only important things. For example documentation is not that important, but label and type.

ivanahuckova · 2023-09-13T12:58:31Z

...lugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/situation.ts

@@ -136,7 +146,7 @@ export type Situation =
    };

 type Resolver = {
-  path: NodeType[];
+  paths: NodeType[][];


...lugins/datasource/loki/components/monaco-query-field/monaco-completion-provider/situation.ts

ivanahuckova · 2023-09-13T13:11:51Z

...gins/datasource/loki/components/monaco-query-field/monaco-completion-provider/completions.ts

+const LOGFMT_ARGUMENT_COMPLETIONS: Completion[] = [
+  {
+    type: 'FUNCTION',
+    label: 'strict',


have you considered using --strict as label. I personally think it would make it more understandable and it would also make autocomplete better

Interesting. I would expect the autocomplete to pick up -, but it's probably generating another new path for logfmt. I'll iterate a bit on this 👍

* Query builder: add params to logfmt definition * Logfmt operation: add default params * Query builder: update deprecated JsonExpression * Operation utils: update logfmt renderer * Query builder: parse LogfmtParser * Query builder: parse LogfmtExpressionParser * Remove console log * Remove unused variable * Remove extra character from render * Update unit tests * Fix unit tests * Operations: remove restParams from logfmt booleans * Parsing: group cases * Formatting * Formatting * Update modifyQuery * LogContextProvider: update with parser changes * LogContextProvider: remove unnecessary type castings It takes more energy to write `as unknow as LokiQuery` than to write a refId. * Formatting

matyax · 2023-09-18T14:14:19Z

public/app/plugins/datasource/loki/LogContextProvider.test.ts

@@ -225,8 +230,9 @@ describe('LogContextProvider', () => {
        10,
        LogRowContextQueryDirection.Backward,
        {
-          expr: '{bar="baz"} | logfmt | line_format = "foo"',


Realized this was incorrect in most line format expressions, so I fixed it.

Also removed some type castings that were not really necessary. FYI @svennergr

Nice, good 👀 !

github-actions · 2023-09-18T14:23:21Z

Backend code coverage report for PR #74619
No changes

github-actions · 2023-09-18T14:23:23Z

Frontend code coverage report for PR #74619

Plugin	Main	PR	Difference
loki	85.47%	85.7%	.23%

ivanahuckova

Nice job! Left some comments, but LGTM.

public/app/plugins/datasource/loki/queryUtils.ts

ivanahuckova · 2023-09-22T08:47:04Z

...datasource/loki/components/monaco-query-field/monaco-completion-provider/completions.test.ts

+    };
+
+    expect(await getCompletions(situation, completionProvider)).toMatchInlineSnapshot(`
+      [


I would think here to check only important things. For example documentation is not that important, but label and type.

* Loki autocomplete: add IN_LOGFMT situation for log queries * Loki autocomplete: add IN_LOGFMT situation for metric queries * Loki autocomplete: improve handling of trailing pipes and spaces * Loki autocomplete: add logfmt arguments completion * Loki autocomplete: add flags support to IN_LOGFMT * Loki autocomplete: extend IN_LOGFMT situation with labels and flag * Loki autocomplete: return logQuery in IN_LOGFMT situation * Loki autocomplete: offer label completions when IN_LOGFMT * Query utils: update parser detection method * Validation: update test * Loki autocomplete: improve IN_LOGFMT detection when in metric query * Loki autocomplete: improve logfmt suggestions * Loki autocomplete: improve logfmt suggestions in different scenarios * Loki autocomplete situation: refactor resolvers to support multiple paths * Situation: add test case * Loki autocomplete: allow user to use 2 flags * Situation: change flag to flags * Remove console log * Validation: import test parser * Completions: better handling of trailing comma scenario * Upgrade lezer-logql * Revert temporary imports * Loki Query Builder: Add support for new logfmt features (#74858) * Query builder: add params to logfmt definition * Logfmt operation: add default params * Query builder: update deprecated JsonExpression * Operation utils: update logfmt renderer * Query builder: parse LogfmtParser * Query builder: parse LogfmtExpressionParser * Remove console log * Remove unused variable * Remove extra character from render * Update unit tests * Fix unit tests * Operations: remove restParams from logfmt booleans * Parsing: group cases * Formatting * Formatting * Update modifyQuery * LogContextProvider: update with parser changes * LogContextProvider: remove unnecessary type castings It takes more energy to write `as unknow as LokiQuery` than to write a refId. * Formatting * Situation: use charAt instead of substring with endsWith * Situation: explain logfmt suggestions * Logfmt: improve flag suggestions * Remove console log * Completions: update test

grafana-delivery-bot bot added this to the 10.2.x milestone Sep 8, 2023

grafana-pr-automation bot added area/frontend datasource/Loki labels Sep 8, 2023

matyax marked this pull request as ready for review September 12, 2023 12:59

matyax requested a review from a team as a code owner September 12, 2023 12:59

matyax added add to changelog no-backport Skip backport of PR labels Sep 12, 2023

matyax commented Sep 12, 2023

View reviewed changes

public/app/plugins/datasource/loki/queryUtils.ts Outdated Show resolved Hide resolved

matyax added 20 commits September 13, 2023 14:43

Loki autocomplete: add IN_LOGFMT situation for log queries

a1a5373

Loki autocomplete: add IN_LOGFMT situation for metric queries

1b7b36f

Loki autocomplete: improve handling of trailing pipes and spaces

c8d74ee

Loki autocomplete: add logfmt arguments completion

47c2910

Loki autocomplete: add flags support to IN_LOGFMT

d13fdc9

Loki autocomplete: extend IN_LOGFMT situation with labels and flag

4afdec0

Loki autocomplete: return logQuery in IN_LOGFMT situation

1f47bc2

Loki autocomplete: offer label completions when IN_LOGFMT

3f6734b

Query utils: update parser detection method

e55f50e

Validation: update test

1e245e1

Loki autocomplete: improve IN_LOGFMT detection when in metric query

763a100

Loki autocomplete: improve logfmt suggestions

6439020

Loki autocomplete: improve logfmt suggestions in different scenarios

4232d38

Loki autocomplete situation: refactor resolvers to support multiple p…

15229a4

…aths

Situation: add test case

c0e14bc

Loki autocomplete: allow user to use 2 flags

5f85dd1

Situation: change flag to flags

9208398

Remove console log

2051b8b

Validation: import test parser

9d21b97

Completions: better handling of trailing comma scenario

1d84bc0

matyax added 2 commits September 13, 2023 14:52

Upgrade lezer-logql

0431d82

Revert temporary imports

e16617d

matyax requested a review from a team as a code owner September 13, 2023 12:53

matyax requested review from ashharrison90 and removed request for a team September 13, 2023 12:53

ivanahuckova reviewed Sep 13, 2023

View reviewed changes

matyax commented Sep 18, 2023

View reviewed changes

matyax added 4 commits September 19, 2023 11:19

Situation: use charAt instead of substring with endsWith

25cd9ac

Situation: explain logfmt suggestions

b3236c7

Logfmt: improve flag suggestions

4935c0f

Merge branch 'main' into matyax/logfmt-with-arguments

57918bf

matyax requested a review from ivanahuckova September 19, 2023 09:49

matyax added 2 commits September 19, 2023 11:59

Remove console log

e50e7c7

Completions: update test

688a9e9

ivanahuckova approved these changes Sep 22, 2023

View reviewed changes

Merge branch 'main' into matyax/logfmt-with-arguments

7d6fecf

matyax enabled auto-merge (squash) September 22, 2023 09:12

Prettier

85fcb89

matyax merged commit 91ed2a6 into main Sep 22, 2023
15 checks passed

matyax deleted the matyax/logfmt-with-arguments branch September 22, 2023 09:34

matyax mentioned this pull request Sep 26, 2023

Loki Query Editor: Improved suggestions for JSON parser #75473

Closed

matyax changed the title ~~Loki Query Editor: Add support for new logfmt features~~ Loki: Added query editor and builder support for new Logfmt features Oct 19, 2023

zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023

dnhn mentioned this pull request Oct 24, 2023

grafana 10.2.0 Homebrew/homebrew-core#152264

Closed

BrewTestBot mentioned this pull request Oct 25, 2023

grafana 10.2.0 Homebrew/homebrew-core#152321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Added query editor and builder support for new Logfmt features #74619

Loki: Added query editor and builder support for new Logfmt features #74619

matyax commented Sep 8, 2023 •

edited

Loading

matyax commented Sep 12, 2023

matyax Sep 12, 2023

ivanahuckova left a comment

ivanahuckova Sep 13, 2023

ivanahuckova Sep 13, 2023

matyax Sep 13, 2023

ivanahuckova Sep 22, 2023

ivanahuckova Sep 13, 2023

ivanahuckova Sep 13, 2023

matyax Sep 13, 2023

matyax Sep 19, 2023

matyax Sep 18, 2023

svennergr Sep 18, 2023

github-actions bot commented Sep 18, 2023

github-actions bot commented Sep 18, 2023 •

edited

Loading

ivanahuckova left a comment

ivanahuckova Sep 22, 2023

	const expected = buildAfterSelectorCompletions('logfmt', 'json', afterPipe, true);
	expect(completions).toEqual(expected);

	test('Returns completion options when the situation is IN_GROUPING', async () => {
	const situation: Situation = { type: 'IN_GROUPING', logQuery: '' };
	const completions = await getCompletions(situation, completionProvider);

	expect(completions).toEqual([
	{
	insertText: 'extracted',
	label: 'extracted',
	triggerOnInsert: false,
	type: 'LABEL_NAME',
	},
	{
	insertText: 'place',
	label: 'place',
	triggerOnInsert: false,
	type: 'LABEL_NAME',
	},
	{
	insertText: 'source',
	label: 'source',
	triggerOnInsert: false,
	type: 'LABEL_NAME',
	},
	]);
	});

Loki: Added query editor and builder support for new Logfmt features #74619

Loki: Added query editor and builder support for new Logfmt features #74619

Conversation

matyax commented Sep 8, 2023 • edited Loading

matyax commented Sep 12, 2023

Choose a reason for hiding this comment

ivanahuckova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

github-actions bot commented Sep 18, 2023 • edited Loading

ivanahuckova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matyax commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 18, 2023 •

edited

Loading