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

Support for painless language autocomplete within monaco #80577

Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Oct 14, 2020

Initial work to add autocomplete functionality for the painless language within the monaco editor. This first phase focuses on autocompletion for class and class members given a Painless context. It has been integrated with Painless Lab for ease of testing. Integration across other plugins will be handled in a separate PR.

TODO:

  • Delete dummy context files, which exports static JSON to handle autocomplete, in favor of incorporating the data directly from ES
    • Remove reference to forked ES branch in script (dependent on code merged to ES master branch)
  • Move logic from the painless_lab plugin into the @kbn/monaco package
  • Support ability to specify a painless context within editor
  • Update current painless editors to consume the language features
    • Painless lab
    • Runtime field in index templates UI Will open separate PR for integration (reverted via 9b15fdf)
    • Script editor and condition editor in ingest node pipelines Will open separate PR for integration (reverted via 9b15fdf)
  • Add readme to @kbn/monaco/painless

Summary

Autocomplete generation

The autocomplete definitions are generated by cloning the ES repo and copying over the allowed classes per context stored here.

Note: This data is currently only available on a feature branch, and has not yet been merged to master. It is also the same information available via the painless context API: GET /_scripts/painless/_context?context=<context>.

The data OOTB is not in a format that can be easily consumed by monaco for autocompletion. Since this is all static content, rather than manipulate the data at runtime, the script that copies the data over also reformats it. All related code can be found under the scripts directory. The script should only need to be run once per release.

There are >30 different painless contexts available. Each context definition is ~1MB. I’ve only added support for a select few contexts for now. Long term, I think we probably need to reevaluate how we handle this if we intend to add more.

Implementation

Please refer to the README for an explanation of the general architecture.

Testing

This work has been integrated in Painless Lab, so testing can be performed there.

What should work?

  1. Autocompletion for keywords (if, else, for, etc.)
  2. Any word prefixed by “new “ should only return available constructors
  3. Typing a class follow by a dot (e.g., “Math.”), should return all available methods or fields for that class.
  4. Autocompletion for document fields. In order to enable this, the consumer must supply the available fields to the editor. This isn’t applicable in Painless Lab, but for testing purposes you can temporarily edit the editor.tsx file in Painless Lab to pass fields to the suggestion provider. For example:
  const fields = [
    {
      name: 'field1',
      type: 'float',
    },
    {
      name: 'field2',
      type: 'boolean',
    },
    {
      name: 'field3',
      type: 'boolean',
    },
    {
      name: 'field4',
      type: 'boolean',
    },
  ];

  const suggestionProvider = PainlessLang.getSuggestionProvider(context, fields);

With this, autocompletion should be available for the doc keyword, and the available fields should populate when you type the ' character inside the bracket, e.g., doc['].

  1. Changing the context should change the autocomplete definitions that are returned. Painless Lab by default uses the painless_test context, but the UI allows you to switch to score or filter as well.

Release note

Painless Lab in DevTools now supports autocompletion for keywords, as well as class and class members for the Painless language based on a given context.

painless_autocomplete_example

@alisonelizabeth alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Painless Lab Dev tool for learning Painless v7.11.0 labels Oct 14, 2020
@alisonelizabeth alisonelizabeth force-pushed the painless-monaco-autocomplete branch from 7ad74a6 to ed23ea1 Compare October 28, 2020 20:31
@alisonelizabeth alisonelizabeth marked this pull request as ready for review November 18, 2020 19:37
@alisonelizabeth alisonelizabeth requested a review from a team as a code owner November 18, 2020 19:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@alisonelizabeth
Copy link
Contributor Author

@jloleysens I believe I addressed your comments. What are your thoughts about moving ahead with the review/merge of this PR without the ES changes merged? Since the ES changes are only used in the script to generate the autocomplete definitions, it seems minimal risk. I will run the script again once the ES changes are merged to address any updates.

I’d like to get this code in so testing can begin and it can be integrated in ingest node pipelines and the runtime fields editor. I went ahead and moved the PR out of draft. If you’re OK with this, would you mind taking another look and also test (if you haven’t yet)?

Also, I want to point out that the autocomplete definitions adds ~9MB to the kbn-ui-shared-deps.js asset. I talked to the ES team about this. There are many classes that are shared across contexts, so there’s potential to create a common list, and then provide a separate definitions file per context that only contains the unique classes. (This would be addressed as a follow-up PR.)

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Thanks for wrestling with my suggestion @alisonelizabeth! I think the code is looking really clean 💪🏻. Great work!

After testing locally I have the following suggestions that we can tackle in a following PR that are quite small:

We could make the code editing experience more natural to users by adding auto closing brackets 👇🏻

somepi

Patch for auto closing brackets
diff --git i/packages/kbn-monaco/src/painless/index.ts w/packages/kbn-monaco/src/painless/index.ts
index 3c81f265f9b..514d8ed3b30 100644
--- i/packages/kbn-monaco/src/painless/index.ts
+++ w/packages/kbn-monaco/src/painless/index.ts
@@ -17,10 +17,28 @@
  * under the License.
  */
 
+import { monaco } from '../monaco_imports';
 import { ID } from './constants';
 import { lexerRules } from './lexer_rules';
 import { getSuggestionProvider } from './language';
 
-export const PainlessLang = { ID, getSuggestionProvider, lexerRules };
+export const PainlessLang = {
+  ID,
+  getSuggestionProvider,
+  lexerRules,
+  languageConfiguration: {
+    brackets: [
+      ['{', '}'],
+      ['[', ']'],
+      ['(', ')'],
+    ],
+    autoClosingPairs: [
+      { open: '{', close: '}' },
+      { open: '[', close: ']' },
+      { open: '(', close: ')' },
+      { open: '"', close: '"' },
+    ],
+  } as monaco.languages.LanguageConfiguration,
+};
 
 export { PainlessContext } from './types';
diff --git i/packages/kbn-monaco/src/register_globals.ts w/packages/kbn-monaco/src/register_globals.ts
index 630467dd817..db97b69c013 100644
--- i/packages/kbn-monaco/src/register_globals.ts
+++ w/packages/kbn-monaco/src/register_globals.ts
@@ -36,6 +36,7 @@ monaco.languages.setMonarchTokensProvider(XJsonLang.ID, XJsonLang.lexerRules);
 monaco.languages.setLanguageConfiguration(XJsonLang.ID, XJsonLang.languageConfiguration);
 monaco.languages.register({ id: PainlessLang.ID });
 monaco.languages.setMonarchTokensProvider(PainlessLang.ID, PainlessLang.lexerRules);
+monaco.languages.setLanguageConfiguration(PainlessLang.ID, PainlessLang.languageConfiguration);
 monaco.languages.register({ id: EsqlLang.ID });
 monaco.languages.setMonarchTokensProvider(EsqlLang.ID, EsqlLang.lexerRules);
 

Another minor improvement we can look into is making text the user has typed, like variable names appear in the auto complete list. The only time I saw these names was when I was creating a new variable (but I had already created a variable with that name). So in the gif above, somePi is the name of a function, and when a create a new function I would get that function name suggested to me (which is OK since it is detecting text) but it would be nice if I got the same suggestion when, for instance, returning the result of calling somePi -> return so<complete> further down.

[EDIT]

👏🏻👏🏻 for test coverage!

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
painlessLab 37.6KB 37.9KB +232.0B

Distributable file count

id before after diff
default 43140 43181 +41
oss 22538 22579 +41
Unknown metric groups

@kbn/ui-shared-deps asset size

id before after diff
kbn-ui-shared-deps.@elastic.js 2.4MB 2.4MB +8.0B
kbn-ui-shared-deps.js 4.8MB 13.6MB ⚠️ +8.7MB
total ⚠️ +8.7MB

History

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

@alisonelizabeth
Copy link
Contributor Author

Thanks @jloleysens for the review!

We could make the code editing experience more natural to users by adding auto closing brackets

Thanks for providing the patch! I had considered this, but I received some feedback from the ES engineers that they find this annoying sometimes. I tend to prefer it. I'll add it in a follow-up PR and try to get more feedback around it.

Another minor improvement we can look into is making text the user has typed, like variable names appear in the auto complete list.

This is tricky. Monaco comes with this kind of autocomplete OOTB. However, once you add custom autocomplete suggestions it no longer works. In other words, if you provide an empty array [] for autocomplete suggestions, it will be enabled. The only related issue I came across was microsoft/monaco-editor#1850. I'll continue looking into it and see if we can improve anything here.

@alisonelizabeth alisonelizabeth merged commit 7691184 into elastic:master Nov 30, 2020
@alisonelizabeth alisonelizabeth deleted the painless-monaco-autocomplete branch November 30, 2020 15:14
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 30, 2020
…bana into add-metadata-to-node-details

* 'add-metadata-to-node-details' of github.com:phillipb/kibana:
  [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended  (elastic#84415)
  Support for painless language autocomplete within monaco (elastic#80577)
  [Lens] Time scale ui (elastic#83904)
  removing beta callouts (elastic#84510)
  [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493)
  Trusted Apps signer API. (elastic#83661)
  increase stdout max listeners for legacy logging (elastic#84497)
  [APM] Service overview: Add throughput chart (elastic#84439)
  [Discover] Unskip main functional tests (elastic#84300)
  Uptime overview overhaul (elastic#83406)
  [APM] Adjust time formats based on the difference between start and end (elastic#84470)
  [ML] Renaming saved object repair to sync (elastic#84311)
  [UsageCollection] Remove `formatBulkUpload` and other unused APIs (elastic#84313)
  [Visualizations] Adds visConfig.title and uiState to build pipeline function (elastic#84456)
  [Elasticsearch Migration] Update docs re UsageCollection (elastic#84322)
  TSVB field list performance issue on using annotations (elastic#84407)
  [Security Solution] Exceptions Cypress tests (elastic#81759)
  [ML] Fix spaces job ID check (elastic#84404)
  [Security Solution][Detections] Handle dupes when processing threshold rules (elastic#83062)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 30, 2020
* master: (25 commits)
  [Alerting] fixes buggy default message behaviour (elastic#84202)
  [Graph] Use new ES client and change license API (elastic#84398)
  [DOCS] Adds redirect to known plugins page (elastic#84001)
  Update IndexPatternSelect to get fields from indexPatternService instead of savedObject attributes (elastic#84376)
  Adding timestamps to created events so the sorting is stable (elastic#84515)
  [DOCS] Redirects for drilldown links (elastic#83846)
  [Fleet] Support for showing an Integration Detail Custom (UI Extension) tab (elastic#83805)
  [Enterprise Search] Migrate shared Schema components (elastic#84381)
  [Discover] Unskip date_nanos and shard links functional tests (elastic#82878)
  [APM] ML anomaly detection integration: Displaying anomaly job results in the Transaction duration chart is not as intended  (elastic#84415)
  Support for painless language autocomplete within monaco (elastic#80577)
  [Lens] Time scale ui (elastic#83904)
  removing beta callouts (elastic#84510)
  [Lens] (Accessibility) add aria-label to chart type icon (elastic#84493)
  Trusted Apps signer API. (elastic#83661)
  increase stdout max listeners for legacy logging (elastic#84497)
  [APM] Service overview: Add throughput chart (elastic#84439)
  [Discover] Unskip main functional tests (elastic#84300)
  Uptime overview overhaul (elastic#83406)
  [APM] Adjust time formats based on the difference between start and end (elastic#84470)
  ...
const { join } = require('path');
const simpleGit = require('simple-git');

// Note: The generated whitelists have not yet been merged to master
Copy link
Contributor

Choose a reason for hiding this comment

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

@alisonelizabeth Per #71398 we should refer to allowlists instead of whitelists and blocklists instead of blacklists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! Will fix in #85055.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Painless Lab Dev tool for learning Painless release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants