Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ES|QL] open suggestions automatically in sources lists and ENRICH #191312

Merged
merged 26 commits into from
Sep 16, 2024

Conversation

drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Aug 26, 2024

Summary

Part of #189662

Also, a follow-on to #187184 with certain source names (e.g. foo$bar, my-policy) and fields in the ENRICH command.

During this effort I discovered #191321 and a bug with the validation of field names in the "ENRICH ... WITH" list (documented here), both of which will be addressed separately.

Sources

General flow

Screen.Recording.2024-08-28.at.2.22.32.PM.mov

Works with wild-card matches

Screen.Recording.2024-08-26.at.2.38.55.PM.mov

METADATA field list

Screen.Recording.2024-08-26.at.4.06.40.PM.mov

ENRICH

Autosuggest now helps you along

Screen.Recording.2024-08-28.at.12.57.32.PM.mov

Also, fixed this bug (follow on to #187184 )

Screen.Recording.2024-08-28.at.12.36.51.PM.mov

Checklist

@drewdaemon drewdaemon added Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana labels Aug 26, 2024
@drewdaemon drewdaemon added release_note:skip Skip the PR/issue when compiling release notes release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 28, 2024
@drewdaemon
Copy link
Contributor Author

/ci

@drewdaemon
Copy link
Contributor Author

/ci

@drewdaemon drewdaemon changed the title [ES|QL] open suggestions automatically in sources list [ES|QL] open suggestions automatically in sources lists and ENRICH Aug 29, 2024
@drewdaemon drewdaemon marked this pull request as ready for review August 29, 2024 03:16
@drewdaemon drewdaemon requested a review from a team as a code owner August 29, 2024 03:16
@elasticmachine
Copy link
Contributor

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

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -845,6 +961,57 @@ describe('autocomplete', () => {
'| ',
]);

describe('ENRICH', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth splitting these new blocks out to new files 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you split it up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I suggested this because there are two separate describe('enrich') blocks in this file. But one is to test the actual suggestions for enrich, and one is to test advancing the cursor and opening the suggestion menu automatically. We can discuss this in office hours how best to structure these autocomplete tests, but imo it might be worth moving the big blocks like advancing the cursor into their own files in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. There's a tension between splitting things up by command or by language feature across commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is best but, yes, worth a discussion!

* @returns
*/
export function findFinalWord(text: string) {
const words = text.split(/\s+/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the regex in this and in findPreviousWord, is it worth trimming the text first? For example "foo bar " would give ['', 'foo', 'bar', ''].

Copy link
Member

@qn895 qn895 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok I see now we do it expect it to be an empty space in some cases.

@@ -272,7 +273,7 @@ export const buildPoliciesDefinitions = (
): SuggestionRawDefinition[] =>
policies.map(({ name: label, sourceIndices }) => ({
label,
text: getSafeInsertText(label, { dashSupported: true }),
text: getSafeInsertText(label, { dashSupported: true }) + ' ',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to think we should make this ' ' a constant for clarity and easy searching. So something like OPEN_SUGGESTION = ' ' and then text + OPEN_SUGGESTION

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ' ' is not what opens the suggestion menu. That is the command property.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant advance cursor. Switched it around in my brain. And this is just a thought, not a blocker.

// ok, this is a bit of cheating as it's using the same logic as in the helper
enrichFields.map((field) => (/[^a-zA-Z\d_\.@]/.test(field) ? `\`${field}\`` : field))
);
.flatMap(({ enrichFields }) => enrichFields.map((field) => getSafeInsertText(field)));
Copy link
Member

@qn895 qn895 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can probably use enrichFields.map(getSafeInsertText) here

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@qn895
Copy link
Member

qn895 commented Sep 10, 2024

LGTM 🎉

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have blocked this PR till we have this #187247 I am blocking also with Request changes (just to ensure that it wont be accidentally being merged). It is coming #192631 🎉

We can also mark it as draft for now.

@drewdaemon
Copy link
Contributor Author

@elasticmachine merge upstream

@drewdaemon
Copy link
Contributor Author

I'm going to keep it out of draft mode since it will be unblocked when #192953 merges

@stratoula stratoula added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed blocked labels Sep 16, 2024
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unblocking the PR now that the blocker got resolved. LGTM, I did a brief testing locally to ensure that it works as expected.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #1 / SolutionFilter when the owner is a single solution renders options correctly

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/esql-validation-autocomplete 10 11 +1

Page load bundle

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

id before after diff
kbnUiSharedDeps-srcJs 3.4MB 3.4MB +656.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@drewdaemon drewdaemon merged commit e404a39 into elastic:main Sep 16, 2024
23 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 16, 2024
…lastic#191312)

## Summary

Part of elastic#189662

Also, a follow-on to elastic#187184
with certain source names (e.g. `foo$bar`, `my-policy`) and fields in
the `ENRICH` command.

During this effort I discovered
elastic#191321 and a bug with the
validation of field names in the "ENRICH ... WITH" list (documented
[here](elastic#177699)), both of which
will be addressed separately.

## Sources

### General flow

https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf

### Works with wild-card matches

https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c

### `METADATA` field list

https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a

## ENRICH

Autosuggest now helps you along

https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f

Also, fixed this bug (follow on to
elastic#187184 )

https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
(cherry picked from commit e404a39)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 16, 2024
…60;ENRICH&#x60; (#191312) (#193013)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[ES|QL] open suggestions automatically in sources lists and
&#x60;ENRICH&#x60;
(#191312)](#191312)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Drew
Tate","email":"drew.tate@elastic.co"},"sourceCommit":{"committedDate":"2024-09-16T13:41:38Z","message":"[ES|QL]
open suggestions automatically in sources lists and `ENRICH`
(#191312)\n\n## Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on
to https://github.com/elastic/kibana/issues/187184\r\nwith certain
source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH`
command.\r\n\r\nDuring this effort I
discovered\r\nhttps://github.com//issues/191321 and a bug
with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list
(documented\r\n[here](#177699)),
both of which\r\nwill be addressed separately.\r\n\r\n##
Sources\r\n\r\n### General
flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n###
Works with wild-card
matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n###
`METADATA` field
list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n##
ENRICH\r\n\r\nAutosuggest now helps you
along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso,
fixed this bug (follow on
to\r\nhttps://github.com//issues/187184
)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"e404a3992e220735ae51918c532a1a032e7f7993","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL"],"title":"[ES|QL]
open suggestions automatically in sources lists and
`ENRICH`","number":191312,"url":"https://github.com/elastic/kibana/pull/191312","mergeCommit":{"message":"[ES|QL]
open suggestions automatically in sources lists and `ENRICH`
(#191312)\n\n## Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on
to https://github.com/elastic/kibana/issues/187184\r\nwith certain
source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH`
command.\r\n\r\nDuring this effort I
discovered\r\nhttps://github.com//issues/191321 and a bug
with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list
(documented\r\n[here](#177699)),
both of which\r\nwill be addressed separately.\r\n\r\n##
Sources\r\n\r\n### General
flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n###
Works with wild-card
matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n###
`METADATA` field
list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n##
ENRICH\r\n\r\nAutosuggest now helps you
along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso,
fixed this bug (follow on
to\r\nhttps://github.com//issues/187184
)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"e404a3992e220735ae51918c532a1a032e7f7993"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/191312","number":191312,"mergeCommit":{"message":"[ES|QL]
open suggestions automatically in sources lists and `ENRICH`
(#191312)\n\n## Summary\r\n\r\nPart of
https://github.com/elastic/kibana/issues/189662\r\n\r\nAlso, a follow-on
to https://github.com/elastic/kibana/issues/187184\r\nwith certain
source names (e.g. `foo$bar`, `my-policy`) and fields in\r\nthe `ENRICH`
command.\r\n\r\nDuring this effort I
discovered\r\nhttps://github.com//issues/191321 and a bug
with the\r\nvalidation of field names in the \"ENRICH ... WITH\" list
(documented\r\n[here](#177699)),
both of which\r\nwill be addressed separately.\r\n\r\n##
Sources\r\n\r\n### General
flow\r\n\r\n\r\nhttps://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf\r\n\r\n###
Works with wild-card
matches\r\n\r\n\r\nhttps://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c\r\n\r\n###
`METADATA` field
list\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a\r\n\r\n##
ENRICH\r\n\r\nAutosuggest now helps you
along\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f\r\n\r\nAlso,
fixed this bug (follow on
to\r\nhttps://github.com//issues/187184
)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85\r\n\r\n\r\n###
Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Stratoula
Kalafateli
<efstratia.kalafateli@elastic.co>","sha":"e404a3992e220735ae51918c532a1a032e7f7993"}}]}]
BACKPORT-->

Co-authored-by: Drew Tate <drew.tate@elastic.co>
markov00 pushed a commit to markov00/kibana that referenced this pull request Sep 18, 2024
…lastic#191312)

## Summary

Part of elastic#189662

Also, a follow-on to elastic#187184
with certain source names (e.g. `foo$bar`, `my-policy`) and fields in
the `ENRICH` command.

During this effort I discovered
elastic#191321 and a bug with the
validation of field names in the "ENRICH ... WITH" list (documented
[here](elastic#177699)), both of which
will be addressed separately.

## Sources

### General flow


https://github.com/user-attachments/assets/4b103621-0e66-4c36-807f-4932f0cb8faf

### Works with wild-card matches


https://github.com/user-attachments/assets/6b47fffc-e922-4e2d-b6aa-3d9a2fc2236c

### `METADATA` field list


https://github.com/user-attachments/assets/d3bdf4dc-1d0c-4d56-81d7-af6bc4e25a4a

## ENRICH

Autosuggest now helps you along


https://github.com/user-attachments/assets/d627484c-e729-4dc7-9e7b-795395a31d4f

Also, fixed this bug (follow on to
elastic#187184 )



https://github.com/user-attachments/assets/aa62a0c3-6db5-434a-829a-59f14c5c4c85


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:ES|QL ES|QL related features in Kibana release_note:enhancement Team:ESQL ES|QL related features in Kibana v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants