[ES|QL] Add ECS information to the editor hint & prioritize fields based on ECS information on the editor #187922

qn895 · 2024-07-09T22:09:19Z

Summary

Closes #181036. This PR adds ECS information to the editor hint & prioritizes fields based on ECS information on the editor. The logic involves:

Identify if dataset has ecs fields (e.g. column contains ecs.version)
If yes, check which fields have ecs info using fieldsMetadata client
Push fields with ecs info up to the top of the list of suggestions and order these fields alphabetically. These pushed fields have "1D" sort text.
Show the rest of the fields without ecs decription at the bottom, order these fields alphabetically.

Screen.Recording.2024-07-11.at.12.27.00.mov

Note for reviewers:

When fetching fields metadata, if there are many fields, we are breaking down into multiple requests (250 fields/request) to avoid payload too large.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

qn895 · 2024-07-10T18:33:54Z

/ci

…fix'

qn895 · 2024-07-10T19:05:33Z

/ci

qn895 · 2024-07-10T20:33:59Z

/ci

…fix'

qn895 · 2024-07-10T21:07:18Z

/ci

elasticmachine · 2024-07-11T00:24:04Z

Pinging @elastic/kibana-esql (Team:ESQL)

stratoula

This is awesome 😍 I love it, can you take a look on my comment below? We might not be able to improve it but if you can confirm for me would be awesome!

Not sure if it is possible with Monaco:

Here if I dont open the popover it feels a bit weird that looks like String ECS..., I would expect at least a separator.

In cases without the description I like the fact that I see the type here

but opening the popover to see the type again is unnecessary.

I wonder if we can do something better here. Possibly not because monaco doesnt allow us but can you take a look just to confirm?

stratoula · 2024-07-11T07:28:57Z

packages/kbn-text-based-editor/src/ecs_metadata_helper.test.ts

+import type { FieldsMetadataPublicStart } from '@kbn/fields-metadata-plugin/public';
+
+describe('getColumnsWithMetadata', () => {
+  it('should return columns as is if fieldsMetadata is not provided', async () => {


Suggested change

it('should return columns as is if fieldsMetadata is not provided', async () => {

it('should return columns if fieldsMetadata is not provided', async () => {

Updated here 22db292 (#187922)

stratoula · 2024-07-11T07:29:12Z

packages/kbn-text-based-editor/src/ecs_metadata_helper.test.ts

+    expect(result).toEqual(columns);
+  });
+
+  it('should return columns as is if ECS version field is not present', async () => {


Suggested change

it('should return columns as is if ECS version field is not present', async () => {

it('should return columns if ECS version field is not present', async () => {

Updated here 22db292 (#187922)

stratoula · 2024-07-11T07:30:28Z

packages/kbn-esql-validation-autocomplete/src/validation/types.ts

@@ -18,6 +18,9 @@ export interface ESQLVariable {
 export interface ESQLRealField {


This name is funny 😄

stratoula · 2024-07-11T07:40:15Z

packages/kbn-text-based-editor/src/text_based_languages_editor.tsx

@@ -430,7 +440,8 @@ export const TextBasedLanguagesEditor = memo(function TextBasedLanguagesEditor({
              undefined,
              abortController
            ).result;
-            return table?.columns.map((c) => ({ name: c.name, type: c.meta.type })) || [];
+            const columns = table?.columns.map((c) => ({ name: c.name, type: c.meta.type })) || [];
+            return await getColumnsWithMetadata(columns, fieldsMetadata);


Very smart you are doing this here, this way we are using the cache 👏

tonyghiani · 2024-07-11T07:55:10Z

packages/kbn-text-based-editor/src/ecs_metadata_helper.ts

+      const fields = await fieldsMetadataClient.find({
+        fieldNames: columns.map((c) => c.name),
+      });
+
+      if (fields.fields) {
+        return columns.map((c) => ({
+          ...c,
+          // Metadata services gives description for
+          // 'ecs.version' but not 'ecs.version.keyword'
+          // but both should show description if available
+          metadata: fields.fields[removeKeywordSuffix(c.name)],
+        }));
+      }


Great usage of the FieldsMetadata service @qn895 👏

Tip
A further optimization here is to reduce a lot the API response payload by only keeping the field attributes you need, which I think it only consists of description so far! You can achieve it by passing the attributes option, this will return only the essential attributes for this use case and reduce quite a lot the API response size:

const {fields} = await fieldsMetadataClient.find({ fieldNames: columns.map((c) => c.name), attributes: ['description'] }); if (fields) { return columns.map((c) => ({ ...c, // Metadata services gives description for // 'ecs.version' but not 'ecs.version.keyword' // but both should show description if available metadata: fields[removeKeywordSuffix(c.name)], })); }

Updated here 2695236

…-fields

jughosta · 2024-07-12T13:20:47Z

packages/kbn-text-based-editor/src/ecs_metadata_helper.ts

+          const metadata = fields.fields[removeKeywordSuffix(c.name)];
+
+          // Need to convert metadata's type (e.g. keyword) to ES|QL type (e.g. string) to check if they are the same
+          if (!metadata || convertEcsFieldTypeToESQLType(metadata.type) !== c.type) return c;


We could also use esFieldTypeToKibanaFieldType(metadata.type) !== c.type

Perfection! Thanks for the hint. Updated here 567cfa9 (#187922)

…fix'

stratoula · 2024-07-15T12:25:17Z

/ci

stratoula · 2024-07-15T12:27:45Z

packages/kbn-text-based-editor/src/overwrite.scss

@@ -60,4 +60,4 @@
 .TextBasedLangEditor--expanded .monaco-editor, .TextBasedLangEditor--expanded .monaco-editor .margin, .TextBasedLangEditor--expanded .monaco-editor .overflow-guard {
  border-top-left-radius: 0;
  border-bottom-left-radius: 0;
-}


If you revert this exactly as it was, we won't need the design team review

stratoula

This is awesome! LGTM! 👏

ryankeairns

Looks great! Other feedback thus far seems to have tightened the UX up even further.

elasticmachine · 2024-07-15T14:14:31Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 8238345

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`esql`	58	64	+6
`lens`	1482	1483	+1
`stackAlerts`	150	151	+1
total			+8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/esql-validation-autocomplete`	182	183	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`esql`	177.1KB	181.5KB	+4.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`esql`	6.5KB	6.6KB	+88.0B
`kbnUiSharedDeps-srcJs`	3.2MB	3.2MB	+269.0B
total			+357.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/esql-validation-autocomplete`	193	194	+1

ESLint disabled line counts

id	before	after	diff
`@kbn/text-based-editor`	5	7	+2

Total ESLint disabled count

id	before	after	diff
`@kbn/text-based-editor`	5	7	+2

History

💚 Build #221424 succeeded 8238345
💔 Build #221169 failed 6036bda
💛 Build #220751 was flaky b239702
💔 Build #220746 failed ac38824
💔 Build #220722 failed ecaa35a

cc @qn895

packages/kbn-text-based-editor/src/ecs_metadata_helper.test.ts

qn895 added 2 commits July 9, 2024 16:05

@todo:CHANGE BRANCH - Initialize editor with ecs description

0aced4b

Push results with ECS description up top

890a3f6

qn895 self-assigned this Jul 9, 2024

qn895 added Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana v8.16.0 labels Jul 9, 2024

qn895 added 2 commits July 10, 2024 13:03

Make styling so that words wrap without being truncated

a9853ca

Update types

ecaa35a

kibanamachine and others added 3 commits July 10, 2024 18:51

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

7265e10

…fix'

Import type

d6f5ae6

Add comment for clarity, update config order

d50322b

qn895 added 2 commits July 10, 2024 14:08

Fix fallback

a90a376

Add tests

ac38824

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

b239702

…fix'

qn895 marked this pull request as ready for review July 11, 2024 00:24

qn895 requested review from a team as code owners July 11, 2024 00:24

qn895 requested a review from stratoula July 11, 2024 00:24

stratoula added the release_note:enhancement label Jul 11, 2024

pgayvallet approved these changes Jul 11, 2024

View reviewed changes

stratoula reviewed Jul 11, 2024

View reviewed changes

tonyghiani reviewed Jul 11, 2024

View reviewed changes

qn895 added 2 commits July 11, 2024 10:32

Merge remote-tracking branch 'upstream/main' into esql-prioritize-ecs…

28c1de9

…-fields

Add markdown, remove check for ecs version

2695236

qn895 added 3 commits July 11, 2024 16:37

Add chunking

e5595d8

Update test

22db292

Update tests

0bf00c7

jughosta reviewed Jul 12, 2024

View reviewed changes

qn895 and others added 4 commits July 12, 2024 09:47

Update to esFieldTypeToKibanaFieldType

567cfa9

Revert overwrite of style

2a04a1f

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6036bda

…fix'

Merge branch 'main' into esql-prioritize-ecs-fields

8238345

stratoula reviewed Jul 15, 2024

View reviewed changes

stratoula approved these changes Jul 15, 2024

View reviewed changes

ryankeairns approved these changes Jul 15, 2024

View reviewed changes

qn895 merged commit 0ac61b7 into elastic:main Jul 15, 2024
21 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jul 15, 2024

drewdaemon reviewed Jul 16, 2024

View reviewed changes

packages/kbn-text-based-editor/src/ecs_metadata_helper.test.ts Show resolved Hide resolved

drewdaemon mentioned this pull request Aug 14, 2024

[ES|QL] Autocomplete suggestions improvements #176033

Open

1 task

drewdaemon mentioned this pull request Oct 21, 2024

[ES|QL] add a robust system for prioritizing suggestions #197113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ES|QL] Add ECS information to the editor hint & prioritize fields based on ECS information on the editor #187922

[ES|QL] Add ECS information to the editor hint & prioritize fields based on ECS information on the editor #187922

qn895 commented Jul 9, 2024 •

edited

Loading

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

elasticmachine commented Jul 11, 2024

stratoula left a comment

stratoula Jul 11, 2024

qn895 Jul 12, 2024

stratoula Jul 11, 2024

qn895 Jul 12, 2024 •

edited

Loading

stratoula Jul 11, 2024

stratoula Jul 11, 2024

tonyghiani Jul 11, 2024

qn895 Jul 12, 2024

jughosta Jul 12, 2024

qn895 Jul 12, 2024

stratoula commented Jul 15, 2024

stratoula Jul 15, 2024

stratoula left a comment

ryankeairns left a comment

elasticmachine commented Jul 15, 2024

API count

ESLint disabled line counts

Total ESLint disabled count

	it('should return columns as is if fieldsMetadata is not provided', async () => {
	it('should return columns if fieldsMetadata is not provided', async () => {

	it('should return columns as is if ECS version field is not present', async () => {
	it('should return columns if ECS version field is not present', async () => {

		@@ -18,6 +18,9 @@ export interface ESQLVariable {
		export interface ESQLRealField {

[ES|QL] Add ECS information to the editor hint & prioritize fields based on ECS information on the editor #187922

[ES|QL] Add ECS information to the editor hint & prioritize fields based on ECS information on the editor #187922

Conversation

qn895 commented Jul 9, 2024 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

qn895 commented Jul 10, 2024

elasticmachine commented Jul 11, 2024

stratoula 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

qn895 Jul 12, 2024 • edited Loading

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

stratoula commented Jul 15, 2024

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 15, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

qn895 commented Jul 9, 2024 •

edited

Loading

qn895 Jul 12, 2024 •

edited

Loading