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

[Step 1][ESQL] Syntax part #146379

Merged
merged 30 commits into from
Dec 7, 2022
Merged

[Step 1][ESQL] Syntax part #146379

merged 30 commits into from
Dec 7, 2022

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Nov 28, 2022

Part of: #144296

Summary

First step of [ESQL] Improve the typing experience (#144296). This PR is the skeleton for later parts, but already supports basic parsing scenarios.

What was done:

  • Added new ESQL language in @kbn/monaco package. Parsing was done using the antl syntax;
  • kbnUiSharedDeps-srcJs bundle has been optimised, all workers have been moved into separate chunks. This gave almost minus 2 MB 🕺
  • existing esql lang was renamed to sql. In order not to confuse anyone in the future
  • some code from painless folder was moved to common and reused in ESQL (probably needs some refactoring in future)

Next steps:

  • improving ANLT syntax to cover all cases
  • implementing Autocomplete feature

How to use new ESQL Lang:

To use new language 2 properties should be set for CodeEditor component

import { ESQL_LANG_ID, ESQL_THEME_ID  } from '@kbn/monaco';
import { CodeEditor  } from '@kbn/kibana-react-plugin/public';

 <CodeEditor
      ...
      languageId={ESQL_LANG_ID}
      options={{
        ...
        theme: ESQL_THEME_ID,
      }}
 />

Currently syntax highlighting looks like:

image

image

Will be updated in Step 2

@alexwizp alexwizp force-pushed the es_ql_syntax branch 3 times, most recently from 07b84e1 to 49e9a93 Compare November 30, 2022 12:06
@alexwizp alexwizp added the WIP Work in progress label Dec 2, 2022
@alexwizp
Copy link
Contributor Author

alexwizp commented Dec 5, 2022

@elasticmachine merge upstream

@alexwizp alexwizp requested review from a team as code owners December 5, 2022 10:03
@stratoula
Copy link
Contributor

-2MB ??? 😍

@@ -39,7 +39,7 @@ snapshots.js
/packages/kbn-test/src/functional_test_runner/lib/config/__tests__/fixtures/
/packages/kbn-ui-framework/dist
/packages/kbn-flot-charts/lib
/packages/kbn-monaco/src/painless/antlr
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be the source of the 2MB drop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Visualizations team changes LGTM

alexwizp added a commit that referenced this pull request Dec 5, 2022
## Summary

Replace @elastic/kibana-app-services ->
@elastic/kibana-global-experience for `kbn-monaco` package

While working on #146379 I just saw that the plugin has the wrong
code-owner. Let's fix it

Co-authored-by: Kibana Machine <[email protected]>
@alexwizp alexwizp removed the request for review from a team December 5, 2022 13:08
@alexwizp alexwizp changed the title [Step 1][ES_QL] Syntax part [Step 1][ESQL] Syntax part Dec 5, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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/monaco 55 60 +5

Async chunks

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

id before after diff
canvas 1.0MB 1.0MB -1.0B
kibanaReact 208.1KB 208.2KB +53.0B
unifiedSearch 216.2KB 216.2KB -1.0B
total +51.0B

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 4.1MB 2.2MB -1.9MB
Unknown metric groups

API count

id before after diff
@kbn/monaco 55 60 +5

ESLint disabled in files

id before after diff
@kbn/monaco 4 5 +1
osquery 1 2 +1
total +2

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
total +20

Total ESLint disabled count

id before after diff
@kbn/monaco 7 8 +1
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +22

History

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

cc @alexwizp

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

This PR unfortunately currently breaks Canvas. To reproduce.

  • Go to canvas
  • Add an element like a Data Table (not an embeddable)
  • Click on the element and move to the data tab
  • Click Elasticsearch Sql.

Canvas shows a blank screen, and the console shows these errors:

Screen Shot 2022-12-05 at 2 43 47 PM

Likely the editor for the data source needs to be re-instated.

We should create functional tests which cover this use case so this doesn't happen again. I have opened an issue to track our functional test additions.

@alexwizp
Copy link
Contributor Author

alexwizp commented Dec 6, 2022

@ThomThomson could you please try again with bootstrapping kibana. It's important cause @kbn/monaco package was changed. From the code perspective I don't see any issues with canvas code

Screen:

Screen.Recording.2022-12-06.at.11.48.09.mov

@alexwizp alexwizp requested a review from ThomThomson December 6, 2022 09:32
@stratoula
Copy link
Contributor

@ThomThomson I can't reproduce it either, I think that Aliaksei is right and you should bootstrap first!

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Sorry for the noise folks, re-reviewed today (properly bootstrapped), and the ESql section of Canvas works as expected! LGTM!

@alexwizp
Copy link
Contributor Author

alexwizp commented Dec 6, 2022

@elastic/kibana-global-experience please have a look

@vadimkibana vadimkibana requested review from rshen91 and dokmic December 7, 2022 10:47
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

@alexwizp alexwizp merged commit 464a9f7 into elastic:main Dec 7, 2022
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Dec 7, 2022
@spalger
Copy link
Contributor

spalger commented Dec 7, 2022

Woot!

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Unified search Unified search related tasks release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.7.0 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants